Merge lp:~charlesk/libindicator/lp-1045372 into lp:libindicator/12.10

[Charles Kerr]

In libindicator, remove the cloak/decloak code in IndicatorObject to address Bug #1045372.

In indicator-loader, support hiding & re-showing IndicatorObjectEntries by caching their menuitems and using gtk_widget_hide / gtk_widget_show.

In tests/test-loader and tests/dummy-indicator-visible.c, support hiding & re-showing IndicatorObjectEntries by caching their parent widgetry instead of using gtk_widget_destroy().

The cloak/decloak code existed to ensure that the IndicatorObjectEntry's widgets were not destroyed by client code on the "entry-removed" signal. Instead, let's fix the client code to understand that IOEs' visibility can be toggled and re-toggled.

See also: https://code.launchpad.net/~charlesk/indicator-applet/lp-1045372

[jenkins (martin-mrazik+qa)]

FAILED: Continuous integration, rev:472
http://jenkins.qa.ubuntu.com/job/libindicator-ci/4/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-ci/./label=quantal/4/console

review: Needs Fixing (continuous-integration)

[lp:~charlesk/libindicator/lp-1045372]

473. By Charles Kerr on 2012-09-12

In tests/test-loader and tests/dummy-indicator-visible.c, support hiding & re-showing IndicatorObjectEntries by caching their parent widgetry instead of using gtk_widget_destroy()

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:473
http://jenkins.qa.ubuntu.com/job/libindicator-ci/5/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-ci/./label=quantal/5/console

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

Looks good, that cloaking thing was indeed a bit weird. Thanks!

review: Approve

=== modified file 'libindicator/indicator-object.c'
--- libindicator/indicator-object.c	2012-02-29 20:58:03 +0000
+++ libindicator/indicator-object.c	2012-09-12 04:29:16 +0000
@@ -113,9 +113,7 @@
 /* entries' visibility */
 static GList * get_entries_default               (IndicatorObject*);
 static GList * get_all_entries                   (IndicatorObject*);
-static void entry_being_removed_default          (IndicatorObject*, IndicatorObjectEntry*);
 static void indicator_object_entry_being_removed (IndicatorObject*, IndicatorObjectEntry*);
-static void entry_was_added_default              (IndicatorObject*, IndicatorObjectEntry*);
 static void indicator_object_entry_was_added     (IndicatorObject*, IndicatorObjectEntry*);
 static IndicatorObjectEntryPrivate * entry_get_private (IndicatorObject*, IndicatorObjectEntry*);
 
@@ -141,8 +139,8 @@
 	klass->get_accessible_desc = NULL;
 	klass->get_entries = get_entries_default;
 	klass->get_location = NULL;
-	klass->entry_being_removed = entry_being_removed_default;
-	klass->entry_was_added = entry_was_added_default;
+	klass->entry_being_removed = NULL;
+	klass->entry_was_added = NULL;
 
 	klass->entry_activate = NULL;
 	klass->entry_activate_window = NULL;
@@ -937,61 +935,3 @@
         }
 }
 
-/***
-****
-***/
-
-/* Cloaked entries are ones which are hidden but may be re-added later.
-   They are reffed + unparented so that they'll survive even if the
-   rest of the widgetry is destroyed */
-#define CLOAKED_KEY "entry-is-cloaked"
-
-static void
-decloak_widget (gpointer w)
-{
-	if (w != NULL) {
-		GObject * o = G_OBJECT(w);
-		if (g_object_steal_data (o, CLOAKED_KEY) != NULL) {
-			g_object_unref (o);
-		}
-	}
-}
-
-static void
-entry_was_added_default (IndicatorObject * io, IndicatorObjectEntry * entry)
-{
-	decloak_widget (entry->image);
-	decloak_widget (entry->label);
-	decloak_widget (entry->menu);
-}
-
-static void
-cloak_widget (gpointer w)
-{
-	if (w != NULL) {
-		GtkWidget * parent;
-
-		/* tag this object as cloaked */
-		GObject * o = G_OBJECT(w);
-		g_object_ref (o);
-		g_object_set_data (o, CLOAKED_KEY, GINT_TO_POINTER(1));
-
-		/* remove it from its surrounding widgetry */
-		if(GTK_IS_MENU(w)) {
-			if (gtk_menu_get_attach_widget (GTK_MENU(w)) != NULL) {
-				gtk_menu_detach (GTK_MENU(w));
-			}
-		}
-		else if((parent = gtk_widget_get_parent(w))) {
-			gtk_container_remove(GTK_CONTAINER(parent), w);
-		}
-	}
-}
-
-static void
-entry_being_removed_default (IndicatorObject * io, IndicatorObjectEntry * entry)
-{
-	cloak_widget (entry->image);
-	cloak_widget (entry->label);
-	cloak_widget (entry->menu);
-}

