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 689735 - Preparations for Search redesign
Preparations for Search redesign
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 685224 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-12-05 20:44 UTC by Cosimo Cecchi
Modified: 2012-12-06 20:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
remote-search: add a Version property to the DBus interface (1.32 KB, patch)
2012-12-05 20:44 UTC, Cosimo Cecchi
reviewed Details | Review
remote-search: add a LaunchSearch() DBus method (3.36 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
reviewed Details | Review
remote-search: document a 'description' key in the meta dictionary (1.63 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
search: remove additional params from activateResult() call (3.25 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
remote-search: add ActivateResult2() method (4.34 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
reviewed Details | Review
remote-search: properly document the DBus interface (13.49 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
reviewed Details | Review
app-display: use a GAppInfo for the settings provider (1.70 KB, patch)
2012-12-05 20:45 UTC, Cosimo Cecchi
reviewed Details | Review
remote-search: properly document the DBus interface (10.08 KB, patch)
2012-12-05 22:45 UTC, Cosimo Cecchi
committed Details | Review
remote-search: add an org.gnome.ShellSearchProvider2 dbus interface (7.82 KB, patch)
2012-12-05 22:45 UTC, Cosimo Cecchi
committed Details | Review
remote-search: first implementation of SearchProvider2 (3.98 KB, patch)
2012-12-05 22:45 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
remote-search: implement LaunchSearch() DBus method (2.00 KB, patch)
2012-12-05 22:45 UTC, Cosimo Cecchi
reviewed Details | Review
search: remove additional params from activateResult() call (3.25 KB, patch)
2012-12-05 22:45 UTC, Cosimo Cecchi
committed Details | Review
remote-search: implement new ActivateResult() method (2.24 KB, patch)
2012-12-05 22:46 UTC, Cosimo Cecchi
reviewed Details | Review
app-display: use a GAppInfo for the settings provider (1.70 KB, patch)
2012-12-05 22:46 UTC, Cosimo Cecchi
committed Details | Review
remote-search: first implementation of SearchProvider2 (6.48 KB, patch)
2012-12-06 14:11 UTC, Cosimo Cecchi
committed Details | Review
remote-search: implement LaunchSearch() DBus method (2.34 KB, patch)
2012-12-06 14:13 UTC, Cosimo Cecchi
committed Details | Review
remote-search: implement new ActivateResult() method (2.20 KB, patch)
2012-12-06 14:15 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2012-12-05 20:44:55 UTC
This patchset covers the non-UI part of the changes, and will be used as a base for the future redesign.
Comment 1 Cosimo Cecchi 2012-12-05 20:44:58 UTC
Created attachment 230817 [details] [review]
remote-search: add a Version property to the DBus interface

We are going to add new methods, and make changes; to be compatible
we'll need a version property.
Comment 2 Cosimo Cecchi 2012-12-05 20:45:01 UTC
Created attachment 230818 [details] [review]
remote-search: add a LaunchSearch() DBus method

This will be used to launch a search in the application itself.
Comment 3 Cosimo Cecchi 2012-12-05 20:45:04 UTC
Created attachment 230819 [details] [review]
remote-search: document a 'description' key in the meta dictionary

This can be specified by the provider and will be displayed in the
search results.
Comment 4 Cosimo Cecchi 2012-12-05 20:45:07 UTC
Created attachment 230820 [details] [review]
search: remove additional params from activateResult() call

The only case when we're interested in using those parameters nowadays
is for DnD, which is handled in a separate method already.
Since we're not going to support DnD for non-app search results anyway,
drop the params from all the activateResults() calls; this will be
useful later since we're going to add another parameter to it.
Comment 5 Cosimo Cecchi 2012-12-05 20:45:10 UTC
Created attachment 230821 [details] [review]
remote-search: add ActivateResult2() method

This allows us to fix the shortcomings of the original ActivateResult()
method. In particular:
- allow to pass the search terms to the provider
- allow to pass a user interaction timestamp
Comment 6 Cosimo Cecchi 2012-12-05 20:45:12 UTC
Created attachment 230822 [details] [review]
remote-search: properly document the DBus interface

Use real annotations in the XML, and use gdbus-codegen to extract those
into docbook. We then include it in the Shell's gtk-doc.
Comment 7 Cosimo Cecchi 2012-12-05 20:45:16 UTC
Created attachment 230823 [details] [review]
app-display: use a GAppInfo for the settings provider

this._gnomecc is currently unused; we actually need a GAppInfo for this
provider if we want to display an icon next to it (see future commits),
so just turn it into one.
We might move this to an external provider altogether in the future.
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:48:19 UTC
Review of attachment 230817 [details] [review]:

I thought the recommended solution was to have it be "org.gnome.ShellSearchProvider1"?
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:49:58 UTC
Review of attachment 230820 [details] [review]:

Sure.
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:50:16 UTC
Review of attachment 230819 [details] [review]:

Sure.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:51:27 UTC
Review of attachment 230818 [details] [review]:

Should we handle this for settings as well?
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:52:16 UTC
Review of attachment 230821 [details] [review]:

mmm, I'd prefer if we did this through the versioning of the interface, replacing the old method.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:53:21 UTC
Review of attachment 230822 [details] [review]:

I thought the <doc:doc> things were the recommended way to document interfaces?
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-12-05 20:54:44 UTC
Review of attachment 230823 [details] [review]:

A bit strange, but sure.

::: js/ui/appDisplay.js
@@ +412,3 @@
+
+    launchSearch: function(terms) {
+        // FIXME: this should be a remote search provider

Does this get fixed?
Comment 15 Cosimo Cecchi 2012-12-05 22:45:01 UTC
Created attachment 230847 [details] [review]
remote-search: properly document the DBus interface

Use real annotations in the XML, and use gdbus-codegen to extract those
into docbook. We then include it in the Shell's gtk-doc.
Comment 16 Cosimo Cecchi 2012-12-05 22:45:20 UTC
Created attachment 230848 [details] [review]
remote-search: add an org.gnome.ShellSearchProvider2 dbus interface

We are going to change the interface, so add a new version of it.
Providers will need to opt-in to the new API.
A summary of the differences compared to the previous API:
- ActivateResult() now also takes the search terms and a timestamp as
  parameters
- a new LaunchSearch() method is added
Comment 17 Cosimo Cecchi 2012-12-05 22:45:31 UTC
Created attachment 230849 [details] [review]
remote-search: first implementation of SearchProvider2

We read the implemented version from the search provider's keyfile, and
then create the DBus proxy for the right interface accordingly.
Wire ActivateResult() to the new method (without actually passing the
new parameters along) - an actual implementation will be added in a
future commit.
Comment 18 Cosimo Cecchi 2012-12-05 22:45:45 UTC
Created attachment 230850 [details] [review]
remote-search: implement LaunchSearch() DBus method

This will be used to launch a search in the application itself.
Comment 19 Cosimo Cecchi 2012-12-05 22:45:54 UTC
Created attachment 230851 [details] [review]
search: remove additional params from activateResult() call

The only case when we're interested in using those parameters nowadays
is for DnD, which is handled in a separate method already.
Since we're not going to support DnD for non-app search results anyway,
drop the params from all the activateResults() calls; this will be
useful later since we're going to add another parameter to it.
Comment 20 Cosimo Cecchi 2012-12-05 22:46:01 UTC
Created attachment 230852 [details] [review]
remote-search: implement new ActivateResult() method

This allows us to fix the shortcomings of the original ActivateResult()
method. In particular:
- allow to pass the search terms to the provider
- allow to pass a user interaction timestamp
Comment 21 Cosimo Cecchi 2012-12-05 22:46:07 UTC
Created attachment 230853 [details] [review]
app-display: use a GAppInfo for the settings provider

this._gnomecc is currently unused; we actually need a GAppInfo for this
provider if we want to display an icon next to it (see future commits),
so just turn it into one.
We might move this to an external provider altogether in the future.
Comment 22 Cosimo Cecchi 2012-12-05 22:58:06 UTC
FWIW I think the implementation of the two different versions is clearer this way with a completely different interface. 
The only downside is that users who run a jhbuild shell inside an older session will now suffer a bug where we will read the version information from the ini file from the jhbuild prefix, but DBus will auto-activate the binary from the system prefix (so that provider of course won't work).
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:18:01 UTC
Review of attachment 230847 [details] [review]:

Works for me, except maybe we should investigate that XSLT thing that Mattias was talking about?
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:19:08 UTC
Review of attachment 230848 [details] [review]:

Fine by me.

::: data/org.gnome.ShellSearchProvider.xml
@@ +10,2 @@
       The interface used for integrating into GNOME Shell's search
+      interface. This interface is deprecated, and org.gnome.Shell.SearchProvider2 should be used instead.

Line wrapping.

::: data/org.gnome.ShellSearchProvider2.xml
@@ +16,3 @@
+        GetInitialResultSet:
+        @terms: Array of search terms, which the provider should treat as logical AND.
+        @results: An array of result identifier strings representing items which match the given search terms. Identifiers must be unique within the provider's domain, but other than that may be chosen freely by the provider.

Line wrapping!
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:20:05 UTC
Review of attachment 230849 [details] [review]:

::: js/ui/remoteSearch.js
@@ +200,3 @@
     Extends: Search.SearchProvider,
 
+    _init: function(appInfo, dbusName, dbusPath, version) {

I was imagining a separate "RemoteSearchProvider2" class that inherited from the first one, but I'm happy with this.
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:20:33 UTC
Review of attachment 230849 [details] [review]:

.
Comment 27 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:22:33 UTC
Review of attachment 230850 [details] [review]:

::: js/ui/remoteSearch.js
@@ +306,3 @@
+
+    canLaunchSearch: function() {
+        return (this._version >= 2);

Yeah, see, this is where the subclass would come in handy. This should really be a getter:

    get canLaunchSearch() { 
        return (this._version >= 2);
    },

If we switch over to the subclass strategy, we could simply put:

    this.canLaunchSearch = true;

in the _init.

::: js/ui/search.js
@@ +181,3 @@
+
+    canLaunchSearch: function() {
+        return false;

Given how few built-in search providers we have, I wonder if it makes sense to have this useless base class.
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:22:58 UTC
Review of attachment 230851 [details] [review]:

Sure.
Comment 29 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:24:14 UTC
Review of attachment 230852 [details] [review]:

::: js/ui/remoteSearch.js
@@ +301,2 @@
         if (this._version >= 2)
+            this._proxy.ActivateResultRemote(id, terms, global.get_current_time());

I'd prefer it if we actually got the timestamp from the event.

::: js/ui/searchDisplay.js
@@ +22,1 @@
     _init: function(provider, metaInfo, terms) {

.oO( We're creating a new SearchResult for every set of terms? We really gotta knock that off... )
Comment 30 Jasper St. Pierre (not reading bugmail) 2012-12-06 05:24:33 UTC
Review of attachment 230853 [details] [review]:

Sure, whatever.
Comment 31 Cosimo Cecchi 2012-12-06 14:11:12 UTC
Created attachment 230890 [details] [review]
remote-search: first implementation of SearchProvider2

--

Now in a separate subclass.
Comment 32 Cosimo Cecchi 2012-12-06 14:13:10 UTC
Created attachment 230891 [details] [review]
remote-search: implement LaunchSearch() DBus method

--

Addresses review comments. The base class is still there for now - I have some patches in a later set that clean some of that up; I will try to fold the base class into that cleanup as well.
Comment 33 Cosimo Cecchi 2012-12-06 14:15:46 UTC
Created attachment 230892 [details] [review]
remote-search: implement new ActivateResult() method

--

Updated for using the subclass.
Not sure if getting the timestamp from the event is worth the trouble here; shell_global_get_current_time() already does seem to return the timestamp of the latest clutter event, why duplicate that code? Also, many other parts in the shell codebase get it from the global object for similar purposes.
Comment 34 Cosimo Cecchi 2012-12-06 14:22:15 UTC
(In reply to comment #23)

> Works for me, except maybe we should investigate that XSLT thing that Mattias
> was talking about?

As far as I understand, using gdbus-codegen this way allows us to avoid shipping the XSLT script ourselves to get the same result.

(In reply to comment #24)

> Line wrapping!

There currently seems to be a bug in the gdbus-codegen docbook parser that messes up documentation for parameters if their comment is split on multiple lines. I filed it as https://bugzilla.gnome.org/show_bug.cgi?id=689767
Comment 35 Jasper St. Pierre (not reading bugmail) 2012-12-06 15:11:37 UTC
Review of attachment 230890 [details] [review]:

Yay.
Comment 36 Jasper St. Pierre (not reading bugmail) 2012-12-06 15:12:30 UTC
Review of attachment 230891 [details] [review]:

Sure.
Comment 37 Jasper St. Pierre (not reading bugmail) 2012-12-06 15:16:03 UTC
Review of attachment 230892 [details] [review]:

Sure.
Comment 38 Cosimo Cecchi 2012-12-06 17:22:30 UTC
Attachment 230847 [details] pushed as 1ef9ee1 - remote-search: properly document the DBus interface
Attachment 230848 [details] pushed as b390b82 - remote-search: add an org.gnome.ShellSearchProvider2 dbus interface
Attachment 230851 [details] pushed as 9b846cb - search: remove additional params from activateResult() call
Attachment 230853 [details] pushed as a43ee41 - app-display: use a GAppInfo for the settings provider
Attachment 230890 [details] pushed as 72d54d9 - remote-search: first implementation of SearchProvider2
Attachment 230891 [details] pushed as 2cc7fd0 - remote-search: implement LaunchSearch() DBus method
Attachment 230892 [details] pushed as ec7ade4 - remote-search: implement new ActivateResult() method
Comment 39 Cosimo Cecchi 2012-12-06 20:55:44 UTC
*** Bug 685224 has been marked as a duplicate of this bug. ***