Merge lp:~charlesk/indicator-appmenu/fix-953479 into lp:indicator-appmenu/0.4

[Charles Kerr]

The issue is that we pass the indefinite type ARRAY into g_variant_builder_init() but it's possible that the array will be empty. According to the glib documentation, it's an error to call g_variant_builder_end() "if the builder was created with an indefinite array or maybe type and no children have been added."

h/t desrt

[Allison Karlitskaya (desrt)]

Ya. This looks good. Certainly fixes the bug.

Look just below, though. This same function is leaking 'appkeys'.

If you have some free time, that whole function could be rewritten to look something like so:

GVariantBuilder array;
GHashTable iter;

g_hash_table_iter_init (&iter, iapp->apps);
g_variant_builder_init (&array, G_VARIANT_TYPE ("a(uso)");
while (g_hash_table_iter_next (&iter, &key, &value))
  {
    ...
    g_variant_builder_add (&array, "(uso)", get_xid(), get_address(), get_path());
  }
return g_variant_new ("(a(uso))", &array);

review: Approve

[lp:~charlesk/indicator-appmenu/fix-953479]

181. By Charles Kerr on 2012-03-16

better use of GVariant suggested by desrt

[Charles Kerr (charlesk)]

Ryan, I made most of the modifications you suggested, but am assuming that your last line wasn't literal -- you wouldn't pass a g_variant_builder_init()'ed GVariantBuilder into g_variant_new()'s vararg list, nor return the result immediately?

[Allison Karlitskaya (desrt)]

It's literal. Check the docs:

http://developer.gnome.org/glib/stable/gvariant-format-strings.html#gvariant-format-strings-arrays

review: Needs Fixing

[lp:~charlesk/indicator-appmenu/fix-953479]

182. By Charles Kerr on 2012-03-16

further GVariant refinements, h/t desrt again

[Allison Karlitskaya (desrt)]

Final version looks good.

review: Approve

[Lars Karlitski (larsu)]

Nothing to add. Great work!

review: Approve

=== modified file 'src/indicator-appmenu.c'
--- src/indicator-appmenu.c	2012-03-06 19:34:45 +0000
+++ src/indicator-appmenu.c	2012-03-16 15:51:24 +0000
@@ -1259,25 +1259,23 @@
 		return NULL;
 	}
 
-	GVariantBuilder array;
-	g_variant_builder_init (&array, G_VARIANT_TYPE_ARRAY);
-	GList * appkeys = NULL;
-	for (appkeys = g_hash_table_get_keys(iapp->apps); appkeys != NULL; appkeys = g_list_next(appkeys)) {
-		gpointer hash_val = g_hash_table_lookup(iapp->apps, appkeys->data);
-
-		if (hash_val == NULL) { continue; }
-
-		GVariantBuilder tuple;
-		g_variant_builder_init(&tuple, G_VARIANT_TYPE_TUPLE);
-		g_variant_builder_add_value(&tuple, g_variant_new_uint32(window_menus_get_xid(WINDOW_MENUS(hash_val))));
-		g_variant_builder_add_value(&tuple, g_variant_new_string(window_menus_get_address(WINDOW_MENUS(hash_val))));
-		g_variant_builder_add_value(&tuple, g_variant_new_object_path(window_menus_get_path(WINDOW_MENUS(hash_val))));
-
-		g_variant_builder_add_value(&array, g_variant_builder_end(&tuple));
+	GVariantBuilder builder;
+	GHashTableIter hash_iter;
+	gpointer value;
+
+	g_variant_builder_init (&builder, G_VARIANT_TYPE("a(uso)"));
+	g_hash_table_iter_init (&hash_iter, iapp->apps);
+	while (g_hash_table_iter_next (&hash_iter, NULL, &value)) {
+		if (value != NULL) {
+			WindowMenus * wm = WINDOW_MENUS(value);
+			g_variant_builder_add (&builder, "(uso)",
+			                       window_menus_get_xid(wm),
+			                       window_menus_get_address(wm),
+			                       window_menus_get_path(wm));
+		}
 	}
 
-	GVariant * varray = g_variant_builder_end(&array);
-	return g_variant_new_tuple(&varray, 1);
+	return g_variant_new ("(a(uso))", &builder);
 }
 
 /* A method has been called from our dbus inteface.  Figure out what it