Created attachment 237293 [details] [review]
Add a shell search provider for passwords and keys

Sounds pretty cool. I haven't had a chance to try out or review the patch yet, as I'm travelling. But just a feature question:

Should we only be searching passwords for now, and only those stored by the user through seahorse? Do you think it makes sense to show application stored passwords and keys in the shell? Those seem like things the user shouldn't really be bothered with.

FWIW, relatedly I do need to do work to make it easier to store a simple password in seahorse, and become more useful as a place to add a password or something you want to keep secret.

I agree that showing application passwords is not useful. But I think that showing keys and certificates is nice. After all, they are user visible content.

Giovanni. That makes sense. (In reply to comment #3)
> I agree that showing application passwords is not useful. But I think that
> showing keys and certificates is nice. After all, they are user visible
> content.

I agree.

So let's filter out application stored passwords for now. These are passwords that don't have the SECRET_SCHEMA_NOTE schema, as returned by secret_item_get_schema_name().

https://developer.gnome.org/libsecret/0.13/SecretItem.html#secret-item-get-schema-name

In addition we should only show personal items, not things like CA certificates and so on. You can do this by setting SeahorsePredicate.flags to SEAHORSE_FLAG_PERSONAL.

Created attachment 238231 [details] [review]
Add a shell search provider for passwords and keys

Turn the seahorse application into a dbus activate service, and install
a handler for the search provider interface. This will allow to search
passwords from the overview, which is useful when using seahorse to
store personal passwords not created by applications.

There is one problem with this patch: passwords stored before the
introduction of libsecret are not matched, because they use the generic
schema.

Review of attachment 237293 [details] [review]:

Overall looks well done. 

Hmmm, looks like I reviewed the last patch, and you were quick on the draw with an update. I hope the comments are useful anyway.

::: data/Makefile.am
@@ +22,3 @@
+
+org.gnome.seahorse.Application.service: org.gnome.seahorse.Application.service.in Makefile
+	$(AM_V_GEN) sed -e "s|\@bindir\@|$(bindir)|" $< > $@

We have SED_SUBST (see Makefile.decl) for this. Or does it not work in this case?

@@ +25,3 @@
+
+searchproviderdir = $(datadir)/gnome-shell/search-providers
+

Where does this file come from? I get: 

make[2]: Entering directory `/data/projects/seahorse/data'
make[2]: *** No rule to make target `seahorse-search-provider.ini', needed by `distdir'.  Stop.

::: libseahorse/seahorse-application.c
@@ +138,2 @@
 	context = g_option_context_new (N_("- System Settings"));
+	g_option_context_set_ignore_unknown_options (context, TRUE);

Is this really necessary? Can't we put --no-window in the options?

::: libseahorse/seahorse-search-provider.c
@@ +6,3 @@
+ * Copyright (C) 2011 Collabora Ltd.
+ * Copyright (C) 2012 Stefan Walter
+ * Copyright (C) 2013 Giovanni Campagna

I doubt all these guys wrote this :)

@@ +95,3 @@
+
+	predicate->custom = object_contains_filtered_text;
+	predicate->custom_target = g_strjoinv (" ", terms);

Are we supposed to be doing an 'and' search on the terms, or searching for them as adjacent words. It seems this does the latter.

@@ +123,3 @@
+
+	for (l = objects; l; l = l->next) {
+		if (SEAHORSE_IS_VIEWABLE(l->data) &&

Missing space here.

@@ +176,3 @@
+
+	g_ptr_array_add (array, NULL);
+	results = (char**) g_ptr_array_free (array, FALSE);

Missing space in the pointer type.

@@ +341,3 @@
+seahorse_search_provider_init (SeahorseSearchProvider *self)
+{
+	self->skeleton = seahorse_shell_search_provider2_skeleton_new ();

Would prefer it if you derived this class from SeahorseShellSearchProvider2Skeleton and used virtual functions instead if possible. This signal stuff is really pretty unreadable, and was a big workaround for tedious subclassing in GObject C code, and inability of some bindings to subclass. But since we have a GObject class here anyway, let's just do it the sane way :)

@@ +354,3 @@
+			  G_CALLBACK (handle_launch_search), self);
+
+	self->collection = GCR_COLLECTION (gcr_union_collection_new ());

This might be were you use seahorse_collection_new_for_predicate() to filter out application keyring items, and only include personal items.

::: libseahorse/seahorse-search-provider.h
@@ +6,3 @@
+ * Copyright (C) 2011 Collabora Ltd.
+ * Copyright (C) 2012 Stefan Walter
+ * Copyright (C) 2013 Giovanni Campagna

Ditto here.

::: src/seahorse-key-manager.xml
@@ +7,3 @@
     <property name="default_width">640</property>
     <property name="default_height">476</property>
+    <signal name="delete-event" handler="on_widget_delete_event" swapped="no"/>

Did this sneak in? Should it be a separate commit?

Oh, and could you update .gitignore for the new generated files?

(In reply to comment #6)
> Review of attachment 237293 [details] [review]:
> 
> ::: libseahorse/seahorse-application.c
> @@ +138,2 @@
>      context = g_option_context_new (N_("- System Settings"));
> +    g_option_context_set_ignore_unknown_options (context, TRUE);
> 
> Is this really necessary? Can't we put --no-window in the options?

Yes, it's necessary: GOptionContext filters away all options that it knows about, and we need --no-window to be sent to the primary instance.

> ::: src/seahorse-key-manager.xml
> @@ +7,3 @@
>      <property name="default_width">640</property>
>      <property name="default_height">476</property>
> +    <signal name="delete-event" handler="on_widget_delete_event"
> swapped="no"/>
> 
> Did this sneak in? Should it be a separate commit?

It is necessary to handle window deletion correctly: otherwise, the destroyed window is not removed from SeahorseWidget tracking table, and thus the window is shown again with no content when activating the app.

Created attachment 239025 [details] [review]
Add a shell search provider for passwords and keys

Turn the seahorse application into a dbus activate service, and install
a handler for the search provider interface. This will allow to search
passwords from the overview, which is useful when using seahorse to
store personal passwords not created by applications.

Fixing build of org.gnome.seahorse.Application.service

Getting this:

(seahorse:30907): GLib-GIO-CRITICAL **: g_dbus_interface_skeleton_export: assertion `G_IS_DBUS_INTERFACE_SKELETON (interface_)' failed

(seahorse:30907): GLib-GIO-CRITICAL **: g_dbus_interface_skeleton_has_connection: assertion `G_IS_DBUS_INTERFACE_SKELETON (interface_)' failed

Created attachment 241906 [details] [review]
Add a shell search provider for passwords and keys

Turn the seahorse application into a dbus activate service, and install
a handler for the search provider interface. This will allow to search
passwords from the overview, which is useful when using seahorse to
store personal passwords not created by applications.

Ops, sorry, I had this fixed locally, but I forgot to attach it...

Thanks. Merged to master with the following changes:

 * Use GOptionContext to parse --no-window (want to eventually do this better
   but this is a start).
 * Don't set application inactivity timeout unless --no-window
 * Gracefully handle null values in handle_get_result_metas()
 * Whitespace fixes

Attachment 241906 [details] pushed as 6c43f3d - Add a shell search provider for passwords and keys

Created attachment 242106 [details] [review]
Patch to include Makefile.decl in data/Makefile.am

I wanted to open this once more because the patch pushed (attachment 241906 [details] [review]) does not compile. The problem is a missing include of Makefile.decl in data/Makefile.am. This patch fixes the issue.

Drat. I saw this was fixed in master earlier today. Disregard the previous patch.