GNOME Bugzilla – Bug 687491
Support settings for search providers
Last modified: 2012-11-19 17:04:47 UTC
See https://live.gnome.org/ThreePointSeven/Features/IntegratedApplicationSearch
Created attachment 227942 [details] [review] view-selector: add missing semicolon
Created attachment 227943 [details] [review] remote-search: require a DesktopId field in search providers We used to support providers specifying Title/Icon fields in the desktop file, and we do still read those for retro-compatibility. Since all the providers use DesktopId now though, make it mandatory, as it makes our life easier - and it's better to always associate remote providers with an application anyway.
Created attachment 227944 [details] [review] remote-search: remove unused icon parameter
Created attachment 227945 [details] [review] search: propagate application id to SearchProvider Save the application ID in the provider object; this will be used when filtering.
Created attachment 227946 [details] [review] view-selector: filter out disabled search providers If the added search provider has an appId in the list of the disabled search providers settings, ignore it.
Created attachment 227947 [details] [review] remote-search: restructure remote search provider loading process Instead of adding search providers to the system as we find them, wait until we loaded information from all the directories, and then add all providers at once. This will be useful when we will sort the providers information according to the sort order saved in GSettings.
Created attachment 227948 [details] [review] remote-search: apply sort-order from GSettings Sort providers according to the GSettings state, and make sure to reload them as soon as the sort order changes.
Created attachment 227949 [details] [review] view-selector: add support for disable-all search setting
Created attachment 227950 [details] [review] remote-search: don't use g_file_query_exists() This is called in the main thread, which we should never block for synchronous I/O. Since the operation we're wrapping is async already, just use g_file_query_info_async() instead.
Created attachment 227951 [details] [review] remote-search: catch errors when calling ActivateResult() Like we do for other methods that involve a GDBus proxy
Created attachment 227952 [details] [review] remote-search: initialize the DBus proxy asynchronously Initializing this synchronously means that we will possibly wait for the process to be auto-activated and answering to our call. If the process is already running it also might not answer immediately our request, as it might be doing sync I/O. The right thing to do is to initialize the proxy asynchronously; there are try/catch blocks in place for when the object is not available, or not properly initialized.
Review of attachment 227942 [details] [review]: Obviously yes.
Review of attachment 227943 [details] [review]: I'd strip out support for Title/Icon fields completely... either you did that and your commit message is confusing, or the parts that read it are somewhere else in the code.
Review of attachment 227944 [details] [review]: ::: js/ui/remoteSearch.js @@ +98,3 @@ Extends: Search.SearchProvider, + _init: function(title, dbusName, dbusPath) { The icon is used for the new search relayout. Perhaps we should just pass the AppInfo instead?
Review of attachment 227945 [details] [review]: Yeah, please pass the app info directly.
Review of attachment 227946 [details] [review]: ::: js/ui/viewSelector.js @@ +438,3 @@ }, + _filterSearchProvider: function(provider) { A better function name would be nice, like "_shouldUseSearchProvider" or something. @@ +443,3 @@ + + let disable = this._searchSettings.get_strv('disable'); + return (disable.indexOf(provider.appId) != -1); Lose the parens. @@ +452,3 @@ + providers.forEach(Lang.bind(this, + function(provider) { + if (!(provider instanceof RemoteSearch.RemoteSearchProvider)) ... yuck. I'd rather just have a separate list consisting of remote search providers. @@ +458,3 @@ + })); + + remove.forEach(Lang.bind(this, this.removeSearchProvider)); What's the point of the two forEaches, instead of one?
Review of attachment 227947 [details] [review]: The flow control in here is very unnatural. I'd prefer to re-think this a bit. ::: js/ui/remoteSearch.js @@ +56,3 @@ continue; + + loadState.numLoading++; This is the sort of place where Deferred/gatherResults would come in handy. @@ +83,3 @@ let objectPath = keyfile.get_string(group, 'ObjectPath'); + if (loadState.objectPaths[objectPath]) We need better-defined ordering here. If we have two directories that have the same search provider, this is a race as to whichever listDirAsync returns first. The easiest (and simplest) way to solve this would be to stop using some "global" state for the entire load operation, but just enumerate providers, and apply some priority after the fact.
Review of attachment 227948 [details] [review]: ::: js/ui/remoteSearch.js @@ +37,3 @@ return; + let searchSettings = new Gio.Settings({ schema: 'org.gnome.desktop.search-providers'}); Didn't you define a global for this schema name way at the top? @@ +55,3 @@ + // if providerA is the last, it goes after everything + if ((idxA + 1) == numSorted) + return 1; I'm not sure I like these semantics. The last item is anchored after search providers that aren't in the list. Why isn't this the case for the first item as well? I'd prefer to just drop this entirely, and let the not-explicitly-sorted items be wherever they want to be. I hope it's not a very common case; as this setting is going to be manipulated with a UI, we're going to see the cases of "empty sort order", and "full sort order". The only case we should see it is if somebody installs a new application.
Review of attachment 227949 [details] [review]: I don't understand this. What's the point of this? Why does it affect everything but apps/system settings/wanda? Do we want to restrict those too?
Review of attachment 227950 [details] [review]: Besides the wacky flow control here, this is fine. I wonder if we should push this up sooner in the patch set. ::: js/ui/remoteSearch.js @@ +92,2 @@ + dir.query_info_async('standard::type', Gio.FileQueryInfoFlags.NONE, + GLib.PRIORITY_DEFAULT, null, Lang.bind(this, You're in a function. What's "this" in this context?
Review of attachment 227951 [details] [review]: The reason it's not caught is because I told Florian to stop this nonsense. Why? Should we simply log a random message that nobody sees about to stderr? Should we instead show a user-visible message? What types of errors did you encounter that made you write this patch? What types of errors do we expect to see in normal cases when calling DBus methods? How should we handle all of them?
Review of attachment 227952 [details] [review]: ::: js/ui/remoteSearch.js @@ +166,2 @@ _init: function(title, appId, dbusName, dbusPath) { + new SearchProviderProxy(Gio.DBus.session, I always found the bare "new" a bit awkward, like we should have SearchProviderProxy.new_async or something. But that's Giovanni's bug, I suppose.
Attachment 227942 [details] pushed as 9aefbd1 - view-selector: add missing semicolon Attachment 227943 [details] pushed as 8e7758e - remote-search: require a DesktopId field in search providers Attachment 227950 [details] pushed as cbc8ec6 - remote-search: don't use g_file_query_exists() Attachment 227952 [details] pushed as 10c1045 - remote-search: initialize the DBus proxy asynchronously Pushed these patches with the suggested improvements.
Created attachment 228324 [details] [review] search: propagate GAppInfo to SearchProvider Save the GAppInfo in the provider object; this will be used when filtering.
Created attachment 228325 [details] [review] search: add API to get a list of remote providers This will be used to reload them in case the configuration changes.
Created attachment 228326 [details] [review] view-selector: filter out disabled search providers If the added search provider has an appId in the list of the disabled search providers settings, ignore it.
Created attachment 228327 [details] [review] remote-search: restructure remote search provider loading process Instead of adding search providers to the system as we find them, wait until we loaded information from all the directories, and then add all providers at once. This will be useful when we will sort the providers information according to the sort order saved in GSettings.
Created attachment 228328 [details] [review] remote-search: apply sort-order from GSettings Sort providers according to the GSettings state, and make sure to reload them as soon as the sort order changes.
Created attachment 228329 [details] [review] view-selector: add support for disable-all search setting
Review of attachment 228329 [details] [review]: Sure. ::: js/ui/viewSelector.js @@ +440,2 @@ _shouldUseSearchProvider: function(provider) { + // the disable-all GSetting only affect application providers affects
Review of attachment 228328 [details] [review]: ::: js/ui/remoteSearch.js @@ +138,3 @@ + if (numSorted > 1) { + // if providerA is the last, it goes after everything + if ((idxA + 1) == numSorted) You didn't answer my questions about these semantics.
Review of attachment 228327 [details] [review]: ::: js/ui/remoteSearch.js @@ +84,3 @@ let objectPath = keyfile.get_string(group, 'ObjectPath'); + if (loadState.objectPaths[objectPath]) The race is still here...
(In reply to comment #14) > The icon is used for the new search relayout. Perhaps we should just pass the > AppInfo instead? Good idea, fixed now. (In reply to comment #16) > ... yuck. I'd rather just have a separate list consisting of remote search > providers. Added this now. > What's the point of the two forEaches, instead of one? removeSearchProvider() modifies the array we iterate on; I refactored the code to get rid of the double loop now. (In reply to comment #17) > We need better-defined ordering here. If we have two directories that have the > same search provider, this is a race as to whichever listDirAsync returns > first. > > The easiest (and simplest) way to solve this would be to stop using some > "global" state for the entire load operation, but just enumerate providers, and > apply some priority after the fact. Unfortunately as soon as we call addSearchProvider, an actor will also be created and packed into the parent container. Applying the priority after the fact would mean that we need to reorder the actors in the container after they have been added, which is not easier and I'd rather avoid (AFAICS the container is also a custom clutter actor, and the code handling the grid is quite complex). I now reworked this patch to use a slightly simpler async flow for clarity. As far as I can see, the ordering problem you mention is also present in the current version of the code, so I wouldn't consider this a blocker; my patch just moves some pieces of that code around :) (In reply to comment #18) > I'm not sure I like these semantics. The last item is anchored after search > providers that aren't in the list. Why isn't this the case for the first item > as well? > > I'd prefer to just drop this entirely, and let the not-explicitly-sorted items > be wherever they want to be. I hope it's not a very common case; as this > setting is going to be manipulated with a UI, we're going to see the cases of > "empty sort order", and "full sort order". The only case we should see it is if > somebody installs a new application. This is also in the GSettings schema documentation, and it's done on purpose to accomodate the intention of the design - basically it should be possible to define one provider which is always the last (i.e. Files) in the order, regardless of the providers installed. I think having that possiblity is a good idea in general, but it's not the only reason it's implemented this way: the setting is changed through the control center UI (and the panel does ensure the whole list of providers is propagated to GSettings when it gets opened) but I don't think we shouldn't rely on that to have run after every upgrade and after every time a new application with a search provider is installed. The current code will sort the providers not in the settings list always before the last one. (In reply to comment #19) > I don't understand this. What's the point of this? Why does it affect > everything but apps/system settings/wanda? Do we want to restrict those too? Yeah, the setting only affects application providers, not built-in providers; those are stock items that should always be searched and should not have user preferences. I added a comment in the code. (In reply to comment #21) > The reason it's not caught is because I told Florian to stop this nonsense. I dropped this patch, it's not necessary anymore.
Review of attachment 228324 [details] [review]: Sure.
Review of attachment 228327 [details] [review]: OK, then. I like this a lot more.
Review of attachment 228326 [details] [review]: mhm
Review of attachment 228325 [details] [review]: This makes sense. ::: js/ui/search.js @@ +182,3 @@ this._providers.push(provider); + + if (provider.appInfo) I'd add an explicit "isRemoteProvider" property for this, in case we want to add an app file for settings (gnome-control-center.desktop). Also use this in the 'disable-all' key, please.
Review of attachment 228328 [details] [review]: The rationale about semantics makes sense; thanks.
Created attachment 228384 [details] [review] search: add API to get a list of remote providers -- Update for review comment
Created attachment 228385 [details] [review] view-selector: filter out disabled search providers -- Update for schema change and previous patch
Created attachment 228386 [details] [review] view-selector: add support for disable-external search setting -- Update for schema name change
Review of attachment 228384 [details] [review]: ::: js/ui/remoteSearch.js @@ +115,3 @@ dbusName, dbusPath, Lang.bind(this, this._onProxyConstructed)); + this.parent(appInfo.get_name().toUpperCase(), appInfo, true); I was just thinking of having "this.isRemoteProvider = true;" somewhere in here, but I guess this works too.
Review of attachment 228386 [details] [review]: Minor comment nit; otherwise good to go. ::: js/ui/viewSelector.js @@ +440,2 @@ _shouldUseSearchProvider: function(provider) { + // the disable-all GSetting only affect application providers remote providers
Created attachment 228494 [details] [review] view-selector: add support for disable-external search setting -- Fix last review comment and another bug where the disable-external setting wasn't listening for changes.
Review of attachment 228494 [details] [review]: ::: js/ui/viewSelector.js @@ +441,2 @@ _shouldUseSearchProvider: function(provider) { + // the disable-external GSetting only affect remote providers affects
Pushed to master since gsettings-desktop-schemas 3.7.2 is out with the new schema included and Bastien will do a CC release with the new panel later this week.