Merge lp:~charlesk/indicator-datetime/lp-1074999 into lp:indicator-datetime/13.04

[Charles Kerr]

Don't use geoclue until the user clicks the "Show time in auto-detected location" checkbox.

Don't use geoclue until the user clicks the "Show time in auto-detected location" checkbox.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:200
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/22/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/./build=pbuilder,distribution=quantal,flavor=amd64/22/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/./build=pbuilder,distribution=quantal,flavor=i386/22/console

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

Nice cleanup!

`geo_master` doesn't need to be a global variable: it's only ever used in geo_start.

Why are you calling geo_stop unconditionally in on_use_geoclue_changed? I'd put it in the `else` below, in case the callback is called even though the value hasn't changed (which may happen according to the docs).

review: Needs Fixing

[Charles Kerr (charlesk)]

> `geo_master` doesn't need to be a global variable: it's only ever used in geo_start.

The issue is that geoclue_master_create_client_async() populates its private GeoclueMasterAsyncData struct with a pointer to that master without reffing it. That's why datetime-service keeps its own ref.

> Why are you calling geo_stop unconditionally in on_use_geoclue_changed? I'd put it in the `else` below, in case the callback is called even though the value hasn't changed (which may happen according to the docs).

It's to ensure that even if the callback triggered multiple calls to geo_start(), we would have a call to geo_stop() in between to guarantee that geo_master, geo_client, and geo_address were cleaned up.

[Lars Karlitski (larsu)]

> > `geo_master` doesn't need to be a global variable: it's only ever used in
> geo_start.
>
> The issue is that geoclue_master_create_client_async() populates its private
> GeoclueMasterAsyncData struct with a pointer to that master without reffing
> it. That's why datetime-service keeps its own ref.

That should be fixed in geoclue.

> > Why are you calling geo_stop unconditionally in on_use_geoclue_changed? I'd
> put it in the `else` below, in case the callback is called even though the
> value hasn't changed (which may happen according to the docs).
>
> It's to ensure that even if the callback triggered multiple calls to
> geo_start(), we would have a call to geo_stop() in between to guarantee that
> geo_master, geo_client, and geo_address were cleaned up.

Sounds reasonable.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/indicator-datetime-autolanding/12/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-autolanding/./build=pbuilder,distribution=quantal,flavor=amd64/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-autolanding/./build=pbuilder,distribution=quantal,flavor=i386/12/console

review: Needs Fixing (continuous-integration)

[Allan LeSage (allanlesage)]

Misconfiguration foiled autolanding; re-approving to send through again.

[Charles Kerr (charlesk)]

Thanks Allan!

=== modified file 'src/datetime-service.c'
--- src/datetime-service.c	2012-11-03 16:26:05 +0000
+++ src/datetime-service.c	2012-11-05 01:31:20 +0000
@@ -64,11 +64,9 @@
  #define SETTINGS_APP_INVOCATION "gnome-control-center datetime"
 #endif
 
-static void geo_create_client (GeoclueMaster * master, GeoclueMasterClient * client, gchar * path, GError * error, gpointer user_data);
 static gboolean update_appointment_menu_items (gpointer user_data);
 static void update_location_menu_items (void);
 static void day_timer_reset (void);
-static void geo_client_invalid (GeoclueMasterClient * client, gpointer user_data);
 static gboolean get_greeter_mode (void);
 
 static void quick_set_tz (DbusmenuMenuitem * menuitem, guint timestamp, gpointer user_data);
@@ -96,10 +94,6 @@
 static GList            * appointment_sources = NULL;
 
 
-/* Geoclue trackers */
-static GeoclueMasterClient * geo_master = NULL;
-static GeoclueAddress * geo_address = NULL;
-
 /* Our 2 important timezones */
 static gchar 			* current_timezone = NULL;
 static gchar 			* geo_timezone = NULL;
@@ -1185,31 +1179,41 @@
 	g_signal_connect(proxy, "g-signal", G_CALLBACK(session_active_change_cb), user_data);
 }
 
