Merge lp:~charlesk/indicator-datetime/lp-772340 into lp:indicator-datetime/0.4

[Charles Kerr]

[Lars Karlitski (larsu)]

Great work, it's much cleaner now!

Two small comments:

* comp_instance_new:
  - instead of putting it on two line, it's customary to do
      ci->source = g_object_ref (comp);

* comp_instance_free:
  - NULL check of 'ci' is missing

review: Needs Fixing

[Charles Kerr (charlesk)]

lars, thanks for those suggestions, I used them both in the merge.

I hadn't realized "foo = g_object_ref (bar)" was an idiom but it makes good sense; I'll start doing that from here on out.

[Lars Karlitski (larsu)]

review: Approve

=== modified file 'src/datetime-service.c'
--- src/datetime-service.c	2012-03-16 18:53:17 +0000
+++ src/datetime-service.c	2012-03-20 18:03:34 +0000
@@ -273,7 +273,7 @@
 
   if (error != NULL) {
     g_warning("Could not set timezone for SettingsDaemon: %s", error->message);
-    g_error_free(error);
+    g_clear_error (&error);
     return;
   }
 
@@ -289,7 +289,7 @@
 
 	if (error != NULL) {
 		g_warning("Could not grab DBus proxy for SettingsDaemon: %s", error->message);
-		g_error_free(error);
+		g_clear_error (&error);
 		g_free (zone);
 		return;
 	}
@@ -363,7 +363,7 @@
 	g_debug("Issuing command '%s'", command);
 	if (!g_spawn_command_line_async(command, &error)) {
 		g_warning("Unable to start %s: %s", (char *)command, error->message);
-		g_error_free(error);
+		g_clear_error (&error);
 	}
 }
 
@@ -532,7 +532,7 @@
 	GSList *accounts_list = gconf_client_get_list (gconf, "/apps/evolution/mail/accounts", GCONF_VALUE_STRING, NULL);
 	const guint n = g_slist_length (accounts_list);
 	g_debug ("found %u evolution accounts", n);
-	g_slist_free (accounts_list);
+	g_slist_free_full (accounts_list, g_free);
 	return n > 0;
 }
 
@@ -613,22 +613,42 @@
 }
 
 static gint
-compare_comp_instances (gconstpointer a, 
-                        gconstpointer b)
-{
-        const struct comp_instance *ci_a = a;
-        const struct comp_instance *ci_b = b;
-        time_t d = ci_a->start - ci_b->start;
-		if (d < 0) return -1;
-		else if (d > 0) return 1; 
-		return 0;
+compare_comp_instances (gconstpointer ga, gconstpointer gb)
+{
+        const struct comp_instance * a = ga;
+        const struct comp_instance * b = gb;
+
+	/* sort by start time */
+	if (a->start < b->start) return -1;
+	if (a->start > b->start) return  1;
+	return 0;
+}
+
+static struct comp_instance*
+comp_instance_new (ECalComponent * comp, time_t start, time_t end, ESource * source)
+{
+	g_object_ref(comp);
+	g_debug("Using times start %s, end %s", ctime(&start), ctime(&end));
+
+	struct comp_instance *ci = g_new (struct comp_instance, 1);
+	ci->comp = comp;
+	ci->source = source;
+	ci->start = start;
+	ci->end = end;
+	return ci;
+}
+static void
+comp_instance_free (struct comp_instance* ci)
+{
+	g_object_unref (ci->comp);
+	g_free (ci);
 }
 
 static gboolean
-populate_appointment_instances (ECalComponent *comp,
-                                time_t instance_start,
-                                time_t instance_end,
-                                gpointer data)
+populate_appointment_instances (ECalComponent * comp,
+                                time_t          start,
+                                time_t          end,
+                                gpointer        data)
 {
 	g_debug("Appending item %p", comp);
 	
@@ -638,20 +658,9 @@
 	icalproperty_status status;
 	e_cal_component_get_status (comp, &status);
 	if (status == ICAL_STATUS_COMPLETED || status == ICAL_STATUS_CANCELLED) return FALSE;
-	
-	g_object_ref(comp);
 
-	struct comp_instance *ci;
-	ci = g_new (struct comp_instance, 1);
-	
-	g_debug("Using times start %s, end %s", ctime(&instance_start), ctime(&instance_end));
-	
-	ci->comp = comp;
-	ci->source = E_SOURCE(data);
-	ci->start = instance_start;
-	ci->end = instance_end;
-	
-	comp_instances = g_list_append(comp_instances, ci);
+	struct comp_instance *ci = comp_instance_new (comp, start, end, E_SOURCE(data));
+	comp_instances = g_list_append (comp_instances, ci);
 	return TRUE;
 }
 
@@ -719,25 +728,18 @@
 	
 	if (!e_cal_get_sources(&sources, E_CAL_SOURCE_TYPE_EVENT, &gerror)) {
 		g_debug("Failed to get ecal sources\n");
+		g_clear_error (&gerror);
 		return FALSE;
 	}
 	
-	// Free comp_instances if not NULL
-	if (comp_instances != NULL) {
-		g_debug("Freeing comp_instances: may be an overlap\n");
-		for (l = comp_instances; l; l = l->next) {
-			const struct comp_instance *ci = l->data;
-			g_object_unref(ci->comp);
-		}
-		g_list_free(comp_instances);
-		comp_instances = NULL;
+	// clear any previous comp_instances
+	g_list_free_full (comp_instances, (GDestroyNotify)comp_instance_free);
+	comp_instances = NULL;
 
-	}
 	GSList *cal_list = gconf_client_get_list(gconf, "/apps/evolution/calendar/display/selected_calendars", GCONF_VALUE_STRING, &gerror);
 	if (gerror) {
 	  g_debug("Failed to get evolution preference for enabled calendars");
-	  g_error_free(gerror);
-	  gerror = NULL;
+	  g_clear_error (&gerror);
 	  cal_list = NULL;
 	}
 	
@@ -759,39 +761,33 @@
 			}
 			if (current_zone && !e_cal_set_default_timezone(ecal, current_zone, &gerror)) {
 				g_debug("Failed to set ecal default timezone %s", gerror->message);
-				g_error_free(gerror);
-				gerror = NULL;
+				g_clear_error (&gerror);
 				g_object_unref(ecal);
 				continue;
 			}
 			
 			if (!e_cal_open(ecal, FALSE, &gerror)) {
 				g_debug("Failed to get ecal sources %s", gerror->message);
-				g_error_free(gerror);
-				gerror = NULL;
+				g_clear_error (&gerror);
 				g_object_unref(ecal);
 				continue;
 			}
