GNOME Bugzilla – Bug 649043
Ability to move annotations
Last modified: 2015-06-20 09:21:16 UTC
It should be possible to reposition existing annotations by dragging the annotation icon.
I was getting a look how this can be implemented, at least for PDF backend. It seems that PopplerAnnot does not have a way to move it. The rectangle is defined in pdf_document_annotations_add_annotation(). I don't know anything on the libpoppler's internal, should this be implemented by adding poppler_annot_move() function and use it in pdf_document_annotations_save_annotation() ?
Yes we need to add new API to poppler first, maybe poppler_annot_set_rectangle() or poppler_annot_move() as you suggests.
Created attachment 304846 [details] [review] [PATCH 1/2] libview: Move ev_view_handle_annotation from button press to button release. This moves the call to ev_view_handle_annotation from the callback ev_view_button_press_event to ev_view_button_release_event. This is a preparation for implementing the ability to move annotations.
Created attachment 304847 [details] [review] [PATCH 2/2] libview: Implement support for moving text annotations. This adds support for moving text annotations by simply dragging them to a new location.
The two patches above implement support for moving annotations by continuously updating the area of an annotation while dragging it. Basically it uses the same logic as interactive annotations, thus suffering from the same performance penalties. On the other hand, if there is a way to improve performance, then it will probably apply to both (moving annotations and creating interactive annotations). While in principle the logic implemented by the patch also works for other types of annotations, i restricted it to text annotations only. The corresponding check in ev_view_button_press_event can be weakened to allow other types of annotations to be moved.
Philipp, fyi my proposal for improving the performance is explained in https://bugs.freedesktop.org/show_bug.cgi?id=83643 It amounts to not save the annotation in the Document until the drag has stopped and use a new poppler function to render the annotation over the top of the page.
Thanks Jose. I looked into it and especially how this API is used in the wip/highlight branch in this commit https://git.gnome.org/browse/evince/commit/?h=wip/highlight&id=ed6df7c79d98012311598890e546967780829e18. From the changes in ev_view_motion_notify_event and ev_view_button_release_event, i think that would i conjectured above is correct. The same approach can be used for improving performance of moving annotation and creating interactive annotations. So the attached patches could be merged and the relevant patches from wip/highlight applied later on, once the API is available in poppler, right? What do you think?
Review of attachment 304846 [details] [review]: ::: libview/ev-view.c @@ -5001,3 @@ ev_view_handle_media (view, media); - } else if ((annot = ev_view_get_annotation_at_location (view, event->x, event->y))) { - ev_view_handle_annotation (view, annot, event->x, event->y, event->time); This is problematic, because we want annots to take precedence over other things, even if we don't do anything here, we don't want the other ifs to be checked. I see this is indeed fixed in the other commit, so I would probably merge both commits to avoid this inconsistent state.
Review of attachment 304847 [details] [review]: Looks good, but there are still a couple of things to fix. ::: libview/ev-view-private.h @@ +127,3 @@ + gboolean annot_move_started; + gboolean moving_annot; + EvAnnotation *annot; Do not line up members of this struct with other structs. ::: libview/ev-view.c @@ +5022,3 @@ + // jumping while moving it. + view_point.x = event->x + view->scroll_x; + view_point.y = event->y + view->scroll_y; You need to save this view point too in the struct, in order to decide in the motion notify if you have moved enough to consider it a move or not. @@ +5442,3 @@ + return TRUE; + + view->moving_annot_info.moving_annot = TRUE; We don't want to start a move, if we haven't moved enough pixels, to prevent involuntary moves, specially in though screens it's very easy to cause a motion just when tapping. You can use gtk_drag_check_threshold() for that, to compare the start point (saved in button_press) with the current point. @@ +5452,3 @@ + &doc_point.x, &doc_point.y); + + rect.x1 = MAX(0, doc_point.x - view->moving_annot_info.cursor_offset.x); MAX( -> MAX ( @@ +5453,3 @@ + + rect.x1 = MAX(0, doc_point.x - view->moving_annot_info.cursor_offset.x); + rect.y1 = MAX(0, doc_point.y - view->moving_annot_info.cursor_offset.y); Ditto. @@ +5652,3 @@ } + if (view->moving_annot_info.moving_annot) { We should also handle the case when we release without moving the annot, moving_annot is FALSE, but move_annot_started is TRUE.
(In reply to Philipp Reinkemeier from comment #7) > Thanks Jose. I looked into it and especially how this API is used in the > wip/highlight branch in this commit > https://git.gnome.org/browse/evince/commit/?h=wip/ > highlight&id=ed6df7c79d98012311598890e546967780829e18. > From the changes in ev_view_motion_notify_event and > ev_view_button_release_event, i think that would i conjectured above is > correct. The same approach can be used for improving performance of moving > annotation and creating interactive annotations. > > So the attached patches could be merged and the relevant patches from > wip/highlight applied later on, once the API is available in poppler, right? > What do you think? Right
Created attachment 305270 [details] [review] libview: Implement support for moving text annotations. - Merged the previous patches into a single patch. - Fixed code style issues. - Renamed member annot_moved_started of struct MovingAnnotInfo to annot_clicked. - Added new member start to struct MovingAnnotInfo and based on that, implemented a threshold for moving annotations. - Fixed incomplete handling of combinations of values of annot_clicked and moving_annot in ev_view_button_release_event.
Review of attachment 305270 [details] [review]: Looks good, I have only few more comments. Thanks! ::: libview/ev-view.c @@ +5018,3 @@ + // on an annotation. We need moving_annot + // to distinguish moving an annotation from + // showing its popup upon button release. Sorry that I missed it in my previous review, but please, do not use C++ style comments, use /* */ instead. @@ +5443,3 @@ ev_view_reload_page (view, annot_page, NULL); + } else if (view->moving_annot_info.annot_clicked) { + GdkPoint view_point; Extra spaces between GdkPoint and view_point @@ +5456,3 @@ + view->moving_annot_info.start.y, + view_point.x, + view_point.y); We could probably return early here. Something like: if (!moving_annot) { if (!gtk_drag_check_threshold ()) return TRUE; moving_annot = TRUE; } And then you don't need to check moving_annot again and you save an indentation level. @@ +5477,3 @@ + rect.y1 = MAX (0, doc_point.y - view->moving_annot_info.cursor_offset.y); + rect.x2 = rect.x1 + current_area.x2 - current_area.x1; + rect.y2 = rect.y1 + current_area.y2 - current_area.y1; We should clamp the rectangle to the document page, since it's currently possible to place the annotation off the page. Try moving an annot in continuous mode, you can release it in the next page (even if it's not rendered), but the annot is saved with wrong rectangle. @@ +5485,3 @@ + ev_document_annotations_save_annotation (EV_DOCUMENT_ANNOTATIONS (view->document), + view->moving_annot_info.annot, + EV_ANNOTATIONS_SAVE_AREA); Funny thing is that for annotations having an appearance stream, changing the rectangle invalidates the appearance stream, so as soon as you start moving them, their icon changes the to the default appearance. I'm not sure why poppler does that, but for now, maybe we should only allow moving annots created by evince itself, because I don't think it's possible to know from evince whether the annot has an AP or not @@ +5677,3 @@ + if (view->moving_annot_info.annot_clicked) { + view->moving_annot_info.annot_clicked = FALSE; + view->moving_annot_info.annot = NULL; We could handle the click here, I think, for this particular case. If it was clicked, but not moved, we already have the annot, so we don't need to get the annot as current location again. @@ +5685,3 @@ + + return FALSE; + } We would handle the click here for this annot, then we just set pressed_button to -1. @@ +5691,3 @@ + EvAnnotation *annot; + + if ((annot = ev_view_get_annotation_at_location (view, event->x, event->y))) { Since there's a single if branch here, I prefer to get the annot first, and then the if
Created attachment 305620 [details] [review] libview: Implement support for moving text annotations. This revised patch should address all issues identified in Comment 12 but one: The issue about appearance streams. I think implementing something like a filter so that only annotations created in evince can be moved, is not a good idea. I guess more people will complain that they cannot move an annotation in a PDF, which has been sent to them, than people complaining about appearance streams being invalidated. But that's just my two cents.
(In reply to Philipp Reinkemeier from comment #13) > Created attachment 305620 [details] [review] [review] > libview: Implement support for moving text annotations. > > This revised patch should address all issues identified in Comment 12 but > one: > The issue about appearance streams. I think implementing something like a > filter so that only annotations created in evince can be moved, is not a > good idea. > I guess more people will complain that they cannot move an annotation in a > PDF, which has been sent to them, than people complaining about appearance > streams being invalidated. But that's just my two cents. I don't think Carlos is against this. However, IIUC before moving other annotations we would need to fix poppler to not set to the default icon appearance any time an annotation is modified, and keep the icon already assigned to such annotation. We will get a complain either way.
I think that the behaviour in poppler is a bug. All the streams are being invalidated in setRect because the BBox is being changed... We should fix the problem in poppler and AFAIK we shouldn't need any API change.
After having a quick look at the PDF spec. i would tend to agree that this is a BUG in poppler. At first i thought that maybe an AP, if present, overrides the rectangle, which would have been a problem. But in Section "12.5.5 Appearance Streams" the following is written: "Each appearance stream is a form XObject (see 8.10, “Form XObjects”): a self-contained content stream that shall be rendered inside the annotation rectangle." This statement is followed by an algorithm for transforming the BBox to "match" with the annotation rectangle. Directly after the algorithm there is following sentence: "The annotation may be further scaled and rotated if either the NoZoom or NoRotate flag is set (see 12.5.3, “Annotation Flags”). Any transformation applied to the annotation as a whole shall also applied to the appearance within it." So if one also considers changing the rectangle as a "transformation applied to the annotation", then poppler does it wrong and should handle this case preserving the appearance.
Philipp, I think we need to move the discussion to Poppler's bugzilla. Could you please file a bug in bugs.freedesktop.org?
Comment on attachment 305620 [details] [review] libview: Implement support for moving text annotations. Excellent, pushed.
(In reply to José Aliste from comment #15) > I think that the behaviour in poppler is a bug. All the streams are being > invalidated in setRect because the BBox is being changed... We should fix > the problem in poppler and AFAIK we shouldn't need any API change. I've checked why we do that, and git blame blamed me :-P http://cgit.freedesktop.org/poppler/poppler/commit/?id=c376559b The idea is to invalidate the AP when a property that affects the appearance is updated. Actually I just moved that behaviour from the qt frontend to the core. So, the point is whether the annot rectangle affects its appearance or not. It's clear it doesn't for text annotations, and removing that line makes moving annots work perfectly in evince, keeping the original annotation AP. But I'm not sure that's true for all other annots.
(In reply to José Aliste from comment #17) > Philipp, I think we need to move the discussion to Poppler's bugzilla. Could > you please file a bug in bugs.freedesktop.org? Yes, please let's discuss this in the poppler bugzilla or mailing list.