+
+/****
+*****  GEOCLUE
+****/
+
+static void geo_start (void);
+static void geo_stop (void);
+static void geo_create_client (GeoclueMaster * master, GeoclueMasterClient * client, gchar * path, GError * error, gpointer user_data);
+static void geo_client_invalid (GeoclueMasterClient * client, gpointer user_data);
+
+static GeoclueMaster * geo_master = NULL;
+static GeoclueMasterClient * geo_client = NULL;
+static GeoclueAddress * geo_address = NULL;
+
+static void
+geo_set_timezone (const gchar * timezone)
+{
+	if (geo_timezone != timezone) {
+		g_clear_pointer (&geo_timezone, g_free);
+		geo_timezone = g_strdup (timezone);
+		g_debug("Geoclue timezone is: %s", timezone ? timezone : "(Null)");
+		update_location_menu_items();
+	}
+}
+
 /* Callback from getting the address */
 static void
 geo_address_cb (GeoclueAddress * address, int timestamp, GHashTable * addy_data, GeoclueAccuracy * accuracy, GError * error, gpointer user_data)
 {
-	if (error != NULL) {
+	if (error == NULL) {
+		geo_set_timezone (g_hash_table_lookup (addy_data, "timezone"));
+	} else {
 		g_warning("Unable to get Geoclue address: %s", error->message);
 		g_clear_error (&error);
-		return;
-	}
-
-	g_debug("Geoclue timezone is: %s", (gchar *)g_hash_table_lookup(addy_data, "timezone"));
-
-	if (geo_timezone != NULL) {
-		g_free(geo_timezone);
-		geo_timezone = NULL;
-	}
-
-	gpointer tz_hash = g_hash_table_lookup(addy_data, "timezone");
-	if (tz_hash != NULL) {
-		geo_timezone = g_strdup((gchar *)tz_hash);
-	}
-
-	update_location_menu_items();
-
-	return;
+	}
 }
 
 /* Clean up the reference we kept to the address and make sure to
@@ -1217,16 +1221,10 @@
 static void
 geo_address_clean (void)
 {
-	if (geo_address == NULL) {
-		return;
+	if (geo_address != NULL) {
+		g_signal_handlers_disconnect_by_func (geo_address, geo_address_cb, NULL);
+		g_clear_object (&geo_address);
 	}
-
-	g_signal_handlers_disconnect_by_func(G_OBJECT(geo_address), geo_address_cb, NULL);
-	g_object_unref(G_OBJECT(geo_address));
-
-	geo_address = NULL;
-
-	return;
 }
 
 /* Clean up and remove all signal handlers from the client as we
@@ -1234,16 +1232,10 @@
 static void
 geo_client_clean (void)
 {
-	if (geo_master == NULL) {
-		return;
+	if (geo_client != NULL) {
+		g_signal_handlers_disconnect_by_func (geo_client, geo_client_invalid, NULL);
+		g_clear_object (&geo_client);
 	}
-
-	g_signal_handlers_disconnect_by_func(G_OBJECT(geo_master), geo_client_invalid, NULL);
-	g_object_unref(G_OBJECT(geo_master));
-
-	geo_master = NULL;
-
-	return;
 }
 
 /* Callback from creating the address */
@@ -1263,14 +1255,11 @@
 	geo_address_clean();
 
 	g_debug("Created Geoclue Address");
-	geo_address = address;
-	g_object_ref(G_OBJECT(geo_address));
-
-	geoclue_address_get_address_async(geo_address, geo_address_cb, NULL);
-
-	g_signal_connect(G_OBJECT(address), "address-changed", G_CALLBACK(geo_address_cb), NULL);
-
-	return;
+	geo_address = g_object_ref (address);
+
+	geoclue_address_get_address_async (geo_address, geo_address_cb, NULL);
+
+	g_signal_connect (address, "address-changed", G_CALLBACK(geo_address_cb), NULL);
 }
 
 /* Callback from setting requirements */
@@ -1281,7 +1270,6 @@
 		g_warning("Unable to set Geoclue requirements: %s", error->message);
 		g_clear_error (&error);
 	}
-	return;
 }
 
 /* Client is killing itself rather oddly */
