Created attachment 253384 [details] [review]
datetime: Add new datetime plugin

For now, this takes care of automatic timezone updating based on
geolocation. It relies on the new GeoClue2 D-Bus service for getting the
location information and updates the system TZ using the
org.freedesktop.timedate1 service.

In the future, it is also going to take care of displaying notifications
for upcoming time related changes ("Daylight Saving Starts Tonight") and
notifying the user about timezone changes.

Created attachment 253385 [details] [review]
datetime: Avoid popping up polkit password prompts

Check if we have permissions to set the timezone. If we don't, just
avoid changing the system TZ on location changes.

Review of attachment 253384 [details] [review]:

Looks good from geoclue's POV and in general. Would like some g-s-d hacker to have another look though.

Also, don't we need a freeze break for this? Does it work? :)

::: plugins/datetime/backward
@@ +1,2 @@
+# <pre>
+# @(#)backward	8.9

Weird filename. Where does it come from?

::: plugins/datetime/geoclue-interface.xml
@@ +1,1 @@
+<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"

I really think geoclue should provide this file. I just don't know how I'm supposed to provide the path to it in the .pc file yet. :(

::: plugins/datetime/gsd-datetime-manager.c
@@ +65,3 @@
+gboolean
+gsd_datetime_manager_start (GsdDatetimeManager *self,
+                            GError **error)

'error' arg is not correctly aligned.

@@ +91,3 @@
+
+static GObject *
+gsd_datetime_manager_constructor (GType type,

Why override constructor only to chain-up?

::: plugins/datetime/gsd-timezone-monitor.c
@@ +74,3 @@
+
+#define EARTH_RADIUS_KM 6372.795
+

Would be nice to have a comment here saying something like "Copy of geocode_location_get_distance_from() from geocode-glib library"

@@ +98,3 @@
+static gint
+sort_locations (TzLocation *a,
+                TzLocation *b)

This is a comparison function so 'compare_locations' would be a more suitable name for it.

@@ +112,3 @@
+process_location (GsdTimezoneMonitor *self,
+                  gdouble latitude,
+                  gdouble longitude)

arg alignment nitpick again. :)

Review of attachment 253385 [details] [review]:

Looks good otherwise.

::: plugins/datetime/gsd-timezone-monitor.c
@@ +337,3 @@
         g_debug ("Starting timezone monitor");
 
+        priv->permission = polkit_permission_new_sync ("org.freedesktop.timedate1.set-timezone", NULL, NULL, NULL);

* Doesn't g-s-d have 80-chars rule?
* Do we really want/need to use 'sync' call everywhere btw?

@@ +339,3 @@
+        priv->permission = polkit_permission_new_sync ("org.freedesktop.timedate1.set-timezone", NULL, NULL, NULL);
+        if (priv->permission == NULL) {
+                g_warning ("Your system does not have the '%s' PolicyKit files installed. Please check your installation",

* Same comment as just above.
* Why use '%s' if value is static?

(In reply to comment #3)
> Review of attachment 253384 [details] [review]:
> 
> Looks good from geoclue's POV and in general. Would like some g-s-d hacker to
> have another look though.
> 
> Also, don't we need a freeze break for this? Does it work? :)

I would say this doesn't require a freeze break: we're in the UI freeze and it doesn't add any new UI.

(I do have plans to add new notifications, and expose the timezone geolocation in control-center; that would definitely need freeze breaks. But the patches here so far shouldn't.)

It works fine as it is right now, but there are a few more issues to solve before exposing it in g-c-c / turning it on by default. I'd like to approach them incrementally though with follow up patches.

1) It doesn't detect the timezone very accurately because it only uses the Olson timezone database and calculates distances to the TZ cities in there. Right now it can sometimes get wrong results at TZ borders.

2) There's an issue with permissions: we'd need to either change systemd's timedate1 policy to allow all users to set timezone, or ship our own policy that relaxes the permissions. (Or ask downstream distributors to ship their own policy that relaxes systemd default for desktop installs.)


> 
> ::: plugins/datetime/backward
> @@ +1,2 @@
> +# <pre>
> +# @(#)backward    8.9
> 
> Weird filename. Where does it come from?

These are from gnome-control-center, the backward and tz.* are just copies of the code from there. Would be nice to break them out to a separate library instead of copying them around between all the apps, but that's for another day. https://bugzilla.gnome.org/show_bug.cgi?id=702194 has some discussion about that.


> ::: plugins/datetime/geoclue-interface.xml
> @@ +1,1 @@
> +<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
> 
> I really think geoclue should provide this file. I just don't know how I'm
> supposed to provide the path to it in the .pc file yet. :(

Yes, I very much agree. I can help with that and we can remove the interface file from g-s-d when geoclue is fixed.

> +static GObject *
> +gsd_datetime_manager_constructor (GType type,
> 
> Why override constructor only to chain-up?

All other g-s-d plugins seem to do this, no idea why. Could be copy-paste code and nobody has noticed before :)

Created attachment 253422 [details] [review]
datetime: Add new datetime plugin

For now, this takes care of automatic timezone updating based on
geolocation. It relies on the new GeoClue2 D-Bus service for getting the
location information and updates the system TZ using the
org.freedesktop.timedate1 service.

In the future, it is also going to take care of displaying notifications
for upcoming time related changes ("Daylight Saving Starts Tonight") and
notifying the user about timezone changes.

Created attachment 253423 [details] [review]
datetime: Avoid popping up polkit password prompts

Check if we have permissions to set the timezone. If we don't, just
avoid changing the system TZ on location changes.

Not sure what to do about the two _sync() dbus calls. They make code a fair bit simpler, but I guess they could block for a bit if the dbus server is overloaded.

Addressed the rest of the comments.

Review of attachment 253422 [details] [review]:

::: plugins/datetime/gsd-datetime-manager.c
@@ +56,3 @@
+        if (enabled && self->priv->timezone_monitor == NULL) {
+                g_debug ("Automatic timezone enabled");
+                self->priv->timezone_monitor = gsd_timezone_monitor_new ();

Do you need to do some sort of cold-plugging here, to make sure you get the currently detected tz when you enable automatic timezones ?

::: plugins/datetime/gsd-timezone-monitor.c
@@ +141,3 @@
+
+        g_list_free (distances);
+}

Just using the timezone capitals for this has to be fairly inaccurate. When I wrote code like this years ago, we used all of the entries of the libgweather database - don't they all have tz information ?

@@ +165,3 @@
+        process_location (self, latitude, longitude);
+
+        g_object_unref (location);

How often is this signal emitted ? If I'm working on a train, is my laptop constantly comparing timezones ?

Review of attachment 253423 [details] [review]:

This looks reasonable.

(In reply to comment #10)
> Review of attachment 253422 [details] [review]:
> 
> ::: plugins/datetime/gsd-datetime-manager.c
> @@ +56,3 @@
> +        if (enabled && self->priv->timezone_monitor == NULL) {
> +                g_debug ("Automatic timezone enabled");
> +                self->priv->timezone_monitor = gsd_timezone_monitor_new ();
> 
> Do you need to do some sort of cold-plugging here, to make sure you get the
> currently detected tz when you enable automatic timezones ?

I don't think so. GsdTimezoneMonitor only changes the tz when it gets new location info. If the automatic timezones are enabled and there's no location info from geoclue, it just leaves the system tz intact, it doesn't reset it to a default value or anything.

(I might have misunderstood the question though, sorry in that case.)


> ::: plugins/datetime/gsd-timezone-monitor.c
> @@ +141,3 @@
> +
> +        g_list_free (distances);
> +}
> 
> Just using the timezone capitals for this has to be fairly inaccurate. When I
> wrote code like this years ago, we used all of the entries of the libgweather
> database - don't they all have tz information ?

Yes, I intentionally left it out to get the basic plugin in first, and then incrementally improve it. Right now it really is inaccurate. This is one of the things I want to improve before turning on the automatic timezones by default.

What I'm planning is something like the following:

1) get location info from geoclue (note that this could be from gps, and we might be offline)

2) try to find out the country:

   a) geoclue's location info might include it, if we are lucky
     (https://bugs.freedesktop.org/show_bug.cgi?id=68645)
   b) otherwise do reverse geocoding through geocode-glib to map the coordinates to a country

3) if we managed to find out the country, then filter our city database to only include cities from that country

4) find the nearest city and the matching timezone

