ayatana-commits team mailing list archive

[Ted Gould <ted@xxxxxxxx>]

Ted Gould has proposed merging lp:~ted/dbusmenu/ordering-issues-again into lp:dbusmenu.

Requested reviews:
  DBus Menu Team (dbusmenu-team)


Most importantly this makes it so the GTK layer uses the realized count for positioning the menu items instead of the absolute count to remove race conditions of realization of menuitems.  It also adds the ability to track realization and some other warning cleanups and comments.
-- 
https://code.launchpad.net/~ted/dbusmenu/ordering-issues-again/+merge/23454
Your team ayatana-commits is subscribed to branch lp:dbusmenu.

=== modified file 'libdbusmenu-glib/client.c'
--- libdbusmenu-glib/client.c	2010-03-31 19:47:29 +0000
+++ libdbusmenu-glib/client.c	2010-04-15 06:05:31 +0000
@@ -35,6 +35,7 @@
 
 #include "client.h"
 #include "menuitem.h"
+#include "menuitem-private.h"
 #include "client-menuitem.h"
 #include "dbusmenu-client.h"
 #include "server-marshal.h"
@@ -645,7 +646,7 @@
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Client has realized a menuitem: %d", dbusmenu_menuitem_get_id(propdata->item));
 	#endif
-	g_signal_emit(G_OBJECT(propdata->item), DBUSMENU_MENUITEM_SIGNAL_REALIZED_ID, 0, TRUE);
+	dbusmenu_menuitem_set_realized(propdata->item);
 
 	if (!handled) {
 		g_signal_emit(G_OBJECT(propdata->client), signals[NEW_MENUITEM], 0, propdata->item, TRUE);
@@ -748,6 +749,7 @@
 			if (parent != NULL) {
 				dbusmenu_menuitem_child_delete(parent, item);
 			}
+			/* XXX: Should this be an unref?  Who's reffing this that it exists without a parent? */
 			g_object_unref(G_OBJECT(item));
 			item = NULL;
 		}
@@ -773,6 +775,7 @@
 		}
 	} else {
 		/* Refresh the properties */
+		/* XXX: We shouldn't need to get the properties everytime we reuse an entry */
 		gchar * properties[1] = {NULL}; /* This gets them all */
 		org_ayatana_dbusmenu_get_properties_async(proxy, id, (const gchar **)properties, menuitem_get_properties_replace_cb, item);
 	}

=== modified file 'libdbusmenu-glib/menuitem-private.h'
--- libdbusmenu-glib/menuitem-private.h	2010-01-21 23:13:52 +0000
+++ libdbusmenu-glib/menuitem-private.h	2010-04-15 06:05:31 +0000
@@ -34,6 +34,8 @@
 G_BEGIN_DECLS
 
 void dbusmenu_menuitem_buildxml (DbusmenuMenuitem * mi, GPtrArray * array);
+gboolean dbusmenu_menuitem_realized (DbusmenuMenuitem * mi);
+void dbusmenu_menuitem_set_realized (DbusmenuMenuitem * mi);
 
 G_END_DECLS
 

=== modified file 'libdbusmenu-glib/menuitem.c'
--- libdbusmenu-glib/menuitem.c	2010-03-31 18:51:48 +0000
+++ libdbusmenu-glib/menuitem.c	2010-04-15 06:05:31 +0000
@@ -59,6 +59,7 @@
 	GList * children;
 	GHashTable * properties;
 	gboolean root;
+	gboolean realized;
 };
 
 /* Signals */
@@ -278,6 +279,7 @@
 	priv->properties = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, _g_value_free);
 
 	priv->root = FALSE;
+	priv->realized = FALSE;
 	
 	return;
 }