+
 			const gchar *ecal_uid = e_source_peek_uid(source);
-			gboolean match = FALSE;
 			g_debug("Checking ecal_uid is enabled: %s", ecal_uid);
-			for (i = 0; i<g_slist_length(cal_list);i++) {
-				char *cuid = (char *)g_slist_nth_data(cal_list, i);
-				if (g_strcmp0(cuid, ecal_uid) == 0) {
-					match = TRUE;
-					break;
-				}
-			}
-			if (!match) {
+			const gboolean in_list = g_slist_find_custom (cal_list, ecal_uid, (GCompareFunc)g_strcmp0) != NULL;
+			if (!in_list) {
 				g_object_unref(ecal);
 				continue;
 			}
+
 			g_debug("ecal_uid is enabled, generating instances");
-			
-			e_cal_generate_instances (ecal, t1, t2, (ECalRecurInstanceFn) populate_appointment_instances, (gpointer) source);
+			e_cal_generate_instances (ecal, t1, t2, (ECalRecurInstanceFn) populate_appointment_instances, source);
 			g_object_unref(ecal);
 		}
 	}
+	g_slist_free_full (cal_list, g_free);
+
 	g_debug("Number of ECalComponents returned: %d", g_list_length(comp_instances));
 	GList *sorted_comp_instances = g_list_sort(comp_instances, compare_comp_instances);
 	comp_instances = NULL;
@@ -1000,6 +996,7 @@
 	  			}
 	  			
 				dbusmenu_menuitem_property_set_image (item, APPOINTMENT_MENUITEM_PROP_ICON, pixbuf);
+				g_clear_object (&pixbuf);
 			} else {
 				g_debug("Creating pixbuf from surface failed");
 			}
@@ -1009,15 +1006,15 @@
 		g_debug("Adding appointment: %p", item);
 	}
 	
-    if (gerror != NULL) g_error_free(gerror);
-	for (l = sorted_comp_instances; l; l = l->next) { 
-		const struct comp_instance *ci = l->data;
-		g_object_unref(ci->comp);
-	}
-	g_list_free(sorted_comp_instances);
+	g_clear_error (&gerror);
+
+	g_list_free_full (sorted_comp_instances, (GDestroyNotify)comp_instance_free);
+	sorted_comp_instances = NULL;
 	
 	GVariant * marks = g_variant_builder_end (&markeddays);
 	dbusmenu_menuitem_property_set_variant (calendar, CALENDAR_MENUITEM_PROP_MARKS, marks);
+
+	g_clear_object (&sources);
 	
 	updating_appointments = FALSE;
 	g_debug("End of objects");
@@ -1204,7 +1201,7 @@
 
 	if (error != NULL) {
 		g_warning("Could not grab DBus proxy for ConsoleKit: %s", error->message);
-		g_error_free(error);
+		g_clear_error (&error);
 		return;
 	}
 
@@ -1217,6 +1214,7 @@
 {
 	if (error != NULL) {
 		g_warning("Unable to get Geoclue address: %s", error->message);
+		g_clear_error (&error);
 		return;
 	}
 
@@ -1278,6 +1276,7 @@
 {
 	if (error != NULL) {
 		g_warning("Unable to create GeoClue address: %s", error->message);
+		g_clear_error (&error);
 		return;
 	}
 
@@ -1304,6 +1303,7 @@
 {
 	if (error != NULL) {
 		g_warning("Unable to set Geoclue requirements: %s", error->message);
+		g_clear_error (&error);
 	}
 	return;
 }
@@ -1366,6 +1366,7 @@
 
 	if (error != NULL) {
 		g_warning("Unable to get a GeoClue client!  '%s'  Geolocation based timezone support will not be available.", error->message);
+		g_clear_error (&error);
 		return;
 	}