Bug 24652

This blocks this Empathy bug: https://bugzilla.gnome.org/show_bug.cgi?id=599162

http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/tp-contact-location

In general this looks good. Some minor changes:

> + * Return the contact's user-defined location or NULL it he didn't define one.

"%NULL if the location is unspecified", to avoid the he / he/she / they problem?

> + * Returns: the same #GHashTable (or NULL) as the #TpContact:location property

%NULL

> +   * If this contact has set a user-defined location, a string to
> +   * #GValue * hash table containing his location. If not, %NULL.

I'd add something like "tp_asv_get_string() and similar functions can be used to access the contents."

> +          tp_cli_connection_interface_location_call_get_locations (

I don't think this slow-path is actually needed. The Contacts interface was made mandatory in 0.17.23, and Location wasn't added until 0.17.27, so I'd be inclined to say that if your CM implements Location, but not Contacts, then you don't deserve geolocation :-)

Notes for future contributions:

> +contact_maybe_set_location (TpContact *contact,

I loosely prefer the first argument to be called self.

(In reply to comment #3)
> In general this looks good. Some minor changes:
> 
> > + * Return the contact's user-defined location or NULL it he didn't define one.
> 
> "%NULL if the location is unspecified", to avoid the he / he/she / they
> problem?

fixed.

> > + * Returns: the same #GHashTable (or NULL) as the #TpContact:location property
> 
> %NULL

fixed.

> > +   * If this contact has set a user-defined location, a string to
> > +   * #GValue * hash table containing his location. If not, %NULL.
> 
> I'd add something like "tp_asv_get_string() and similar functions can be used
> to access the contents."

done.

> > +          tp_cli_connection_interface_location_call_get_locations (
> 
> I don't think this slow-path is actually needed. The Contacts interface was
> made mandatory in 0.17.23, and Location wasn't added until 0.17.27, so I'd be
> inclined to say that if your CM implements Location, but not Contacts, then you
> don't deserve geolocation :-)

I'd be tempted to keep it as:
a) it's already done :)
b) it keeps the code more symetric
c) it will make testing with Empathy easier if one wants to try to implement a small CM implementing, say, google latittude (may actually be one of my secret hypothetic project ;)

> Notes for future contributions:
> 
> > +contact_maybe_set_location (TpContact *contact,
> 
> I loosely prefer the first argument to be called self.

So do it. I changed it.

You'll need to update docs/reference/*-sections.txt to include the new API. In future I'd like this to be part of the same commit that introduces it (otherwise make check will fail); please make check with gtk-doc enabled.

(In reply to comment #4)
> > > +          tp_cli_connection_interface_location_call_get_locations (
> > 
> > I don't think this slow-path is actually needed. The Contacts interface was
> > made mandatory in 0.17.23, and Location wasn't added until 0.17.27, so I'd be
> > inclined to say that if your CM implements Location, but not Contacts, then you
> > don't deserve geolocation :-)
> 
> I'd be tempted to keep it as:
> a) it's already done :)
> b) it keeps the code more symetric
> c) it will make testing with Empathy easier if one wants to try to implement a
> small CM implementing, say, google latittude (may actually be one of my secret
> hypothetic project ;)

OK, but only because you already implemented it :-)

I consider Contacts to be a mandatory feature in CMs, and new telepathy-glib features shouldn't go to any particular effort to work with pre-Contacts CMs. telepathy-qt4 already doesn't support any extra features, like the alias, on Tp::Contact objects from an old CM.

Merged; will be in 0.11.1