GNOME Bugzilla – Bug 571666
[2/3] Empathy should publish my geolocation
Last modified: 2009-06-01 15:49:09 UTC
Empathy should have support for org.freedesktop.Telepathy.Connection.Interface.Location.DRAFT telepathy spec. This second branch implements location discovery using Geoclue. This location will be published to all accounts (this currently need to be fixed, only accounts supporting the Location interface should have the location published to).
Created attachment 128663 [details] [review] Proposed fix in branch geolocation-base http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-publish configure.ac | 61 +++ data/empathy.schemas.in | 88 ++++- libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-conf.h | 6 + libempathy-gtk/empathy-location-manager.c | 618 +++++++++++++++++++++++++++++ libempathy-gtk/empathy-location-manager.h | 62 +++ libempathy/empathy-location.h | 1 + src/empathy-preferences.c | 158 ++++++-- src/empathy-preferences.glade | 252 ++++++++++++ src/empathy.c | 12 + 10 files changed, 1224 insertions(+), 38 deletions(-)
Created attachment 132302 [details] [review] Proposed fix in branch geolocation-publish http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-publish configure.ac | 30 ++ data/empathy.schemas.in | 88 ++++- libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-conf.h | 6 + libempathy-gtk/empathy-location-manager.c | 626 +++++++++++++++++++++++++++++ libempathy-gtk/empathy-location-manager.h | 62 +++ libempathy/empathy-location.h | 1 + src/empathy-preferences.c | 158 ++++++-- src/empathy-preferences.glade | 252 ++++++++++++ src/empathy.c | 12 + 10 files changed, 1201 insertions(+), 38 deletions(-)
Is that comment from http://live.gnome.org/Empathy/Branches still apply? "It should check if the CM implements the interface "
Most certainly. There is the fact that I didn't know how to implement that back then.
Look how Empathy checks if the file transfer iface is implemented.
• we should raise an error when trying to build empathy with --enable-location=yes while geoclue is not installed • what's the enable_map check in configure? • empathy.schemas.in: 'to its contacts" should be "to his contacts" I think • shouldn't you use "#ifdef HAVE_GEOCLUE" instead of "#if HAVE_GEOCLUE" ? • empathy-preferences.ui contains lot of not location related changes. I guess they have been introduced when saving the file in glade. If they are, please record such changes in a separated commit. Basically, open the file and then save it directly without changing anything. empathy-preferences • l 301: remove trailing space/tab • l 1072: idem empathy-location-manager.[ch] • update the copyright • this file should use Telepathy coding style. See http://telepathy.freedesktop.org/wiki/Style Function declarations and definitions are wrong. I saw some "if (ptr)" too. Some lines are too long • this file shouldn't be built if location was disabled (instead of having a big #if LOCATION containing all the code) • EmpathyLocationManagerPriv: document the hash table • is_setup seems a poor name. geoclue_is_setup? • remove PROP_0 we don't need/use it • publish_location: some comment explaining the logic of the tests would be good • publish_location: factory is not unrefed when you early return • Don't use C++ comments • Some static declarations in the top of the file are useless • priv->mc and priv->location are never freed • remove {get,set}_property • document public API • maybe empathy_location_manager_get_default () should be renamed _dup_default and return a new ref? Check what's the current convention in Empathy. • you could use new tp_gvalue_slice_new_$TYPE helper functions (bump tp-glib dep if needed) • initial_position_cb: if you use brackets in the if block then use them in the else block as well • use hash table iterators instead of _foreach • priv->gc_* objects are not freed (maybe they don't have to) • why use g_printerr instead of DEBUG? • Is it safe to always use the same value for reduce_value? Shouldn't we generate a new one each time? I built this branch without location support and get this crash when I try to open the preference dialog: GLib-GObject-WARNING **: invalid uninstantiatable type `(null)' in cast to `GObject' aborting... Program received signal SIGABRT, Aborted.
+ Trace 215094
Thread 140196935911328 (LWP 4838)
Please update or rebase your branch on top of master and be sure that "make check" works; it should perform some coding style checks.
Crash seems fixed with the last version of your branch.
Yes I had to redo the .ui changes from scratch. Also, the branch is not entirely ready, the Preferences UI needs some love and I am working on the manual location setting.
Empathy master now prints a configure report. Please upgrade it according the new optionnal dep(s).
Created attachment 135577 [details] [review] New version of the branch geolocation-publish http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-publish configure.ac | 4 +- data/empathy.schemas.in | 72 +++- libempathy-gtk/Makefile.am | 10 + libempathy-gtk/empathy-conf.h | 5 + libempathy-gtk/empathy-location-manager.c | 673 +++++++++++++++++++++++++++++ libempathy-gtk/empathy-location-manager.h | 57 +++ libempathy/empathy-location.h | 1 + src/empathy-preferences.c | 54 +++ src/empathy-preferences.ui | 206 +++++++++- src/empathy.c | 12 + 10 files changed, 1086 insertions(+), 8 deletions(-)
So that latest version solves all standing issues and has fixes for all the comments Guillaume had. Based on Sjoerd's comments, the Manual mode has been dropped as it shouldn't be a setting in Empathy but in Geoclue. Geoclue already offers a manual mode but it forgets the values on restart. Anyway, removing this clearly simplify the code. This branch takes 10 secondes to update the location leaving some time to geoclue to get the location and to the user to change settings in the configuration screen.
Some comments for EmpathyLocationManager: - empathy_location_manager_dup_default --> We use _dup_singleton usually. And we make the singleton logic in constructor class function. See any other singleton to see how we do. - EmpathyLocationManagerPriv: token is not used. - You should reorder functions in EmpathyLocationManager to not declare them on top. - You don't need MissionControl, EmpathyAccountManager does the account<>connection mapping for you. You should connect the "new-connection" signal and *never* take care of accounts. All you care is connections. - position_changed_cb() --> inserting values can be easier with tp_asv_set_*. Same for update_timestamp(). Note that you can use it in EmpathyTpContactFactory::geocode_cb - update_timestamp, position_changed_cb: Why do you have priv=GET_PRIV in the middle of declarations? You can set it in the same line as the priv declaration. - address_changed_cb: Don't call geoclue_accuracy_get_details in the middle of declarations. - Why do you remove all items from the ht in address_changed_cb and not in position_changed_cb? - address_changed_cb: useless comment: /* do something with key and value */ - address_changed_cb: new_value = tp_g_value_slice_new_string (value); --> that seems suspicious to me... are you sure it's not tp_g_value_slice_dup? - maybe nitpicking on accuracy: you calculate priv->reduce_value only once. So if you are moving with GPS connected, you'll publish the exact path but with a delta. Shouldn't we get a new random number each time we publish a new position? That's all :)
Here are my comments on the issues I didn't address yet: > - EmpathyLocationManagerPriv: token is not used. I don't unsderstand, it is used to store various vars. > - You don't need MissionControl, EmpathyAccountManager does the > account<>connection mapping for you. You should connect the "new-connection" > signal and *never* take care of accounts. All you care is connections. I don't understand how EmpathyAccountManager can help me get rid of MissionControl: I need the TpConnection to create the EmpathyTpContactFactory on which to publish the location > - position_changed_cb() --> inserting values can be easier with tp_asv_set_*. > Same for update_timestamp(). Note that you can use it in > EmpathyTpContactFactory::geocode_cb tp_asv is not exactly what location is: the keys are not copied. > - Why do you remove all items from the ht in address_changed_cb and not in > position_changed_cb? The position information is complimentary to the address information. This is somewhat of a design bug IMHO in geoclue. You can easily end up with invalid address/position combinaison using its API. In think the current logic is not perfect but gives reasonable results. > - maybe nitpicking on accuracy: you calculate priv->reduce_value only once. So > if you are moving with GPS connected, you'll publish the exact path but with a > delta. Shouldn't we get a new random number each time we publish a new > position? You are right on that, but if we update the reduce_value each time the position changes, and that you a mostly sitting at a static place, you will be able to determine the exact position using a mean of the positions over a period of time.
Created attachment 135684 [details] [review] New version of the branch geolocation-publish http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/geolocation-publish configure.ac | 4 +- data/empathy.schemas.in | 72 ++++- libempathy-gtk/Makefile.am | 10 + libempathy-gtk/empathy-conf.h | 5 + libempathy-gtk/empathy-location-manager.c | 639 +++++++++++++++++++++++++++++ libempathy-gtk/empathy-location-manager.h | 57 +++ libempathy/empathy-location.h | 1 + src/empathy-preferences.c | 54 +++ src/empathy-preferences.ui | 206 +++++++++- src/empathy.c | 12 + 10 files changed, 1052 insertions(+), 8 deletions(-)
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.