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 690577 - add a shell search provider
add a shell search provider
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks: 690824
 
 
Reported: 2012-12-20 21:26 UTC by Giovanni Campagna
Modified: 2013-02-18 18:27 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
Add a gnome-shell search-provider for settings panel (62.54 KB, patch)
2012-12-28 16:53 UTC, Giovanni Campagna
needs-work Details | Review
Add a gnome-shell search-provider for settings panel (42.88 KB, patch)
2013-01-31 12:15 UTC, Giovanni Campagna
none Details | Review
Split the search provider into its own process (36.05 KB, patch)
2013-01-31 12:15 UTC, Giovanni Campagna
none Details | Review
P (54.92 KB, patch)
2013-02-15 07:21 UTC, Bastien Nocera
needs-work Details | Review
shell: split the panel loader from the panel list (9.45 KB, patch)
2013-02-15 21:03 UTC, Giovanni Campagna
reviewed Details | Review
Add a gnome-shell search-provider for settings panel (32.31 KB, patch)
2013-02-15 21:05 UTC, Giovanni Campagna
accepted-commit_now Details | Review
shell: add a way to compile the panel loader without GType functions (4.45 KB, patch)
2013-02-18 14:30 UTC, Giovanni Campagna
committed Details | Review
Add a gnome-shell search-provider for settings panel (32.65 KB, patch)
2013-02-18 14:30 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2012-12-20 21:26:24 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.
Comment 1 Bastien Nocera 2012-12-20 21:57:30 UTC
I'd rather we added a search provider, re-adding those files for the sake of gnome-shell seems a bit weird.
Comment 2 Giovanni Campagna 2012-12-28 16:53:54 UTC
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.
Comment 3 Cosimo Cecchi 2013-01-03 01:05:42 UTC
(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.
Comment 4 Bastien Nocera 2013-01-07 17:30:46 UTC
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.
Comment 5 Bastien Nocera 2013-01-15 11:17:30 UTC
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.
Comment 6 Giovanni Campagna 2013-01-31 12:15:12 UTC
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.
Comment 7 Giovanni Campagna 2013-01-31 12:15:34 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2013-02-15 00:12:28 UTC
What's the status on this?
Comment 9 Bastien Nocera 2013-02-15 07:21:35 UTC
Created attachment 236216 [details] [review]
P

Squashed patch for review
Comment 10 Bastien Nocera 2013-02-15 07:27:45 UTC
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.
Comment 11 Giovanni Campagna 2013-02-15 21:03:33 UTC
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.
Comment 12 Giovanni Campagna 2013-02-15 21:05:47 UTC
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!
Comment 13 Bastien Nocera 2013-02-18 08:51:50 UTC
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
Comment 14 Bastien Nocera 2013-02-18 08:55:04 UTC
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?
Comment 15 Giovanni Campagna 2013-02-18 14:30:27 UTC
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.
Comment 16 Giovanni Campagna 2013-02-18 14:30:53 UTC
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 17 Bastien Nocera 2013-02-18 14:46:47 UTC
Comment on attachment 236594 [details] [review]
shell: add a way to compile the panel loader without GType functions

Looks good.
Comment 18 Giovanni Campagna 2013-02-18 15:09:03 UTC
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
Comment 19 Bastien Nocera 2013-02-18 18:27:09 UTC
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.