GNOME Bugzilla – Bug 763943
Allow Adding annotations by right click on selection
Last modified: 2018-05-22 16:35:31 UTC
Right now to annotate a text, The user have to go to Annotate the document->Add highlight annotation and select the text. It would be easier for the user if its possible to add annotations by selecting the text and selecting the option from right click.
Indeed - and might as well make it also work by selecting the text then pressing the button, as that might be some users' intuition or preferred workflow. Currently it seems very unintuitive that by selecting text then pressing the button, this doesn't annotate the text you previously had selected, but instead demands that you select it again...
I have also seen many users that try to select text first and then try to add a highlight annotation by clicking on the Toogle-Button in the menubar... So +1 to change this...
Created attachment 365303 [details] [review] Fix add_annotation() to update area based on bounding box for Markup annotations, same way this is done if using save_annotation(). When pdf_document_annotations_save_annotation() saves a Markup annotation it will, prior to saving, update the area of annotation to be based on its bounding box. But this was not being done if we just used pdf_document_annotations_add_annotation() (because we currently add annotations via a DnD operation that involves both an initial add_annotation() plus subsequent save_annotation() calls on motion_notify). So this fix is needed to be able to correctly create a Markup annotation by a single call to pdf_document_annotations_add_annotation()
Created attachment 365304 [details] [review] Allow adding markup annotations from text selection As this is a natural, intuitive way to add markup annotations to a document. The "Annotate Selected Text" action will show on contextual menu when there is a text selection on current document page. If a text selection crosses two pages we will add both annotations accordingly.
Created attachment 365305 [details] Screencast of adding annotation to text selection on Evince This is a small screencast of Evince with this feature implemented. Also, what do you think about this possible improvement on it: Providing a right click submenu under "Annotate Selected Text" with four options ("highlight","strike out", "underline", "squiggly") so we shortcut the creation of these markup annotations. And I would default the color of "strike out" and "squiggly" to be red and "underline" to be blue.
Review of attachment 365303 [details] [review]: OK, but note that Markup annot is not the same as Text Markup annot. Here you mean Text Markup annots, so please update the commits message and the comments to refer to text markup (or just highlight annots)
Review of attachment 365304 [details] [review]: ::: libview/ev-view.c @@ +5642,3 @@ + * + * If @crosspage is %TRUE, it will add a similar annotation for the + * selected text (if any) on the previous or following page of @page. This is confusing. What is selection covers more than 3 pages? Annotations need to be added to a particular page, this is clear when using the dnd mode, because you can't mark in two pages a t the same time. I don't think it's a good idea to do a different thing here, the user might think it's the same annotation, while it's not. Maybe we could call the action "Annotate selected text in current page", so the user has to do the same in every page with selections to mark all pages. I guess we shouldn't clear selections in that case when annotating. ::: shell/ev-window.c @@ +333,3 @@ GVariant *parameter, gpointer user_data); +static void ev_window_popup_cmd_annot_selected_text (GSimpleAction *action, annot -> annotate @@ +5760,3 @@ { "toggle-edit-annots", NULL, NULL, "false", ev_window_cmd_toggle_edit_annots }, /* Popups specific items */ + { "annot-selected-text", ev_window_popup_cmd_annot_selected_text }, annotate-selected-text
Created attachment 365892 [details] Adobe Reader crosspage annotation (In reply to Carlos Garcia Campos from comment #7) > Review of attachment 365304 [details] [review] [review]: > > ::: libview/ev-view.c > @@ +5642,3 @@ > + * > + * If @crosspage is %TRUE, it will add a similar annotation for the > + * selected text (if any) on the previous or following page of @page. > > This is confusing. What is selection covers more than 3 pages? Annotations > need to be added to a particular page, this is clear when using the dnd > mode, because you can't mark in two pages a t the same time. I don't think > it's a good idea to do a different thing here, the user might think it's the > same annotation, while it's not. Maybe we could call the action "Annotate > selected text in current page", so the user has to do the same in every page > with selections to mark all pages. I guess we shouldn't clear selections in > that case when annotating. > I think is a more common case that a user wants to annotate a chunk of text that crosses two pages, than the hipotetical >=3 pages case. If the text selection spreads to more than 2 pages we can simply 'not show' the "annotate text" action in the menu. I attached a webm video showing that Adobe Reader supports this, allowing user to annotate a crosspage text (by creating two annotations). Imho (and according to Adobe Reader) this is what the user expects (his text selection to be annotated), if we had to create two annotations for that is in fact a help to the user. * Adobe Reader actually don't have a limit in this, if you have a text selection spreading on 5 pages it will annotate all of that, adding the corresponding 5 annotations. But I think is fine we don't support this as it's a rare case that people wants to annotate several full pages, but supporting text crossing two pages I think is more common and we should support as Reader does.
Created attachment 365893 [details] [review] Fix add_annotation() to update area based on bounding box for Markup annotations, same way this is done if using save_annotation(). When pdf_document_annotations_save_annotation() saves a Markup annotation it will, prior to saving, update the area of annotation to be based on its bounding box. But this was not being done if we just used pdf_document_annotations_add_annotation() (because we currently add annotations via a DnD operation that involves both an initial add_annotation() plus subsequent save_annotation() calls on motion_notify). So this fix is needed to be able to correctly create a Markup annotation by a single call to pdf_document_annotations_add_annotation() Part of https://bugzilla.gnome.org/show_bug.cgi?id=763943
Created attachment 365894 [details] [review] Allow adding Text Markup annotations from text selection As this is a natural, intuitive way to add text markup annotations to a document. The "Annotate Selected Text" action will show on contextual menu of a page when there is a text selection on it. The action will be hidden if the selection spreads over more than 2 pages. If text selection spreads over two pages only, (which can be a common case) we will add both annotations accordingly to meet the user expectation of selected text to be annotated. https://bugzilla.gnome.org/show_bug.cgi?id=763943
Created attachment 365895 [details] Evince annotating text selection on dual page view Carlos, the new revision of my patches incorporate the following: - your review comments wrt everything but the crosspage feature. - I kept the crosspage feature but limited to two page spread selection, if the selection spreads more than 2 pages then the action won't show up on the context menu. - My previous patch was working on just the current document page, so was not showing the "Annotate text selection" action for text selected on partially shown pages that are not the current page. Same problem happened also in the dual page view were only the current page could be annotated. I fixed it by saving the page that the context menu was triggered on, so we know on what page to act on later when the action is clicked. I attached a video showing how annotating pages on a dual page view works well now. If after testing my new revision you still don't like the crosspage feature for inclusion, let me know and I'll re-do the patch without it. :-)
Comment on attachment 365893 [details] [review] Fix add_annotation() to update area based on bounding box Sorry for the delay, this looks good to me.
Review of attachment 365894 [details] [review]: Ok, let's try this way. I still have a few comments, though. ::: libview/ev-view.c @@ +5670,3 @@ + * currently selected text on @page . + * + * If @crosspage is %TRUE, it will add a similar annotation for the Since we are the only callers of this, I prefer to not expose this and assume it's always TRUE. We should just document that this is limited to 3 pages. ::: shell/ev-window.c @@ +5125,3 @@ + ev_window->priv->page_under_view_popup = page; + has_text_selected = !has_annot && ev_view_get_has_selection_in_page (view, page); + n_pages_selected = ev_view_get_n_pages_with_selection (view); I think we can simplify this by moving this logic to the view. The 3 page limitation is a view implementation detail, and we could even change it in the future. We could simply add ev_view_can_annotate_current_selection() (or can_add_text_markup_annotation_for_selected_text() for consistency), for example, and then we don't need to expose find_page_at_location + has_selection_in_page + n_pages_with_selection.
Created attachment 368867 [details] [review] Allow adding Text Markup annotations from text selection
(In reply to Carlos Garcia Campos from comment #13) > Review of attachment 365894 [details] [review] [review]: > > Ok, let's try this way. I still have a few comments, though. Thank you Carlos, and sorry for being late in updating the patch, I've been ill and busy with family matters. > ::: libview/ev-view.c > @@ +5670,3 @@ > + * currently selected text on @page . > + * > + * If @crosspage is %TRUE, it will add a similar annotation for the > > Since we are the only callers of this, I prefer to not expose this and > assume it's always TRUE. We should just document that this is limited to 3 > pages. Done. > ::: shell/ev-window.c > @@ +5125,3 @@ > + ev_window->priv->page_under_view_popup = page; > + has_text_selected = !has_annot && ev_view_get_has_selection_in_page (view, > page); > + n_pages_selected = ev_view_get_n_pages_with_selection (view); > > I think we can simplify this by moving this logic to the view. The 3 page > limitation is a view implementation detail, and we could even change it in > the future. We could simply add ev_view_can_annotate_current_selection() (or > can_add_text_markup_annotation_for_selected_text() for consistency), for > example, and then we don't need to expose find_page_at_location + > has_selection_in_page + n_pages_with_selection. Done, except for find_page_at_location, I would like to preserve this, because otherwise EvWindow cannot find what page we opened the pop-up menu under (for the annotate text action) and this can be tricky in dual-mode where 4 pages are shown and you can open popup menu in one page but be displayed over another. Looking forward to your comments. :-)
Nelson, I don't want to overstep here (and the last patch looks good!) but why are we restricting ourselves to two page selection? Since we are doing the markup_annotation_for_selected_text in the view, we could just iterate over the selection and add an annotation for each page in the selection no? Or was it a limitation of the implementation when the patch was in the ev-window ?
(In reply to José Aliste from comment #16) > Nelson, I don't want to overstep here (and the last patch looks good!) but > why are we restricting ourselves to two page selection? Since we are doing > the markup_annotation_for_selected_text in the view, we could just iterate > over the selection and add an annotation for each page in the selection no? > Or was it a limitation of the implementation when the patch was in the > ev-window ? It was a preference because it's the minimum required to support a cross-page annotation, which is a common user case I wanted to support, but certainly it's feasible to support any number of pages in the selection as you pointed out. It's up to you and Carlos whether you want to support the minimum (two pages) or all the selection, if it's the later I'll happily rework the patch. thank you for your feedback.
(In reply to Nelson Benitez from comment #17) > (In reply to José Aliste from comment #16) > > Nelson, I don't want to overstep here (and the last patch looks good!) but > > why are we restricting ourselves to two page selection? Since we are doing > > the markup_annotation_for_selected_text in the view, we could just iterate > > over the selection and add an annotation for each page in the selection no? > > Or was it a limitation of the implementation when the patch was in the > > ev-window ? > > It was a preference because it's the minimum required to support a > cross-page annotation, which is a common user case I wanted to support, but > certainly it's feasible to support any number of pages in the selection as > you pointed out. > > It's up to you and Carlos whether you want to support the minimum (two > pages) or all the selection, if it's the later I'll happily rework the patch. > > thank you for your feedback. We are only 5 days away from the hard code freeze, so I don't think this is going to make it into 3.28. To make it into 3.28 we would need to ask for a Feature, UI and String break! So I would prefer that you rework the patch to not call find_selection_for_page and do a direct loop on the selections (right now we are looping twice over the selections) and add one annotation for each page. Then we can merge this early in master after we branch (I'll branch after hard code freeze begins and we release 3.27.92, which should be on March 5th). BTW, I also noticed that we can't do cross page highlighting when doing it by dragging... Fixing this would be much more difficult and not completely related, so I will add a new bug for that.
(In reply to José Aliste from comment #18) > (In reply to Nelson Benitez from comment #17) > > (In reply to José Aliste from comment #16) > > > Nelson, I don't want to overstep here (and the last patch looks good!) but > [snip] > We are only 5 days away from the hard code freeze, so I don't think this is > going to make it into 3.28. To make it into 3.28 we would need to ask for a > Feature, UI and String break! > > So I would prefer that you rework the patch to not call > find_selection_for_page and do a direct loop on the selections (right now we > are looping twice over the selections) and add one annotation for each page. > Then we can merge this early in master after we branch (I'll branch after > hard code freeze begins and we release 3.27.92, which should be on March > 5th). > > BTW, I also noticed that we can't do cross page highlighting when doing it > by dragging... Fixing this would be much more difficult and not completely > related, so I will add a new bug for that. Fine, I will re-do the patch this weekend.
Created attachment 369547 [details] [review] Allow adding Text Markup annotations from text selection As this is a natural, intuitive way to add text markup annotations to a document. The "Annotate Selected Text" action will show on contextual menu of a page when there is a text selection on it. If text selection spreads over several pages, one annotation per page will be added accordingly to fulfill the user expectation of all selected text to be annotated. --- Jose, looking forward to your review. This should be committed along the other patch already approved in this bug.
Review of attachment 369547 [details] [review]: Nelson, this looks good to me. I just have some nitpicks before we can merge this in. Also, something taht can be after this patch (and it's related with the shortcut) is the behavior of the Highlight button. In Inskscape and other image editros, the zoom to selection buttons have two different effects. If you have a selection, then it zooms to the selection. If you don't have a selection, then it lets you select a region and then just after that it will zoom to the new selection.... Mimmicking this could be natural enough... So, the shortcut and the highlight button could do the following. If there is no selection, they enter highlighting mode as now, if there is a selection, they do what your patch does and just transform the selection into a annotation. What other people think about this? ::: libview/ev-view.c @@ +5669,3 @@ + * Returns: %TRUE if annotations were added successfully, %FALSE otherwise. + * + * the currently selected text on the document. I guess the since here should be 3.30. I also wonder, since we are adding new API, whether we should at this point add a second parameter for other types, and maybe even a third parameter for color? ... Since we are adding this very early in the cycle, we can just add it as it is, and modify it later. ::: shell/evince-menus.ui @@ +298,3 @@ </item> + <item> + <attribute name="label" translatable="yes">Annotate Selected Text</attribute> Wouldn't be better to call it "Highlight Selected Text"?
(In reply to José Aliste from comment #21) > the shortcut and the highlight button could do the following. If there is no > selection, they enter highlighting mode as now, if there is a selection, > they do what your patch does and just transform the selection into a > annotation. > > What other people think about this? I'd go for it! My Comment 1 and Fabian's following Comment 2 suggested that, even. :)
Jose, I agree with all your points in comment 21 . Will re-do patch to add parameters and the string change. One idea I had, was to have a submenu with three options: Annotate selected text -> highlight (in yellow) strikethrough (in red) underline (in blue) do you like?
(In reply to Nelson Benitez from comment #23) > Jose, I agree with all your points in comment 21 . Will re-do patch to add > parameters and the string change. > > One idea I had, was to have a submenu with three options: > > Annotate selected text -> highlight (in yellow) > strikethrough (in red) > underline (in blue) > > do you like? Nelson, I wouldn't do that right now. I think having a context menu with a submenu is maybe too much...(and I want to rework the popup menu) Take a look at https://github.com/husain3/evince. Maybe we can add some of this to the annotation toolbar :) plus the modifications to the actions in the button. I just pushed your patches with minor modifications to master.
Review of attachment 369547 [details] [review]: ::: libview/ev-view.c @@ +5669,3 @@ + * Returns: %TRUE if annotations were added successfully, %FALSE otherwise. + * + * Since: 3.28 the Since should be 3.30 or whatever our new version will be ::: shell/evince-menus.ui @@ +298,3 @@ </item> + <item> + <attribute name="label" translatable="yes">Annotate Selected Text</attribute> I don't know if Carlos reviewed this string before, but this should probably be "Highlight selected text"
Review of attachment 365893 [details] [review]: good
(In reply to José Aliste from comment #24) > (In reply to Nelson Benitez from comment #23) > > Jose, I agree with all your points in comment 21 . Will re-do patch to add > > parameters and the string change. > > > > One idea I had, was to have a submenu with three options: > > > > Annotate selected text -> highlight (in yellow) > > strikethrough (in red) > > underline (in blue) > > > > do you like? > > Nelson, I wouldn't do that right now. I think having a context menu with a > submenu is maybe too much...(and I want to rework the popup menu) Take a > look at https://github.com/husain3/evince. Maybe we can add some of this to > the annotation toolbar :) plus the modifications to the actions in the > button. I agree with José. The way to change the annotation text type so far is in the properties dialog. Not optimal, but it works so far. One interaction that may work is having a widget like: +------+-----+ | | | | | | v | +------+-----+--------+ | | | | | | | | | | +---------------------+ At the top left a 2-state(?) button. The user activates the annotation mode the button, which has the annotation type as an icon (highlight, comment, polygon, anything). The button stays pressed until the user presses Esc or pushes the button again to stop annotating the document. At the top right, an combo that opens a menu (bottom) to change the annotation type. The menu may have the icon + text of the annotation type. Pros: - There is no need of a second toolbar for annotations. - There is a visual cue about the mode and annotation type in use Cons: - It requires more space than the current button for the HeaderBar. What do you feel about this?
Review of attachment 369547 [details] [review]: New review... I missed some points. Nelson, I have a fixing commit for this. ::: shell/ev-window.c @@ +5082,3 @@ + g_simple_action_set_enabled (G_SIMPLE_ACTION (action), enable); +} + GAction *action; Nelson... I was reviewing another patch and realized... Whats the difference of this function with ev_window_set_enabled? Why can't we just use that?
Created attachment 370225 [details] [review] shell: there is no need for view_menu_text_selection_popup We introduced it while fixing bug 763493, but this function is identical to ev_window_set_enabled so we don't need it. See bug 763493
Review of attachment 370225 [details] [review]: I just committed this after German reviewed a similar patch
(In reply to Daniel Boles from comment #22) > (In reply to José Aliste from comment #21) > > the shortcut and the highlight button could do the following. If there is no > > selection, they enter highlighting mode as now, if there is a selection, > > they do what your patch does and just transform the selection into a > > annotation. > > > > What other people think about this? > > I'd go for it! My Comment 1 and Fabian's following Comment 2 suggested that, > even. :) I'm attaching a small patch that implements that.
Created attachment 371577 [details] [review] annotations-toolbar: add text markup annotation from selection When there's a text selection, and user clicks the toolbutton for "Add text markup annotation" then we will directly add a text markup annotation from the selected text, as that is an intuitive straightforward way to do it. Part of https://bugzilla.gnome.org/show_bug.cgi?id=763943
-- 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/663.