Created attachment 217440 [details] [review]
contact: Move get_eds_addressbooks() to App

Contact currently references App in a couple of places, which
means the class depends on pretty much any other class. In order
to reuse the Contact class in the shell search provider (which will
be a separate executable), reduce the Contact class' dependencies.

Created attachment 217441 [details] [review]
contact: Use our own store reference rather than the on in App

Contact currently references App in a couple of places, which
means the class depends on pretty much any other class. In order
to reuse the Contact class in the shell search provider (which will
be a separate executable), reduce the Contact class' dependencies.

Created attachment 217442 [details] [review]
contacts: Pass store as parameter to create_primary_persona_from_store

Contact currently references App in a couple of places, which
means the class depends on pretty much any other class. In order
to reuse the Contact class in the shell search provider (which will
be a separate executable), reduce the Contact class' dependencies.

Created attachment 217443 [details] [review]
Add search provider for GNOME Shell

GNOME Shell provides a DBus interface for external search provider
implementations; add a small auto-activated service which implements
that interface, to replace the Shell's built-in contact search with
results provided directly by GNOME Contacts.

Review of attachment 217443 [details] [review]:

::: src/contacts-contact.vala
@@ +148,3 @@
+      if (individual.avatar == null) {
+        if (_default_avatar == null)
+          _default_avatar = new ThemedIcon ("avatar-default");

The search provider already has a fallback check like this. Maybe not duplicate it. Its kinda weird to sometimes return null, and sometimes a default fallback.

@@ +153,3 @@
+
+      if (individual.avatar.to_string () != null)
+        return individual.avatar;

This check makes the general "Icon avatar_icon" property name a bit weird, since its really only non-null for serializable icons. I think this would be better as a get_serializable_avatar_icon() method.

@@ +159,3 @@
+  }
+
+  private Gdk.Pixbuf? _avatar_icon;

Why is this not a local variable? It does not seem to be required to keep the icon around if we keep the icon data around.

@@ +160,3 @@
+
+  private Gdk.Pixbuf? _avatar_icon;
+  private Variant? _avatar_icon_data;

This needs to be invalidated on avatar change, just like _small_avatar.

::: src/contacts-shell-search-provider.vala
@@ +1,3 @@
+using Gee;
+
+[DBus (name = "org.gnome.Shell.SearchProvider")]

Can you just add this, don't we need some kind of dependency check for it in configure?

@@ +17,3 @@
+    store.added.connect ( (c) => {
+      update_contacts_map ();
+    });

These seem both inefficient and wrong. You only need to add the new id, and if the individual id changed then you need to also remove the old one from the map.

