[Bug 24652] TpContact should support Location

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=24652

--- Comment #4 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-04-05 06:34:03 PDT ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.