Merge lp:~charlesk/indicator-datetime/fix-833337 into lp:indicator-datetime/0.4

[Ted Gould]

This addresses Bug #833337 (Duplicate locations) and Bug #833325 (locations not in time order).

We have three sources that we draw location candidates from:
  - our geographic location, drawn from geoclue
  - our current location, drawn from /etc/timezone
  - the user-specified locations in g-c-c -> Time & Date -> Clock -> Time in other locations + Choose Locations

This patch builds a list from these three sources, and omits duplicates by testing for entries that have the same 'name' property *and* the same timezone offset from UTC. So if there were entries for Amsterdam NY and Amsterdam Netherlands, both would be used. But if there were two entries for Amsterdam NY, one would be omitted.

The list is then sorted in timezone order for Bug #833325, and DbusmenuMeuitems are created for each item in the list.

This code is branched from lp:~charlesk/indicator-datetime/fix-leaks, which does what it says on the tin.

[Ted Gould (ted)]

Resubmitted to make the fix-leaks branch a prereq branch

[Ted Gould (ted)]

Looks good.

review: Approve

=== modified file 'src/datetime-service.c'
--- src/datetime-service.c	2012-03-03 04:10:23 +0000
+++ src/datetime-service.c	2012-03-03 04:10:24 +0000
@@ -68,6 +68,8 @@
 static void geo_address_change (GeoclueMasterClient * client, gchar * a, gchar * b, gchar * c, gchar * d, gpointer user_data);
 static gboolean get_greeter_mode (void);
 
+static void quick_set_tz (DbusmenuMenuitem * menuitem, guint timestamp, gpointer user_data);
+
 static IndicatorService * service = NULL;
 static GMainLoop * mainloop = NULL;
 static DbusmenuServer * server = NULL;
@@ -80,12 +82,9 @@
 static DbusmenuMenuitem * settings = NULL;
 static DbusmenuMenuitem * events_separator = NULL;
 static DbusmenuMenuitem * locations_separator = NULL;
-static DbusmenuMenuitem * geo_location = NULL;
-static DbusmenuMenuitem * current_location = NULL;
-//static DbusmenuMenuitem * ecal_location = NULL;
 static DbusmenuMenuitem * add_appointment = NULL;
 static GList            * appointments = NULL;
-static GList            * dconflocations = NULL;
+static GList            * location_menu_items = NULL;
 static GList            * comp_instances = NULL;
 static gboolean           updating_appointments = FALSE;
 static time_t             start_time_appointments = (time_t) 0;
@@ -108,115 +107,140 @@
         ESource *source;
 };
 
