GNOME Bugzilla – Bug 693836
Large cleanup of search display code
Last modified: 2013-05-29 16:08:39 UTC
This doesn't need to go into 3.8, but we're tracking a few search regressions, and I'm filing these cleanups now in case a future regression fix relies on this branch.
Created attachment 236132 [details] [review] ShellSearchProvider: Update documentation about 'name' and 'id' Document and enforce that a meta ID corresponds to the result ID that we've passed in. This does not break any existing search providers as far as I'm aware.
Created attachment 236133 [details] [review] search: Rename pushResults to setResults pushResults, and the original async search API, were originally intended so search results that weren't immediate could be added as they come in. Since then, we've decided that the design of search results is that they should finish at once with all results. Thus, the code was modified so that pushResults always overwrote the current result set. As such, it makes sense to rename the method so that the name matches the behavior.
Created attachment 236134 [details] [review] searchDisplay: Remove pendingClear It turns out that every time we call getResultsForDisplay is directly after a setResults, so pendingClear is always true.
Created attachment 236135 [details] [review] searchDisplay: Add a base class for common parts of search results Right now, this doesn't give us very much, since IconGrid and StBoxLayout have different APIs. But since we want to introduce result caching, it makes to reduce the duplication we already have so we don't need to add the code to do so in both places.
Created attachment 236136 [details] [review] searchDisplay: Remove clearDisplayForProvider Since the two paths that call this want to keep the actor in two different states, it makes sense to just call the one function that's the same between both individually.
Created attachment 236137 [details] [review] searchDisplay: Make the results display in charge of updating the actor While this is a very simple translation right now, soon enough it will be so that it will have a less crazy "public" API and can do things like cache result metas.
Created attachment 236138 [details] [review] searchDisplay: Make renderResults private There's no need to call this from outside now.
Created attachment 236139 [details] [review] searchDisplay: Remove focus management code It turns out that this focus code broke sometime in the 3.6 cycle -- when updating results, the focus is always on the text entry, so this never gets called. We'll eventually replace it with something that keeps track of the focused result meta, but for now, remove it.
Created attachment 236140 [details] [review] searchDisplay: Remove the providerIcon from the providerMeta Since the resultsDisplay is tracking it, it's not needed.
Created attachment 236141 [details] [review] searchDisplay: Move the provider actors to the results display Since the provider icon only appears in the list results, it makes sense for that to be stored with the results class, rather than outside, triggered by which sort of display it is.
Created attachment 236142 [details] [review] searchDisplay: Remove the "provider meta" As everything is tracked on the SearchResults or subclasses of now, just use that, which we call the "provider display".
Created attachment 236143 [details] [review] searchDisplay: Remove getVisibleResultCount This is only used in one place, so we can quickly substitute where it's needed.
Created attachment 236144 [details] [review] searchDisplay: Remove the setResults/getResultsForDisplay dance Now that we control our own destiny, I noticed that getResultsToDisplay is the only user of this._notDisplayedResult, and it's called immediately after setResults, which is the only thing that sets it. Just remove the stateness entirely.
(In reply to comment #0) > This doesn't need to go into 3.8, but we're tracking a few search > regressions, and I'm filing these cleanups now in case a future > regression fix relies on this branch. Sorry, but no. At this point regressions should be fixed on master and not be used as an excuse to push large cleanups (that are not unlikely to introduce regressions themselves). 3.10 material.
(In reply to comment #1) > Created an attachment (id=236132) [details] [review] > ShellSearchProvider: Update documentation about 'name' and 'id' > > Document and enforce that a meta ID corresponds to the result ID > that we've passed in. This does not break any existing search providers > as far as I'm aware. I think this patch should go in. It doesn't have any code changes -- just a doc change, but it would be handy for us to solidify this assumption here. All known search providers follow it, and it's handy to know which result ID matches to which result meta.
Review of attachment 236132 [details] [review]: Yeah, this makes sense
Comment on attachment 236132 [details] [review] ShellSearchProvider: Update documentation about 'name' and 'id' Attachment 236132 [details] pushed as af7a345 - ShellSearchProvider: Update documentation about 'name' and 'id'
I'll review the rest of the patchset, up to you to decide whether this will land this cycle or not.
Review of attachment 236133 [details] [review]: Seems harmless
Review of attachment 236134 [details] [review]: Makes sense.
Review of attachment 236135 [details] [review]: ++
Review of attachment 236136 [details] [review]: ++
Review of attachment 236137 [details] [review]: ::: js/ui/searchDisplay.js @@ +203,3 @@ + updateSearch: function(providerResults, terms, callback) { + if (providerResults.length == 0) { + this.clear(); Where are you calling setResults in this code path? @@ +204,3 @@ + if (providerResults.length == 0) { + this.clear(); + }, I'd like it better if you returned early here. @@ -489,3 @@ - // the first search result stays the same, we hide the - // content while filling in the results. - Is this code not needed anymore?
Review of attachment 236138 [details] [review]: I usually like to keep the functions "public" between a base class and subclasses, but looks fine otherwise
Review of attachment 236140 [details] [review]: ++
Review of attachment 236141 [details] [review]: ++
Review of attachment 236142 [details] [review]: Makes sense
Review of attachment 236143 [details] [review]: ++
Review of attachment 236144 [details] [review]: Nice
Review of attachment 236139 [details] [review]: Not the biggest fan of this, I'd rather we fixed the bug instead :)
Attachment 236133 [details] pushed as 37e2b60 - search: Rename pushResults to setResults Attachment 236134 [details] pushed as 62e1c08 - searchDisplay: Remove pendingClear Attachment 236135 [details] pushed as e602199 - searchDisplay: Add a base class for common parts of search results Attachment 236136 [details] pushed as 5ab4c48 - searchDisplay: Remove clearDisplayForProvider Attachment 236137 [details] pushed as 19749bb - searchDisplay: Make the results display in charge of updating the actor Attachment 236138 [details] pushed as 9c94e98 - searchDisplay: Make renderResults private Attachment 236139 [details] pushed as 32a53f7 - searchDisplay: Remove focus management code Attachment 236140 [details] pushed as 747faa4 - searchDisplay: Remove the providerIcon from the providerMeta Attachment 236141 [details] pushed as 74a6ca5 - searchDisplay: Move the provider actors to the results display Attachment 236142 [details] pushed as 98eaef6 - searchDisplay: Remove the "provider meta" Attachment 236143 [details] pushed as 1ec82d2 - searchDisplay: Remove getVisibleResultCount Attachment 236144 [details] pushed as a7e9655 - searchDisplay: Remove the setResults/getResultsForDisplay dance
(In reply to comment #19) > Review of attachment 236133 [details] [review]: > > Seems harmless Well, it's a API break for extensions, for a very commonly used API. Next time, we should avoid this, or have a compatibility wrapper.