<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Annotations of /Subtype /Popup are not added to /Annots array of a page" href="https://bugs.freedesktop.org/show_bug.cgi?id=89136#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Annotations of /Subtype /Popup are not added to /Annots array of a page" href="https://bugs.freedesktop.org/show_bug.cgi?id=89136">bug 89136</a> from <a class="email" href="mailto:philipp.reinkemeier@offis.de" title="philipp.reinkemeier@offis.de">philipp.reinkemeier@offis.de</a> <pre>(In reply to Carlos Garcia Campos from <a href="show_bug.cgi?id=89136#c9">comment #9</a>) > Comment on <a href="attachment.cgi?id=113476" name="attach_113476" title="Alternative fix if done in poppler core">attachment 113476</a> <a href="attachment.cgi?id=113476&action=edit" title="Alternative fix if done in poppler core">[details]</a> <a href='page.cgi?id=splinter.html&bug=89136&attachment=113476'>[review]</a> [review] > Alternative fix if done in poppler core > > Review of <a href="attachment.cgi?id=113476" name="attach_113476" title="Alternative fix if done in poppler core">attachment 113476</a> <a href="attachment.cgi?id=113476&action=edit" title="Alternative fix if done in poppler core">[details]</a> <a href='page.cgi?id=splinter.html&bug=89136&attachment=113476'>[review]</a> [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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>