=== modified file 'tests/dummy-indicator-visible.c'
--- tests/dummy-indicator-visible.c	2012-01-23 11:39:50 +0000
+++ tests/dummy-indicator-visible.c	2012-09-12 04:29:16 +0000
@@ -88,17 +88,25 @@
 static void
 dummy_indicator_entry_being_removed (IndicatorObject * io, IndicatorObjectEntry * entry)
 {
+	IndicatorObjectClass * indicator_object_class = INDICATOR_OBJECT_CLASS (dummy_indicator_visible_parent_class);
+
 	g_object_set_data(G_OBJECT(entry->label), "is-hidden", GINT_TO_POINTER(1));
 
-        INDICATOR_OBJECT_CLASS(dummy_indicator_visible_parent_class)->entry_being_removed (io, entry);
+	if (indicator_object_class->entry_being_removed != NULL) {
+		indicator_object_class->entry_being_removed (io, entry);
+	}
 }
 
 static void
 dummy_indicator_entry_was_added (IndicatorObject * io, IndicatorObjectEntry * entry)
 {
+	IndicatorObjectClass * indicator_object_class = INDICATOR_OBJECT_CLASS (dummy_indicator_visible_parent_class);
+
 	g_object_steal_data(G_OBJECT(entry->label), "is-hidden");
 
-        INDICATOR_OBJECT_CLASS(dummy_indicator_visible_parent_class)->entry_was_added (io, entry);
+	if (indicator_object_class->entry_was_added != NULL) {
+		indicator_object_class->entry_was_added (io, entry);
+	}
 }
 
 static void

=== modified file 'tests/test-loader.c'
--- tests/test-loader.c	2012-01-25 17:11:43 +0000
+++ tests/test-loader.c	2012-09-12 04:29:16 +0000
@@ -156,22 +156,24 @@
 static void
 visible_entry_added (IndicatorObject * io, IndicatorObjectEntry * entry, gpointer box)
 {
-	// make a frame for the entry, and add the frame to the box
-	GtkWidget * frame = gtk_frame_new (NULL);
-	GtkWidget * child = GTK_WIDGET(entry->label);
+	GtkWidget * child = GTK_WIDGET (entry->label);
 	g_assert (child != NULL);
-	gtk_container_add (GTK_CONTAINER(frame), child);
-	gtk_box_pack_start (GTK_BOX(box), frame, FALSE, FALSE, 0);
-	g_object_set_data (G_OBJECT(child), "frame-parent", frame);
+
+	if (g_object_get_data (G_OBJECT(child), "frame-parent") == NULL)
+	{
+		GtkWidget * frame = gtk_frame_new (NULL);
+		gtk_container_add (GTK_CONTAINER(frame), child);
+		gtk_box_pack_start (GTK_BOX(box), frame, FALSE, FALSE, 0);
+		g_object_set_data (G_OBJECT(child), "frame-parent", frame);
+	}
 }
 
 static void
 visible_entry_removed (IndicatorObject * io, IndicatorObjectEntry * entry, gpointer box)
 {
-	// destroy this entry's frame
-	gpointer parent = g_object_steal_data (G_OBJECT(entry->label), "frame-parent");
-	if (GTK_IS_WIDGET(parent))
-		gtk_widget_destroy(GTK_WIDGET(parent));
+	GtkWidget * child = GTK_WIDGET (entry->label);
+	g_assert (child != NULL);
+	g_assert (g_object_get_data (G_OBJECT(child), "frame-parent") != NULL);
 }
 
 void
@@ -218,7 +220,7 @@
 	g_assert(GTK_IS_LABEL(label));
         g_assert(g_object_get_qdata(G_OBJECT(label), is_hidden_quark) != NULL);
 	list = gtk_container_get_children (GTK_CONTAINER(box));
