GNOME Bugzilla – Bug 571664
[1/3] Empathy should support Geolocation
Last modified: 2009-05-07 18:04:04 UTC
Empathy should have support for org.freedesktop.Telepathy.Connection.Interface.Location.DRAFT telepathy spec. This first branch implements Location storing inside EmpathyContact. This location will be updated if telepathy-gabble (starting with 0.7.20) receives it.
Created attachment 128660 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-base extensions/Connection_Interface_Location.xml | 330 ++++++++++++++++++++++++++ extensions/Makefile.am | 1 + extensions/all.xml | 12 + extensions/misc.xml | 1 + libempathy/Makefile.am | 1 + libempathy/empathy-contact-factory.c | 12 + libempathy/empathy-contact-factory.h | 3 + libempathy/empathy-contact.c | 60 +++++- libempathy/empathy-contact.h | 4 + libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 3 +- libempathy/empathy-location.h | 55 +++++ libempathy/empathy-tp-contact-factory.c | 105 ++++++++- libempathy/empathy-tp-contact-factory.h | 3 +- 14 files changed, 587 insertions(+), 4 deletions(-)
There is some plan to kill EmpathyContact and use TpContact instead. Maybe your branch should be updated at some point.
Created attachment 132300 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-base configure.ac | 66 +++ data/empathy.schemas.in | 88 ++++- extensions/Connection_Interface_Location.xml | 330 ++++++++++++++ extensions/Makefile.am | 1 + extensions/all.xml | 12 + extensions/misc.xml | 1 + libempathy-gtk/Makefile.am | 6 + libempathy-gtk/empathy-conf.h | 7 + libempathy-gtk/empathy-contact-dialogs.c | 5 +- libempathy-gtk/empathy-contact-widget.c | 107 +++++ libempathy-gtk/empathy-contact-widget.glade | 157 ++++--- libempathy-gtk/empathy-contact-widget.h | 1 + libempathy-gtk/empathy-location-manager.c | 626 ++++++++++++++++++++++++++ libempathy-gtk/empathy-location-manager.h | 62 +++ libempathy/Makefile.am | 1 + libempathy/empathy-contact.c | 60 +++ libempathy/empathy-contact.h | 4 + libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 3 +- libempathy/empathy-location.h | 56 +++ libempathy/empathy-tp-contact-factory.c | 118 +++++- libempathy/empathy-tp-contact-factory.h | 3 +- src/Makefile.am | 21 +- src/empathy-main-window.c | 22 + src/empathy-main-window.glade | 8 + src/empathy-map-view-marker.svg | 109 +++++ src/empathy-map-view.c | 386 ++++++++++++++++ src/empathy-map-view.glade | 60 +++ src/empathy-map-view.h | 33 ++ src/empathy-preferences.c | 158 +++++-- src/empathy-preferences.glade | 252 +++++++++++ src/empathy.c | 20 + 32 files changed, 2683 insertions(+), 101 deletions(-)
Created attachment 132301 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-base extensions/Connection_Interface_Location.xml | 330 ++++++++++++++++++++++++++ extensions/Makefile.am | 1 + extensions/all.xml | 12 + extensions/misc.xml | 1 + libempathy/Makefile.am | 1 + libempathy/empathy-contact.c | 60 +++++ libempathy/empathy-contact.h | 4 + libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 3 +- libempathy/empathy-location.h | 55 +++++ libempathy/empathy-tp-contact-factory.c | 118 +++++++++- libempathy/empathy-tp-contact-factory.h | 3 +- 12 files changed, 586 insertions(+), 3 deletions(-)
A good first step would be to add location features to TpContact.
Hum, I didn't consider that hehe, I thought that if it still worked it was ok :)
Created attachment 132844 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-base extensions/Connection_Interface_Location.xml | 330 ++++++++++++++++++++++++++ extensions/Makefile.am | 1 + extensions/all.xml | 12 + extensions/misc.xml | 1 + libempathy/Makefile.am | 1 + libempathy/empathy-contact.c | 60 +++++ libempathy/empathy-contact.h | 4 + libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 3 +- libempathy/empathy-location.h | 55 +++++ libempathy/empathy-tp-contact-factory.c | 118 +++++++++- libempathy/empathy-tp-contact-factory.h | 3 +- 12 files changed, 586 insertions(+), 3 deletions(-)
empathy-contact • Please document the location hash table in EmpathyContactPriv: type of keys and values, who owns it, etc • "location" property should have the STATIC_STRINGS flag • empathy_contact_get_location: doc. I'd change " @contact: the contact" to "an #EmpathyContact" • doc should also said that the hash table doens't have to be destroyed and that it's valid while the contact is alive • shouldn't it return a const GHashTable* actually? • empathy_contact_set_location's doc: some explanation about what this hash table is supposed to contain would be good empathy-tp-contact-factory.c • use if (ptr != NULL) instead of if (ptr) as said in our coding style • tp_contact_factory_got_locations: remove this comment: + /* do something with key and value */ • tp_contact_factory_got_locations: not sure about the FIXME. Xavier: what do you think? • trailing space at line 467 • + g_hash_table_insert (location, g_strdup(key), tp_g_value_slice_dup (value)); missing space after g_strdup • tp_contact_factory_location_updated_cb : update_location_for_each will g_strdup the key but they won' be freed later as the 3rd argument of g_hash_table_new_full is NULL • I'd replace g_hash_table_foreach calls by iterator • tp_contact_factory_location_updated_cb: properly manage the case if tp_contact_factory_find_by_handle returns NULL (or assert it doesn't if it's not supposed to) • tp_contact_factory_add_contact: the _run_ call makes me cry but I guess we can't easily get rid of it and it will disappear once TpContact supports Location. Please file an empathy bug saying we should use TpContact directly for location and link the relevelant tp-glib bug (report it if needed)
So I fixed all your comments, here are worth of mention changes: • shouldn't it return a const GHashTable* actually? I made it so that you have to unref the returned table now. It is now mentioned in the documentation. • tp_contact_factory_got_locations: not sure about the FIXME. Xavier: what do you think? This was a copy/paste from another function, doesn't apply here. • tp_contact_factory_add_contact: the _run_ call makes me cry This function already contains calls to _run_ for capabilities, and get_known_avatar_tokens... I just followed the pattern! :) I'll push the new branch to my repo as soon as my ssh key gets accepted.
2 more comments. • Be coherent in the @contact documentation and always use "an #EmpathyContact" • tp_contact_factory_add_contact: I know you just copied the current pattern but as the location is not really a critical information, couldn't we get them async and update the contact object once we got them? To say that otherwise, do we really need to get the location when tp_contact_factory_add_contact returns?
• tp_contact_factory_add_contact: I know you just copied the current pattern but as the location is not really a critical information, couldn't we get them async and update the contact object once we got them? To say that otherwise, do we really need to get the location when tp_contact_factory_add_contact returns? Sure we don't need that information when we come out of this function but this call should be quite fast as it actually doesn't require any information to be fetched on the network. As per the Spec, Gabble will return the cached information immediately and if none available it'll fetch some that will be returned later using LocationChanged.
(In reply to comment #11) > Sure we don't need that information when we come out of this function but this > call should be quite fast as it actually doesn't require any information to be > fetched on the network. As per the Spec, Gabble will return the cached > information immediately and if none available it'll fetch some that will be > returned later using LocationChanged. That's not the point. We are working on removing all the _run_ calls in Empathy, so we shouldn't introduce a new one if we can easily avoid it.
I updated my branch on git.collabora.co.uk. It should address all the commments you said. I even simplified the code a bit as you actually didn't need to copy the passed GHashTable, you just need to ref it :D It shows that I learned a lot on memory management since I started writing this code.
Last comments and then we're good for merging: +void +empathy_contact_set_location (EmpathyContact *contact, + GHashTable *location) remove the trailing space after 'void' remove g_print calls What happen when using a CM not supporting Location? I'm a bit worry to flood debug output with lot of error messages.
+1 please merge
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.