GNOME Bugzilla – Bug 655220
Search providers can't be asynchronous
Last modified: 2011-08-30 15:56:03 UTC
Relevant patch from Bug 640659 will be reviewed here :)
Created attachment 192573 [details] [review] search: support for asynchronous search providers Original patch by Jasper St. Pierre and Seif Lofty. I reworked this a bit to apply on master and did some adaptations based on Jasper's review.
I've been using this patch for a new extension adding a Grilo SearchProvider which I plan to upload to Bugzilla soon :)
Review of attachment 192573 [details] [review]: Your commit message shouldn't explain the history of the patch or the reviews. It should explain the motivation behind the change. In this case: Add support for asynchronous search providers Some search providers may want to change their results, or may not want to block on an external service to get their results (DBus, etc.) Set up an infrastructure to allow search providers to add their search results at a later time. Based on a patch by Jasper St. Pierre and Seif Lotfy. ::: js/ui/searchDisplay.js @@ -266,3 @@ style_class: 'dash-search-button-label' }); - button.label_actor = title; Why? @@ +370,3 @@ + if (providerResults.length == 0) + this._clearDisplayForProvider(i); + else { nit: braces on both sides, or no braces on both sides. @@ +371,3 @@ + this._clearDisplayForProvider(i); + else { + if (this._providerMetaResults[provider.title] != providerResults) { Array equality always returns false, so this is broken.
(In reply to comment #3) > Review of attachment 192573 [details] [review]: > > Your commit message shouldn't explain the history of the patch or the reviews. > It should explain the motivation behind the change. In this case: > > Add support for asynchronous search providers > > Some search providers may want to change their results, or may > not want to block on an external service to get their results (DBus, etc.) > Set up an infrastructure to allow search providers to add their search > results at a later time. > > Based on a patch by Jasper St. Pierre and Seif Lotfy. All right. > > ::: js/ui/searchDisplay.js > @@ -266,3 @@ > style_class: 'dash-search-button-label' }); > > - button.label_actor = title; > > Why? That one slipped in by accident. Will restore it! > > @@ +370,3 @@ > + if (providerResults.length == 0) > + this._clearDisplayForProvider(i); > + else { > > nit: braces on both sides, or no braces on both sides. > > @@ +371,3 @@ > + this._clearDisplayForProvider(i); > + else { > + if (this._providerMetaResults[provider.title] != > providerResults) { > > Array equality always returns false, so this is broken. Ok. I'll update the patch. Thanks for the quick review!
Created attachment 192586 [details] [review] Add support for asynchronous search providers Some search providers may want to change their results, or may not want to block on an external service to get their results (DBus, etc.) Set up an infrastructure to allow search providers to add their search results at a later time. Based on a patch by Jasper St. Pierre and Seif Lotfy.
Review of attachment 192586 [details] [review]: Looks good! I'd suggest adding a test to make sure it works. In my first implementation, I added a dummy search provider that would set a timeout when a magic search value was entered, like "test async", and added the result after a few seconds.
Ok I wrote this search provider but where should I put it? Would it be fine to register it along with the other providers? Also what icon would you suggest to use for the result item?
It's just a test search provider, it's not going to go into the Shell itself. It can go in a separate patch. Icons? Doesn't matter, I think I used 'system-run' as a placeholder.
Created attachment 192703 [details] [review] Sample Asynchronous Search Provider This search provider is not added to the Shell by default. It's meant as a simple show-case for the asynchronous search. If the provider is active and if the user looks for "test async" the search provider will schedule a task in the mainloop to fire after 2 seconds and add a dummy search result.
Review of attachment 192703 [details] [review]: testcommon isn't really designed for something like this, especially as it makes it hard to import at runtime. It may make sense to have some sort of new directory for docs/examples that can be loaded at runtime, but for now, just append this to the end of search.js, and we won't commit it. ::: tests/testcommon/dummySearch.js @@ +10,3 @@ +// This is a sample asynchronous search provider. It can be added to +// the Shell at runtime with the following call: +// Main.overview.viewSelector.addSearchProvider(new DummySearchProvider()); This is a lie unless testcommon has some magic I'm not aware of.
Created attachment 193400 [details] [review] Sample Asynchronous Search Provider This search provider is not added to the Shell by default. It's meant as a simple show-case for the asynchronous search. If the provider is active and if the user looks for "test async" the search provider will schedule a task in the mainloop to fire after 2 seconds and add a dummy search result.
Review of attachment 192586 [details] [review]: Sure.
Review of attachment 193400 [details] [review]: Don't push this, it's just an example.
(In reply to comment #12) > Review of attachment 192586 [details] [review]: > > Sure. Can you commit/push please? I don't have a git account on GNOME git yet.
Ping? Can this be committed please.
Attachment 192586 [details] pushed as 3418e6e - Add support for asynchronous search providers