GNOME Bugzilla – Bug 571667
[3/3] Empathy should display my location
Last modified: 2009-05-27 18:28:02 UTC
Empathy should have support for org.freedesktop.Telepathy.Connection.Interface.Location.DRAFT telepathy spec. This third branch implements Location displaying using libchamplain. A new dialog named "Contact Map View" is available under the Chat menu in the contact list, and a map is displayed (if location information is available for the contact) under its Contact Information dialog.
Created attachment 128664 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-ui configure.ac | 5 + libempathy-gtk/Makefile.am | 2 + libempathy-gtk/empathy-conf.h | 1 + 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 + 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 | 378 +++++++++++++++++++++++++++ src/empathy-map-view.glade | 60 +++++ src/empathy-map-view.h | 33 +++ src/empathy.c | 9 + 15 files changed, 859 insertions(+), 59 deletions(-)
As libchamplain is not an official GNOME external dependency (yet ;) it should be optionnal.
Yeah, it is optionally built right now. I do plan to have a more detailed UI (once all these patch goes in). The detailed UI would explicitly list the Address fields, so someone without libchamplain would be able to get information.
I don't consider this a GNOME 2.28 target blocker, it's "just" a very nice enhancement. Feel free to set the module specific target milestone though
Created attachment 132303 [details] [review] Proposed fix in branch geolocation-ui http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-ui configure.ac | 36 +++ libempathy-gtk/Makefile.am | 2 + libempathy-gtk/empathy-conf.h | 1 + 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 + 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.c | 8 + 15 files changed, 896 insertions(+), 60 deletions(-)
While I can't figure out geolocation-publish's problem, I updated this branch. It doesn't depend on geolocation-publish anymore. I also added a basic UI (that still needs to be made pretty) which will list the location information in tooltips and under the map.
This is a first review from someone who doesn't know anything about libchamplain. • New UI file should be added to po/POTFILES.in (and new .c file probably too; even if it doesn't contain any translatable string atm) empathy-contact-dialogs.c • empathy_contact_edit_dialog_show: why did you remove this comment ? empathy-contact-widget.c • Please add a comment explaining why _GNU_SOURCE is needed • contact_widget_location_update: when checking if the EMPATHY_LOCATION_TIMESTAMP value if NULL, add { } in the if branch (to be coherent with the else branch) • contact_widget_location_update: is the LAT and LON info really mandatory? I mean should we early return if we don't have these information? Could be worth to display, say, the city even if we don't have the precise location • contact_widget_location_update: as you said, we should display a pretty name instead of the key • + if (/* information->flags & EMPATHY_CONTACT_WIDGET_FOR_TOOLTIP || */ Why is that part commented? • contact_widget_location_update: the champlain objects created in this function are never freed empathy.c • + gtk_clutter_init(&argc, &argv); missing space before '(' empathy-map-view • Update copyright in the header file • I guess you should use your collabora address • Don't use C++ style comments • fix FIXME • empathy_map_view_show: use g_slice_new_new ? • empathy_builder_get_file and empathy_builder_connect are lot easier to read if you set only one paire by line • champlain_view_center_on () where this 36 come from? A DEFINE would be better if it's a magic number • window->layer is never freed • Maybe EmpathyMapView should be a widget inheriting from GtkWindow? • map_view_geocode_cb: why are you reffing/unreffing the hash table? • map_view_geocode_cb: hash table is not unreffed if error != NULL • map_view_geocode_cb: you could use tp_g_value_slice_new_double • map_view_geocode_cb: some comments explaining what this function does would be good. It's not obvious for someone who doesn't know geoclue • map_view_marker_update_position: is the address hash table destroyed at some point? and the GeoclueGeocode object? • map_view_contacts_foreach: when is the marker freed? Got this crash when trying to test your branch (without libchamplain): Gtk-CRITICAL **: gtk_widget_hide: assertion `GTK_IS_WIDGET (widget)' failed aborting... Program received signal SIGABRT, Aborted.
+ Trace 215401
Thread 139835388077984 (LWP 9516)
As suggested on bug #562468 the "Show Contacts on a map" menu item should be in a "View" section. Please record a patch adding this new section seperately so it can be easily cherry-picked.
Please update or rebase your branch on top of master and be sure that "make check" works; it should perform some coding style checks.
Created attachment 135126 [details] [review] Reworked branch geolocation-ui http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-ui configure.ac | 66 +++++ libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-contact-dialogs.c | 1 + libempathy-gtk/empathy-contact-widget.c | 237 +++++++++++++++++++ libempathy-gtk/empathy-contact-widget.h | 1 + libempathy-gtk/empathy-contact-widget.ui | 62 +++++- po/POTFILES.in | 2 + src/Makefile.am | 16 +- src/empathy-main-window.c | 19 ++ src/empathy-main-window.ui | 8 + src/empathy-map-view.c | 380 ++++++++++++++++++++++++++++++ src/empathy-map-view.h | 32 +++ src/empathy-map-view.ui | 59 +++++ src/empathy.c | 8 + 14 files changed, 890 insertions(+), 5 deletions(-)
location_key_to_label Could be worth to use a hashtable for faster association
IMHO a button in the map view to go back to to max zoom out could be useful.
The "View contacts on a map" menu entry should be in the View menu once #583566 has been merged.
Midair colision reverted.
Empathy master now prints a configure report. Please upgrade it according the new optionnal dep(s).
Created attachment 135336 [details] [review] Proposed fix in branch geolocation-ui http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-ui configure.ac | 68 ++++++ libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-contact-dialogs.c | 1 + libempathy-gtk/empathy-contact-widget.c | 240 +++++++++++++++++++ libempathy-gtk/empathy-contact-widget.h | 1 + libempathy-gtk/empathy-contact-widget.ui | 62 +++++- po/POTFILES.in | 2 + src/Makefile.am | 16 +- src/empathy-main-window.c | 19 ++ src/empathy-main-window.ui | 7 + src/empathy-map-view.c | 386 ++++++++++++++++++++++++++++++ src/empathy-map-view.h | 32 +++ src/empathy-map-view.ui | 59 +++++ src/empathy.c | 8 + 14 files changed, 900 insertions(+), 5 deletions(-)
This new patch places the menu option in View, fix a bug where markers were created for every user (and not only for users with a location) and print a report at the end of configure.
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.