<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body>
<p>
<div>
<b><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#c9">Comment # 9</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 <span class="vcard"><a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><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></span> <a href='page.cgi?id=splinter.html&bug=89136&attachment=113476'>[review]</a>
Alternative fix if done in poppler core
Review of <span class=""><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></span> <a href='page.cgi?id=splinter.html&bug=89136&attachment=113476'>[review]</a>:
-----------------------------------------------------------------
The commit message doesn't seem to be correctly formatted. Thanks for the
patch.
::: poppler/Annot.cc
@@ +2095,5 @@
<span class="quote">> + // dangling references to it.
> + if (popup != NULL && popup->getPageNum() != 0) {
> + Page *pageobj = popup->getDoc()->getPage(popup->getPageNum());
> + if (pageobj) {
> + pageobj->removeAnnot(popup);</span >
Maybe we could reuse the existing object, instead of remove + add new.
@@ +2114,5 @@
<span class="quote">> + // 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) {</span >
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 @@
<span class="quote">>
> annots->appendAnnot(annot);</span >
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 @@
<span class="quote">> + if (annot_markup) {
> + AnnotPopup *annot_popup = annot_markup->getPopup();
> + if (annot_popup) {
> + this->addAnnot(annot_popup);
> + }</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>