Created attachment 113460 [details] PDF with broken annotation created using evince (via glib-frontend) An annotation of /Subtype /Popup can be created and associated with a Markup annotation (e.g. /Subtype /Text) by using poppler_annot_markup_set_popup(). However, this Popup annotation is not an explicit object of the API of the glib frontend. This means currently, this annotation cannot be added to a pdf page using poppler_page_add_annot (). Only the Markup annotation can be added. But ALL annotations of a pdf page need to be referenced from an /Annots array (see pdf specification). Poppler fills this array by keeping track of all annotations that have been added to a page. So if poppler_annot_markup_set_popup() is used to create a Popup annotation and link it to a Markup annotation (say "annot"), and "annot" is added to a page, then the Popup annotation is missing in the /Annots array of that page. Btw.: This is how text annotations are currently implemented in evince. The resulting pdf is NOT correct with respect to the pdf specification. The effect is that an annotation created by evince has no popup window in acrobat reader. In evince and okular it has a popup window. It seems that poppler is more forgiving when loading such a pdf and accepts that the Popup annotation is missing in the /Annots array. The attached uncompressed pdf is a minimal test case. It contains a pdf annotation of Subtype Text created using evince. If this pdf is opened in acrobat reader, no popup window is shown when clicking on the annotation. If one now manually adds the annotation of Subtype Popup to the /Annots array, then everything works fine.
Created attachment 113461 [details] [review] Proposed fix The attached patch fixes this problem. Since annotations of /Subtype /Popup are not explicit objects in the API of the glib frontend, the idea of this patch is to add some automatic handling: Each time a Markup annotation is added to a pdf page, then its associated Popup annotation (if it has one) is also added to the same page. Each time the Popup rectangle of a Markup annotation is changed using poppler_annot_markup_set_popup(), two things happen: - If an old Popup annotation existed and it is associated to a page, then it is removed from that page before it gets overwritten. This is to guarantee that we have no dangling references to it. - If the Markup annotation already has an associated page, then the new Popup annotation is added to the same page. This automatic handling ensures that one can either first add a Markup annotation to a page and set a Popup rectangle later on. Or one can first set a Popup rectangle and add the Markup annotation to a page later on. Both cases work just fine. Note that the core library of poppler already takes care of removing a Popup annotation from a page, once its associated Markup annotation is removed from a page. So there is nothing to do on the removal part.
Created attachment 113462 [details] PDF with correct /Annots array (create after applying the patch) Btw.: The attached PDF has been created using evince and a poppler-glib library with the proposed patch applied.
Does this bug only affects evince or also Okular? if it also affecs Okular, then the patch should be done in poppler core.
As far as i could see this does not affect okular, since it does not seem to store Popup annotations. It just stores the Markup annotations. At least that's what i see in the PDF document. Having said that, i remember that i stumbled upon a comment in the code of the qt4 frontend. In "poppler-annotation.h" there is Annotation::setPopup() and a warning: "Currently does nothing \since 0.20". In the implementation of this method there are a comment: "TODO: Remove old popup and add AnnotPopup to page", as well as "Create a new AnnotPopup and assign it to pdfAnnot". So you may want to CC someone from the qt guys. But indeed it may be reasonable to put this kind of logic in the poppler core instead, since also the API of the qt frontend misses an explicit object for Popup annotations that could be used to add to a pdf page object. So it may also be beneficial to them.
From what you say, I think it's desirable to move the logic to core and then fix glib frontend. Then, the qt side could use the logic in core to fix those comments. Changing the component so Albert can comment on whether w can this on glib or in core.
Created attachment 113476 [details] [review] Alternative fix if done in poppler core I just couldn't help it (i want this fixed). So here is an alternative patch if we want to do this kind of logic in poppler core. I tested it and it works as intended. Actually, i think it is tempting to do it in the core. No fix is needed on the glib side and nothing breaks on the qt side, since the method AnnotMarkup::setPopup(..) is currently only used in the glib frontend. But of course i do not know whether the logic is compatible with the plans how Popup annotations shall be dealt with in the qt frontend in the future.
Yes, we don't add the popup annot to the list of annots, because we ignore popup annots when building the list of annots :-) But they should be added to the document itself when saved, yes.
Comment on attachment 113461 [details] [review] Proposed fix Review of attachment 113461 [details] [review]: ----------------------------------------------------------------- I think this should be done in Annot::setPopup()
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. ::: 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. @@ +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. ::: 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() @@ +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.
(In reply to Carlos Garcia Campos from comment #9) > Comment on attachment 113476 [details] [review] [review] > Alternative fix if done in poppler core > > Review of attachment 113476 [details] [review] [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.
Created attachment 113520 [details] [review] Add annotation of Subtype Popup to pdf page This should fix the issues identified in the review.
(In reply to philipp.reinkemeier from comment #10) > (In reply to Carlos Garcia Campos from comment #9) > > @@ +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. Btw.: I just tested what happens when i removed that code and create a Text annotation in evince. Indeed evince first calls AnnotMarkup::setPopup() and then Page::addAnnot(), meaning the resulting pdf is broken like without the patch.
I would really appreciate to see more comments on the patch or somebody merging it or whatever. Thanks.
Comment on attachment 113520 [details] [review] Add annotation of Subtype Popup to pdf page Review of attachment 113520 [details] [review]: ----------------------------------------------------------------- Sorry for the delay reviewing this, I've been quite busy. I've addressed my comments and pushed to git master. Let's fix the removal in a follow up patch if needed. ::: poppler/Annot.cc @@ +2093,5 @@ > + // associated with a page, then we need to remove that > + // popup annotation from the page. Otherwise we would have > + // dangling references to it. > + if (popup != NULL && popup->getPageNum() != 0) { > + Page *pageobj = popup->getDoc()->getPage(popup->getPageNum()); I guess we can use doc here instead of getting the doc from the popup, it must be the same doc. ::: poppler/Page.cc @@ +448,3 @@ > annot->setPage(num, gTrue); > + > + AnnotMarkup *annot_markup = dynamic_cast<AnnotMarkup*>(annot); We don't use underscores for name variables in the core. @@ +450,5 @@ > + AnnotMarkup *annot_markup = dynamic_cast<AnnotMarkup*>(annot); > + if (annot_markup) { > + AnnotPopup *annot_popup = annot_markup->getPopup(); > + if (annot_popup) { > + this->addAnnot(annot_popup); So, this makes me wonder, should we also remove the popup associated to a markup annotation in Page::removeAnnot() when a markup annot is given?
(In reply to Carlos Garcia Campos from comment #14) > Comment on attachment 113520 [details] [review] [review] > Add annotation of Subtype Popup to pdf page > > Review of attachment 113520 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: poppler/Page.cc > @@ +450,5 @@ > > + AnnotMarkup *annot_markup = dynamic_cast<AnnotMarkup*>(annot); > > + if (annot_markup) { > > + AnnotPopup *annot_popup = annot_markup->getPopup(); > > + if (annot_popup) { > > + this->addAnnot(annot_popup); > > So, this makes me wonder, should we also remove the popup associated to a > markup annotation in Page::removeAnnot() when a markup annot is given? Fortunately this does already happen. Method Page::removeAnnot(Annot *annot) calls annot->removeReferencedObjects(). This in turn is virtual and AnnotMarkup::removeReferencedObjects() calls Page::removeAnnot() again, this time with its AnnotPopup.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.