@@ -1289,25 +1277,28 @@
 geo_client_invalid (GeoclueMasterClient * client, gpointer user_data)
 {
 	g_warning("Master client invalid, rebuilding.");
-
-	/* Client changes we can assume the address is now invalid so we
-	   need to unreference the one we had. */
-	geo_address_clean();
-
-	/* And our master client is invalid */
-	geo_client_clean();
-
-	GeoclueMaster * master = geoclue_master_get_default();
-	geoclue_master_create_client_async(master, geo_create_client, NULL);
-
-	if (geo_timezone != NULL) {
-		g_free(geo_timezone);
-		geo_timezone = NULL;
-	}
-
-	update_location_menu_items();
-
-	return;
+	geo_stop ();
+	geo_start ();
+}
+
+static void
+geo_stop (void)
+{
+	geo_set_timezone (NULL);
+
+	geo_address_clean ();
+	geo_client_clean ();
+	g_clear_object (&geo_master);
+}
+
+static void
+geo_start (void)
+{
+	g_warn_if_fail (geo_master == NULL);
+
+	g_clear_object (&geo_master);
+	geo_master = geoclue_master_get_default();
+	geoclue_master_create_client_async (geo_master, geo_create_client, NULL);
 }
 
 /* Callback from creating the client */
@@ -1316,7 +1307,7 @@
 {
 	g_debug("Created Geoclue client at: %s", path);
 
-	geo_master = client;
+	geo_client = client;
 
 	if (error != NULL) {
 		g_warning("Unable to get a GeoClue client!  '%s'  Geolocation based timezone support will not be available.", error->message);
@@ -1324,17 +1315,17 @@
 		return;
 	}
 
-	if (geo_master == NULL) {
+	if (client == NULL) {
 		g_warning(_("Unable to get a GeoClue client!  Geolocation based timezone support will not be available."));
 		return;
 	}
 
-	g_object_ref(G_OBJECT(geo_master));
+	g_object_ref (geo_client);
 
 	/* New client, make sure we don't have an address hanging on */
 	geo_address_clean();
 
-	geoclue_master_client_set_requirements_async(geo_master,
+	geoclue_master_client_set_requirements_async(geo_client,
 	                                             GEOCLUE_ACCURACY_LEVEL_REGION,
 	                                             0,
 	                                             FALSE,
@@ -1342,12 +1333,23 @@
 	                                             geo_req_set,
 	                                             NULL);
 
-	geoclue_master_client_create_address_async(geo_master, geo_create_address, NULL);
-
-	g_signal_connect(G_OBJECT(client), "invalidated", G_CALLBACK(geo_client_invalid), NULL);
-
-	return;
-}
+	geoclue_master_client_create_address_async(geo_client, geo_create_address, NULL);
+
+	g_signal_connect(client, "invalidated", G_CALLBACK(geo_client_invalid), NULL);
+}
+
+static void
+on_use_geoclue_changed_cb (GSettings * settings, gchar * key, gpointer unused G_GNUC_UNUSED)
+{
+	geo_stop ();
+
+	if (g_settings_get_boolean (conf, SETTINGS_SHOW_DETECTED_S))
+		geo_start ();
+}
+
+/****
+*****
+****/
 
 static gboolean
 get_greeter_mode (void)
@@ -1413,7 +1415,9 @@
 
 	/* Set up GSettings */
 	conf = g_settings_new(SETTINGS_INTERFACE);
-	// TODO Add a signal handler to catch gsettings changes and respond to them
+	g_signal_connect (conf, "changed::show-auto-detected-location",
+                          G_CALLBACK(on_use_geoclue_changed_cb), NULL);
+	// TODO Add a signal handler to catch other gsettings changes and respond to them
 
 	/* Build our list of appointment calendar sources.
 	   When a source changes, update our menu items.
@@ -1439,8 +1443,8 @@
 	update_current_timezone();
 
 	/* Setup geoclue */
-	GeoclueMaster * master = geoclue_master_get_default();
-	geoclue_master_create_client_async(master, geo_create_client, NULL);
+	if (g_settings_get_boolean (conf, SETTINGS_SHOW_DETECTED_S))
+		geo_start ();
 
 	/* Setup dbus interface */
 	dbus = g_object_new(DATETIME_INTERFACE_TYPE, NULL);
@@ -1471,7 +1475,6 @@
 	free_appointment_sources();
 
 	g_object_unref(G_OBJECT(conf));
-	g_object_unref(G_OBJECT(master));
 	g_object_unref(G_OBJECT(dbus));
 	g_object_unref(G_OBJECT(service));
 	g_object_unref(G_OBJECT(server));
@@ -1480,8 +1483,7 @@
 
 	icaltimezone_free_builtin_timezones();
 
-	geo_address_clean();
-	geo_client_clean();
+	geo_stop ();
 
 	return 0;
 }