GNOME Bugzilla – Bug 729483
Turn search into search-as-you-type
Last modified: 2014-05-15 17:50:22 UTC
Created attachment 275768 [details] [review] [PATCH] Don't require pressing Enter to search, just type https://fedoraproject.org/wiki/Changes/AppInstallerContinued said you want search to be "search as you type" I've decided to try implementing that, and here is what I have so far. It's a WIP patch, and has few problems. Namely, performance. Since it fires a refresh on every key press, fast-typing users will notice a severe delay and the entire application sometimes freezes when searching. The console output is flooded with (org.gnome.Software:25511): Gtk-CRITICAL **: gtk_list_box_get_selection_mode: assertion 'GTK_IS_LIST_BOX (box)' failed when typing, might be related. Perhaps it fires searches when the spinner is still there and that confuses it. So I think the solution would be adding a slight delay to this, to only search when the user finished typing, or at least every few keypresses instead of every single keypress. The code style might be off too, but I hope it's a good start. If not, feel free to discard this patch and close this bug.
To get that delay, you should use the ::search-changed signal of GtkSearchEntry which already does that for you. In fact, we already connect to that signal in GsShellSearch, but don't do anything in the handler.
Created attachment 275904 [details] [review] [PATCH] Turn search to search-as-you-type Okay, second attempt. I had to write the handler in gs-shell and not gs-shell-search, because the one in gs-shell-search was not triggered in all cases (if you're in the overview mode, for example). This one works much better and is indeed much better on the performance side of things, but if I search and delete some of the string and then type again, the app freezes and uses 99% CPU, and I get a lot of these in the console: (org.gnome.Software:11904): Gtk-CRITICAL **: gtk_list_box_get_selection_mode: assertion 'GTK_IS_LIST_BOX (box)' failed So the patch still needs some work: I need to figure out where those assertions are coming from, and why does it freeze.
the listbox warning has nothing to do with gnome-software; it is a bug in gtk+'s a11y code that I've just fixed.
Created attachment 275959 [details] [review] [PATCH] Turn search to search-as-you-type One last tweak. I guess this code is ready to be reviewed, I can't reproduce the freezes as often as they happened before, and even if I could I don't know what causes them.
I noticed a problem with this patch... sometimes when I search the list I get is basically all apps... The search-changed signal is fired before I finish typing, which will cause a refresh to start - and only few letters, one or two, will be the search string. Any refresh that will come after that will not execute because priv->waiting will be True when the search is running. When the search is for one or two letters, the search obviously takes a while, thus causing the performance and freezing problems I saw earlier. Can anyone give me any hints about how to delay the searching a bit more?
Delaying the search more isn't probably going to help a lot. Some people type faster and some slower and you'll always find some who type slow enough. What I would do is rework gs_shell_search_refresh() so that instead of returning, it would cancel the currently running search and start a new search, each time you get a new search string. This could be done by creating a separate GCancellable for search in gs_shell_search_refresh, so that instead of: if (priv->waiting) return; you'd do something like: if (priv->search_cancellable != NULL) { g_cancellable_cancel (self->search_cancellable); g_clear_object (&search_cancellable); ] ... priv->search_cancellable = g_cancellable_new (); gs_plugin_loader_search_async (..., priv->search_cancellable, ...);
(In reply to comment #5) > When the search is for one or two letters, the search obviously takes a while /.../ Also, could check the search string length in the search-changed callback and avoid doing a search if it's just one letter.
Created attachment 276188 [details] [review] [PATCH] Turn search to search-as-you-type Wow, it's super fast now, just like a real search as you type implementation should be.
Review of attachment 276188 [details] [review]: Looks nice, I'm just wondering if it would make sense to merge text-changed and search-changed handlers? text-changed currently only handles switching back to the overview when the search string is deleted. ::: src/gs-shell-search.c @@ +306,3 @@ + g_cancellable_cancel (priv->search_cancellable); + g_clear_object (&priv->search_cancellable); + } Should do the same in finalize as well to avoid leaking the cancellable. ::: src/gs-shell.c @@ +416,3 @@ + + text = gtk_entry_get_text (GTK_ENTRY (entry)); + if (text[0] != '\0' && strlen(text) > 3) { I wonder if 3 might be a bit excessive: quite a few apps have short names (0ad for example). But sure, if it's unusably slow otherwise, I guess it's a necessary evil for now. You can drop the != '\0' (it's just a 0 length guard) if you have the strlen check. And there's a missing space before the opening brace :-)
(In reply to comment #9) > Review of attachment 276188 [details] [review]: > > Looks nice, I'm just wondering if it would make sense to merge text-changed and > search-changed handlers? text-changed currently only handles switching back to > the overview when the search string is deleted. > Well... I thought we wanted to return to the overview as soon as the user deletes everything, without waiting the 150ms of search-changed. But thinking of it again, it makes sense. I'll do it. > I wonder if 3 might be a bit excessive People can still type "0ad" and press Enter, but you're right, I'll change it to two. Now that we have a callable, it shouldn't affect performance too much to lower that limit but one character. > Should do the same in finalize as well to avoid leaking the cancellable. finalize already has g_object_unref() for clearing the other cancellable, so I'm not sure: should I use g_object_unref() for the search_cancellable too, or do I want to use g_clear_object()?
Created attachment 276204 [details] [review] [PATCH] Turn search to search-as-you-type All fixed now, apart from that missing space you claim there was, which I didn't find. As suggested on IRC, I also made sure to connect the cancellable with the search_cancellable so if the calling code wants to cancel, it will cancel the search as well. I also removed the old, empty search-changed handler from gs-shell-search.c Let me know if you find any other issues with it.
It seems that gnome-software ignores space when searching, unless I click the search bar... This is the offending line https://git.gnome.org/browse/gnome-software/tree/src/gs-shell.c#n386 I guess it's done for accessibility? it means that when I want to search for, say, "virtual machine", it will write "virtualmachine" and thus show no results. Is there a way we can remove this line so searching would be intuitive even with spaces? Perhaps we can make it so starting to type would move the focus to the search box, where spaces would work, but if the user moves the focus away using Tab they would still be able to press space to click on widgets... I tried doing that using gtk_widget_grab_focus() after every refresh, but that doesn't work well because it also selects all the text, which means the next letter I type will replace what I already wrote. This space problem also happens without my patch, but when you don't have search-as-you-type you can simply click the search bar with your mouse and then you can type spaces. With search as you type, however, it becomes even more annoying because the search entry loses its focus on every search results refresh. I want to fix this as well (and I'm doing it in this bug and not another because it directly affects the user experience of search-as-you-type), but I don't know how. Any ideas? Is there a way to grab the focus without having it selecting the text?
We don't want to steal key events that are relevant for keyboard navigation from the focus widget - space is used to activate buttons, thus it is on the list. The entry needs to grab focus when we start searching - no way around that.
As discussed on IRC: Stealing the focus is the way it should be done, but it only makes sense if the search bar is hidden and only shown when you start typing/click on the toggle button. Since we don't have that yet, I'll leave focus-stealing out of the search-as-you-type patch, and have it be a part of a second patch I'll write for bug #710640.
Review of attachment 276204 [details] [review]: I like the latest patch a lot, thanks. Do you have GNOME push access? If you do, the patch looks good to me to push with the whitespace fix below. ::: src/gs-shell-search.c @@ +469,3 @@ + + +} We don't add newlines before the closing } brackets at the end of functions.
Pushed as 999042c