Merge lp:~jjardon/indicator-datetime/fix-836017 into lp:indicator-datetime/0.3

[Javier Jardón]

[Andrea Cimitan (cimi)]

Hi Javier! You need to free the GSlist *accounts_list with:

if (accounts_list)
  g_slist_free (accounts_list);

before setting it to NULL.

review: Needs Fixing

[lp:~jjardon/indicator-datetime/fix-836017]

141. By Javier Jardón on 2011-10-03

datetime-service: Free accounts list in the correct place

[Javier Jardón (jjardon)]

Thanks for the review Andrea, Could you take a look to this new patch?

review: Needs Resubmitting

[Andrea Cimitan (cimi)]

It depends... if error is != NULL it enters the first if but not the second, so accounts_list is set to NULL and not free'd. Now, it could be that if error is != NULL then accounts_list is NULL and it does not need to be free'd, so your code is not leaking.
However, I'm not sure of that (that accounts_list is NULL and thus does not need to be free'd if error is != NULL), so in my ignorance I'd put the code as I wrote before, adding

if (accounts_list)
  g_slist_free (accounts_list);

before setting it to NULL in the first if.

If you're sure of my concern, go ahead, otherwise we might need to look at gconf code and see which GSList we can receive from gconf_client_get_list when error is not NULL.

review: Needs Information

[lp:~jjardon/indicator-datetime/fix-836017]

142. By Javier Jardón on 2011-10-04

datetime-service: Be sure to free the returned list

[Javier Jardón (jjardon)]

Ok, lets be super-sure to free the possible returned list

review: Needs Resubmitting

[Charles Kerr (charlesk)]

1. we don't appear to use "error" except for testing for failure, and on failure list will be NULL anyway so that test is redundant. Let's remove the error variable altogether.

2. g_slist_free() is NULL-safe, so there "if (list != NULL)" test before g_slist_free(list) is unnecessary.

3. I think we're also leaking the "evo" variable in greeter mode.

What would you think about moving it to a standalone function like this:

static gboolean
calendar_app_is_usable (void)
{
        /* confirm that it's installed... */
        gchar *evo = g_find_program_in_path("evolution");
        if (evo == NULL)
                return FALSE;
        g_debug ("found evolution at '%s'", evo);
        g_free (evo);

        /* confirm that there's an account set up */
        GSList *accounts_list = gconf_client_get_list (gconf, "/apps/evolution/mail/accounts", GCONF_VALUE_STRING, NULL);
        g_debug ("found %u evolution accounts", g_slist_length(accounts_list));
        const gboolean is_configured = accounts_list != NULL;
        g_slist_free (accounts_list);
        return is_configured;
}

review: Needs Fixing

[Charles Kerr (charlesk)]

Javier, I'm going to move your code into the standalone function to fix the 'evo' memory leak and merge. I hope you don't mind this -- I want to make sure your fix gets in before the deadline.

review: Approve

[Javier Jardón (jjardon)]

Yeah, sure. I think is a good idea

=== modified file 'src/datetime-service.c'
--- src/datetime-service.c	2011-09-29 19:57:50 +0000
+++ src/datetime-service.c	2011-10-04 19:24:25 +0000
@@ -484,6 +484,8 @@
 static gboolean
 check_for_calendar (gpointer user_data)
 {
+	GError *error = NULL;
+
 	g_return_val_if_fail (calendar != NULL, FALSE);
 	
 	dbusmenu_menuitem_property_set_bool(date, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
@@ -494,16 +496,24 @@
 		
 		g_signal_connect (G_OBJECT(date), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
 		                  G_CALLBACK (activate_cb), "evolution -c calendar");
-		
-		events_separator = dbusmenu_menuitem_new();
-		dbusmenu_menuitem_property_set(events_separator, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
-		dbusmenu_menuitem_child_add_position(root, events_separator, 2);
-		add_appointment = dbusmenu_menuitem_new();
-		dbusmenu_menuitem_property_set (add_appointment, DBUSMENU_MENUITEM_PROP_LABEL, _("Add Event…"));
-		dbusmenu_menuitem_property_set_bool(add_appointment, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-		g_signal_connect(G_OBJECT(add_appointment), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK(activate_cb), "evolution -c calendar");
-		dbusmenu_menuitem_child_add_position (root, add_appointment, 3);
 
+		GSList *accounts_list = gconf_client_get_list (gconf, "/apps/evolution/mail/accounts", GCONF_VALUE_STRING, &error);
+		if (error != NULL) {
+			g_debug("%s: Failed to get evolution mail accounts", G_STRFUNC);
+			g_clear_error (&error);
+			if (accounts_list)
+				g_slist_free (accounts_list);
+		} else if (accounts_list != NULL) {
+			g_slist_free (accounts_list);
+			events_separator = dbusmenu_menuitem_new();
+			dbusmenu_menuitem_property_set(events_separator, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
+			dbusmenu_menuitem_child_add_position(root, events_separator, 2);
+			add_appointment = dbusmenu_menuitem_new();
+			dbusmenu_menuitem_property_set (add_appointment, DBUSMENU_MENUITEM_PROP_LABEL, _("Add Event…"));
+			dbusmenu_menuitem_property_set_bool(add_appointment, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
+			g_signal_connect(G_OBJECT(add_appointment), DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, G_CALLBACK(activate_cb), "evolution -c calendar");
+			dbusmenu_menuitem_child_add_position (root, add_appointment, 3);
+		}
 
 		if (g_settings_get_boolean(conf, SETTINGS_SHOW_EVENTS_S)) {
 			dbusmenu_menuitem_property_set_bool(add_appointment, DBUSMENU_MENUITEM_PROP_VISIBLE, TRUE);