After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733603 - Popup window doesn't show in evince for annotations drawn in okular
Popup window doesn't show in evince for annotations drawn in okular
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: pdf annotations
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-23 13:38 UTC by Anuj Khare
Modified: 2015-04-12 09:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test case with annotations done in different tools (3.31 KB, application/pdf)
2015-02-12 09:04 UTC, Philipp Reinkemeier
  Details
Always have popup window for Markup annotations (2.39 KB, patch)
2015-02-14 16:31 UTC, Philipp Reinkemeier
needs-work Details | Review
Have a popup window for most Markup annotations (3.00 KB, patch)
2015-02-16 09:52 UTC, Philipp Reinkemeier
none Details | Review
Added can-have-popup property to EvAnnotationMarkups. (6.81 KB, patch)
2015-03-18 19:47 UTC, Philipp Reinkemeier
committed Details | Review
Create an EvAnnotationWindow for EvAnnotationMarkups allowing this. (1.54 KB, patch)
2015-03-18 19:48 UTC, Philipp Reinkemeier
committed Details | Review

Description Anuj Khare 2014-07-23 13:38:14 UTC
Popup for Text annotations created with okular do not show popup in evince. The same work properly in acrobat reader.

Steps to reproduce:
1. Annotate a pdf in okular (test file attached), with Text (pop-up note in okular).

2. Open it in evince, and double click on the annotation.

Expected:
Popup with contents will show.

