GNOME Bugzilla – Bug 675328
Port all (currently) synchronous search providers to the async API
Last modified: 2012-05-07 19:47:39 UTC
... and scrap the synchronous API. There are still some focus issues that happen with multiple asynchronous search providers in play that I'll have to look into, but overall this is a decent code cleanup in my opinion, and it should hopefully let us track down other bugs with the async search provider API and lead into the search rework.
Created attachment 213325 [details] [review] search: Allow synchronous searches to be defined with the async API To allow this to happen, we need to make sure that we don't overwrite the previousResults when calling the async method. Note that this is a bug of some sort, we were already using this synchronous style when a remote search failed.
Created attachment 213326 [details] [review] search: Remove unused MUTLIPLE_* match types
Created attachment 213327 [details] [review] placeDisplay: Create our own PlacesManager Since we don't have a section showing off the available places somewhere, we don't need a global PlacesManager.
Created attachment 213328 [details] [review] search: Port all search providers over to the async API
Created attachment 213329 [details] [review] search: Remove the ability to add synchronous search providers As shown in the previous commits, synchronous search is easily implemented by the asynchronous search API. The only reason we still have a synchronous search API is of historical reasons. Well, we're not a museum, and git log can keep our fossils safe if need be....
Review of attachment 213326 [details] [review]: Looks good.
Review of attachment 213327 [details] [review]: ::: js/ui/placeDisplay.js @@ +383,3 @@ }); } + callback(metas); Seems to belong to attachment 213328 [details] [review]. And then there are some more patch splitting mistakes below.
Review of attachment 213328 [details] [review]: Needs work as noted on the previous patch.
Review of attachment 213325 [details] [review]: Looks fine.
Created attachment 213399 [details] [review] placeDisplay: Create our own PlacesManager Since we don't have a section showing off the available places somewhere, we don't need a global PlacesManager.
Created attachment 213400 [details] [review] search: Port all search providers over to the async API
Created attachment 213401 [details] [review] search: Remove the ability to add synchronous search providers As shown in the previous commits, synchronous search is easily implemented by the asynchronous search API. The only reason we still have a synchronous search API is of historical reasons. Well, we're not a museum, and git log can keep our fossils safe if need be....
Review of attachment 213399 [details] [review]: ::: js/ui/placeDisplay.js @@ +393,3 @@ _compareResultMeta: function (idA, idB) { + let infoA = this.placesManager.lookupPlaceById(idA); + let infoB = this.placesManager.lookupPlaceById(idB); The .sort() calls that use this method have to use Lang.bind(this, ...) now. @@ +424,2 @@ getSubsearchResultSet: function(previousResults, terms) { + let places = previousResults.map(function (id) { return this.placesManager.lookupPlaceById(id); }); This also needs Lang.bind().
Review of attachment 213400 [details] [review]: Looks good
Review of attachment 213401 [details] [review]: Ok
Created attachment 213603 [details] [review] searchDisplay: Remove a couple of no longer used variables
Created attachment 213604 [details] [review] searchDisplay: Remove the sync search completed code path Now that all searches are async we can remove the code path for the SearchSystem::search-completed signal which is no longer useful. This patch ends up fixing the status text not being updated for when there are no results. -- This just followed from reviewing. Neat stuff indeed.
Review of attachment 213603 [details] [review]: ::: js/ui/searchDisplay.js @@ -212,3 @@ for (let i = 0; i < this._providers.length; i++) { this.createProviderMeta(this._providers[i]); - this._providerMetaResults[this.providers[i].title] = []; You forgot to remove the set to here in _updateProviderResults.
Review of attachment 213604 [details] [review]: Yeah, this makes sense.
Created attachment 213617 [details] [review] searchDisplay: Remove a couple of no longer used variables -- (In reply to comment #19) > - this._providerMetaResults[this.providers[i].title] = []; > > You forgot to remove the set to here in _updateProviderResults. Yeah, that's a patch splitting issue. Should be fixed here. The other patch remains the same except without the removal of the line that moved into this one.
Review of attachment 213617 [details] [review]: Go for it.
Attachment 213325 [details] pushed as 0bf6c93 - search: Allow synchronous searches to be defined with the async API Attachment 213326 [details] pushed as 7ba8c7c - search: Remove unused MUTLIPLE_* match types Attachment 213399 [details] pushed as f2d883d - placeDisplay: Create our own PlacesManager Attachment 213400 [details] pushed as 58f77a1 - search: Port all search providers over to the async API Attachment 213401 [details] pushed as 333e380 - search: Remove the ability to add synchronous search providers Pushed with fixes.
Attachment 213604 [details] pushed as 4ce2f80 - searchDisplay: Remove the sync search completed code path Attachment 213617 [details] pushed as 0862d1c - searchDisplay: Remove a couple of no longer used variables