-	g_assert(g_list_length(list) == 0);
+	g_assert(g_list_length(list) == 1);
 	g_list_free(list);
 
 	// restore the entries and confirm that the label survived

=== modified file 'tools/indicator-loader.c'
--- tools/indicator-loader.c	2012-01-19 17:48:32 +0000
+++ tools/indicator-loader.c	2012-09-12 04:29:16 +0000
@@ -25,6 +25,8 @@
 #include <gtk/gtk.h>
 #include <libindicator/indicator-object.h>
 
+static GHashTable * entry_to_menuitem = NULL;
+
 #define ENTRY_DATA_NAME "indicator-custom-entry-data"
 
 static void
@@ -40,20 +42,18 @@
 	return;
 }
 
-static void
-entry_added (IndicatorObject * io, IndicatorObjectEntry * entry, gpointer user_data)
+static GtkWidget*
+create_menu_item (IndicatorObjectEntry * entry)
 {
-	g_debug("Signal: Entry Added");
-
-	if (entry->parent_object == NULL) {
-		g_warning("Entry '%p' does not have a parent object", entry);
-	}
-
-	GtkWidget * menuitem = gtk_menu_item_new();
+	GtkWidget * hbox;
+	GtkWidget * menuitem;
+
+	menuitem = gtk_menu_item_new();
+
 #if GTK_CHECK_VERSION(3,0,0)
-	GtkWidget * hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 3);
+	hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 3);
 #else
-	GtkWidget * hbox = gtk_hbox_new(FALSE, 3);
+	hbox = gtk_hbox_new(FALSE, 3);
 #endif
 
 	if (entry->image != NULL) {
@@ -69,26 +69,34 @@
 		gtk_menu_item_set_submenu(GTK_MENU_ITEM(menuitem), GTK_WIDGET(entry->menu));
 	}
 
-	g_signal_connect(G_OBJECT(menuitem), "activate", G_CALLBACK(activate_entry), io);
-
-	gtk_menu_shell_append(GTK_MENU_SHELL(user_data), menuitem);
-	gtk_widget_show(menuitem);
-
-	g_object_set_data(G_OBJECT(menuitem), ENTRY_DATA_NAME, entry);
-
-	return;
+	return menuitem;
 }
 
 static void
-entry_removed_cb (GtkWidget * widget, gpointer userdata)
+entry_added (IndicatorObject * io, IndicatorObjectEntry * entry, gpointer user_data)
 {
-	gpointer data = g_object_get_data(G_OBJECT(widget), ENTRY_DATA_NAME);
-
-	if (data != userdata) {
-		return;
-	}
-
-	gtk_widget_destroy(widget);
+	GtkWidget * menuitem;
+
+	g_debug("Signal: Entry Added");
+
+	if (entry->parent_object == NULL) {
+		g_warning("Entry '%p' does not have a parent object", entry);
+	}
+
+	menuitem = g_hash_table_lookup (entry_to_menuitem, entry);
+	if (menuitem == NULL) {
+		g_debug ("This is the first time this entry's been added -- creating a new menuitem for it");
+		menuitem = create_menu_item (entry);
+		g_hash_table_insert (entry_to_menuitem, entry, menuitem);
+
+		g_object_set_data(G_OBJECT(menuitem), ENTRY_DATA_NAME, entry);
+		g_signal_connect (G_OBJECT(menuitem), "activate", G_CALLBACK(activate_entry), io);
+
+		gtk_menu_shell_append (GTK_MENU_SHELL(user_data), menuitem);
+
+	}
+	gtk_widget_show (menuitem);
+
 	return;
 }
 
@@ -97,7 +105,9 @@
 {
 	g_debug("Signal: Entry Removed");
 
-	gtk_container_foreach(GTK_CONTAINER(user_data), entry_removed_cb, entry);
+	GtkWidget * menuitem = g_hash_table_lookup (entry_to_menuitem, entry);
+	if (menuitem != NULL)
+		gtk_widget_hide (menuitem);
 
 	return;
 }
@@ -162,6 +172,8 @@
 
 	gtk_init(&argc, &argv);
 
+	entry_to_menuitem = g_hash_table_new (g_direct_hash, g_direct_equal);
+
 	if (argc != 2) {
 		g_error("Need filename");
 		return 1;
@@ -183,5 +195,6 @@
 
 	gtk_main();
 
+	g_hash_table_destroy (entry_to_menuitem);
 	return 0;
 }