@@ +25,3 @@
+  private void update_contacts_map () {
+    foreach (var c in store.get_contacts ()) {
+      id_to_contact_map.insert (c.individual.id, c);

This does not remove old individuals from the map.
Honestly, it just seems painful to reuse the possibly changing individual id to reference the Contact. I think it would make more sense to just assign each new contact a unique id. Something like:

Gee.HashMap<string, Contact> contacts_map;
on add
 var id = next_id++.to_string();
 contact.set_data("search-id", id)
 contacts_map.add (contact, id)
on remove
 contacts_map.remove (contact.get_data<string>("search-id")

@@ +37,3 @@
+
+      if (c.contains_strings (terms))
+          results.add (c.individual.id);

There is a lot of "garbage" in the full contacts set, we should probably also filter on contact.is_main, or at least sort those first.

@@ +56,3 @@
+    var results = new ArrayList<HashTable> ();
+    foreach (var id in ids) {
+      var contact = id_to_contact_map.lookup (id);

There is a race here, the contact could be gone now, so this needs to check for null.

@@ +108,3 @@
+  var app = new SearchProviderApp ();
+
+  if (Environment.get_variable ("CONTACTS_SEARCH_PROVIDER_PERSIST") != null)

What does this do?

Attachment 217440 [details] pushed as 72a7a5c - contact: Move get_eds_addressbooks() to App
Attachment 217441 [details] pushed as 93c9ae6 - contact: Use our own store reference rather than the on in App
Attachment 217442 [details] pushed as d891e08 - contacts: Pass store as parameter to create_primary_persona_from_store

(In reply to comment #5)
> +[DBus (name = "org.gnome.Shell.SearchProvider")]
> 
> Can you just add this, don't we need some kind of dependency check for it in
> configure?

To be honest, I'm not sure. I removed the interface definition installed by gnome-shell from both the system and jhbuild directories, and the search provider still compiles fine, so I guess it's OK to just add this ...


> @@ +159,3 @@
> +  }
> +
> +  private Gdk.Pixbuf? _avatar_icon;

> Why is this not a local variable? It does not seem to be required to keep the
> icon around if we keep the icon data around.

This is actually how we keep the icon data around - neither gdk_pixbuf_get_pixels() nor g_variant_new_from_data() copy the actual pixel data. Maybe I should just keep the pixbuf and create the variant in the search provider as needed?


> @@ +108,3 @@
> +  var app = new SearchProviderApp ();
> +
> +  if (Environment.get_variable ("CONTACTS_SEARCH_PROVIDER_PERSIST") != null)
> 
> What does this do?

In normal operation, the service is autostarted via DBus and exits after some time of inactivity. However if the above variable is set, the service does not exit automatically, which can come in handy for debugging (the approach is shamelessly copied from Cosimo, who always adds FOO_PERSIST handling to DBus services)

> This is actually how we keep the icon data around - neither
> gdk_pixbuf_get_pixels() nor g_variant_new_from_data() copy the actual pixel
> data. Maybe I should just keep the pixbuf and create the variant in the search
> provider as needed?

Just ref the pixbuf and pass g_object_unref as destroy notify in g_variant_new_from_data and the pixbuf as user_data.

Also, why don't you reuse small_avatar? It seems to do the same thing...

(In reply to comment #8)
> Just ref the pixbuf and pass g_object_unref as destroy notify in
> g_variant_new_from_data and the pixbuf as user_data.

Noob question - how do I do that in vala?


> Also, why don't you reuse small_avatar? It seems to do the same thing...

Good point, will do.

var v = Variant.new_from_data (VariantType.BYTESTRING,
                               pixbuf.get_pixels_with_length (), 
                               true, pixbuf);

Thanks!

(In reply to comment #9)
> > Also, why don't you reuse small_avatar? It seems to do the same thing...
> 
> Good point, will do.

Mmmh, there is a small difference, as small_avatar adds rounded corners individual.avatar doesn't have. However I'll ignore that for now - I suspect that designers will ask for rounded corners on *all* the icons, which means it should be implemented in gnome-shell, which would then hide the difference anyway ...

Yeah, the rounded corners is a request from the designers, so we should probably have them everywhere we show the contact avatar anyway.

Right, though I suspect that we'll want rounded corners for non-contact icons as well - otherwise we could just send icon-data for all icons except the fallback one (though I don't like the idea of sending pixel data over dbus unless really necessary)

We'll drop the fallback icon for robohash style icons eventually anyway.

Created attachment 217607 [details] [review]
Add search provider for GNOME Shell

This should address all comments and fix a couple of issues that went unnoted :-)


(In reply to comment #5)
> +      if (c.contains_strings (terms))
> +          results.add (c.individual.id);
> 
> There is a lot of "garbage" in the full contacts set, we should probably also
> filter on contact.is_main, or at least sort those first.

The general idea for search providers is to return the exact same result set as the corresponding in-app search. There are a couple more conditions in the list-box filter_func, but as far as I can tell from the code none of them is relevant to the search view, so the filter here should be fine.

However, I completely failed to sort the results, which should be fixed now. I'm not quite sure whether sorting the result set is the best way to do it though - the number of elements is smaller, but we need to re-sort a lot more.


Apart from that, I have an issue with result activation, which I'm somehow unable to figure out - gnome-contacts often gives me an error about not finding the requested contact. It might be related to jhbuild (switching to a contact after having started contacts with jhbuild run is a lot less likely to fail), but there's probably more to it. If you have any quick idea what might be wrong, I'd appreciate the hint, otherwise I'll be doing some more investigation.

gnome-contacts normally only shows the contacts in the primary persona-store (i.e. google contacts or e-d-s local addressbook). However, when searching it shows both, first the primary ones, then a separator/header then the rest. Not sure how you can do something like that in the shell though, although we can do the same sort.

(In reply to comment #17)
> gnome-contacts normally only shows the contacts in the primary persona-store
> (i.e. google contacts or e-d-s local addressbook). However, when searching it
> shows both, first the primary ones, then a separator/header then the rest. Not
> sure how you can do something like that in the shell though, although we can do
> the same sort.

Yup, the current patch should do exactly that (minus the separator of course).

Do i need to do anything to get the shell to pick this up?

Created attachment 217617 [details] [review]
Add search provider for GNOME Shell

Gah, forgot to chain up startup() ...

Attachment 217617 [details] pushed as 19751af - Add search provider for GNOME Shell

It lacked a service file and Name/Icon for the search provider file. I added that and i18n for the data files and pushed to master.

Created attachment 217623 [details] [review]
Add search provider for GNOME Shell

(In reply to comment #19)
> Do i need to do anything to get the shell to pick this up?

First, I forgot to install a .service file to auto-activate the search provider - fixed here.

Then it gets tricky I'm afraid.

In general, this is how it's supposed to work:
 (1) on startup, shell parses provider .ini files in
     $dir/gnome-shell/search-providers for all directories
     in $XDG_DATA_DIRS
 (2) for each file that is succesfully parsed, a dbus proxy
     object is created
 (3) any time a search is started, dbus-daemon should auto-activate
     the appriate services

Now the tricky bits:
 (1) I'm using 'DesktopId' in the .ini file, which is only understood
     by the shell since 3.5.3; if you are not using gnome-shell from
     jhbuild or current rawhide, you need to add
        Title=Contacts
        Icon=x-office-address-book
     to the gnome-contacts-search-provider.ini.in
 (3) assuming gnome-contacts is run from jhbuild and dbus-daemon is not,
     auto-activation will fail; either hack the .service file to
     execute jhbuild run, or start the search provider manually with
     CONTACTS_SEARCH_PROVIDER_PERSIST=1

(In reply to comment #22)
> It lacked [...] Name/Icon for the search provider file. I added
> that and i18n for the data files and pushed to master.

The plan here is that 'DesktopId' will supersede the Name/Icon combo, mostly to make the i18n glue unnecessary.

Ok. I'll remove that then.

If you still want some compatibility with older shell versions ("older" == "< 3.5.3"), I'd suggest to keep Name/Icon, but leave it untranslated - newer shell versions will pick up translations from the .desktop file

Review of attachment 217617 [details] [review]:

::: src/contacts-shell-search-provider.vala
@@ +113,3 @@
+
+    if (contact == null)
+      return;

I guess you want to app.release () before leaving that method.

(In reply to comment #27)
> I guess you want to app.release () before leaving that method.

Right. Would you mind doing a patch?