Actual:
There is no popup window shown.
Comment 1 Philipp Reinkemeier 2015-02-10 15:10:50 UTC
I can confirm this + i like to add something that could be helpful to pinpoint the problem: Adding an annotation via evince and opening it in okular or acrobat reader, popup is not shown in any other tool than evince.
Since acrobat reader can be seen as a kind of reference implementation, i think there is something broken with the way annotations are stored in evince (could be either evince itself or the glib frontend of poppler).
Comment 2 Philipp Reinkemeier 2015-02-11 08:18:38 UTC
I used the glib-demo of poppler to look at the annotation properties of a text annotation as stored by evince VS as stored by okular. The difference is that evince stores two additional properties regarding popup: "Popup is open" and "Popup geometry". Both properties are NOT stored by okular and therefore are not present when opening a pdf annotated by okular with evince. Note that the popup can still be shown in acrobat reader.
Now by looking into the source code of evince it seems that evince uses the presence of the popup geometry thing to determine whether a popup is there at all. This explains why a popup is not shown in evince when the annotation was created in okular. So it seems the condition for detecting whether a popup is there should be something different.
Further, one of these properties could also be the reason why popups do not show in acrobat reader for annotations created by evince. I did not manage to investigate it though, since there doesn't exist an easy way to remove a particular property of an annotation.
Comment 3 Germán Poo-Caamaño 2015-02-11 09:10:35 UTC
(In reply to Philipp Reinkemeier from comment #2)
> [...]
> Further, one of these properties could also be the reason why popups do not
> show in acrobat reader for annotations created by evince. I did not manage
> to investigate it though, since there doesn't exist an easy way to remove a
> particular property of an annotation.

You can edit the PDF with a text editor.  If the PDF is compressed, you can uncompress it with pdftk.

Likely easier if you create a PDF with only one or two blank pages and add an annotation later.
Comment 4 Germán Poo-Caamaño 2015-02-11 09:12:50 UTC
(In reply to Philipp Reinkemeier from comment #2)
> I used the glib-demo of poppler to look at the annotation properties of a
> text annotation as stored by evince VS as stored by okular. The difference
> is that evince stores two additional properties regarding popup: "Popup is
> open" and "Popup geometry". Both properties are NOT stored by okular and
> therefore are not present when opening a pdf annotated by okular with
> evince. Note that the popup can still be shown in acrobat reader.

If you add the annotation with glib-demo, do you get the same behaviour than evince?
Comment 5 Philipp Reinkemeier 2015-02-11 10:31:02 UTC
I would certainly like to try that, but couldn't find a way to save a pdf in glib-demo after adding an annotation. If this is possible, can you tell me how to do that?

Though i could not save a pdf in glib-demo, an annotation added in that demo does not have the two properties mentioned above.
Comment 6 Germán Poo-Caamaño 2015-02-11 10:49:09 UTC
Oh. I had an ugly patch to do that. It was ugly because I just added a button in the "Print" tab with a fixed name :-)

A could not find it with a quick search, but it should be straightforward.
IIRC, you only need to call either poppler_document_save() poppler_document_save_a_copy():

gboolean poppler_document_save         (PopplerDocument *document,
                                        const char      *uri,
                                        GError         **error);
gboolean poppler_document_save_a_copy  (PopplerDocument *document,
                                        const char      *uri,
                                        GError         **error);
Comment 7 Philipp Reinkemeier 2015-02-12 09:04:12 UTC
Created attachment 296661 [details]
Test case with annotations done in different tools

The attached PDF contains some annotations made in different tools: The yellow text annotation was created in evince. The green text annotation was created in okular. The red text and highlight annotations were created in glib-demo of poppler-0.30.0.

Using evince only the popup of the yellow annotation is shown.

Using okular all popups are shown except the red text annotation created in glib-demo. However, if you do not use a double click, but navigate to the annotation in the list view found under "Reviews" and open the popup using the context menu of that annotation, then it works. I have no clue why.

Using acrobat reader all popups are shown except the yellow annotation created in evince.
Comment 8 Philipp Reinkemeier 2015-02-12 11:08:27 UTC
Got it. The problem with okular to show the popup for the red text annotation created in glib-demo is because glib-demo sets coordinates of a text annotation with x1==x2 and y1==y2. Increasing x2 and y2 a bit results in an annotation that also behaves well in okular. So in fact okular can show all popups no matter whether created in evince or glib-demo.

Further, i modified the code in glib-demo a little bit so that a popup rectangle is created and stored. This is exactly what causes acrobat reader to NOT show the popup for this annotation anymore.

So concluding: My guess is correct. Storing the popup rectangle property confuses acrobat reader. So i would suggest that evince should not store this property anymore for text annotations and also should not treat the absence of this property as an indication that no popup is present at all.

Since i am not really familiar with the code base of evince i cannot propose a patch at this point, but at least it is more or less clear now what needs to be done to fix this issue.
Comment 9 Germán Poo-Caamaño 2015-02-12 11:24:53 UTC
Good catch!.  I think "we" should look what the PDF specification says about it.

https://www.adobe.com/devnet/pdf/pdf_reference.html

"we" because I am interested in the topic, but I am busy now with other stuffs to take a look at it.
Comment 10 Germán Poo-Caamaño 2015-02-12 19:09:41 UTC
By the way, the relevant code in Evince should be in:

libdocument/ev-annotation.[ch]
libview/ev-annotation-window.[ch]
shell/ev-sidebar-annotations.[ch]
backend/pdf/ev-poppler.[cc|h]

* backend provides the interface with poppler, so I would start there.
* libdocument is the document abstraction
* libview provides the widgets to view a document
* shell is the UI that integrates everything

You can compare the code against the use in poppler-glib-demo (see
glib/demo/annots.[ch]
Comment 11 Philipp Reinkemeier 2015-02-12 20:08:46 UTC
I had a look at the specification. Actually, it should be possible to add an additional Popup Annotation associated with a Markup Annotation if one wants better control over the placement and dimensions of the popup.

Indeed the serialization of the annotations themselves is correct with regard to the specification. BUT: What is wrong is actually the glib frontend. The mistake is that a Popup Annotation added via the glib frontend using "poppler_annot_markup_set_popup(PopperAnnotMarkup *poppler_annot)" is NOT added to the PDF page when the markup annotation it is associated to is added to a pdf page using "poppler_page_add_annot()".
According to the specification there has to be an array called "Annots" per page, which contains references to all pdf annotations. However, since the Popup annotation is not added to the page, it is missing in this array.

So precisely speaking, a pdf resulting from adding a Popup annotation by means of the glib frontend is not conforming to the standard. It seems that poppler is more forgiving when loading a pdf, whose "Annots" array is inconsistent with the annotations. Therefore, this actually broken pdf is nevertheless loaded and showed fine in evince and okular.

I already have a patch that should fix things. I will file a bug report against the glib frontend of poppler and submit it there, since this seems to be the most logical place where this can be fixed.
Comment 12 Philipp Reinkemeier 2015-02-12 20:20:34 UTC
Note that of course this will NOT fix the original issue Anuj has reported. However, the pdf specification is clear about the fact that "Popup" annotation is something optional. There still can be a popup window for a "Text" annotation without having an explicit "Popup" annotation. Having a "Popup" annotation just provides additional hints for the viewer.
Meaning, evince misbehaves here. If no Popup object is present, then it should just display a popup window where it fits best. If a "Popup" annotation is present for a "Markup" annotation, then it should render the popup windows as instructed in that "Popup" annotation.
Comment 13 Philipp Reinkemeier 2015-02-13 13:14:02 UTC
Germán FYI. I submitted a patch fixing parts of the problems about Popups here: https://bugs.freedesktop.org/show_bug.cgi?id=89136
Comment 14 Philipp Reinkemeier 2015-02-14 16:31:43 UTC
Created attachment 296833 [details] [review]
Always have popup window for Markup annotations

This patch implements exactly the strategy i talked about. If an EvAnnotation is created representing some PopplerAnnot *poppler_annot, then EvAnnotation will now always have a popup if popper_annot is of type PopplerAnnotMarkup.

The idea is simple: Either poppler_annot has some associated Popup annotation, meaning we can fetch a rectangle for its popup window, or it has no Popup annotation. In the latter case, there is a fallback defining a popup window of w:200,h:150, that is put somewhere to the right of poppler_annot. This is also consistent with the behavior of acrobat reader if a Popup annotation is missing.

With this patch, annotations created in okular now have a popup window when clicked on in evince.
Comment 15 Carlos Garcia Campos 2015-02-15 15:20:56 UTC
(In reply to Philipp Reinkemeier from comment #8)

> So concluding: My guess is correct. Storing the popup rectangle property
> confuses acrobat reader. So i would suggest that evince should not store
> this property anymore for text annotations and also should not treat the
> absence of this property as an indication that no popup is present at all.
> 

I know it could be confusing, but poppler_annot_markup_get_popup_rectangle() returns FALSE when the markup annotation doesn't have a popup annotation associated. The rect property of any annotation is a required field, so having a popup rectangle is a requirement for having a popup. I agree, though, that the user should be able to add a popup for a markup annotation that doesn't have one by default.
Comment 16 Carlos Garcia Campos 2015-02-15 15:34:54 UTC
Review of attachment 296833 [details] [review]:

Thanks for the patch. Please, use a single line for the commit message followed by an empty line and then a brief description properly formatted. I have some comments about the patch.

::: backend/pdf/ev-poppler.cc
@@ +2970,3 @@
 
+			gdouble width;
+			gdouble height;

Even though this is a C++ file, we follow a C coding style, so we don't use inline declarations, please move these to the declaration block of the current scope.

@@ +2991,3 @@
 			} else {
+				EvRectangle ev_rect;
+				poppler_annot_get_rectangle(poppler_annot, &poppler_rect);

Leave an empty line between declarations block and body. And a space between function name and (

@@ +2993,3 @@
+				poppler_annot_get_rectangle(poppler_annot, &poppler_rect);
+
+				ev_rect.x1 = (width - 200 > 0) ? (width - 200) : 0;

ev_rect.x1 = MAX (0, width - 200);

@@ +2998,3 @@
+				ev_rect.y2 = ((height - poppler_rect.y2 + 150) <= height) ?
+						(height - poppler_rect.y2 + 150) :
+						height;

ev_rect.y2 = MIN (height - poppler_rect.y2 + 150, height);

@@ +3003,3 @@
+						"rectangle", &ev_rect,
+						"popup_is_open", FALSE,
+						"has_popup", TRUE,

I don't think this approach is correct. Until now, we were only getting the popup annotations that were already part of the document, assuming the document was correct, so we check if there's a popup annotation for any markup annotation, but not all markup annotations can have a popup associated. If we are going to unconditionally add one, we need to check the type. The spec says:

"Most other markup annotations have an associated pop-up window that may contain text. The annotation’s Contents entry specifies the text that shall be displayed when the pop-up window is opened. These include text, line, square, circle, polygon, polyline, highlight, underline, squiggly-underline, strikeout, rubberstamp, caret, ink, and file attachment annotations."

So, for example a free text annotation doesn't have a popup associated. The other problem of this approach is that the popup we are creating here is not added to the document, I'm not sure that's actually a problem, though.
Comment 17 Philipp Reinkemeier 2015-02-15 22:21:55 UTC
I am not sure (In reply to Carlos Garcia Campos from comment #15)
> (In reply to Philipp Reinkemeier from comment #8)
> 
> > So concluding: My guess is correct. Storing the popup rectangle property
> > confuses acrobat reader. So i would suggest that evince should not store
> > this property anymore for text annotations and also should not treat the
> > absence of this property as an indication that no popup is present at all.
> > 
> 
> I know it could be confusing, but poppler_annot_markup_get_popup_rectangle()
> returns FALSE when the markup annotation doesn't have a popup annotation
> associated. The rect property of any annotation is a required field, so
> having a popup rectangle is a requirement for having a popup. I agree,
> though, that the user should be able to add a popup for a markup annotation
> that doesn't have one by default.
Comment 18 Philipp Reinkemeier 2015-02-15 22:46:21 UTC
(In reply to Philipp Reinkemeier from comment #17)
> I am not sure (In reply to Carlos Garcia Campos from comment #15)
> > (In reply to Philipp Reinkemeier from comment #8)
> > 
> > > So concluding: My guess is correct. Storing the popup rectangle property
> > > confuses acrobat reader. So i would suggest that evince should not store
> > > this property anymore for text annotations and also should not treat the
> > > absence of this property as an indication that no popup is present at all.
> > > 
> > 
> > I know it could be confusing, but poppler_annot_markup_get_popup_rectangle()
> > returns FALSE when the markup annotation doesn't have a popup annotation
> > associated. The rect property of any annotation is a required field, so
> > having a popup rectangle is a requirement for having a popup. I agree,
> > though, that the user should be able to add a popup for a markup annotation
> > that doesn't have one by default.

Sorry. Damn touch thing.
I wanted to say that i am not sure i understand what you are after. What i intended to do was just to let markup annotation have a popup window opened when clicked on, no matter whether they have an associated popup annotation or not.
Form reading your comment i understand that you argue that having a popup annotation associated with a markup annotation is a necessary condition for having a popup window shown when clicking in the markup annotation. If so, then i disagree. I cannot find an equivalence between popup annotation and popup window in the spec.
Comment 19 Philipp Reinkemeier 2015-02-15 23:13:33 UTC
(In reply to Carlos Garcia Campos from comment #16)
> Review of attachment 296833 [details] [review] [review]:
> 
> I don't think this approach is correct. Until now, we were only getting the
> popup annotations that were already part of the document, assuming the
> document was correct, so we check if there's a popup annotation for any
> markup annotation, but not all markup annotations can have a popup
> associated. If we are going to unconditionally add one, we need to check the
> type. The spec says:
> 
> "Most other markup annotations have an associated pop-up window that may
> contain text. The annotation’s Contents entry specifies the text that shall
> be displayed when the pop-up window is opened. These include text, line,
> square, circle, polygon, polyline, highlight, underline, squiggly-underline,
> strikeout, rubberstamp, caret, ink, and file attachment annotations."
> 
> So, for example a free text annotation doesn't have a popup associated. The
> other problem of this approach is that the popup we are creating here is not
> added to the document, I'm not sure that's actually a problem, though.

OK, fair enough. Free text and sound annotations will need to be excluded here. I will try to modify the patch in that way and change style + formatting.
Comment 20 Philipp Reinkemeier 2015-02-16 09:52:51 UTC
Created attachment 296910 [details] [review]
Have a popup window for most Markup annotations

I think this revised patch should address the concerns of the review.
Comment 21 Philipp Reinkemeier 2015-02-18 08:17:55 UTC
(In reply to Carlos Garcia Campos from comment #16)
> So, for example a free text annotation doesn't have a popup associated. The
> other problem of this approach is that the popup we are creating here is not
> added to the document, I'm not sure that's actually a problem, though.

Let me comment on the last one about not adding such popups to the document, because i was thinking a bit about that:
Actually, i think it is even better to not store it in the document right away (i suppose by "storing it in the document" you mean calling poppler_annot_markup_set_popup() for the corresponding markup annot, right?).

In the following i will denote such popups "virtual", since they are not stored in the pdf document. Now think about some feature also present in other popular pdf viewers, like moving an EvAnnotationWindow around or resizing it marks the document as unsaved, and upon saving also the popup rectangle is saved. Then not storing a virtual popup right away has the benefit that if the popup window is not moved or resized, then this annotation would be really left untouched. Further, if just the contents of the markup annotation are changed and the document is saved, then it still would not contain a popup annotation. I guess this behavior would be more consistent, because only things are saved to the pdf file that you changed. If the virtual popup is added to the document right away, then no matter what you changed (could also be a completely unrelated annotation or form), if the pdf is saved, then it would also contain a real popup annotation.
Comment 22 Philipp Reinkemeier 2015-02-21 10:58:53 UTC
Btw.: I filed another BUG for the feature sketched above and attached a patch there. See https://bugzilla.gnome.org/show_bug.cgi?id=744886
Comment 23 Philipp Reinkemeier 2015-02-27 20:20:48 UTC
I like to kindly request the same here, just like on the BUG filed against poppler:
I would really appreciate to see more comments on the patch or somebody merging it or whatever.
Note: I think this is really a no-brainer, so not much time needed. Thanks.
Comment 24 Carlos Garcia Campos 2015-03-16 17:51:28 UTC
(In reply to Philipp Reinkemeier from comment #18)
> (In reply to Philipp Reinkemeier from comment #17)
> > I am not sure (In reply to Carlos Garcia Campos from comment #15)
> > > (In reply to Philipp Reinkemeier from comment #8)
> > > 
> > > > So concluding: My guess is correct. Storing the popup rectangle property
> > > > confuses acrobat reader. So i would suggest that evince should not store
> > > > this property anymore for text annotations and also should not treat the
> > > > absence of this property as an indication that no popup is present at all.
> > > > 
> > > 
> > > I know it could be confusing, but poppler_annot_markup_get_popup_rectangle()
> > > returns FALSE when the markup annotation doesn't have a popup annotation
> > > associated. The rect property of any annotation is a required field, so
> > > having a popup rectangle is a requirement for having a popup. I agree,
> > > though, that the user should be able to add a popup for a markup annotation
> > > that doesn't have one by default.
> 
> Sorry. Damn touch thing.
> I wanted to say that i am not sure i understand what you are after. What i
> intended to do was just to let markup annotation have a popup window opened
> when clicked on, no matter whether they have an associated popup annotation
> or not.
> Form reading your comment i understand that you argue that having a popup
> annotation associated with a markup annotation is a necessary condition for
> having a popup window shown when clicking in the markup annotation. If so,
> then i disagree. I cannot find an equivalence between popup annotation and
> popup window in the spec.

"A pop-up annotation (PDF 1.3) displays text in a pop-up window for entry and editing. It shall not appear alone but is associated with a markup annotation, its parent annotation, and shall be used for editing the parent’s text. It shall have no appearance stream or associated actions of its own and shall be  identified by the Popup entry in the parent’s annotation dictionary"
Comment 25 Carlos Garcia Campos 2015-03-16 18:40:08 UTC
Review of attachment 296910 [details] [review]:

Sorry for the delay reviewing this pataches. I think we are in the right direction. Thanks!

::: backend/pdf/ev-poppler.cc
@@ +2837,3 @@
+		case POPPLER_ANNOT_CARET:
+		case POPPLER_ANNOT_INK:
+		case POPPLER_ANNOT_FILE_ATTACHMENT:

This is not correctly indented.

@@ +2838,3 @@
+		case POPPLER_ANNOT_INK:
+		case POPPLER_ANNOT_FILE_ATTACHMENT:
+		return true;

TRUE

@@ +2840,3 @@
+		return true;
+		default:
+		return false;

FALSE

@@ +3026,3 @@
+					      "rectangle", &ev_rect,
+					      "popup_is_open", FALSE,
+					      "has_popup", TRUE,

I see how doing this here is more convenient, but I think this should be done in the view, not in the backend. In ev_view_annotation_show_popup_window() handle the case of window being NULL and create a new window there if the annotation allows it. Maybe we could add a new property to markup annotations "can-have-popup" for example, and set it here to TRUE/FALSE.
Comment 26 Philipp Reinkemeier 2015-03-18 19:47:42 UTC
Created attachment 299754 [details] [review]
Added can-have-popup property to EvAnnotationMarkups.

This patch adds a property can-have-popup to EvAnnotationMarkup. When
the pdf backend creates EvAnnotations from poppler pdf annotations,
this property is set accordingly (following the PDF spec).
Comment 27 Philipp Reinkemeier 2015-03-18 19:48:38 UTC
Created attachment 299755 [details] [review]
Create an EvAnnotationWindow for EvAnnotationMarkups allowing this.

Depending on the property can-have-popup of EvAnnotationMarkup,
EvView will dynamically create an EvAnnotationWindow with some
default dimension and location, if no EvAnnotationWindow existed
when creating the EvAnnotationMarkup from the backend.
Comment 28 Philipp Reinkemeier 2015-03-18 19:52:20 UTC
Just a comment: I tried to follow your suggestion. However doing this in ev_view_annotation_show_popup_window() handle is not possible, since there you just have access to the EvView instance, but not to the EvAnnotation the user clicked on.
Therefore i implemented this logic in ev_view_handle_annotation(), which has access to the EvAnnotation and calls ev_view_annotation_show_popup_window() after it fetched the window or created a fresh one.
Comment 29 Carlos Garcia Campos 2015-04-12 09:42:24 UTC
Review of attachment 299754 [details] [review]:

I've split the patch in two and pushed to git master with my sugestions addressed. Thanks, and sorry again for the delay reviewing this.

::: backend/pdf/ev-poppler.cc
@@ +2838,3 @@
+		case POPPLER_ANNOT_INK:
+		case POPPLER_ANNOT_FILE_ATTACHMENT:
+			return true;

TRUE

@@ +2840,3 @@
+			return true;
+		default:
+			return false;

FALSE

::: libdocument/ev-annotation.c
@@ +905,3 @@
+gboolean
+ev_annotation_markup_set_can_have_popup (EvAnnotationMarkup *markup,
+				    gboolean            can_have_popup)

This property is a special case, we need it to be writable so that backends can set the value, but we don't expect views to be able to change this. So, I would remove the public method to set the property to avoid confusions
Comment 30 Carlos Garcia Campos 2015-04-12 09:45:15 UTC
Review of attachment 299755 [details] [review]:

ev_view_handle_annotation() looks indeed like a better place for this. Pushed slightly modified version. Thanks!

::: libview/ev-view.c
@@ +3143,3 @@
+			g_object_get (annot,
+							"can_have_popup", &can_have_popup,
+							NULL);

All this could be simplified by doing something like:

if (!window && ev_annotation_markup_can_have_popup())

@@ +3149,3 @@
+
+				popup_rect.x1 = 0;
+				popup_rect.y1 = 0;

I don't think 0,0 is the best place to show the popup, we could use the annot x2, y2 coords to show the popup below the annotation.

@@ +3151,3 @@
+				popup_rect.y1 = 0;
+				popup_rect.x2 = popup_rect.x1 + 200;
+				popup_rect.y2 = popup_rect.y1 + 150;

We should give a name to these magic numbers