[poppler] [PATCH] Add setContents() to modify the annot contents

[Albert Astals Cid]

A Dijous, 23 d'abril de 2009, Carlos Garcia Campos va escriure:
> El mié, 22-04-2009 a las 00:36 +0200, Albert Astals Cid escribió:
> > A Dimarts, 21 d'abril de 2009, Carlos Garcia Campos va escriure:
> > > El lun, 20-04-2009 a las 21:56 +0200, Albert Astals Cid escribió:
> > > > A Dilluns, 20 d'abril de 2009, Carlos Garcia Campos va escriure:
> > > > > El dom, 19-04-2009 a las 23:41 +0200, Albert Astals Cid escribió:
> > > > > > A Dissabte, 18 d'abril de 2009, Carlos Garcia Campos va escriure:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > we are still lacking write support in annotations. This patch,
> > > > > > > to be able to modify the contents of an annotation, it's just a
> > > > > > > first step.
> > > > > > >
> > > > > > > ok to commit?
> > > > > >
> > > > > > contents = new GooString(new_content);
> > > > > >
> > > > > > crashes if new_content is null, so you either protect this or
> > > > > > remove the protection in
> > > > >
> > > > > fair enough. It makes me wonder what should we do in this case, I
> > > > > mean, the Contents field is optional in the annotation array,
> > > > > should we remove the key from the dict when NULL is set? or
> > > > > shouldn't we even allow passing NULL to setContents()?
> > > >
> > > > I just would mark on the header that passing a NULL value to set
> > > > contents is not ok, if you want to clear something just pass
> > > > GooString() and that's all
> > > >
> > > > > > if (new_content && !contents->hasUnicodeMarker()) {
> > > > > >
> > > > > > I think obj1 is missing a free().
> > > > >
> > > > > No, because it's owned by the Dict.
> > > >
> > > > Right, i got tricked thinking that operator= in Object was the same
> > > > as copy.
> > > >
> > > > > > Also you should probably update the modified member too.
> > > > >
> > > > > What do you mean with the modified member?
> > > >
> > > > GooString *modified; inside class Annot.
> > >
> > > I have committed the path without this for the moment. In page 1113 of
> > > the pdf spec it says: "Acrobat viewers update the annotation
> > > dictionary’s M entry only for text
> > > annotations"
> > >
> > > Should we follow this and implement it only for text annotations?
> >
> > Hmpf, any reason in favor or against that? I'm leaning to doing it for
> > all of them, unless someone knows why Acrobat decided to do it only for
> > text and seems to make sense.
>
> I also think it makes more sense to always update M when the annot is
> modified.
>
> > BTW could you add a comment into the header near setContents saying the
> > ownership of the GooString is not taken and so the caller is responsible
> > of deleting it?
>
> Sure, I forgot it.
>
> Attached is a new set of patches, ok to commit?

There are two typos in the setContents comment in the header.

Otherwise, looks ok, other thing is that you forgot to update copyright 
notices in previous and this patches, please update them.

Albert

>
> > Albert
> >
> > > > Albert
> > > >
> > > > > > Otherwise it think this can't hurt to let it go in :D
> > > > >
> > > > > Great, I'll commit it when I know what to do with the NULL case.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > > Albert
> > > > > > _______________________________________________
> > > > > > poppler mailing list
> > > > > > poppler at lists.freedesktop.org
> > > > > > http://lists.freedesktop.org/mailman/listinfo/poppler
> > > >
> > > > _______________________________________________
> > > > poppler mailing list
> > > > poppler at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/poppler
> >
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler