GNOME Bugzilla – Bug 707200
datetime: Use reverse geocoding for country detection
Last modified: 2013-09-02 20:02:56 UTC
.
Created attachment 253725 [details] [review] datetime: Use reverse geocoding for country detection In an ideal world, we'd be able to get the timezone directly from reverse geocoding. Nominatim, the service that geocode-glib uses, however doesn't have that information, and so far there's only a feature request: https://trac.openstreetmap.org/ticket/4200 Instead, we use geocode-glib to find out the country, and then do timezone mapping ourselves.
Created attachment 253728 [details] [review] datetime: Use geocode_location_get_distance_from() Now that we're linking with geocode-glib anyway, we can drop copy-paste distance calculation code and use geocode_location_get_distance_from() from geocode-glib instead.
Review of attachment 253725 [details] [review]: Looks fine otherwise. ::: configure.ac @@ +254,3 @@ AC_SUBST(LIBM) +PKG_CHECK_MODULES(DATETIME, [geocode-glib-1.0 >= $GEOCODE_GLIB_REQUIRED_VERSION polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION]) I can't even see this line in splinter, can you please break it. ::: plugins/datetime/gsd-timezone-monitor.c @@ +221,3 @@ +process_location (GsdTimezoneMonitor *self, + GeocodeLocation *new_location, + const gchar *country) Better use more descriptive name: country_code. @@ +241,3 @@ + GError *error = NULL; + GsdTimezoneMonitor *self = user_data; + const gchar *country; Same comment for this variable. @@ +247,3 @@ + &error); + if (error != NULL) { + g_warning ("Failed to reverse lookup country: %s", error->message); * Shouldn't we be using the old codepath in case of geocoding failure, which is going to happen always when offline (unless request was cached)? For now geoclue only uses network source to find location so its not that big a deal right now but in (hopefully near) future, it will not depend on that and in very near future it will provide last known location so you'll get location while offline but not necessarily be able to reverse geocode it. * This error message will also need updating after rename of function. @@ +255,3 @@ + + g_debug ("Geocode lookup resolved country to %s", country); + process_location (self, location, country); Why not just pass place to process_location and let it take what it needs? @@ +276,3 @@ + geocode_reverse_resolve_async (reverse, + priv->cancellable, + on_country_lookup_ready, This call is not looking for country but rather just a reverse geocoding request so callback's name is misleading. My suggestion would be 'on_reverse_geocoding_ready' or 'on_place_ready'.
Review of attachment 253728 [details] [review]: Looks good. ::: plugins/datetime/gsd-timezone-monitor.c @@ +120,3 @@ + loc = geocode_location_new (tz_location->latitude, + tz_location->longitude, + GEOCODE_LOCATION_ACCURACY_CITY); ACCURACY_UNKNOWN would be better even though in here it hardly matters but it might confuse reader a bit..
Created attachment 253747 [details] [review] datetime: Use reverse geocoding for country detection Previously, we would get the current coordinates from geolocation and then try to match them to the offline timezone database. This commit adds one more step to make the matching more accurate: reverse geocoding in order to narrow down the location to a specific country. In an ideal world, we'd be able to get the timezone directly from reverse geocoding. Nominatim, the service that geocode-glib uses, however doesn't offer timezone information yet: https://trac.openstreetmap.org/ticket/4200 Instead, we use geocode-glib to find out the country, and then do timezone mapping ourselves.
Created attachment 253748 [details] [review] datetime: Use reverse geocoding for country detection Previously, we would get the current coordinates from geolocation and then try to match them to the offline timezone database. This commit adds one more step to make the matching more accurate: reverse geocoding in order to narrow down the location to a specific country. In an ideal world, we'd be able to get the timezone directly from reverse geocoding. Nominatim, the service that geocode-glib uses, however doesn't offer timezone information yet: https://trac.openstreetmap.org/ticket/4200 Instead, we use geocode-glib to find out the country, and then do timezone mapping ourselves.
Created attachment 253749 [details] [review] datetime: Use geocode-glib for calculating distances Now that we're linking with geocode-glib anyway, we can drop copy-paste distance calculation code and use geocode_location_get_distance_from() from geocode-glib instead.
(Oops, sorry for double-posting the first patch, bugzilla was giving me error 500) (In reply to comment #3) > + if (error != NULL) { > + g_warning ("Failed to reverse lookup country: %s", > error->message); > > * Shouldn't we be using the old codepath in case of geocoding failure, which is > going to happen always when offline (unless request was cached)? For now > geoclue only uses network source to find location so its not that big a deal > right now but in (hopefully near) future, it will not depend on that and in > very near future it will provide last known location so you'll get location > while offline but not necessarily be able to reverse geocode it. We have to be careful here to avoid fluctuating between different timezones depending on whether the computer is online or offline. Even more so when GeoClue2 starts caching results for offline use. For some locations that are at timezone borders, the reverse geocoding result means putting the user in one timezone or the other. And it would look bad if they boot up and end up at one timezone because they are offline, and at next boot the timezone changes again because this time they are online. I think it's best to just not change the timezone for now when we don't have the reverse geocoding result; the offline (GPS) case needs some more thought. I've also changed the g_warning() above to a g_debug() so that it doesn't spam logs when offline. Fixed up the other issues pointed out.
(In reply to comment #8) > (Oops, sorry for double-posting the first patch, bugzilla was giving me error > 500) > > (In reply to comment #3) > > + if (error != NULL) { > > + g_warning ("Failed to reverse lookup country: %s", > > error->message); > > > > * Shouldn't we be using the old codepath in case of geocoding failure, which is > > going to happen always when offline (unless request was cached)? For now > > geoclue only uses network source to find location so its not that big a deal > > right now but in (hopefully near) future, it will not depend on that and in > > very near future it will provide last known location so you'll get location > > while offline but not necessarily be able to reverse geocode it. > > We have to be careful here to avoid fluctuating between different timezones > depending on whether the computer is online or offline. Even more so when > GeoClue2 starts caching results for offline use. > > For some locations that are at timezone borders, the reverse geocoding result > means putting the user in one timezone or the other. And it would look bad if > they boot up and end up at one timezone because they are offline, and at next > boot the timezone changes again because this time they are online. Valid point. > I think it's best to just not change the timezone for now when we don't have > the reverse geocoding result; the offline (GPS) case needs some more thought. > I've also changed the g_warning() above to a g_debug() so that it doesn't spam > logs when offline. Agreed. I think its pretty fine for automatic tz to require network (at least for now). > Fixed up the other issues pointed out. Cool, thanks.
Review of attachment 253748 [details] [review]: Apart from these nitpicks, looks good as previously. ::: configure.ac @@ +256,3 @@ +PKG_CHECK_MODULES(DATETIME, + geocode-glib-1.0 >= $GEOCODE_GLIB_REQUIRED_VERSION + polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION Shouln't these be aligned with DATETIME? ::: plugins/datetime/gsd-timezone-monitor.c @@ +262,2 @@ +static void +lookup_country (GsdTimezoneMonitor *self, This function actually in the end sets the tz through the country so I'd also rename this one. set_timezone_from_location? If its named for only what it does within its body, this name is even more misleading. @@ +263,3 @@ +lookup_country (GsdTimezoneMonitor *self, + gdouble latitude, + gdouble longitude) Same thing about these args, perhaps we should be passing the GeoclueLocation itself. Which also makes me wonder if we should simply merge it with on_location_proxy_ready?
Review of attachment 253748 [details] [review]: Yikes, wrong resolution. :)
Review of attachment 253749 [details] [review]: ack
(In reply to comment #10) > Review of attachment 253748 [details] [review]: > > Apart from these nitpicks, looks good as previously. > > ::: configure.ac > @@ +256,3 @@ > +PKG_CHECK_MODULES(DATETIME, > + geocode-glib-1.0 >= $GEOCODE_GLIB_REQUIRED_VERSION > + polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION > > Shouln't these be aligned with DATETIME? Just following existing style here. > ::: plugins/datetime/gsd-timezone-monitor.c > @@ +262,2 @@ > +static void > +lookup_country (GsdTimezoneMonitor *self, > > This function actually in the end sets the tz through the country so I'd also > rename this one. set_timezone_from_location? If its named for only what it does > within its body, this name is even more misleading. I'd rather name it for what it does within its body. There's a long chain of async functions that lead up to setting the timezone, and we have to distinguish them somehow, can't call all of them set_timezone_from_location. How about start_reverse_geocoding() instead of lookup_country() ? > @@ +263,3 @@ > +lookup_country (GsdTimezoneMonitor *self, > + gdouble latitude, > + gdouble longitude) > > Same thing about these args, perhaps we should be passing the GeoclueLocation > itself. Which also makes me wonder if we should simply merge it with > on_location_proxy_ready? The reason why I am passing latitude and longitude is that there are two different Location types: - lookup_country() deals with geocode-glib and uses GeocodeLocation - on_location_proxy() deals with geoclue2 and uses GeoclueLocation If I merge these two together, I'll have a GeocodeLocation and a GeoclueLocation in the same function, making code harder to read and understand.
(In reply to comment #13) > (In reply to comment #10) > > Review of attachment 253748 [details] [review] [details]: > > > > Apart from these nitpicks, looks good as previously. > > > > ::: configure.ac > > @@ +256,3 @@ > > +PKG_CHECK_MODULES(DATETIME, > > + geocode-glib-1.0 >= $GEOCODE_GLIB_REQUIRED_VERSION > > + polkit-gobject-1 >= $POLKIT_REQUIRED_VERSION > > > > Shouln't these be aligned with DATETIME? > > Just following existing style here. Ah ok. > > ::: plugins/datetime/gsd-timezone-monitor.c > > @@ +262,2 @@ > > +static void > > +lookup_country (GsdTimezoneMonitor *self, > > > > This function actually in the end sets the tz through the country so I'd also > > rename this one. set_timezone_from_location? If its named for only what it does > > within its body, this name is even more misleading. > > I'd rather name it for what it does within its body. There's a long chain of > async functions that lead up to setting the timezone, and we have to > distinguish them somehow, can't call all of them set_timezone_from_location. The solution to that should be just using different names, its ok to use longer names if a function starts a chain of async operations. > How about start_reverse_geocoding() instead of lookup_country() ? Thats fine too I guess, as it indicates that function is starting a process rather than looking-up for a country. > > @@ +263,3 @@ > > +lookup_country (GsdTimezoneMonitor *self, > > + gdouble latitude, > > + gdouble longitude) > > > > Same thing about these args, perhaps we should be passing the GeoclueLocation > > itself. Which also makes me wonder if we should simply merge it with > > on_location_proxy_ready? > > The reason why I am passing latitude and longitude is that there are two > different Location types: > > - lookup_country() deals with geocode-glib and uses GeocodeLocation > - on_location_proxy() deals with geoclue2 and uses GeoclueLocation > > If I merge these two together, I'll have a GeocodeLocation and a > GeoclueLocation in the same function, making code harder to read and > understand. you can rename the variables to reduce any confusions: geoclue_location and location. I just don't see the point of extracting props, passing it to another function only to get it re-merged into another (almost same) object. Anyways, no blocker or anything such.
Alright, I've fixed up the function name. Thanks again for all the reviews! Attachment 253748 [details] pushed as 41a9dd1 - datetime: Use reverse geocoding for country detection Attachment 253749 [details] pushed as 911b60a - datetime: Use geocode-glib for calculating distances