GNOME Bugzilla – Bug 744886
Changes in Popup window rectangle are not saved to PDF file
Last modified: 2018-05-22 16:08:44 UTC
Markup annotations like Subtype \Text have a popup window when clicked on in evince. When that popup window is moved or resized by the the user its new coordinates should be saved to the PDF file when saving a copy. Steps to reproduce: 1. Annotate a PDF and save a copy. 2. Close the PDF file. 3. Open the copy with the annotation. 4. Move the annotation popup window to a different location. 5. Save another copy of the PDF file. 6. Open that copy. Expected: When clicking on the annotation, the popup window should be shown at the location it has been moved to. Actual: When clicking on the annotation, the popup window is shown in the same place as in step 1.
Placement of a popup window can be controlled by means of an annotation of Subtype \Popup. The problem is that unlike contents of an annotation, changes to the coordinates of a popup window are not saved in the pdf backend of evince.
Created attachment 297484 [details] [review] Add support to save popup rectangles to pdf backend This patch adds support in the PDF backend of evince to save a popup rectangle to the PopplerAnnot underlying an EvAnnotation.
Created attachment 297485 [details] [review] Save popup rectangle when EvAnnotationWindow is moved This patch replies on the former by syncing new coordinates of an EvAnnotationWindow to its EvAnnotation. Similarly to the way annotation contents are saved to the pdf backend, the EvView listens to changes of the rectangle of an EvAnnotation and saves it to the pdf backend. Note: I moved some code out of ev_annotation_window_focus_in_event() to a new method ev_annotation_window_notify_if_moved(), since unfortunately the "focus-in" event is NOT fired when a move operation initiated by gtk_window_begin_resize_drag() is finished. This would only work for gtk windows having decorations. So the logic to potentially fire the "moved" signal of an EvAnnotationWindow is called at some more places, where a move cannot be in progress anymore.
Review of attachment 297484 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +3334,3 @@ + poppler_rect.y2 = height - ev_rect.y1; + + poppler_annot_markup_set_popup (markup, &poppler_rect); This creates a new Popup object internally in poppler, so I think we should always call here poppler_annot_markup_set_popup_is_open() as well, to ensure that this is also in sync.
Review of attachment 297485 [details] [review]: ::: libview/ev-annotation-window.c @@ +203,3 @@ if (window->annotation) { ev_annotation_window_sync_contents (window); + ev_annotation_window_notify_if_moved (window); Can the window be in move while disposed? I think you need this here because the current way we notify about window moved is buggy. I don't remember why we do this in focus in. I don't know why we don't use button release. @@ +536,2 @@ ev_annotation_window_sync_contents (window); + ev_annotation_window_notify_if_moved (window); Instead of this, I think we should probably have a different method, sync_position or something like that that saves the rectangle, like we do with the contents. The view could connect to notify::rectangle signal. @@ +662,2 @@ window->rect = *rect; + if (EV_IS_ANNOTATION_MARKUP (annot)) { Only markup annotations can have a popup window, so this must be a markup annot. @@ +664,3 @@ + EvAnnotationMarkup *ev_markup = EV_ANNOTATION_MARKUP (annot); + ev_annotation_markup_set_rectangle (ev_markup, rect); + } It seems ev_annotation_window_set_rectangle is dead code, nobody seems to call this. ::: libview/ev-view.c @@ +2953,3 @@ child->orig_y = doc_rect.y1; + + ev_annotation_window_set_rectangle (window, &doc_rect); Ah, I see, you do this here, because we need the doc_rect, it makes sense...
(In reply to Carlos Garcia Campos from comment #4) > Review of attachment 297484 [details] [review] [review]: > > ::: backend/pdf/ev-poppler.cc > @@ +3334,3 @@ > + poppler_rect.y2 = height - ev_rect.y1; > + > + poppler_annot_markup_set_popup (markup, &poppler_rect); > > This creates a new Popup object internally in poppler, so I think we should > always call here poppler_annot_markup_set_popup_is_open() as well, to ensure > that this is also in sync. Hm. This doesn't make sense to me. You have the parameter EvAnnotationsSaveMask passed into this pdf_document_annotations_save_annotation() function. Directly after the changes i added, there already are the following lines of code: if (mask & EV_ANNOTATIONS_SAVE_POPUP_IS_OPEN) poppler_annot_markup_set_popup_is_open (markup, ev_annotation_markup_get_popup_is_open (ev_markup)); So the open-state of the popup is synced if requested by the EvAnnotationsSaveMask. So always calling poppler_annot_markup_set_popup_is_open() when saving the popup rectangle would contradict the semantics of the EvAnnotationsSaveMask, correct?
Created attachment 299758 [details] [review] Save popup rectangle when EvAnnotationWindow is moved Currently, only the contents of an annotation are saved when changed in the corresponding EvAnnotationWindow. This patch causes also the popup rectangle to be saved, when the corresponding EvAnnotationWindow is moved.
(In reply to Carlos Garcia Campos from comment #5) > Review of attachment 297485 [details] [review] [review]: > > ::: libview/ev-annotation-window.c > @@ +203,3 @@ > if (window->annotation) { > ev_annotation_window_sync_contents (window); > + ev_annotation_window_notify_if_moved (window); > > Can the window be in move while disposed? I think you need this here because > the current way we notify about window moved is buggy. I don't remember why > we do this in focus in. I don't know why we don't use button release. Please see my comment 3. Buttom release also does not work for a move operation of a window without decorations initiated by gtk_window_begin_resize_drag(). Tested that with some minimal example: No way. Having said that, i would say this is a BUG in GTK. One can create a minimal example showing this problem. > > @@ +536,2 @@ > ev_annotation_window_sync_contents (window); > + ev_annotation_window_notify_if_moved (window); > > Instead of this, I think we should probably have a different method, > sync_position or something like that that saves the rectangle, like we do > with the contents. The view could connect to notify::rectangle signal. Hm. That's contained in the patch. Did you missed something? > > @@ +662,2 @@ > window->rect = *rect; > + if (EV_IS_ANNOTATION_MARKUP (annot)) { > > Only markup annotations can have a popup window, so this must be a markup > annot. Ok. Removed the if. > > @@ +664,3 @@ > + EvAnnotationMarkup *ev_markup = EV_ANNOTATION_MARKUP (annot); > + ev_annotation_markup_set_rectangle (ev_markup, rect); > + } > > It seems ev_annotation_window_set_rectangle is dead code, nobody seems to > call this. > > ::: libview/ev-view.c > @@ +2953,3 @@ > child->orig_y = doc_rect.y1; > + > + ev_annotation_window_set_rectangle (window, &doc_rect); > > Ah, I see, you do this here, because we need the doc_rect, it makes sense...
(In reply to Philipp Reinkemeier from comment #6) > (In reply to Carlos Garcia Campos from comment #4) > > Review of attachment 297484 [details] [review] [review] [review]: > > > > ::: backend/pdf/ev-poppler.cc > > @@ +3334,3 @@ > > + poppler_rect.y2 = height - ev_rect.y1; > > + > > + poppler_annot_markup_set_popup (markup, &poppler_rect); > > > > This creates a new Popup object internally in poppler, so I think we should > > always call here poppler_annot_markup_set_popup_is_open() as well, to ensure > > that this is also in sync. > > Hm. This doesn't make sense to me. You have the parameter > EvAnnotationsSaveMask passed into this > pdf_document_annotations_save_annotation() function. Directly after the > changes i added, there already are the following lines of code: > > if (mask & EV_ANNOTATIONS_SAVE_POPUP_IS_OPEN) > poppler_annot_markup_set_popup_is_open (markup, > ev_annotation_markup_get_popup_is_open (ev_markup)); > > So the open-state of the popup is synced if requested by the > EvAnnotationsSaveMask. So always calling > poppler_annot_markup_set_popup_is_open() when saving the popup rectangle > would contradict the semantics of the EvAnnotationsSaveMask, correct? Actually, I think that what we want is to add a popup when the annot doesn't one already, and update the rect when it has. So we need poppler_annot_markup_set_popup_rectangle() in poppler.
See http://cgit.freedesktop.org/poppler/poppler/commit/?id=056ee89122385bc2df7cb2c05e1cb1770af8ecce.
Comment on attachment 297484 [details] [review] Add support to save popup rectangles to pdf backend Pushed to git master, as well as a follow up patch to use poppler_annot_markup_set_popup_rectangle() if available.
See also bug #770012.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/570.