GNOME Bugzilla – Bug 689735
Preparations for Search redesign
Last modified: 2012-12-06 20:55:44 UTC
This patchset covers the non-UI part of the changes, and will be used as a base for the future redesign.
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.
Created attachment 230818 [details] [review] remote-search: add a LaunchSearch() DBus method This will be used to launch a search in the application itself.
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.
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.
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
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.
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.
Review of attachment 230817 [details] [review]: I thought the recommended solution was to have it be "org.gnome.ShellSearchProvider1"?
Review of attachment 230820 [details] [review]: Sure.
Review of attachment 230819 [details] [review]: Sure.
Review of attachment 230818 [details] [review]: Should we handle this for settings as well?
Review of attachment 230821 [details] [review]: mmm, I'd prefer if we did this through the versioning of the interface, replacing the old method.
Review of attachment 230822 [details] [review]: I thought the <doc:doc> things were the recommended way to document interfaces?
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?
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.
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
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.
Created attachment 230850 [details] [review] remote-search: implement LaunchSearch() DBus method This will be used to launch a search in the application itself.
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.
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
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.
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).
Review of attachment 230847 [details] [review]: Works for me, except maybe we should investigate that XSLT thing that Mattias was talking about?
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!
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.
Review of attachment 230849 [details] [review]: .
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.
Review of attachment 230851 [details] [review]: Sure.
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... )
Review of attachment 230853 [details] [review]: Sure, whatever.
Created attachment 230890 [details] [review] remote-search: first implementation of SearchProvider2 -- Now in a separate subclass.
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.
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.
(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
Review of attachment 230890 [details] [review]: Yay.
Review of attachment 230891 [details] [review]: Sure.
Review of attachment 230892 [details] [review]: Sure.
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
*** Bug 685224 has been marked as a duplicate of this bug. ***