I'm unsure what you want here. Can you give an example?

Specifically,

 * We need to update the mockups to show a clearer relationship of Contacts and System Settings to the app
 * Have a way to launch the providing app from the search results
 * Separate the recently used items per application
 * Provide a way for other applications to add search results (Documents, Music, Photos, Mail, Web, Notes, ...)
 * Places and Devices - not sure - provided by Files?

 * Possibly Search Google and Wikipedia could be provided by the Web app too...

OK, that's not "specific enough" for me.

Let's just focus on one of the bullet points. Let's say that we want the Music app to provide search results.

Do we want the Music app to provide information on the current song? On the user's library? Do we want to have "actions" in the search, so the user can type "play" or "pause" or "next" in the search field? What do we do when the Music application isn't running? Start it in the background?

Do we want any application in the world to be able to add their own results? Do we want to look at integrating with GNOME Do?

Created attachment 207673 [details] [review]
search: Make asynchronous providers more explicit

Currently, asynchronous search providers are expected to call
startAsync() in getInitialResultSet()/getSubsearchResultSet(),
which will trigger async mode until the search is canceled or
updated. Switching between synchronous and asynchronous mode like
this makes asynchronous search an implementation detail, but being
transparent to the searchDisplay means that certain optimizations
don't work as expected. Namely, updating asynchronous search results
causes flickering, and the automatic selection never focuses
asynchronous results.
So change the API to require providers being either synchronous (with
the current getInitialResultSet()/getSubsearchResultSet() methods)
or asynchronous (with asynchronous variants), and handle asynchronous
providers explicitly in searchDisplay.

Created attachment 207674 [details] [review]
search: Fix initial selection for async providers

As search results are selected before asynchronous results are
added, it is impossible to automatically select the latter. Fix
by delaying the selection until no results are pending before the
selection candidate.

Created attachment 207675 [details] [review]
search-display: Always reset selection when updating search

Currently, a manually set selection takes precedence over automatic
selection; as changing the search terms is likely to change the
result set, it seems more logical to reset the selection (so the
"new" first result is automatically focused).

Created attachment 207676 [details] [review]
search: Rename search_providers to open-search-providers

We will allow applications to hook into shell's search by registering
a service which implements a well-known DBus interface.
"search-providers" is a reasonable directory name for applications to
drop their registration files, but it conflicts with "search_providers"
used by open search providers - rename the latter to avoid confusion.

Created attachment 207677 [details] [review]
search: Add RemoteSearchProvider

Add an asynchronous search provider for results from a DBus service
implementing the org.gnome.Shell.SearchProvider interface; this
will allow applications to hook into the Shell's search without
implementing it in Shell itself or requiring an extension.

Created attachment 207678 [details] [review]
overview: Load RemoteSearchProviders

Allow applications to register search providers by dropping a keyfile
into a well-known directory. For now, initialize all found providers;
long term, we probably want to give users the ability to restrict the
set of active search providers.

Created attachment 207682 [details] [review]
overview: Load RemoteSearchProviders

Sorry, a minor rebase error sneaked in ...

Just a curiosity. Was it considered to use the same API as Unity?
I know that it is a bit Ubuntu-only, and I see that it has some features that don't apply (like custom filters). But patches already exists for many applications, and it would greatly reduce the regression from the removal of recent items.

Review of attachment 207673 [details] [review]:

Overall, I like the cleanup. But what ultimately caused the flickering, and how did you solve it here?

::: js/ui/search.js
@@ +115,3 @@
         this.title = title;
         this.searchSystem = null;
+        this.async  = false;

Extra space.

::: js/ui/searchDisplay.js
@@ +347,3 @@
+        let [provider, providerResults, pending] = results;
+        if (pending)
+            return;

What ever sets "pending" to be false so we follow through updating?

@@ +351,3 @@
+        this._content.hide();
+        this._updateProviderResults(provider, providerResults, terms);
+        if (this._selectedOpenSearchButton == -1 && this._pendingResults == 0)

If a pending result takes a long time, and the user has selected some other result in between, wouldn't this mean that we go to the next selection for no reason?

Review of attachment 207674 [details] [review]:

When you push, please squash this patch with the former.

::: js/ui/searchDisplay.js
@@ +354,3 @@
+
+            if (meta.actor.visible)
+                break; // select this one!

What are we doing here? Just testing to see if we can select down? Why not put that logic in this.selectDown, and return whether we did:

    this._initialSelectionSet = this.selectDown(false);

