GNOME Bugzilla – Bug 599162
Should use TpContact get Location
Last modified: 2010-04-06 17:02:58 UTC
Once TpContact supports Location ( https://bugs.freedesktop.org/show_bug.cgi?id=24652 ) Empathy should use it instead of manually request the location.
Created attachment 157964 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/location-tp-contact-599162 libempathy/empathy-contact.c | 179 +++++++++++++++++++++++- libempathy/empathy-contact.h | 2 - libempathy/empathy-tp-contact-factory.c | 231 +------------------------------ 3 files changed, 178 insertions(+), 234 deletions(-)
This branch is blocked by https://bugs.freedesktop.org/show_bug.cgi?id=24652
Review of attachment 157964 [details] [review]: Hi, I inlined some comments below (mostly style fixes). ::: libempathy/empathy-contact.c @@ +1164,2 @@ priv->location = g_hash_table_ref (location); + update_geocode (contact); You could #ifdef this call here and use a single #ifdef HAVE_GEOCLUE later. @@ +1215,3 @@ +geocode_cb (GeoclueGeocode *geocode, + GeocluePositionFields fields, + double latitude, EmpathyContact already uses mixed styles for indentation, but we should probably use the new style for added code in these functions here. @@ +1219,3 @@ + double altitude, + GeoclueAccuracy *accuracy, + GError *error, If the error really belongs to Geoclue here, this should probably say const GError* <nitpick/>. @@ +1348,3 @@ + geocode_cb, contact); + + g_hash_table_unref (address); Does geoclue assume a ref to the hash table? If so, you could probably add a comment here.
Review of attachment 157964 [details] [review]: ::: libempathy/empathy-contact.c @@ +1164,2 @@ priv->location = g_hash_table_ref (location); + update_geocode (contact); Done. @@ +1215,3 @@ +geocode_cb (GeoclueGeocode *geocode, + GeocluePositionFields fields, + double latitude, Very good point; fixed. @@ +1219,3 @@ + double altitude, + GeoclueAccuracy *accuracy, + GError *error, Yeah, geoclue's API is wrong here. @@ +1348,3 @@ + geocode_cb, contact); + + g_hash_table_unref (address); I don't understand this comment. Geoclue pass the hash table to dbus-glib which does all its black magic.
(In reply to comment #4) > I don't understand this comment. Geoclue pass the hash table to dbus-glib which > does all its black magic. It just felt strange to unref the hash table after passing it to a method, as if it was a GObject, feel free to ignore that comment :) The latest version of the branch looks good to me.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.