5) set settings keys to indicate the detected city, so that g-c-c and other apps could just show cached values:

org.gnome.desktop.datetime last-timezone="Europe/Stockholm"
org.gnome.desktop.datetime last-timezone-description="Göteborg, Sweden"

6) Display a notification


> @@ +165,3 @@
> +        process_location (self, latitude, longitude);
> +
> +        g_object_unref (location);
> 
> How often is this signal emitted ? If I'm working on a train, is my laptop
> constantly comparing timezones ?

Depends on how GeoClue ends up implementing this.

Currently GeoClue only emits the "location-changed" signal once. It's supposed to poll the servers to detect changes and emit the signal more often, but right now it's unimplemented. Zeeshan said it's on his todo list to add location change polling to GeoClue, though.

(In reply to comment #12)
> (In reply to comment #10)
> > Review of attachment 253422 [details] [review] [details]:
> >
> > @@ +165,3 @@
> > +        process_location (self, latitude, longitude);
> > +
> > +        g_object_unref (location);
> > 
> > How often is this signal emitted ? If I'm working on a train, is my laptop
> > constantly comparing timezones ?
> 
> Depends on how GeoClue ends up implementing this.
> 
> Currently GeoClue only emits the "location-changed" signal once. It's supposed
> to poll the servers to detect changes and emit the signal more often, but right
> now it's unimplemented. Zeeshan said it's on his todo list to add location
> change polling to GeoClue, though.

Yes, I was hoping to do that today actually. Since currently location is based on IP and accuracy is usually city-level, there won't be any spam of 'location-changed' signals. However, when we'll have wifi, GPS and 3G support, location will update very often depending on availability of these sources. Having said that, there is going to be 'DistanceThreshold' property that apps can set to control this.

(In reply to comment #12)
> (In reply to comment #10)
> > Review of attachment 253422 [details] [review] [details]:
> > 
> > ::: plugins/datetime/gsd-datetime-manager.c
> > @@ +56,3 @@
> > +        if (enabled && self->priv->timezone_monitor == NULL) {
> > +                g_debug ("Automatic timezone enabled");
> > +                self->priv->timezone_monitor = gsd_timezone_monitor_new ();
> > 
> > Do you need to do some sort of cold-plugging here, to make sure you get the
> > currently detected tz when you enable automatic timezones ?
> 
> I don't think so. GsdTimezoneMonitor only changes the tz when it gets new
> location info. If the automatic timezones are enabled and there's no location
> info from geoclue, it just leaves the system tz intact, it doesn't reset it to
> a default value or anything.
> 
> (I might have misunderstood the question though, sorry in that case.)
> 

Fly from Boston to Vienna, your laptop has automatic location enabled, and is turned off. In Vienna, you open the laptop. My laptop show boston time until geoclue sends a location changed signal - I guess things will be fine if geoclue sends a changed signal while g-s-d is listening. But what if geoclue already sent that location-changed signal before I logged in ? Will I be stuck in Boston time ?

Ah, now I understand, thanks for explaining the issue!

geoclue always sends the location changed signal after a new client connects and calls start(). So even if we connect to an existing geoclue daemon, clients don't need to do anything special to get updated location info at startup, geoclue will send the signal in any case. I've asked Zeeshan about it and he said it's something clients should be able to rely on.

Thanks for the reviews, everybody! This now has release team approval as well:

https://mail.gnome.org/archives/release-team/2013-August/msg00116.html

Attachment 253422 [details] pushed as 8765903 - datetime: Add new datetime plugin
Attachment 253423 [details] pushed as 35888bd - datetime: Avoid popping up polkit password prompts

Forgive my ignorance but where is the schema org.gnome.desktop.datetime to be found?

With this plugin enabled, and the schema loaded with that that name (g_settings_new() in the plugin), my g-s-d just crashes saying the schema is not loaded... I'm guessing I'm missing something somewhere, but I cannot see it!

nvm: Kalev pointed me in the right direct... just me being a bit dim as I didn't see the commits listed here :)

https://git.gnome.org/browse/gsettings-desktop-schemas/commit/?id=e123fa0170d3c5f03dcc523787769eb559ee4310