GNOME Bugzilla – Bug 687490
Add a panel for Search settings
Last modified: 2012-11-27 12:03:02 UTC
See https://live.gnome.org/ThreePointSeven/Features/IntegratedApplicationSearch
Created attachment 227939 [details] [review] Import egg-list-box as a git submodule
Created attachment 227940 [details] [review] search: add a panel to configure search providers The panel lists all the search providers registered on the system, and allows to reoder or disable them, or disable the search providers feature entirely. The panel will also allow configuration, for which a first implementation will be added in a separate commit. https://live.gnome.org/Design/SystemSettings/Search
Created attachment 227941 [details] [review] search: add a dialog to configure Tracker search locations Ideally applications could configure Tracker independent from each other, but we don't have such a feature currently. In the meanwhile, bring up a settings dialog that can allow configuration of the locations indexed by the Tracker files miner, as suggested by the current design. https://live.gnome.org/Design/SystemSettings/Search
Created attachment 227953 [details] [review] search: add a panel to configure search providers -- Correct patch
Created attachment 227954 [details] how it looks 1
Created attachment 227955 [details] how it looks 2
Review of attachment 227939 [details] [review]: Looks fine, pending the rest of the code.
Review of attachment 227953 [details] [review]: Pending on the gsettings-desktop-schemas patch too. ::: configure.ac @@ +148,3 @@ $IBUS_MODULE) PKG_CHECK_MODULES(SCREEN_PANEL, $COMMON_MODULES) +PKG_CHECK_MODULES(SEARCH_PANEL, $COMMON_MODULES) this is where you should add the egg-list-box-uninstalled check. ::: panels/search/Makefile.am @@ +4,3 @@ + $(PANEL_CFLAGS) \ + $(SEARCH_PANEL_CFLAGS) \ + $(EGGLISTBOX_CFLAGS) \ That would be removed once configure.ac is changed. ::: panels/search/cc-search-panel.c @@ +233,3 @@ +static void +down_button_clicked (GtkWidget *widget, + gpointer user_data) Use CcSearchPanel *self directly. @@ +242,3 @@ +static void +up_button_clicked (GtkWidget *widget, + gpointer user_data) Use CcSearchPanel *self directly. @@ +507,3 @@ + +static gboolean +add_disable_all_switch (gpointer user_data) You'd use CcSearchPanel *self in the function declaration directly. @@ +565,3 @@ + &error); + + if (error != NULL) You're supposed to check the retval from gtk_builder_add_from_file(), not the value of the error. @@ +605,3 @@ + gtk_widget_reparent (widget, (GtkWidget *) self); + + g_idle_add (add_disable_all_switch, self); This is the 3rd panel that uses an idle for this, the first one passed through the net, the 2nd one we were under time pressure, I think we should fix it properly for the 3rd one. ::: panels/search/gnome-search-panel.desktop.in.in @@ +15,3 @@ +X-GNOME-Settings-Panel=search +# Translators: those are keywords for the search control-center panel +_Keywords=Search;Find; Index;Hide;Privacy;Results;
Review of attachment 227941 [details] [review]: Isn't there work to use the same code in GTK+ and nautilus for those sidebars? Can we use that instead? Also, where are you destroying the dialogue when the panel goes away?
Created attachment 228391 [details] [review] Import egg-list-box as a git submodule -- Minor changes to configure.ac
Created attachment 228392 [details] [review] search: add a panel to configure search providers
Created attachment 228394 [details] [review] search: add a dialog to configure Tracker search locations
(In reply to comment #8) Latest patch should address your review comments. > You're supposed to check the retval from gtk_builder_add_from_file(), not the > value of the error. Why? Most of the other panels also check the GError...anyway I changed the code to check for the retval instead. > This is the 3rd panel that uses an idle for this, the first one passed through > the net, the 2nd one we were under time pressure, I think we should fix it > properly for the 3rd one. Yeah, turns out tt's just a matter of adding the header widget in constructed(), since the CcShell property gets set at construction time. I will prepare a patch for the other panels in a separate bug report.
(In reply to comment #9) > Isn't there work to use the same code in GTK+ and nautilus for those sidebars? > Can we use that instead? In an ideal world in theory we could, but: - I don't want to block on GtkPlacesSidebar for this - the current iteration of GtkPlacesSidebar doesn't use EggListBox but a GtkTreeView, so it wouldn't be possible to add switches - I don't think the current code for GtkPlacesSidebar is flexible enough to accomodate custom locations like we want to do here We will definitely need to keep this use case in mind for further developments of GtkPlacesSidebar itself though. > Also, where are you destroying the dialogue when the panel goes away? Fixed that now
I've tried this panel now - works fine in my testing. One comment I have is that it would eventually be nice if the 'internal' search provider for apps could be manipulated in the list as well. I also don't think that the search locations dialog really solves the 'my pdfs are in ~/my-other/folder/hierarchy/not/Documents' use case. I've tried it by adding a ~/test-folder/ search location where I put a pdf. I verified that tracker-needle sees the .pdf just fine, but a) it does not show up in Documents at all, and in the shell search, it comes in as a 'Files' result.
(In reply to comment #15) > I've tried this panel now - works fine in my testing. > > One comment I have is that it would eventually be nice if the 'internal' search > provider for apps could be manipulated in the list as well. What kind of manipulation are you thinking of for the application provider? I don't think it makes much sense to reoder or disable it. > I also don't think that the search locations dialog really solves the 'my pdfs > are in ~/my-other/folder/hierarchy/not/Documents' use case. I've tried it by > adding a ~/test-folder/ search location where I put a pdf. I verified that > tracker-needle sees the .pdf just fine, but a) it does not show up in Documents > at all, and in the shell search, it comes in as a 'Files' result. Yeah, Documents will also need a separate patch to make it include custom added locations from tracker's GSettings in its query filter.
Created attachment 228432 [details] [review] search: add a panel to configure search providers -- Fixed a bug where the switches came up with the wrong color at panel creation.
Created attachment 228502 [details] [review] search: add a dialog to configure Tracker search locations -- Fix a bug where stray headings were added to the dialog.
Created attachment 228975 [details] [review] search: make options insensitive if tracker is not available Instead of either hard-depending on Tracker being installed, or crashing when the Tracker GSettings schema is not found, make the configuration insensitive if the schema is not available.
Review of attachment 228391 [details] [review]: I found a problem with this, trying to use egg-list-box for the upcoming notifications panel: libegglistbox is a static library, so if different panels load it, the symbols are not merged. This causes a problem as both copies try to register the EggListBox GType.
Yeah, this has been noted before - we need to make the cc shell link against egg-list-box, so all panels can use it
Created attachment 229928 [details] [review] Link to egg-list-box in the gnome-control-center binary Multiple panels want to use egg-list-box, and they can't do so by linking it directly since it is a static library, so it breaks with GType registration. This is a way to link egg-list-box in gnome-control-center. Not nice, granted, but I couldn't find a better one.
Created attachment 229939 [details] [review] Import egg-list-box as a git submodule -- Includes latest Giovanni's patch and a slightly less ugly call to g_type_ensure() for the list box. I also split some configure.ac cleanups from Giovanni's patch in bug 689110.
Created attachment 229940 [details] [review] search: add a panel to configure search providers The panel lists all the search providers registered on the system, and allows to reoder or disable them, or disable the search providers feature entirely. The panel will also allow configuration, for which a first implementation will be added in a separate commit. https://live.gnome.org/Design/SystemSettings/Search
Comment on attachment 229939 [details] [review] Import egg-list-box as a git submodule Pushed with a number of fixes (the egg-list-box directory was built after everything else, and its _get_type() was used but the header not included)
Bug 689156 for the reordering via drag'n'drop. Attachment 228502 [details] pushed as defe92e - search: add a dialog to configure Tracker search locations Attachment 228975 [details] pushed as 2ad13d8 - search: make options insensitive if tracker is not available Attachment 229940 [details] pushed as 7ab2d50 - search: add a panel to configure search providers