@@ -423,6 +425,46 @@
 }
 
 /**
+	dbusmenu_menuitem_realized:
+	@mi: #DbusmenuMenuitem to check on
+
+	This function returns whether the menuitem has been realized or
+	not.  This is significant mostly in client implementations that
+	can use this additional state to see if the second layers of
+	the implementation have been built yet.
+
+	Return value: Returns whether or not the menu item has been realized
+		yet or not.
+*/
+gboolean
+dbusmenu_menuitem_realized (DbusmenuMenuitem * mi)
+{
+	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(mi), FALSE);
+	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
+	return priv->realized;
+}
+
+/**
+	dbusmenu_menuitem_set_realized:
+	@mi: #DbusmenuMenuitem to realize
+
+	Sets the internal variable tracking whether it's been realized and
+	signals the DbusmenuMenuitem::realized event.
+*/
+void
+dbusmenu_menuitem_set_realized (DbusmenuMenuitem * mi)
+{
+	g_return_if_fail(DBUSMENU_IS_MENUITEM(mi));
+	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
+	if (priv->realized) {
+		g_warning("Realized entry realized again?  ID: %d", dbusmenu_menuitem_get_id(mi));
+	}
+	priv->realized = TRUE;
+	g_signal_emit(G_OBJECT(mi), signals[REALIZED], 0, TRUE);
+	return;
+}
+
+/**
 	dbusmenu_menuitem_get_children:
 	@mi: The #DbusmenuMenuitem to query.
 
@@ -449,6 +491,7 @@
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Menuitem %d (%s) signalling child removed %d (%s)", ID(user_data), LABEL(user_data), ID(data), LABEL(data));
 	#endif
+	g_object_unref(G_OBJECT(data));
 	g_signal_emit(G_OBJECT(user_data), signals[CHILD_REMOVED], 0, DBUSMENU_MENUITEM(data), TRUE);
 	return;
 }
@@ -517,6 +560,50 @@
 }
 
 /**
+	dbusmenu_menuitem_get_position_realized:
+	@mi: The #DbusmenuMenuitem to find the position of
+	@parent: The #DbusmenuMenuitem who's children contain @mi
+
+	This function is very similar to #dbusmenu_menuitem_get_position
+	except that it only counts in the children that have been realized.
+
+	Return value: The position of @mi in the realized children of @parent.
+*/
+guint
+dbusmenu_menuitem_get_position_realized (DbusmenuMenuitem * mi, DbusmenuMenuitem * parent)
+{
+	#ifdef MASSIVEDEBUGGING
+	if (!DBUSMENU_IS_MENUITEM(mi))     g_warning("Getting position of %d (%s), it's at: %d (mi fail)", ID(mi), LABEL(mi), 0);
+	if (!DBUSMENU_IS_MENUITEM(parent)) g_warning("Getting position of %d (%s), it's at: %d (parent fail)", ID(mi), LABEL(mi), 0);
+	#endif
+
+	/* TODO: I'm not too happy returning zeros here.  But that's all I've got */
+	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(mi), 0);
+	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(parent), 0);
+
+	GList * childs = dbusmenu_menuitem_get_children(parent);
+	if (childs == NULL) return 0;
+	guint count = 0;
+	for ( ; childs != NULL; childs = childs->next, count++) {
+		if (!dbusmenu_menuitem_realized(DBUSMENU_MENUITEM(childs->data))) {
+			count--;
+			continue;
+		}
+		if (childs->data == mi) {
+			break;
+		}
+	}
+
+	if (childs == NULL) return 0;
+
+	#ifdef MASSIVEDEBUGGING
+	g_debug("Getting position of %d (%s), it's at: %d", ID(mi), LABEL(mi), count);
+	#endif
+
+	return count;
+}
+
+/**
 	dbusmenu_menuitem_child_append:
 	@mi: The #DbusmenuMenuitem which will become a new parent
 	@child: The #DbusmenMenuitem that will be a child
@@ -533,10 +620,13 @@
 	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(child), FALSE);
 
 	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
+	g_return_val_if_fail(g_list_find(priv->children, child) == NULL, FALSE);
+
 	priv->children = g_list_append(priv->children, child);
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Menuitem %d (%s) signalling child added %d (%s) at %d", ID(mi), LABEL(mi), ID(child), LABEL(child), g_list_length(priv->children) - 1);
 	#endif
+	g_object_ref(G_OBJECT(child));
 	g_signal_emit(G_OBJECT(mi), signals[CHILD_ADDED], 0, child, g_list_length(priv->children) - 1, TRUE);
 	return TRUE;
 }
@@ -558,10 +648,13 @@
 	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(child), FALSE);
 
 	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
+	g_return_val_if_fail(g_list_find(priv->children, child) == NULL, FALSE);
+
 	priv->children = g_list_prepend(priv->children, child);
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Menuitem %d (%s) signalling child added %d (%s) at %d", ID(mi), LABEL(mi), ID(child), LABEL(child), 0);
 	#endif
+	g_object_ref(G_OBJECT(child));
 	g_signal_emit(G_OBJECT(mi), signals[CHILD_ADDED], 0, child, 0, TRUE);
 	return TRUE;
 }
@@ -588,6 +681,7 @@
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Menuitem %d (%s) signalling child removed %d (%s)", ID(mi), LABEL(mi), ID(child), LABEL(child));
 	#endif
+	g_object_unref(G_OBJECT(child));
 	g_signal_emit(G_OBJECT(mi), signals[CHILD_REMOVED], 0, child, TRUE);
 	return TRUE;
 }
@@ -611,10 +705,13 @@
 	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(child), FALSE);
 
 	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
+	g_return_val_if_fail(g_list_find(priv->children, child) == NULL, FALSE);
+
 	priv->children = g_list_insert(priv->children, child, position);
 	#ifdef MASSIVEDEBUGGING
 	g_debug("Menuitem %d (%s) signalling child added %d (%s) at %d", ID(mi), LABEL(mi), ID(child), LABEL(child), position);
 	#endif
+	g_object_ref(G_OBJECT(child));
 	g_signal_emit(G_OBJECT(mi), signals[CHILD_ADDED], 0, child, position, TRUE);
 	return TRUE;
 }

=== modified file 'libdbusmenu-glib/menuitem.h'
--- libdbusmenu-glib/menuitem.h	2010-03-31 18:51:48 +0000
+++ libdbusmenu-glib/menuitem.h	2010-04-15 06:05:31 +0000
@@ -148,6 +148,7 @@
 GList * dbusmenu_menuitem_get_children (DbusmenuMenuitem * mi);
 GList * dbusmenu_menuitem_take_children (DbusmenuMenuitem * mi) G_GNUC_WARN_UNUSED_RESULT;
 guint dbusmenu_menuitem_get_position (DbusmenuMenuitem * mi, DbusmenuMenuitem * parent);
+guint dbusmenu_menuitem_get_position_realized (DbusmenuMenuitem * mi, DbusmenuMenuitem * parent);
 
 gboolean dbusmenu_menuitem_child_append (DbusmenuMenuitem * mi, DbusmenuMenuitem * child);
 gboolean dbusmenu_menuitem_child_prepend (DbusmenuMenuitem * mi, DbusmenuMenuitem * child);

=== modified file 'libdbusmenu-glib/server.c'
--- libdbusmenu-glib/server.c	2010-03-31 17:29:20 +0000
+++ libdbusmenu-glib/server.c	2010-04-15 06:05:31 +0000
@@ -404,6 +404,16 @@
 		}
 	} else {
 		DbusmenuMenuitem * item = dbusmenu_menuitem_find_id(priv->root, parent);
+		if (item == NULL) {
+			if (error != NULL) {
+				g_set_error(error,
+				            error_quark(),
+				            INVALID_MENUITEM_ID,
+				            "The ID supplied %d does not refer to a menu item we have",
+				            parent);
+			}
+			return FALSE;
+		}
 		dbusmenu_menuitem_buildxml(item, xmlarray);
 	}
 	g_ptr_array_add(xmlarray, NULL);

=== modified file 'libdbusmenu-gtk/client.c'
--- libdbusmenu-gtk/client.c	2010-03-31 18:51:48 +0000
+++ libdbusmenu-gtk/client.c	2010-04-15 06:05:31 +0000
@@ -308,7 +308,7 @@
 
 	/* Oh, we're a child, let's deal with that */
 	if (parent != NULL) {
-		new_child(parent, item, dbusmenu_menuitem_get_position(item, parent), DBUSMENU_GTKCLIENT(client));
+		new_child(parent, item, dbusmenu_menuitem_get_position_realized(item, parent), DBUSMENU_GTKCLIENT(client));
 	}
 
 	return;
