After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 662453 - It should be possible to search for documents from within gnome-shell
It should be possible to search for documents from within gnome-shell
Status: RESOLVED FIXED
Product: gnome-documents
Classification: Core
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: GNOME documents maintainer(s)
GNOME documents maintainer(s)
: 670122 (view as bug list)
Depends on: 663125
Blocks: 670150
 
 
Reported: 2011-10-22 12:12 UTC by Julien Olivier
Modified: 2012-02-21 22:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
utils: Add helper to serialize Pixbufs as GVariants (2.28 KB, patch)
2012-02-15 16:51 UTC, Florian Müllner
committed Details | Review
Implement a ShellSearchProvider (17.45 KB, patch)
2012-02-15 16:51 UTC, Florian Müllner
reviewed Details | Review
Implement a ShellSearchProvider (17.91 KB, patch)
2012-02-20 20:14 UTC, Florian Müllner
none Details | Review
Implement a ShellSearchProvider (17.91 KB, patch)
2012-02-20 20:39 UTC, Florian Müllner
reviewed Details | Review
Implement a ShellSearchProvider (17.93 KB, patch)
2012-02-21 15:13 UTC, Florian Müllner
committed Details | Review

Description Julien Olivier 2011-10-22 12:12:40 UTC
I don't if it's a gnome-shell bug or a gnome-documents bug, but it seems there is no way to search for a document from gnome-shell's overview. It works well for gnome-contacts so I assumed it would be an expected feature.
Comment 1 Cosimo Cecchi 2012-01-16 16:57:56 UTC
Support for this has already been added on the gnome-documents side, so this needs to be implemented on the shell side now.
Comment 2 Julien Olivier 2012-02-15 09:13:04 UTC
*** Bug 670122 has been marked as a duplicate of this bug. ***
Comment 3 Florian Müllner 2012-02-15 16:51:10 UTC
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 ...
Comment 4 Florian Müllner 2012-02-15 16:51:18 UTC
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.
Comment 5 Florian Müllner 2012-02-15 16:53:38 UTC
(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 ...
Comment 6 Cosimo Cecchi 2012-02-19 15:48:58 UTC
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.
Comment 7 Cosimo Cecchi 2012-02-19 15:50:23 UTC
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
Comment 8 Florian Müllner 2012-02-20 19:34:11 UTC
(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.
Comment 9 Florian Müllner 2012-02-20 20:14:48 UTC
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.
Comment 10 Florian Müllner 2012-02-20 20:39:35 UTC
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.
Comment 11 Florian Müllner 2012-02-20 20:41:08 UTC
(ugh, sorry for the spam - git-bz was throwing an exception, but apparently it _did_ manage to attach the new patch first)
Comment 12 Cosimo Cecchi 2012-02-21 09:02:38 UTC
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.
Comment 13 Cosimo Cecchi 2012-02-21 09:05:45 UTC
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.
Comment 14 Florian Müllner 2012-02-21 15:13:09 UTC
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 ...
Comment 15 Cosimo Cecchi 2012-02-21 16:12:05 UTC
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.
Comment 16 Florian Müllner 2012-02-21 22:09:59 UTC
Attachment 207679 [details] pushed as abb7fcf - utils: Add helper to serialize Pixbufs as GVariants
Attachment 208132 [details] pushed as 274adca - Implement a ShellSearchProvider