GNOME Bugzilla – Bug 720993
Use Gtk.SearchEntry's "search-changed" signal
Last modified: 2016-07-04 15:11:42 UTC
Introduced in 3.10, the "search-changed" signal is fired after the user has stopped typing for 150ms. Using this means we can rip out our own timer code in GearyController.
Created attachment 330819 [details] [review] Use the "search-changed" signal Hello, does this work? I'm not so sure myself but it seems correct to me.
Review of attachment 330819 [details] [review]: This looks good, thanks for the patch. Just a few things to tidy up. In general I don't mind that the timeout has dropped down to 150ms, since that will be more consistent with the desktop. Let's run it for a bit and see if anyone complains. Also, if you are interested in tidying this up a bit more, the SearchBar::on_search_key_press method should probably be replaced with two handlers: One for "activate" instead of looking for a Return key press, and one for "stop-search" instead of looking for an Esc key press. ::: src/client/application/geary-controller.vala @@ +2797,1 @@ do_search(main_window.search_bar.search_text); Since search text is passed in as a param here, that should be used instead of retreiving it again from the search_bar. ::: src/client/components/search-bar.vala @@ +25,3 @@ search_entry.tooltip_text = _("Search all mail in account for keywords (Ctrl+S)"); search_entry.changed.connect(on_search_entry_changed); + search_entry.search_changed.connect(on_search_entry_search_changed); Since the on_search_entry_changed method is a single line, I would use a lambda expression here instead and remove the separate method. @@ +56,3 @@ } private void on_search_entry_changed() { This method can be removed altogether, since GTK handles showing/hiding the clear button itself.
Created attachment 330846 [details] [review] Use built-in GTK signals for the search bar
Review of attachment 330846 [details] [review]: Looks good - just two small things to look at and I'll be happy to commit this. ::: src/client/components/search-bar.vala @@ +27,3 @@ + search_text_changed(search_entry.text); + }); + // Handle user pressing Enter to search and canceling it. This comment is a bit confusing since it applies to both of the next two signals, but only talks about pressing Enter. Maybe just drop it since it's pretty obvious what is going on? @@ -59,3 @@ - // Enable/disable clear button. - search_entry.secondary_icon_name = search_entry.text != "" ? - (get_direction() == Gtk.TextDirection.RTL ? ICON_CLEAR_RTL_NAME : ICON_CLEAR_NAME) : null; Do ICON_CLEAR_RTL_NAME and ICON_CLEAR_NAME get used anywhere else in this file? If not then their declarations up the top should be removed.
Created attachment 330849 [details] [review] Use built-in GTK signals for the search bar
Pushed to master as f80f52f. Thanks again!