@@ -335,7 +335,7 @@
 	} 
 
 	GtkMenuItem * childmi  = dbusmenu_gtkclient_menuitem_get(gtkclient, child);
-	gtk_menu_shell_insert(GTK_MENU_SHELL(menu), GTK_WIDGET(childmi), position);
+	gtk_menu_shell_insert(GTK_MENU_SHELL(menu), GTK_WIDGET(childmi), dbusmenu_menuitem_get_position_realized(child, mi));
 	gtk_widget_show(GTK_WIDGET(menu));
 	
 	return;
@@ -373,7 +373,7 @@
 	}
 
 	GtkMenuItem * childmi  = dbusmenu_gtkclient_menuitem_get(gtkclient, child);
-	gtk_menu_reorder_child(GTK_MENU(ann_menu), GTK_WIDGET(childmi), new);
+	gtk_menu_reorder_child(GTK_MENU(ann_menu), GTK_WIDGET(childmi), dbusmenu_menuitem_get_position_realized(child, mi));
 
 	return;
 }

=== modified file 'libdbusmenu-gtk/menu.c'
--- libdbusmenu-gtk/menu.c	2010-03-31 18:49:32 +0000
+++ libdbusmenu-gtk/menu.c	2010-04-15 06:05:31 +0000
@@ -237,7 +237,7 @@
 	GtkMenuItem * mi = dbusmenu_gtkclient_menuitem_get(priv->client, child);
 	if (mi != NULL) {
 		GtkWidget * item = GTK_WIDGET(mi);
-		gtk_menu_insert(GTK_MENU(menu), item, position);
+		gtk_menu_insert(GTK_MENU(menu), item, dbusmenu_menuitem_get_position_realized(child, root));
 		#ifdef MASSIVEDEBUGGING
 		menu_pos_t menu_pos;
 		menu_pos.mi = mi;
@@ -260,7 +260,7 @@
 	g_debug("Root child moved");
 	#endif
 	DbusmenuGtkMenuPrivate * priv = DBUSMENU_GTKMENU_GET_PRIVATE(menu);
-	gtk_menu_reorder_child(GTK_MENU(menu), GTK_WIDGET(dbusmenu_gtkclient_menuitem_get(priv->client, child)), newposition);
+	gtk_menu_reorder_child(GTK_MENU(menu), GTK_WIDGET(dbusmenu_gtkclient_menuitem_get(priv->client, child)), dbusmenu_menuitem_get_position_realized(child, root));
 	return;
 }
 
@@ -300,7 +300,7 @@
 
 	if (child_widget != NULL) {
 		gtk_menu_append(menu, child_widget);
-		gtk_menu_reorder_child(GTK_MENU(menu), child_widget, dbusmenu_menuitem_get_position(child, dbusmenu_client_get_root(DBUSMENU_CLIENT(priv->client))));
+		gtk_menu_reorder_child(GTK_MENU(menu), child_widget, dbusmenu_menuitem_get_position_realized(child, dbusmenu_client_get_root(DBUSMENU_CLIENT(priv->client))));
 	} else {
 		g_warning("Child is realized, but doesn't have a GTK Widget!");
 	}