[Poppler-bugs] [Bug 89136] Annotations of /Subtype /Popup are not added to /Annots array of a page

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=89136

--- Comment #10 from philipp.reinkemeier at offis.de ---
(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 113476 [details] [review]
> Alternative fix if done in poppler core
> 
> Review of attachment 113476 [details] [review]:
> -----------------------------------------------------------------
> 
> The commit message doesn't seem to be correctly formatted. Thanks for the
> patch.
> 
Ok. I will fix this.

> ::: poppler/Annot.cc
> @@ +2095,5 @@
> > +  // dangling references to it.
> > +  if (popup != NULL && popup->getPageNum() != 0) {
> > +    Page *pageobj = popup->getDoc()->getPage(popup->getPageNum());
> > +    if (pageobj) {
> > +      pageobj->removeAnnot(popup);
> 
> Maybe we could reuse the existing object, instead of remove + add new.
> 
Hm. I cannot see why. It has been that way before. All that is changed is
removing the popup annotation from a page if it has been associated to a page,
before deleting the old popup annotation like before.
I guess you mean just copy over relevant stuff from the new popup annotation
(like rectangle and so on)? If so, doesn't this just add additional code
complexity without a real gain?

> @@ +2114,5 @@
> > +    // If this annotation is already added to a page, then we
> > +    // add the new popup annotation to the same page.
> > +    if (page != 0) {
> > +      Page *pageobj = doc->getPage(page);
> > +      if (pageobj) {
> 
> This should be an error, so we should either add an assert() instead of add
> an error message when this happens.
> 
Ok. I will fix this.

> ::: poppler/Page.cc
> @@ +444,2 @@
> >  
> >    annots->appendAnnot(annot);
> 
> We ignore popup annots associated to a markup annotation in the list of
> annots, so here, I would check if the newly added is a popup annot
> associated to a markup annotation (you could simply check if it has parent),
> to not call annots->appendAnnot()
> 
Ah. So calling annot->setPage(..) is enough for an annotation to appear in the
/Annots array later on when saving the document?

> @@ +449,5 @@
> > +  if (annot_markup) {
> > +    AnnotPopup *annot_popup = annot_markup->getPopup();
> > +    if (annot_popup) {
> > +      this->addAnnot(annot_popup);
> > +    }
> 
> I don't think we need this. If a new markup annotation is created with a
> popup, setPopup will be called at some point, so we don't need to add the
> popup when the markup annot itself is added.

Yes, setPopup will be called at some point. But you do not know whether it will
be called before or after Page::addAnnot(). So removing this would require to
first call Page::addAnnot(ma) for a markup annotation "ma" and then call
ma->setPopup(). Doing it the other way round would then result in the same
problem as before: The popup annotation does not appear in the /Annots array.
Btw.: IIRC the last call sequence (setting popup, then adding to page) that
would break if the snippet above is removed, is the one that is implemented in
evince.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20150216/c9f2c2f3/attachment.html>