Review of attachment 207675 [details] [review]:

I'm not convinced. If I make my selection, accidentally type '\' and then backspace, not sure I should have to re-select.

(Of course, it's more likely that I type 'fooo<Down><Down>\<Enter>' in a quick succession, opening up a Wikipedia search instead.)

Review of attachment 207676 [details] [review]:

Sure.

Review of attachment 207677 [details] [review]:

Put the changes to search.js in a new file (remoteSearch.js or something)

I'm not satisfied in the interface that we're exposing. Here's something I'm fine with:

 * Drop GetResultsMeta. It's a bit awkward in a local system, and is even more awkward when trying to shuttle this across DBus.
 * GetInitialResultsSet/GetSubsequenceResultsSet: returns an "a{sv}" that contains either the things returned by GetResultsMeta, or a new key:

    'reference': Reference an earlier ID established from a previous result. If the "a{sv}" contains this pair, then the rest of the keys are ignored.

    (We could also accomplish this with something like "bsa{sv}", where the first boolean marks a reference, otherwise we look at the asv. Pick whichever one makes you feel warm and fuzzy inside.)

 * ClearResultsCache: Clear the set of "previously established things", making applications pass us new metas.

::: js/ui/search.js
@@ +115,3 @@
+    <arg type="s" direction="in" />
+</method>
+</interface>

Missing semicolon.

@@ +329,3 @@
+
+    getResultMeta: function(id) {
+        let result = this._proxy.GetResultMetaSync(id)[0];

While it would require more engineering, I don't think we should block and wait for gnome-documents to shuffle a pixbuf over to us.

Review of attachment 207682 [details] [review]:

::: js/ui/overview.js
@@ +252,3 @@
     },
 
+    _loadRemoteSearchProviders: function() {

I'm sure the answer is "no", but do we want to support search providers in userdatdir or in another system data dir (/usr/local/?)

Do we want to have a file-monitor if a new file comes into the datadir or any of these places?

@@ +254,3 @@
+    _loadRemoteSearchProviders: function() {
+        let dir = Gio.file_new_for_path(global.datadir + '/search-providers');
+        FileUtils.listDirAsync(dir, Lang.bind(this, function(files) {

Since we're putting the remote search providers in a new file, let's have a:

    RemoteSearch.scanRemoteSearchProviders(function(title, icon, busName, objectPath) {
    });

That this function uses.

@@ +257,3 @@
+            for (let i = 0; i < files.length; i++) {
+                let keyfile = new GLib.KeyFile();
+                let path = global.datadir + '/search-providers/' + files[i].get_name();

files[i].get_path() ?

@@ +264,3 @@
+                    continue;
+
+                let group = 'Shell Search Provider';

const KEY_FILE_GROUP = 'Shell Search Provider';

@@ +265,3 @@
+
+                let group = 'Shell Search Provider';
+                let title = keyfile.get_string(group, 'Title');

keyfile.get_locale_string(group, 'Title', null);

Unless we're assuming that keyfiles would be hard-localized on disk.

(In reply to comment #16)
> I'm not satisfied in the interface that we're exposing. Here's something I'm
> fine with:
> 
>  * Drop GetResultsMeta. It's a bit awkward in a local system, and is even more
> awkward when trying to shuttle this across DBus.
>  * GetInitialResultsSet/GetSubsequenceResultsSet: returns an "a{sv}" that
> contains either the things returned by GetResultsMeta, or a new key:

I have considered something like that, but I'm not convinced it's a good idea. Shuffling 1000 pixbufs over the bus to display eight images sounds terribly wasteful.

(In reply to comment #18)
> I have considered something like that, but I'm not convinced it's a good idea.
> Shuffling 1000 pixbufs over the bus to display eight images sounds terribly
> wasteful.

It is, but at least it's async. With all the talk about the Shell search being slow (because it *is*), I do NOT want to make it any slower.

Can we calculate a conservative max amount of items beforehand (one row of items at the default size), and send that number to the application so they know the maximum amount of results to send back (this might also help the application filter out other sorts of irrelevant data and make sure the items returned are the most relevant).

... In fact, why don't we do that all around the Shell - make getInitialResultsSet/getSubsequenceResultsSet take a maxItems parameter, so we can eliminate calculating things we don't care about.

(In reply to comment #17)
> I'm sure the answer is "no", but do we want to support search providers in
> userdatdir or in another system data dir (/usr/local/?)

I don't think we want userdatadir, other system dirs might make sense.


> Do we want to have a file-monitor if a new file comes into the datadir or any
> of these places?

Yes, good point.


> @@ +257,3 @@
> +            for (let i = 0; i < files.length; i++) {
> +                let keyfile = new GLib.KeyFile();
> +                let path = global.datadir + '/search-providers/' +
> files[i].get_name();
> 
> files[i].get_path() ?

No. I admit it's confusing, but files[i] is a GFileInfo, not a GFile (e.g. there is not get_path())

(In reply to comment #19)
> It is, but at least it's async. With all the talk about the Shell search being
> slow (because it *is*), I do NOT want to make it any slower.

Nobody wants to make search slower (and yes, it *is* slow). But async is not a magic fix for that - it just means that we don't block the shell UI while searching, it does not make search results appear faster. Adding an async variant of getResultMeta() sounds like a good idea to me (though as you noted, it's not trivial with the existing code structure) - better in fact than blindly increasing traffic under the assumption that slowing down an async method doesn't matter.


> Can we calculate a conservative max amount of items beforehand (one row of
> items at the default size), and send that number to the application so they
> know the maximum amount of results to send back (this might also help the
> application filter out other sorts of irrelevant data and make sure the items
> returned are the most relevant).
> 
> ... In fact, why don't we do that all around the Shell - make
> getInitialResultsSet/getSubsequenceResultsSet take a maxItems parameter, so we
> can eliminate calculating things we don't care about.

That would render getSubsearchResultSet() useless. Now, there's only one search provider which actually uses it, so it might still be worth considering ...

(In reply to comment #12)
> Review of attachment 207673 [details] [review]:
> 
> Overall, I like the cleanup. But what ultimately caused the flickering, and how
> did you solve it here?

It's caused by a style change with a transition - the fix is to hide the display actor while updating results (we already do that for "normal" providers, see bug 646019 for the related discussion)

(In reply to comment #21)
> (In reply to comment #19)
> > It is, but at least it's async. With all the talk about the Shell search being
> > slow (because it *is*), I do NOT want to make it any slower.
> 
> Nobody wants to make search slower (and yes, it *is* slow). But async is not a
> magic fix for that - it just means that we don't block the shell UI while
> searching, it does not make search results appear faster.

That's what I mean by "slow". I don't care about how late the results show up (maybe we should have a spinner after the title to show that the results for that section are still loading, though). All I care about is that you don't have to wait 4-5 seconds after typing to be able to use your system at all (including cancelling the search with Escape or something).

> Adding an async
> variant of getResultMeta() sounds like a good idea to me (though as you noted,
> it's not trivial with the existing code structure) - better in fact than
> blindly increasing traffic under the assumption that slowing down an async
> method doesn't matter.

Not a fan of that -- doing one async operation per result isn't a good idea -- unless we add complexity to the application, they'd still process the async calls sequentially, and it would just delay each result showing up by some time. Perhaps we can have an async "getResultsMeta" that takes a list of metas as a compromise, with a shared assumption that we can cache result metas returned from here for IDs returned by later operations.

Of course, what this means is that if the user types "a" and gnome-documents returns 52 results or so, and gnome-documents starts to fetch metas for them, if the user then types "b", we can either cancel the current meta fetching operation and send a new one (after the results come back, of course), or finish the current one and just use the cache. It depends on how much processing is being done on useless things (yet another win for a maxItems)

> That would render getSubsearchResultSet() useless. Now, there's only one search
> provider which actually uses it, so it might still be worth considering ...

I never thought that the win provided by getSubsearchResultSet was big enough. The slowness doesn't come from the math and filtering...


Another minor win we can do: allow and encourage an 'icon-file' parameter if possible so we can load that from disk (cached, too!) as an async operation instead of a sync uncached pixbuf load.

(In reply to comment #23)
> Another minor win we can do: allow and encourage an 'icon-file' parameter if
> possible so we can load that from disk (cached, too!) as an async operation
> instead of a sync uncached pixbuf load.

You mean like the existing 'gicon' parameter (which the documents provider uses for ~99% of icons)?

(In reply to comment #24)
> (In reply to comment #23)
> > Another minor win we can do: allow and encourage an 'icon-file' parameter if
> > possible so we can load that from disk (cached, too!) as an async operation
> > instead of a sync uncached pixbuf load.
> 
> You mean like the existing 'gicon' parameter (which the documents provider uses
> for ~99% of icons)?

Huh, I didn't realize GIcon did anything other than themed icons.

(In reply to comment #25)
> Huh, I didn't realize GIcon did anything other than themed icons.

It does, see GFileIcon.

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > It is, but at least it's async. With all the talk about the Shell search being
> > > slow (because it *is*), I do NOT want to make it any slower.
> > 
> > Nobody wants to make search slower (and yes, it *is* slow). But async is not a
> > magic fix for that - it just means that we don't block the shell UI while
> > searching, it does not make search results appear faster.
> 
> That's what I mean by "slow". I don't care about how late the results show up
> (maybe we should have a spinner after the title to show that the results for
> that section are still loading, though). All I care about is that you don't
> have to wait 4-5 seconds after typing to be able to use your system at all
> (including cancelling the search with Escape or something).

You may not, but I care very much how late the results show up...

(In reply to comment #27)
> You may not, but I care very much how late the results show up...

When talking about the difference between sync and async, it's going to be the difference between "search takes 3 seconds to calculate, and your machine is frozen for those three seconds" and "search takes 3 seconds to calculate, and some results may be loading for some time, but your machine is still working", respectively.

Choosing between those two options, I think you'll prefer the latter.

Obviously, we should optimize things to make the search results appear faster, but our first priority should be to make sure we aren't blocking, or at least aren't adding to the "block time" before we investigate anything else.

Doing a synchronous DBus method call is not something I'm going to take as a patch.

(In reply to comment #21)
> (In reply to comment #19)
> > It is, but at least it's async. With all the talk about the Shell search being
> > slow (because it *is*), I do NOT want to make it any slower.
> 
> Nobody wants to make search slower (and yes, it *is* slow). But async is not a
> magic fix for that - it just means that we don't block the shell UI while
> searching, it does not make search results appear faster. 

I agree with Jasper here this are two distinct issues. Blocking the compositor for a noticeable amount of time is a no go IMO. We should learn from mistakes we made in the past not keep repeating them.

Created attachment 208075 [details] [review]
search: Replace getResultMeta() with getResultMetas()

Save some function calls by fetching all search results we want to
display for a provider at once, rather than one result at a time.

Created attachment 208076 [details] [review]
searchDisplay: Split renderResults()

renderResults() updates the results set, determines the number of
results to display, retrieves the corresponding result metas and
adds a new results actor for each meta.
Splitting the function in those parts allows to move the retrieval
of the result metas into SearchResults, which is where we ensure
flicker-free rendering and control the selection - we want to keep
both features for asynchronous result metas which we are about to
introduce.

Review of attachment 208075 [details] [review]:

::: js/ui/docDisplay.js
@@ +27,3 @@
+                                 return docInfo.createIcon(size);
+                             }
+                });

Indentation style differs between this and the near identical code in the other *Display.js files.

::: js/ui/search.js
@@ +196,3 @@
      * Return an object with 'id', 'name', (both strings) and 'createIcon'
+     * (function(size) returning a Clutter.Texture) properties for each member
+     * of @ids which describe the given search result.

"Return an object.... for each member of @ids" doesn't really read that well. Let's explicitly say that we want an array returned.

Review of attachment 208076 [details] [review]:

Waiting on the asynchronous metas patch to review this...

Created attachment 208077 [details] [review]
search: Make asynchronous providers more explicit

(In reply to comment #12)
> +        let [provider, providerResults, pending] = results;
> +        if (pending)
> +            return;
> 
> What ever sets "pending" to be false so we follow through updating?

It's set in pushResults() (from where the signal corresponding to this handler is emitted) - it is actually always false, so I could just remove the check ... 


> @@ +351,3 @@
> If a pending result takes a long time, and the user has selected some other
> result in between, wouldn't this mean that we go to the next selection for no
> reason?

Obsoleted by squashing with the following patch.


(In reply to comment #13)
> When you push, please squash this patch with the former.

Done.


> ::: js/ui/searchDisplay.js
> @@ +354,3 @@
> +
> +            if (meta.actor.visible)
> +                break; // select this one!
> 
> What are we doing here? Just testing to see if we can select down? Why not put
> that logic in this.selectDown, and return whether we did:
> 
>     this._initialSelectionSet = this.selectDown(false);

selectDown() selects the next visible results actor. An actor may be hidden either because there aren't any results for its provider, or because its results are still pending. For manual selection we don't care about that distinction and just call selectDown(), but for automatic selection we only want to skip actors without results (and bail out early if there's an actor with pending results before the first visible actor). So it's either adding an additional mode parameter to selectDown(), or using a separate function for automatic selection.

Created attachment 208079 [details] [review]
search-display: Always reset selection when updating search

(In reply to comment #14)
> I'm not convinced. If I make my selection, accidentally type '\' and then
> backspace, not sure I should have to re-select.
> 
> (Of course, it's more likely that I type 'fooo<Down><Down>\<Enter>' in a quick
> succession, opening up a Wikipedia search instead.)

The primary reason for this change was 'termn' <backspace> - no results with the typo means that Wikipedia gets focus and keeps it even after correcting the typo. I guess it's possible to cover both use cases by differentiating manual/automatic selection and only resetting in the latter case, but for now I've just kept the patch (it should be either dropped or squashed anyway) ...

Created attachment 208080 [details] [review]
search: Rename search_providers to open-search-providers

Only reattaching to maintain patch order

Created attachment 208081 [details] [review]
search: Add RemoteSearchProvider

(In reply to comment #16)
> Put the changes to search.js in a new file (remoteSearch.js or something)

Done.


> I'm not satisfied in the interface that we're exposing. Here's something I'm
> fine with:
> 
>  * Drop GetResultsMeta. It's a bit awkward in a local system, and is even more
> awkward when trying to shuttle this across DBus.
>  * GetInitialResultsSet/GetSubsequenceResultsSet: returns an "a{sv}" that
> contains either the things returned by GetResultsMeta, or a new key:

I've already mentioned that this may be extremely wasteful (e.g. sending data for 1000 search results although we'll only display six results) - async providers are now supposed to provide GetResultMetasAsync(), which I think is the better solution.


> ::: js/ui/search.js
> @@ +115,3 @@
> +    <arg type="s" direction="in" />
> +</method>
> +</interface>
> 
> Missing semicolon.

Fixed.

Created attachment 208082 [details] [review]
overview: Load RemoteSearchProviders

(In reply to comment #17)
> I'm sure the answer is "no", but do we want to support search providers in
> userdatdir or in another system data dir (/usr/local/?)

I've gone with XDG_DATA_DIRS for now. Now that we scan multiple directories, I should filter out duplicates - I'll look into that tomorrow.


> Do we want to have a file-monitor if a new file comes into the datadir or any
> of these places?

Yeah, I've got some work-in-progress patch locally, but it's too late to clean that up now ;-)


> @@ +254,3 @@
> +    _loadRemoteSearchProviders: function() {
> +        let dir = Gio.file_new_for_path(global.datadir + '/search-providers');
> +        FileUtils.listDirAsync(dir, Lang.bind(this, function(files) {
> 
> Since we're putting the remote search providers in a new file, let's have a:
> 
>     RemoteSearch.scanRemoteSearchProviders(function(title, icon, busName,
> objectPath) {
>     });

Done.


> @@ +264,3 @@
> +                    continue;
> +
> +                let group = 'Shell Search Provider';
> 
> const KEY_FILE_GROUP = 'Shell Search Provider';

Done.


> @@ +265,3 @@
> +
> +                let group = 'Shell Search Provider';
> +                let title = keyfile.get_string(group, 'Title');
> 
> keyfile.get_locale_string(group, 'Title', null);
> 
> Unless we're assuming that keyfiles would be hard-localized on disk.

Done.

Review of attachment 208081 [details] [review]:

(In reply to comment #37)
> > I'm not satisfied in the interface that we're exposing. Here's something I'm
> > fine with:
> > 
> >  * Drop GetResultsMeta. It's a bit awkward in a local system, and is even more
> > awkward when trying to shuttle this across DBus.
> >  * GetInitialResultsSet/GetSubsequenceResultsSet: returns an "a{sv}" that
> > contains either the things returned by GetResultsMeta, or a new key:
> 
> I've already mentioned that this may be extremely wasteful (e.g. sending data
> for 1000 search results although we'll only display six results) - async
> providers are now supposed to provide GetResultMetasAsync(), which I think is
> the better solution.

As I said above, the problem with this is that we can potentially get metas for things we don't need if the user is a fast typer: "a" will generate 52 results, so we go out and fetch metas, and then the user now searches for "ab", and we cancel the current request and ask for a new one containing most of the same search terms as before.

Looking at the patch, it seems we're not caching metas either - if we have an ID that's the same as before, we're still fetching the meta for it.

Review of attachment 208081 [details] [review]:

(In reply to comment #37)
> > I'm not satisfied in the interface that we're exposing. Here's something I'm
> > fine with:
> > 
> >  * Drop GetResultsMeta. It's a bit awkward in a local system, and is even more
> > awkward when trying to shuttle this across DBus.
> >  * GetInitialResultsSet/GetSubsequenceResultsSet: returns an "a{sv}" that
> > contains either the things returned by GetResultsMeta, or a new key:
> 
> I've already mentioned that this may be extremely wasteful (e.g. sending data
> for 1000 search results although we'll only display six results) - async
> providers are now supposed to provide GetResultMetasAsync(), which I think is
> the better solution.

As I said above, the problem with this is that we can potentially get metas for things we don't need if the user is a fast typer: "a" will generate 52 results, so we go out and fetch metas, and then the user now searches for "ab", and we cancel the current request and ask for a new one containing most of the same search terms as before.

Looking at the patch, it seems we're not caching metas either - if we have an ID that's the same as before, we're still fetching the meta for something we already have.

Review of attachment 208082 [details] [review]:

::: js/ui/remoteSearch.js
@@ +49,3 @@
+        for (let i = 0; i < files.length; i++) {
+            let keyfile = new GLib.KeyFile();
+            let path = dirPath + '/' + files[i].get_name();

GLib.build_filenamev (ugh, if only everything in GLib could take a GFile instead of/in addition to a path name. GLib 3.0, I guess)

@@ +68,3 @@
+                let objectPath = keyfile.get_string(group, 'ObjectPath');
+
+                remoteProvider = new RemoteSearchProvider(title,

Not exactly what I meant for this API, but I guess it really doesn't matter too much.

(I was talking about something that split the act of loading the key file datas from the directories and building the actual search provider from it)

@@ +73,3 @@
+                                                          objectPath);
+            } catch(e) {
+                log('Failed to add search provider "%s"'.format(title));

If we want to fail noisily, we should just let the exception propagate. If someone's shipping a completely broken keyfile, a log warning in ~/.xsession-errors is quite invisible.

Review of attachment 208081 [details] [review]:

That said, I'm happy with this. It's async, it's not blocking the compositor, and this is all local, so there's no big latency delay. This is fine to land, caching would be icing on the cake at this point.

Let's just kill the word "Metas" from the public API. It's not "metadata", the icon and title are the data itself. "getResultDisplays" or something less awkward would be fine with me.

::: js/ui/remoteSearch.js
@@ +40,3 @@
+                                              dbusName, dbusPath);
+
+        this.title = title.toUpperCase();

this.parent(title.toUpperCase());

@@ +78,3 @@
+                                                  this._cancellable);
+        } catch(e) {
+            log('Error calling GetInitialResultSet for provider ' + this.title);

Again, if we're going to fail noisily, let's propagate the error.

Review of attachment 208077 [details] [review]:

As a curiosity, how hard would it be to make all search providers async? That would clean up the code considerably, here... (not saying it's needed for this patch/bug to land, just a possible code cleanup in the future).

(Mini-rant while trying to review the patch set: Can we remove the word "meta" from the public API, and perhaps purge it from the entire codebase, restricting it only to meaning "short for Metacity"? Between "search provider metas", "result metas", "extension metas", I'm sick of the word and it's lost all meaning... not that it really had any to begin with.)

> selectDown() selects the next visible results actor. An actor may be hidden
> either because there aren't any results for its provider, or because its
> results are still pending. For manual selection we don't care about that
> distinction and just call selectDown(), but for automatic selection we only
> want to skip actors without results (and bail out early if there's an actor
> with pending results before the first visible actor). So it's either adding an
> additional mode parameter to selectDown(), or using a separate function for
> automatic selection.

rtcm is working on a search selection rework. Have you talked to him about the correct behavior? I just want to make sure we're all on the same page here.

Otherwise, this looks fine.

::: js/ui/search.js
@@ +383,3 @@
+
+        this._previousResults[i][1] = results;
+        this._previousResults[i][2] = false; // no longer pending

Not a fan of using lists as structs if we're going to do things like this.

While we're not using object destructuring in the Shell anywhere yet, it can help us with this below:

    let { provider: provider, previousResults: previousResults } = this._previousResults[i];

::: js/ui/searchDisplay.js
@@ +382,2 @@
         let meta = this._metaForProvider(provider);
+        meta.hasPendingResults = pending;

Why both hasPendingResults in the providerMeta, and pending in the results? They are the same flag, right? Are they ever out of sync? Can we 'merge' the two?

Review of attachment 208080 [details] [review]:

Sure thing.

Review of attachment 208079 [details] [review]:

Yeah, that makes sense. Of course, I'm not sure I know anybody that likes the OpenSearch system (broadcast your typos to the world, thanks), so I filed bug #670168 a little while ago. Design input there?

Squash ahoy.

Review of attachment 208076 [details] [review]:

::: js/ui/searchDisplay.js
@@ +120,3 @@
+                    return;
+
+                let metas = provider.getResultMetas(results);

Huh, it doesn't look like you change this to a potentially async call in the future.

@@ +131,3 @@
+    getResultsForDisplay: function() {
+        let alreadyVisible = this._pendingClear ? 0
+                                                : this._grid.visibleItemsCount();

This doesn't need line wrap.

@@ +148,3 @@
         this._notDisplayedResult = results.slice(0);
         this._terms = terms.slice(0);
+        this._pendingClear = true;

I'm confused at what's happening here. The only thing this seems to change is the number of new children that we return in getResultsForDisplay, but it seems that both calls to setResults are preceded by a call to clear.

(In reply to comment #41)
> @@ +73,3 @@
> +                                                          objectPath);
> +            } catch(e) {
> +                log('Failed to add search provider "%s"'.format(title));
> 
> If we want to fail noisily, we should just let the exception propagate. If
> someone's shipping a completely broken keyfile, a log warning in
> ~/.xsession-errors is quite invisible.

I don't think allowing a broken keyfile to break unrelated search providers is a good idea. It's unfortunate that we have the choice between "hide a warning in an obscure dot file" and "break the world", but I really think the former is the lesser evil (just imagine we did the same for .desktop files, and someone installed an erroneous aardvark.desktop ...)

(In reply to comment #43)
> rtcm is working on a search selection rework. Have you talked to him about the
> correct behavior? I just want to make sure we're all on the same page here.

It's mostly "done" actually. Bug 663901. But it will have to be rebased on this work it seems.

Created attachment 208122 [details] [review]
search: Replace getResultMeta() with getResultMetas()

Fixed indentation and comment; I've kept the "meta" name though - "ResultDisplay" is already taken, so the only other names I can think of are "ResultInfo" and "ResultData", which are equally vague (and even worse, as we don't indicate plural) ...

Created attachment 208125 [details] [review]
searchDisplay: Split renderResults()

(In reply to comment #46)
> @@ +120,3 @@
> +                    return;
> +
> +                let metas = provider.getResultMetas(results);
> 
> Huh, it doesn't look like you change this to a potentially async call in the
> future.

Uhm - yes? It is in the "make async providers more explicit" where it should be ...


> @@ +131,3 @@
> +    getResultsForDisplay: function() {
> +        let alreadyVisible = this._pendingClear ? 0
> +                                                :
> this._grid.visibleItemsCount();
> 
> This doesn't need line wrap.

It does in an 80-character terminal, but I don't care deeply - fixed :)


> @@ +148,3 @@
>          this._notDisplayedResult = results.slice(0);
>          this._terms = terms.slice(0);
> +        this._pendingClear = true;
> 
> I'm confused at what's happening here. The only thing this seems to change is
> the number of new children that we return in getResultsForDisplay, but it seems
> that both calls to setResults are preceded by a call to clear.

Yeah, you're right - setResults() and clearDisplay() are swapped in the async patch (otherwise we would clear the display, wait for getResultMetasAsync() and render the results => flickering). I've left it here for now, but I can move it to the async patch if you want.

Created attachment 208131 [details] [review]
search: Make asynchronous providers more explicit

(In reply to comment #43)
> Review of attachment 208077 [details] [review]:
> 
> As a curiosity, how hard would it be to make all search providers async? That
> would clean up the code considerably, here... (not saying it's needed for this
> patch/bug to land, just a possible code cleanup in the future).

In the future I'd like to see most search providers moved out of the shell actually - "contacts" should really be provided by gnome-contacts itself, "places" could be provided by nautilus (though maybe it should just die altogether) ... basically just leaving application search. Making that provider asynchronous should certainly be possible (but obviously not in the scope for this patch).


> (Mini-rant while trying to review the patch set: Can we remove the word "meta"
> from the public API, and perhaps purge it from the entire codebase, restricting
> it only to meaning "short for Metacity"? Between "search provider metas",
> "result metas", "extension metas", I'm sick of the word and it's lost all
> meaning... not that it really had any to begin with.)

Well, yes - it's one of those "data", "handler", "manager", "info" etc words which are most often than not just vague. Just missing an alternative which is actually better for now (though I've dealt with _two_ "metas" in the same function, so I sympathize with the rant ;-)


> rtcm is working on a search selection rework. Have you talked to him about the
> correct behavior? I just want to make sure we're all on the same page here.

We haven't really talked about it, but I'm not changing much wrt behavior here - apart from the one-line change which resets the selection when updating the search terms, my changes are only about fixing brokenness for async providers.


> ::: js/ui/search.js
> @@ +383,3 @@
> +
> +        this._previousResults[i][1] = results;
> +        this._previousResults[i][2] = false; // no longer pending
> 
> Not a fan of using lists as structs if we're going to do things like this.

I've cleaned it up a bit, but still using an array (as before); I don't mind changing it to an object later, but this is really an unrelated clean-up, so I prefer leaving it out for now.


> ::: js/ui/searchDisplay.js
> @@ +382,2 @@
>          let meta = this._metaForProvider(provider);
> +        meta.hasPendingResults = pending;
> 
> Why both hasPendingResults in the providerMeta, and pending in the results?
> They are the same flag, right? Are they ever out of sync? Can we 'merge' the
> two?

The idea was to set "pending" in the search system (where the decision of whether to use sync or async queries is made) and pass it on to the display. It is possible to kill the former if we assume that async providers have their results pending until we receive the 'search-updated' signal - did just that now.

Created attachment 208134 [details] [review]
search: Add RemoteSearchProvider

Add an asynchronous search provider for results from a DBus service
implementing the org.gnome.Shell.SearchProvider interface; this
will allow applications to hook into the Shell's search without
implementing it in Shell itself or requiring an extension.

Created attachment 208135 [details] [review]
overview: Load RemoteSearchProviders

Allow applications to register search providers by dropping a keyfile
into a well-known directory. For now, initialize all found providers;
long term, we probably want to give users the ability to restrict the
set of active search providers.

Created attachment 208159 [details] [review]
searchDisplay: Split renderResults()

Move the "pending" part into the next patch (where it hopefully makes more sense).

Created attachment 208160 [details] [review]
search: Make asynchronous providers more explicit

Dto.

Review of attachment 208122 [details] [review]:

Yep.

Review of attachment 208122 [details] [review]:

Yep.

::: js/ui/wanda.js
@@ +171,3 @@
+    getResultMetas: function(fish) {
+        return [{ 'id': fish[0], // there may be many fish in the sea, but
+                                 // only one which speaks truth!

"speaks the truth" :)

Review of attachment 208135 [details] [review]:

Looks fine.

Review of attachment 208134 [details] [review]:

Code looks good.

::: js/ui/remoteSearch.js
@@ +30,3 @@
+
+/**
+ * RemoteSearchProvider:

Useless comment, remove.

@@ +45,3 @@
+    },
+
+    createIcon: function(size, icon) {

function(size, meta)

@@ +58,3 @@
+        }
+
+        // Ugh, but we want to fall back to something ...

Why? Can't we just require 'gicon' or 'icon-data'?

(In reply to comment #59)
> +        // Ugh, but we want to fall back to something ...
> 
> Why? Can't we just require 'gicon' or 'icon-data'?

You mean 'require' as in 'remove results which don't provide one of them'? Yeah, could do that ...

Review of attachment 208160 [details] [review]:

Looks fine.

Review of attachment 208159 [details] [review]:

Sure.

Attachment 208080 [details] pushed as 89fe43f - search: Rename search_providers to open-search-providers
Attachment 208122 [details] pushed as 53d9ea7 - search: Replace getResultMeta() with getResultMetas()
Attachment 208134 [details] pushed as f6749fb - search: Add RemoteSearchProvider
Attachment 208135 [details] pushed as 34c6ff9 - overview: Load RemoteSearchProviders
Attachment 208159 [details] pushed as eb0d803 - searchDisplay: Split renderResults()
Attachment 208160 [details] pushed as e2c66ce - search: Make asynchronous providers more explicit