GNOME Bugzilla – Bug 794538
New feature: Search for selected text
Last modified: 2018-03-29 22:40:01 UTC
Created attachment 369924 [details] [review] Implement Search for Selected Text patch. Hello everyone! First, thank you for all the work that you do to make evince such a fully featured "document" viewer. It's a great tool. In the course of working on producing a big document, I realized that it would be great to be able to highlight some text and start a search automatically from that text. Here is code that will implement such a feature. With this code, the popup menu will now offer a Search for Selected Text option when there is selected text. I know that there will be inevitable issues with the patch. I tried to follow all the proper coding standards but I am sure that I missed some. Also, I may not have followed all the proper best practices w.r.t to signals, etc. If this is a feature that you think is useful, I would absolutely love feedback on the code in the patch so that I can get it compatible with your really well designed corpus of existing code. Thanks for all the help that people gave me in the IRC. I can't wait for your feedback. Will
Review of attachment 369924 [details] [review]: I think this looks good, but I have a few nitpicks... ::: libview/ev-view.c @@ +5676,1 @@ Why are you declaring this const? AFAIK, you get a normalized copy of the selected text and you have to free it after use, so the "ownership" is transferred to the user of this function. In that sense, I don't see this as const, but maybe I am missing something? ::: shell/ev-window.c @@ +5130,3 @@ + + view_menu_text_selection_search_popup (ev_window, can_search_for_text); + you need better names for your variables and functions... Like, instead of can_search_for_text, just use has_selection, mimmicking what's already there. instead of view_menu_text_selection_search_popup say something like toggle_text_selection_search_action or another better option you might come up with.
(In reply to José Aliste from comment #1) > Review of attachment 369924 [details] [review] [review]: > > I think this looks good, but I have a few nitpicks... Thanks! Really happy to have the feedback! > > ::: libview/ev-view.c > @@ +5676,1 @@ > > > Why are you declaring this const? AFAIK, you get a normalized copy of the > selected text and you have to free it after use, so the "ownership" is > transferred to the user of this function. In that sense, I don't see this as > const, but maybe I am missing something? Thanks for the feedback. You are correct. You do get a normalized string as a result (from g_utf8_normalized) and you should free it. I assumed that the code base's semantics did not assume that const-ness symbolized ownership. I was relying, instead, on the concept that the string of highlighted text is constant in the sense that you can't change what the user has highlighted. Now that I understand your semantics, I will definitely make this change. > > ::: shell/ev-window.c > @@ +5130,3 @@ > + > + view_menu_text_selection_search_popup (ev_window, can_search_for_text); > + > > you need better names for your variables and functions... Like, instead of > can_search_for_text, just use has_selection, mimmicking what's already > there. Great feedback. Will do! > > instead of view_menu_text_selection_search_popup say something like > toggle_text_selection_search_action or another better option you might come > up with. Also great feedback and I will do that. Expect a version 2.0 of the patch sometime later tonight (depending on your timezones!). Thanks again for the feedback. I really appreciate the time you took to review. Will
Created attachment 369931 [details] [review] Implement Search for Selected Text patch (v2) I attempted to incorporate all of Mr. Aliste's feedback. If there are things that I can still do better, please let me know!
Just wanted to bump this. I would love to address any other feedback that only might have. I hope that this is a feature that other people think are useful for evince users. Thank you all for doing all the work you do to maintain such a vital part of the "Linux on the Desktop" infrastructure! Will
Review of attachment 369931 [details] [review]: Yeah, almost good, please fix the comments. ::: shell/ev-window.c @@ +5085,3 @@ + g_simple_action_set_enabled (G_SIMPLE_ACTION (action), enable); +} + GAction *action; Hi, I just realized that we already have a method for that, see ev_window_set_action_enabled and use that instead.
Created attachment 370280 [details] [review] shell: Use selection to populate find bar if available. When a selection is made, if user clicks on the Search icon or hits Ctrl+F, the search bar will be shown with the selection copied to the search bar entry. This immitates what Gedit does when the Find action is activated and there is a selection Based on a patch by William Hawkins <hawkinsw@borlaugic.com>
Review of attachment 369931 [details] [review]: Sorry Will, while merging your patch I realized we have a better option. Will upload a patch
Created attachment 370281 [details] [review] libview: Add API to get the selected text To add search for selected text, we need to pass the selected text from the EvView to the shell. Thus we add a method to get the selected text from the View.
If you see the last two patches, are based on yours but instead of adding a new action, we reuse the find Action. If there is a selection, it will just do what your action did, so you get for free the ctrl + F shortcut and the Search button in the headerbar to activate the search. I believe this is better than our previous approach of adding a popup item. What do you think?
(In reply to José Aliste from comment #9) > If you see the last two patches, are based on yours but instead of adding a > new action, we reuse the find Action. If there is a selection, it will just > do what your action did, so you get for free the ctrl + F shortcut and the > Search button in the headerbar to activate the search. I believe this is > better than our previous approach of adding a popup item. What do you think? I am absolutely okay with this. I think that it works well. I just used the popup item because for me, for some reason, it seemed ergonomic (using the phrase to mean, "the place where I think the option should be"). But, as long as you think that putting it in those places will be obvious for users, then that's great! Sorry it took me a minute to respond to your previous comments on the patch. I was hoping that you'd use my patch just to decrease your workload, but I am a-okay with the approach that you propose. Just let me know if there is anything else that you need from me. Or, propose another way that I can contribute to this awesome project! Thanks for everything, Will
Ok, so I just pushed this to git master, so I am closing the bug.