GNOME Bugzilla – Bug 693458
Don't start a search when we only have whitespace
Last modified: 2013-02-14 19:21:29 UTC
I noticed this a few weeks ago, but I never sat down to write a patch. Note that this is part of a large cleanup branch I have to the search system, so I'm making some more invasive changes than I'd want. I've been testing the search timeout one for two hours now and I *really* like it, and haven't seen any noticeable slowdown. Search feels really snappy without an artificial delay :). If we have complaints from people, we can revert it. Testing is appreciated.
Created attachment 235567 [details] [review] viewSelector: Don't strip whitespace from search strings This is already done by the search system itself.
Created attachment 235568 [details] [review] viewSelector: Remove the search timeout Now that we no longer have many synchronous providers, we don't need the timeout to not freeze anymore.
Created attachment 235569 [details] [review] search: Fix style
Created attachment 235570 [details] [review] searchDisplay: Remove doSearch This is nothing but a middle man, as the view selector already owns the search system. We want to start being a bit more tricky with what we do with the search system so that we ignore whitespace, so let's cut the middle-man out now.
Created attachment 235571 [details] [review] viewSelector: Make sure not to start searching when we only have whitespace As this is thrown out before we actually query providers, we end up with a confusing screen that says "Searching..." that won't ever get results.
Review of attachment 235567 [details] [review]: Looks good
Review of attachment 235568 [details] [review]: ::: js/ui/viewSelector.js @@ -344,3 @@ - - if (this._searchTimeoutId == 0) - this._searchTimeoutId = Mainloop.timeout_add(150, I have to say I'm a little afraid of removing the timeout entirely. I think this is here mostly to accumulate entry changes - if you type fast you don't want to trigger a lot of useless calls to remote providers. Maybe we could use a very short timeout instead?
Review of attachment 235569 [details] [review]: Sure
Review of attachment 235570 [details] [review]: ++
Review of attachment 235571 [details] [review]: ::: js/ui/search.js @@ +61,3 @@ }, + getTermsForSearchString: function(searchString) { If you do this, there's no reason for the function to be a method of SearchSystem at this point. ::: js/ui/viewSelector.js @@ +340,3 @@ this._entry.set_secondary_icon(this._activeIcon); + this._searchSystem.updateSearch(terms); If you do the getTermsForSearchString refactor, you need to call this._searchSystem.updateSearchResults() here instead.
(In reply to comment #7) > I have to say I'm a little afraid of removing the timeout entirely. > I think this is here mostly to accumulate entry changes - if you type fast you > don't want to trigger a lot of useless calls to remote providers. Maybe we > could use a very short timeout instead? The cancellable should be enough, no? Feel free to try the patch yourself.
(In reply to comment #11) > The cancellable should be enough, no? Feel free to try the patch yourself. I tried the patch now, and I didn't change my mind...it feels a bit faster indeed, but it's also a lot more noisy, as updates happen now on the screen while you're typing. I still think we need a short timeout here.
Created attachment 236128 [details] [review] viewSelector: Make sure not to start searching when we only have whitespace As this is thrown out before we actually query providers, we end up with a confusing screen that says "Searching..." that won't ever get results. pace OK, let's drop the search timeout removal for now.
Review of attachment 236128 [details] [review]: Looks good to me now
Attachment 235567 [details] pushed as c8365b7 - viewSelector: Don't strip whitespace from search strings Attachment 235569 [details] pushed as 35a7c94 - search: Fix style Attachment 235570 [details] pushed as 569797d - searchDisplay: Remove doSearch Attachment 236128 [details] pushed as 65e4652 - viewSelector: Make sure not to start searching when we only have whitespace