-static void
-set_timezone_label (DbusmenuMenuitem * mi, const gchar * location)
-{
-	gchar * zone, * name;
-	split_settings_location (location, &zone, &name);
-
-	dbusmenu_menuitem_property_set (mi, TIMEZONE_MENUITEM_PROP_NAME, name);
-	dbusmenu_menuitem_property_set (mi, TIMEZONE_MENUITEM_PROP_ZONE, zone);
-
-	g_free (zone);
-	g_free (name);
-}
-
-static void
-set_current_timezone_label (DbusmenuMenuitem * mi, const gchar * location)
-{
-	gchar * name = get_current_zone_name (location);
-
-	dbusmenu_menuitem_property_set (mi, TIMEZONE_MENUITEM_PROP_NAME, name);
-	dbusmenu_menuitem_property_set (mi, TIMEZONE_MENUITEM_PROP_ZONE, location);
-
-	g_free (name);
-}
-
-/* Check to see if our timezones are the same */
-static void
-check_timezone_sync (void) {
-	gchar * label;
-	gboolean in_sync = FALSE;
-	
-	if (geo_timezone == NULL) {
-		in_sync = TRUE;
-	}
-
-	if (current_timezone == NULL) {
-		in_sync = TRUE;
-	}
-
-	if (!in_sync && g_strcmp0(geo_timezone, current_timezone) == 0) {
-		in_sync = TRUE;
-	}
-
-	if (in_sync) {
-		g_debug("Timezones in sync");
+/**
+ * A temp struct used by update_location_menu_items() for pruning duplicates.
+ */
+struct TimeLocation
+{
+	gint32 offset;
+	gchar * zone;
+	gchar * name;
+};
+static void
+time_location_free (struct TimeLocation * loc)
+{
+	g_free (loc->name);
+	g_free (loc->zone);
+	g_free (loc);
+}
+static struct TimeLocation*
+time_location_new (const char * zone, const char * name)
+{
+	struct TimeLocation * loc = g_new (struct TimeLocation, 1);
+	GTimeZone * tz = g_time_zone_new (zone);
+	loc->offset = g_time_zone_get_offset (tz, 0);
+	loc->zone = g_strdup (zone);
+	loc->name = g_strdup (name);
+	g_time_zone_unref (tz);
+	return loc;
+}
+static int
+time_location_compare (const struct TimeLocation * a, const struct TimeLocation * b)
+{
+	int ret;
+	if (a->offset != b->offset)
+		ret = a->offset - b->offset;
+	else
+		ret = g_strcmp0 (a->name, b->name);
+	g_debug ("%s comparing '%s' (%d) to '%s' (%d), returning %d", G_STRLOC, a->name, (int)a->offset, b->name, (int)b->offset, ret);
+	return ret;
+}
+static GSList*
+locations_add (GSList * locations, const char * zone, const char * name)
+{
+	struct TimeLocation * loc = time_location_new (zone, name);
+
+	if (g_slist_find_custom (locations, loc, (GCompareFunc)time_location_compare) == NULL) {
+		g_debug ("%s Adding zone '%s', name '%s'", G_STRLOC, zone, name);
+		locations = g_slist_prepend (locations, loc);
 	} else {
-		g_debug("Timezones are different");
-	}
-
-	gboolean show = g_settings_get_boolean (conf, SETTINGS_SHOW_LOCATIONS_S);
-
-	if (geo_location != NULL && current_location != NULL) {
-		g_debug("Got timezone %s", current_timezone);
-		g_debug("Got timezone %s", geo_timezone);
-		// Show neither current location nor geo location if both are the same
-		// however, we want to set their time and label accordingly
-		if (in_sync) {
-			if (current_timezone == NULL && geo_timezone == NULL) {
-				dbusmenu_menuitem_property_set_bool(locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-				dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-				dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-				update_location_menu_items();
-				return;
-			}
-			
-			dbusmenu_menuitem_property_set_bool (locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-			dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-			dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-			dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-			dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-			
-			if (current_timezone != NULL) {
-				label = current_timezone;
-			} else {
-				label = geo_timezone;
-			}
-			
-			if (label != NULL) {
-				// TODO work out the current location name in a nice way
-				set_current_timezone_label (current_location, label);
-				// TODO work out the current time at that location 
-				dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-				dbusmenu_menuitem_property_set_bool(current_location, TIMEZONE_MENUITEM_PROP_RADIO, TRUE);
-			} else {
-				g_debug("Label for current location is null, this shouldn't happen");
-			}
-			if (geo_timezone != NULL) {	
-				// TODO work out the geo location name in a nice way
-				set_current_timezone_label (geo_location, geo_timezone);
-				// TODO work out the current time at that location 
-				dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-			}
-		} else {
-			// TODO work out the geo location name in a nice way
-			set_current_timezone_label (geo_location, geo_timezone);
-			// TODO work out the current time at that location 
-			dbusmenu_menuitem_property_set_bool(geo_location, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-			
-			// TODO work out the current location name in a nice way
-			set_current_timezone_label (current_location, current_timezone);
-			// TODO work out the current time at that location 
-			dbusmenu_menuitem_property_set_bool(current_location, TIMEZONE_MENUITEM_PROP_RADIO, TRUE);
-			dbusmenu_menuitem_property_set_bool(current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-			dbusmenu_menuitem_property_set_bool(locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
+		g_debug("%s Skipping duplicate zone '%s' name '%s'", G_STRLOC, zone, name);
+		time_location_free (loc);
+	}
+	return locations;
+}
+
+/* Update the timezone entries */
+static void
+update_location_menu_items (void)
+{
+	if (locations_separator == NULL)
+		return;
+
+	GSList * locations = NULL;
+
+	/* remove the previous locations */
+	while (location_menu_items != NULL) {
+		DbusmenuMenuitem * item = DBUSMENU_MENUITEM(location_menu_items->data);
+		location_menu_items = g_list_remove(location_menu_items, item);
+		dbusmenu_menuitem_child_delete(root, DBUSMENU_MENUITEM(item));
+		g_object_unref(G_OBJECT(item));
+	}
+
+	/***
+	****  Build a list of locations to add: use geo_timezone,
+	****  current_timezone, and SETTINGS_LOCATIONS_S, but omit duplicates.
+	***/
+
+	/* maybe add geo_timezone */
+	if (geo_timezone != NULL) {
+		gchar * name = get_current_zone_name (geo_timezone);
+		locations = locations_add (locations, geo_timezone, name);
+		g_free (name);
+	}
+
+	/* maybe add current_timezone */
+	if (current_timezone != NULL) {
+		gchar * name = get_current_zone_name (current_timezone);
+		locations = locations_add (locations, current_timezone, name);
+		g_free (name);
+	}
+
+	/* maybe add the user-specified custom locations */
+	gchar ** user_locations = g_settings_get_strv(conf, SETTINGS_LOCATIONS_S);
+	if (user_locations != NULL) { 
+		gint i;
+		const guint location_count = g_strv_length(user_locations);
+		g_debug ("%s Found %u user-specified locations", G_STRLOC, location_count);
+		for (i=0; i<location_count; i++) {
+			gchar * zone;
+			gchar * name;
+			split_settings_location (user_locations[i], &zone, &name);
+			locations = locations_add (locations, zone, name);
+			g_free (name);
+			g_free (zone);
 		}
-	}
-	g_debug("Finished checking timezone sync");
-	update_location_menu_items();
-
-	return;
+		g_strfreev (user_locations);
+		user_locations = NULL;
+	}
+
+	/* sort the list by timezone offset */
+	locations = g_slist_sort (locations, (GCompareFunc)time_location_compare);
+
+	/* finally create menuitems for each location */
+	gint offset = dbusmenu_menuitem_get_position (locations_separator, root)+1;
+	const gboolean show_locations = g_settings_get_boolean (conf, SETTINGS_SHOW_LOCATIONS_S);
+	GSList * l;
+	for (l=locations; l!=NULL; l=l->next) {
+		struct TimeLocation * loc = l->data;
+		g_debug("%s Adding location: zone '%s', name '%s'", G_STRLOC, loc->zone, loc->name);
+		DbusmenuMenuitem * item = dbusmenu_menuitem_new();
+		dbusmenu_menuitem_property_set      (item, DBUSMENU_MENUITEM_PROP_TYPE, TIMEZONE_MENUITEM_TYPE);
+		dbusmenu_menuitem_property_set      (item, TIMEZONE_MENUITEM_PROP_NAME, loc->name);
+		dbusmenu_menuitem_property_set      (item, TIMEZONE_MENUITEM_PROP_ZONE, loc->zone);
+		dbusmenu_menuitem_property_set_bool (item, TIMEZONE_MENUITEM_PROP_RADIO, FALSE);
+		dbusmenu_menuitem_property_set_bool (item, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
+		dbusmenu_menuitem_property_set_bool (item, DBUSMENU_MENUITEM_PROP_VISIBLE, show_locations);
+		dbusmenu_menuitem_child_add_position (root, item, offset++);
+		g_signal_connect(G_OBJECT(item), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK(quick_set_tz), NULL);
+		location_menu_items = g_list_append (location_menu_items, item);
+		time_location_free (loc);
+	}
+	g_slist_free (locations);
+	locations = NULL;
+
+	/* if there's at least one item being shown, show the separator too */
+	dbusmenu_menuitem_property_set_bool (locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, show_locations && (location_menu_items!=NULL));
 }
 
 /* Update the current timezone */
@@ -235,7 +259,7 @@
 
 	g_debug("System timezone is: %s", current_timezone);
 
-	check_timezone_sync();
+	update_location_menu_items();
 
 	return;
 }
@@ -566,62 +590,6 @@
 	return FALSE;
 }
 
-
-static void
-update_location_menu_items(void) {
-	g_debug("Updating timezone menu items");
-
-	if (locations_separator == NULL || current_location == NULL) {
-		return;
-	}
-
-	gchar ** locations = g_settings_get_strv(conf, SETTINGS_LOCATIONS_S);
-
-	if (locations == NULL) { 
-		g_debug("No locations configured (NULL)");
-		return;
-	} 
-	const guint len = g_strv_length(locations);
-	g_debug("Found %u locations from %s", len, SETTINGS_LOCATIONS_S);
-	
-	/* Remove all of the previous locations */
-	if (dconflocations != NULL) {
-		while (dconflocations != NULL) {
-			DbusmenuMenuitem * item =  DBUSMENU_MENUITEM(dconflocations->data);
-			// Remove all the existing menu items which are in dconflocations.
-			dconflocations = g_list_remove(dconflocations, item);
-			dbusmenu_menuitem_child_delete(root, DBUSMENU_MENUITEM(item));
-			g_object_unref(G_OBJECT(item));
-		}
-	}
-	
-	const gboolean show = g_settings_get_boolean (conf, SETTINGS_SHOW_LOCATIONS_S);
-	dbusmenu_menuitem_property_set_bool (locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-	dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-	dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-
-	gint i;
-	gint offset = dbusmenu_menuitem_get_position (current_location, root)+1;
-	for (i = 0; i < len; i++) {
-		// Iterate over configured places and add any which aren't already listed
-		if ((current_timezone == NULL || !g_str_has_prefix(locations[i], current_timezone)) &&
-		    (geo_timezone == NULL || !g_str_has_prefix(locations[i], geo_timezone))) {
-			DbusmenuMenuitem *item;
-			g_debug("Adding timezone in update_timezones %s", locations[i]);
-			item = dbusmenu_menuitem_new();
-			dbusmenu_menuitem_property_set      (item, DBUSMENU_MENUITEM_PROP_TYPE, TIMEZONE_MENUITEM_TYPE);
-			set_timezone_label (item, locations[i]);
-			dbusmenu_menuitem_property_set_bool (item, TIMEZONE_MENUITEM_PROP_RADIO, FALSE);
-			dbusmenu_menuitem_property_set_bool (item, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-			dbusmenu_menuitem_property_set_bool (item, DBUSMENU_MENUITEM_PROP_VISIBLE, show);
-			dbusmenu_menuitem_child_add_position (root, item, offset++);
-			g_signal_connect(G_OBJECT(item), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK(quick_set_tz), NULL);
-			dconflocations = g_list_append(dconflocations, item);
-		}
-	}
-	g_strfreev (locations);
-}
-
 // Authentication function
 static gchar *
 auth_func (ECal *ecal, 
@@ -1098,7 +1066,7 @@
 show_locations_changed (void)
 {
 	/* Re-calculate */
-	check_timezone_sync();
+	update_location_menu_items();
 }
 
 static void
@@ -1141,22 +1109,7 @@
 		dbusmenu_menuitem_property_set_bool (locations_separator, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
 		dbusmenu_menuitem_child_append(root, locations_separator);
 
-		geo_location = dbusmenu_menuitem_new();
-		dbusmenu_menuitem_property_set      (geo_location, DBUSMENU_MENUITEM_PROP_TYPE, TIMEZONE_MENUITEM_TYPE);
-		set_current_timezone_label (geo_location, "");
-		dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_ENABLED, FALSE);
-		dbusmenu_menuitem_property_set_bool (geo_location, DBUSMENU_MENUITEM_PROP_VISIBLE, TRUE);
-		g_signal_connect(G_OBJECT(geo_location), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK(quick_set_tz), NULL);
-		dbusmenu_menuitem_child_append(root, geo_location);
-
-		current_location = dbusmenu_menuitem_new();
-		dbusmenu_menuitem_property_set      (current_location, DBUSMENU_MENUITEM_PROP_TYPE, TIMEZONE_MENUITEM_TYPE);
-		set_current_timezone_label (current_location, "");
-		dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_ENABLED, FALSE);
-		dbusmenu_menuitem_property_set_bool (current_location, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
-		dbusmenu_menuitem_child_append(root, current_location);
-	
-		check_timezone_sync();
+		update_location_menu_items();
 	
 		g_signal_connect (conf, "changed::" SETTINGS_SHOW_LOCATIONS_S, G_CALLBACK (show_locations_changed), NULL);
 		g_signal_connect (conf, "changed::" SETTINGS_LOCATIONS_S, G_CALLBACK (show_locations_changed), NULL);
@@ -1297,7 +1250,7 @@
 		geo_timezone = g_strdup((gchar *)tz_hash);
 	}
 
-	check_timezone_sync();
+	update_location_menu_items();
 
 	return;
 }
@@ -1394,7 +1347,7 @@
 		geo_timezone = NULL;
 	}
 
-	check_timezone_sync();
+	update_location_menu_items();
 
 	return;
 }
@@ -1416,7 +1369,7 @@
 		geo_timezone = NULL;
 	}
 
-	check_timezone_sync();
+	update_location_menu_items();
 
 	return;
 }