GNOME Bugzilla – Bug 690577
add a shell search provider
Last modified: 2013-02-18 18:27:09 UTC
I'm refering to commit 10f292dc6b89a7dc646937da2929b5b620c0b96b. Those files are used by gnome-shell to implement setting search, and they're needed until we have a search provider in the control center.
I'd rather we added a search provider, re-adding those files for the sake of gnome-shell seems a bit weird.
Created attachment 232335 [details] [review] Add a gnome-shell search-provider for settings panel Replace the gnome-shell builtin settings search, which relied on removed menu files, with one that uses the remote search infrastructure and CcSearchModel, and features the ability to continue searching within the control center application. Ok, here you have your search provider. It's kind of a big patch, because the search provider architecture is really designed around nautilus, and expects that the process answering queries is the same as the one showing results. Also, cc-panel-loader just won't work without the modules loaded, and I didn't feel like going back to libgmenu.
(In reply to comment #2) > It's kind of a big patch, because the search provider architecture is really > designed around nautilus, and expects that the process answering queries > is the same as the one showing results. This is not correct, you can still use a separate process and implement LaunchSearch() if you have another way of passing the search string to the main application; you also get a timestamp to use for this. Using the same process just makes it easier.
Review of attachment 232335 [details] [review]: I'd rather the code to handle the search provider and the shell itself be separate. I'm also not sure there's much of a point reusing the same code for it (though the all_panels list might be useful). Eg. the "drilling down" searches are useful for file searches, but we have 20 names with very little metadata.
I've reverted the patch: commit d2bfa254d32ed7eca769e44687ea50251b167404 Author: Bastien Nocera <hadess@hadess.net> Date: Tue Jan 15 12:07:46 2013 +0100 Revert "shell: Remove unused gnome-menus helper files" The files are used by gnome-shell to display the control-center search resulst in the overview. https://bugzilla.gnome.org/show_bug.cgi?id=690577 This reverts commit 10f292dc6b89a7dc646937da2929b5b620c0b96b. Conflicts: po/POTFILES.in shell/Makefile.am As the search provider cannot be used as-is (it needs special-casing in the shell). See also: https://bugzilla.gnome.org/show_bug.cgi?id=691779 I've committed the sections of the patch that made sense in terms of cleaning up the shell model.
Created attachment 234903 [details] [review] Add a gnome-shell search-provider for settings panel Replace the gnome-shell builtin settings search, which relied on removed menu files, with one that uses the remote search infrastructure and CcSearchModel, and features the ability to continue searching within the control center application. This is just a rebase of the original patch, for the records. The next patch splits the two processes, and then we can decide which way to go.
Created attachment 234904 [details] [review] Split the search provider into its own process While this requires splitting the panel loader in two, because the search provider cannot compile the GType functions, it is generally considered cleaner to separate DBus services and applications.
What's the status on this?
Created attachment 236216 [details] [review] P Squashed patch for review
Review of attachment 236216 [details] [review]: Changes to the shell binary should be as minimal as possible, and we do want to go with a separate binary for the search. ::: search-provider/Makefile.am @@ +27,3 @@ + cc-search-provider.c \ + cc-search-provider.h \ + $(MARSHAL_FILES) MARSHAL_FILES is empty ::: search-provider/control-center-search-provider.c @@ +90,3 @@ + + g_application_set_inactivity_timeout (G_APPLICATION (self), + 60 * 1000); Move that value to be a constant. ::: shell/control-center.c @@ +82,3 @@ + +GnomeControlCenter * +cc_application_get_view (CcApplication *app) Why do we need to make g-c-c into a GtkApplication? If we need this, it needs to be a separate commit.
Created attachment 236303 [details] [review] shell: split the panel loader from the panel list We want to access the list of panels from a different process, so we can't have it refer to the GType functions directly.
Created attachment 236304 [details] [review] Add a gnome-shell search-provider for settings panel Replace the gnome-shell builtin settings search, which relied on removed menu files, with one that uses the remote search infrastructure and CcSearchModel, and features the ability to continue searching within the control center application. Now with less shell changes!
Review of attachment 236303 [details] [review]: I'd like to avoid the duplicated list of panels. Looks good otherwise. ::: shell/cc-panel-list.c @@ +26,3 @@ +#include "cc-panel-list.h" + +const char *all_panels[] = { You can build this code twice, once for the shell, once for the search provider. When building for the search provider, make it so the definition for the _get_type() function is unused. See totem-pl-parser's way of dismissing parsing functions when the library is only used to detect playlists: http://git.gnome.org/browse/totem-pl-parser/tree/plparse/totem-pl-parser.c#n170
Review of attachment 236304 [details] [review]: Looks fine. ::: search-provider/Makefile.am @@ +3,3 @@ + cc-shell-search-provider-generated.h + +$(dbus_shell_search_provider_built_sources) : Makefile.am $(srcdir)/org.gnome.ShellSearchProvider2.xml Please mention where the upstream version for the D-Bus definition is (git link is good) @@ +14,3 @@ + -I$(top_srcdir) \ + $(SHELL_CFLAGS) \ + $(CHEESE_CFLAGS) \ Why do you need CHEESE_CFLAGS for the search-provider?
Created attachment 236594 [details] [review] shell: add a way to compile the panel loader without GType functions This will allow to use the panel loader in the search provider, which is a separate executable and doesn't link all the panel modules.
Created attachment 236598 [details] [review] Add a gnome-shell search-provider for settings panel Replace the gnome-shell builtin settings search, which relied on removed menu files, with one that uses the remote search infrastructure and CcSearchModel, and features the ability to continue searching within the control center application. Reattaching to preserve order.
Comment on attachment 236594 [details] [review] shell: add a way to compile the panel loader without GType functions Looks good.
Attachment 236594 [details] pushed as 3911ef1 - shell: add a way to compile the panel loader without GType functions Attachment 236598 [details] pushed as 59f873e - Add a gnome-shell search-provider for settings panel
And: commit 5f4e14007826cb41f13921b02da1f374b67f1570 Author: Bastien Nocera <hadess@hadess.net> Date: Mon Feb 18 19:24:48 2013 +0100 shell: Remove unused gnomecc.menu file Searching for Settings is now handled through a search provider in gnome-control-center, for which the display is special-cased in gnome-shell.