Support for this has already been added on the gnome-documents side, so this needs to be implemented on the shell side now.

*** Bug 670122 has been marked as a duplicate of this bug. ***

Created attachment 207679 [details] [review]
utils: Add helper to serialize Pixbufs as GVariants

Support for GVariants in GJS is quite good nowadays, but not quite
good enough to serialize a GdkPixbuf ...

Created attachment 207680 [details] [review]
Implement a ShellSearchProvider

GNOME Shell provides a DBus interface for external search provider
implementations; add a small auto-activated service which implements
that interface, so that search results for GNOME Documents show up
in the Shell's overview.

(In reply to comment #1)
> Support for this has already been added on the gnome-documents side, so this
> needs to be implemented on the shell side now.

For the shell side, see bug 663125; the particular search provider should still be implemented in gnome-documents, so re-assigning again ...

Review of attachment 207680 [details] [review]:

Hey Florian, thanks for this...looks generally good!
I have some comments below.

::: src/shellSearchProvider.js
@@ +71,3 @@
+</interface>;
+
+function Application() {

s/Application/ShellSearchProvider

@@ +92,3 @@
+    _onBusAcquired: function() {
+        this._dbusImpl = Gio.DBusExportedObject.wrapJSObject(SearchProviderIface, this);
+        this._dbusImpl.export(Gio.DBus.session, SEARCH_PROVIDER_PATH);

You can make this._dbusImpl a local variable to this function.

@@ +138,3 @@
+        Global.collectionManager = new Manager.BaseManager();
+        Global.documentManager = new Documents.DocumentManager();
+        Global.trackerController = new TrackerController.TrackerController();

It's a little unfortunate that you need to create all these global objects here...is it really needed? If so, we should probably make initialization of some of these lazy (not in the scope of this patch anyway).

@@ +143,3 @@
+    _resetTimeout: function() {
+        if (this._timeoutId)
+            Mainloop.source_remove(this._timeoutId);

Reset this._timeoutId to 0 here for consistency.

@@ +223,3 @@
+
+        let pixbufs = [];
+        collectionUrns.forEach(Lang.bind(this, function(urn) {

Indentation:
collectionUrns.forEach(Lang.bind(this,
  function(urn) {
  }));

@@ +235,3 @@
+                log("Failed to query tracker: " + e);
+                cursor.close();
+            }

I think in case you get the exception or the call to cursor.next() fails, you are supposed to skip the forEach cycle iteration.

@@ +253,3 @@
+                    log("Unable to load pixbuf: " + e);
+                }
+            } else if (icon.load) {

Use the same instanceof Gio.FileIcon trick here instead of looking for the presence of the load method.

@@ +310,3 @@
+
+            this._cache[result.id] = result;
+            ids.push(result.id);

I think it's more readable to have this as

this._cache[urn] = result;
ids.push(urn);

@@ +336,3 @@
+            meta['gicon'] = GLib.Variant.new('s', gicon.to_string());
+        else if (pixbuf)
+            meta['icon-data'] = Gd.create_variant_from_pixbuf(pixbuf);

Can it happen that both gicon and pixbuf are null? If it can't happen, we should remove the if(pixbuf) from the else branch, otherwise we should probably have another fallback? What happens if we don't provide any icon data at all to the shell?

@@ +342,3 @@
+
+    ActivateResult: function(id) {
+        try {

I think it's better to do something like

let app = Gio.DesktopAppInfo.new('gnome-documents');
app.launch([ id ]);

You should also be able to remove BIN_DIR from Path at that point.

Review of attachment 207679 [details] [review]:

This looks good.

::: src/lib/gd-utils.c
@@ +659,3 @@
+/**
+ * gd_create_variant_from_pixbuf:
+ * @pixbuf: (transfer none):

I don't think this transfer none annotation actually does anything here

(In reply to comment #7)
> I don't think this transfer none annotation actually does anything here

Right. I had added it anyway as I've seen warning spam due to GI dropping assumptions about default values in case of missing annotations, but I don't have any problem dropping it either - not attaching again though, as the change is completely trivial.

Created attachment 208059 [details] [review]
Implement a ShellSearchProvider

Apart from addressing comments from the review, there are some further changes:

 - GetResultMeta() is now GetResultMetas()
   (e.g. rather than calling it once for each result we want displayed, we call
    it once for all results we want displayed)

 - only get matching URNs in _doSearch
   The reason I did populate the cache there was that GetResultMeta() was called
   synchronously before, so it made sense to do expensive operations like
   pixbuf generation in an asynchronously called method; however, I'm gonna
   change the shell part to call all DBus methods asynchronously, so it makes
   now sense to only create the cache for items that are actually displayed


(In reply to comment #6)
> ::: src/shellSearchProvider.js
> @@ +71,3 @@
> +</interface>;
> +
> +function Application() {
> 
> s/Application/ShellSearchProvider

Sure.


> @@ +92,3 @@
> You can make this._dbusImpl a local variable to this function.

Oh, I was worried that GC would kick in in that case - looks you're right though, fixed.


> @@ +138,3 @@
> +        Global.collectionManager = new Manager.BaseManager();
> +        Global.documentManager = new Documents.DocumentManager();
> +        Global.trackerController = new TrackerController.TrackerController();
> 
> It's a little unfortunate that you need to create all these global objects
> here...is it really needed? If so, we should probably make initialization of
> some of these lazy (not in the scope of this patch anyway).

Yeah, the dependency list is a bit crazy here - as few of those are actually used directly, it's probably possible to cut down the list by making some of those optional (e.g. only initialize them when in "ui-mode").


> @@ +143,3 @@
> +    _resetTimeout: function() {
> +        if (this._timeoutId)
> +            Mainloop.source_remove(this._timeoutId);
> 
> Reset this._timeoutId to 0 here for consistency.

Mmmh, "foo = 0; foo = new_value();" is a bit pointless, but sure :-)


> @@ +223,3 @@
> Indentation:
> collectionUrns.forEach(Lang.bind(this,
>   function(urn) {
>   }));

Fixed.


> @@ +235,3 @@
> +                log("Failed to query tracker: " + e);
> +                cursor.close();
> +            }
> 
> I think in case you get the exception or the call to cursor.next() fails, you
> are supposed to skip the forEach cycle iteration.

Yes. Somehow I didn't get a collection icon in that case, but that was actually a separate bug. Fixed.


> @@ +253,3 @@
> +                    log("Unable to load pixbuf: " + e);
> +                }
> +            } else if (icon.load) {
> 
> Use the same instanceof Gio.FileIcon trick here instead of looking for the
> presence of the load method.

The idea was that "load" is a method on the GLoadableIcon interface, and we cannot check for interfaces. But as we only use GThemedIcon or GFileIcon, this is of course completely irrelevant, so I made the change.


> @@ +310,3 @@
> I think it's more readable to have this as
> 
> this._cache[urn] = result;
> ids.push(urn);

I agree, though the code is no longer be present.


> @@ +336,3 @@
> +            meta['gicon'] = GLib.Variant.new('s', gicon.to_string());
> +        else if (pixbuf)
> +            meta['icon-data'] = Gd.create_variant_from_pixbuf(pixbuf);
> 
> Can it happen that both gicon and pixbuf are null? If it can't happen, we
> should remove the if(pixbuf) from the else branch, otherwise we should probably
> have another fallback? What happens if we don't provide any icon data at all to
> the shell?

It should not happen in practise, but it is possible - pixbuf is only used for collections (and gicon _will_ be set for everything else), so pixbuf is actually the result of gd_create_collection_icon(). As far as I can see, that method returns NULL if gdk_pixbuf_get_from_surface() fails (which I suspect will only happen here in cases like OOM where a search provider is the least of our worries) ...

I guess we could add a fallback here, though there already is one in gnome-shell - I've left it as-is for now, but I don't have a strong opinion about it, so feel free to complain.


> @@ +342,3 @@
> I think it's better to do something like
> 
> let app = Gio.DesktopAppInfo.new('gnome-documents');
> app.launch([ id ]);

Mmmh, OK - g_app_info_launch() does not work here (because it expects actual files rather than arbitrary arguments?), but g_app_info_launch_uris() does; still a bit ugly though, as a URN is neither a file nor a URI ... not going to argue though :-)


> You should also be able to remove BIN_DIR from Path at that point.

Sure, fixed.

Created attachment 208060 [details] [review]
Implement a ShellSearchProvider

Implement a ShellSearchProvider

Apart from addressing comments from the review, there are some further changes:

 - GetResultMeta() is now GetResultMetas()
   (e.g. rather than calling it once for each result we want displayed, we call
    it once for all results we want displayed)

 - only get matching URNs in _doSearch
   The reason I did populate the cache there was that GetResultMeta() was called
   synchronously before, so it made sense to do expensive operations like
   pixbuf generation in an asynchronously called method; however, I'm gonna
   change the shell part to call all DBus methods asynchronously, so it makes
   now sense to only create the cache for items that are actually displayed


(In reply to comment #6)
> ::: src/shellSearchProvider.js
> @@ +71,3 @@
> +</interface>;
> +
> +function Application() {
> 
> s/Application/ShellSearchProvider

Sure.


> @@ +92,3 @@
> You can make this._dbusImpl a local variable to this function.

Oh, I was worried that GC would kick in in that case - looks you're right though, fixed.


> @@ +138,3 @@
> +        Global.collectionManager = new Manager.BaseManager();
> +        Global.documentManager = new Documents.DocumentManager();
> +        Global.trackerController = new TrackerController.TrackerController();
> 
> It's a little unfortunate that you need to create all these global objects
> here...is it really needed? If so, we should probably make initialization of
> some of these lazy (not in the scope of this patch anyway).

Yeah, the dependency list is a bit crazy here - as few of those are actually used directly, it's probably possible to cut down the list by making some of those optional (e.g. only initialize them when in "ui-mode").


> @@ +143,3 @@
> +    _resetTimeout: function() {
> +        if (this._timeoutId)
> +            Mainloop.source_remove(this._timeoutId);
> 
> Reset this._timeoutId to 0 here for consistency.

Mmmh, "foo = 0; foo = new_value();" is a bit pointless, but sure :-)


> @@ +223,3 @@
> Indentation:
> collectionUrns.forEach(Lang.bind(this,
>   function(urn) {
>   }));

Fixed.


> @@ +235,3 @@
> +                log("Failed to query tracker: " + e);
> +                cursor.close();
> +            }
> 
> I think in case you get the exception or the call to cursor.next() fails, you
> are supposed to skip the forEach cycle iteration.

Yes. Somehow I didn't get a collection icon in that case, but that was actually a separate bug. Fixed.


> @@ +253,3 @@
> +                    log("Unable to load pixbuf: " + e);
> +                }
> +            } else if (icon.load) {
> 
> Use the same instanceof Gio.FileIcon trick here instead of looking for the
> presence of the load method.

The idea was that "load" is a method on the GLoadableIcon interface, and we cannot check for interfaces. But as we only use GThemedIcon or GFileIcon, this is of course completely irrelevant, so I made the change.


> @@ +310,3 @@
> I think it's more readable to have this as
> 
> this._cache[urn] = result;
> ids.push(urn);

I agree, though the code is no longer be present.


> @@ +336,3 @@
> +            meta['gicon'] = GLib.Variant.new('s', gicon.to_string());
> +        else if (pixbuf)
> +            meta['icon-data'] = Gd.create_variant_from_pixbuf(pixbuf);
> 
> Can it happen that both gicon and pixbuf are null? If it can't happen, we
> should remove the if(pixbuf) from the else branch, otherwise we should probably
> have another fallback? What happens if we don't provide any icon data at all to
> the shell?

It should not happen in practise, but it is possible - pixbuf is only used for collections (and gicon _will_ be set for everything else), so pixbuf is actually the result of gd_create_collection_icon(). As far as I can see, that method returns NULL if gdk_pixbuf_get_from_surface() fails (which I suspect will only happen here in cases like OOM where a search provider is the least of our worries) ...

I guess we could add a fallback here, though there already is one in gnome-shell - I've left it as-is for now, but I don't have a strong opinion about it, so feel free to complain.


> @@ +342,3 @@
> I think it's better to do something like
> 
> let app = Gio.DesktopAppInfo.new('gnome-documents');
> app.launch([ id ]);

Mmmh, OK - g_app_info_launch() does not work here (because it expects actual files rather than arbitrary arguments?), but g_app_info_launch_uris() does; still a bit ugly though, as a URN is neither a file nor a URI ... not going to argue though :-)


> You should also be able to remove BIN_DIR from Path at that point.

Sure, fixed.

(ugh, sorry for the spam - git-bz was throwing an exception, but apparently it _did_ manage to attach the new patch first)

Review of attachment 208060 [details] [review]:

Thanks, this looks almost ready to go.
Some more comments below, mostly minor things or style fixes.

::: src/shellSearchProvider.js
@@ +91,3 @@
+    },
+
+    _onBusAcquired: function() {

Do we need to reset the timeout here as well?

@@ +145,3 @@
+        if (this._timeoutId)
+            Mainloop.source_remove(this._timeoutId);
+        this._timeoutId = 0;

Yeah it's a bit pointless, but we might return from this if the provider is set to persist. We might add another return condition in the future, so it's better to stay on the safe side and ensure this._timeoutId is always reset to zero here.

Just make it like

if (this._timeoutId) {
  Mainloop.source_remove(this._timeoutId);
  this._timeoutId = 0;
}

@@ +203,3 @@
+
+    _createCollectionPixbuf: function(urn) {
+        let query, cursor;

You can declare these variables when getting the retval from the methods you call.

@@ +262,3 @@
+                    try {
+                        let [stream, type] = icon.load(Utils.getIconSize(),
+                                                       null);

Since you don't need |type| I'd rather this be

let stream = icon.load(blah)[0];

instead (it's the style we use everywhere else in Documents if we don't need the second argument) </nitpick>

@@ +329,3 @@
+
+        if (!title || title == '')
+            title = Gd.filename_strip_extension(filename);

If filename is NULL, strip_extension(filename) will return NULL as well; this could lead to a situation where we later try to create a string variant with a NULL string (in GetResultMetas), which I don't think is valid.
You can probably just add a 

if (!filename)
  filename = ''; 

after this as a fix.

Review of attachment 208060 [details] [review]:

Thanks, this looks almost ready to go.
Some more comments below, mostly minor things or style fixes.

::: src/shellSearchProvider.js
@@ +91,3 @@
+    },
+
+    _onBusAcquired: function() {

Do we need to reset the timeout here as well?

@@ +145,3 @@
+        if (this._timeoutId)
+            Mainloop.source_remove(this._timeoutId);
+        this._timeoutId = 0;

Yeah it's a bit pointless, but we might return from this if the provider is set to persist. We might add another return condition in the future, so it's better to stay on the safe side and ensure this._timeoutId is always reset to zero here.

Just make it like

if (this._timeoutId) {
  Mainloop.source_remove(this._timeoutId);
  this._timeoutId = 0;
}

@@ +203,3 @@
+
+    _createCollectionPixbuf: function(urn) {
+        let query, cursor;

You can declare these variables when getting the retval from the methods you call.

@@ +262,3 @@
+                    try {
+                        let [stream, type] = icon.load(Utils.getIconSize(),
+                                                       null);

Since you don't need |type| I'd rather this be

let stream = icon.load(blah)[0];

instead (it's the style we use everywhere else in Documents if we don't need the second argument) </nitpick>

@@ +329,3 @@
+
+        if (!title || title == '')
+            title = Gd.filename_strip_extension(filename);

If filename is NULL, strip_extension(filename) will return NULL as well; this could lead to a situation where we later try to create a string variant with a NULL string (in GetResultMetas), which I don't think is valid.
You can probably just add a 

if (!filename)
  filename = ''; 

after this as a fix.

Created attachment 208132 [details] [review]
Implement a ShellSearchProvider

(In reply to comment #13)
> ::: src/shellSearchProvider.js
> @@ +91,3 @@
> +    },
> +
> +    _onBusAcquired: function() {
> 
> Do we need to reset the timeout here as well?

Mmmh, I don't think so? At this point, _resetTimeout() really serves as startTimeout(), and as I understand it, we cannot process requests before we acquire the name - which is the whole point of having been activated, so I don't see why we should timeout before.

I'm far from being a DBus expert though, so feel free to correct me.


> @@ +145,3 @@
> +        if (this._timeoutId)
> +            Mainloop.source_remove(this._timeoutId);
> +        this._timeoutId = 0;
> 
> Just make it like
> 
> if (this._timeoutId) {
>   Mainloop.source_remove(this._timeoutId);
>   this._timeoutId = 0;
> }

I usually prefer the style above to stress that the property will always be 0 after resetting, but sure - fixed.


> @@ +203,3 @@
> +
> +    _createCollectionPixbuf: function(urn) {
> +        let query, cursor;
> 
> You can declare these variables when getting the retval from the methods you
> call.

Yeah - I didn't do that because I was reusing those variables later. Fixed to use different local variables instead.


> @@ +262,3 @@
> Since you don't need |type| I'd rather this be
> 
> let stream = icon.load(blah)[0];

Sure.


> @@ +329,3 @@
> +
> +        if (!title || title == '')
> +            title = Gd.filename_strip_extension(filename);
> 
> If filename is NULL, strip_extension(filename) will return NULL as well; this
> could lead to a situation where we later try to create a string variant with a
> NULL string (in GetResultMetas), which I don't think is valid.

Hmm, bummer - we really, really want a title ... added a (somewhat lame) fallback now ...

Review of attachment 208132 [details] [review]:

Great! Looks good to commit after the shell part lands, and with these two suggestions below.

::: src/shellSearchProvider.js
@@ +175,3 @@
+
+        let ident = cursor.get_string(Query.QueryColumns.IDENTIFIER)[0];
+        let isRemote = ident && ident.indexOf('https://docs.google.com') != -1;

Missing braces around ident.indexOf('https://docs.google.com') != -1

@@ +330,3 @@
+
+        if (!title || title == '')
+            title = _('Unknown');

"Untitled Document" might be better, since it's consistent with e.g. the default filename Nautilus uses for newly created files.

Attachment 207679 [details] pushed as abb7fcf - utils: Add helper to serialize Pixbufs as GVariants
Attachment 208132 [details] pushed as 274adca - Implement a ShellSearchProvider