Merge lp:~chrisccoulson/libdbusmenu/lp729187 into lp:libdbusmenu/0.5

[Chris Coulson]

Don't set a default label for menuitems. Some applications (eg, xchat and pidgin) do silly things like creating GtkMenuItems's without a label for separators (rather than using the GtkSeparatorMenuItem class).

GTK correctly renders these as separators, so we need to handle it too by not setting a default label on these

[Ted Gould (ted)]

I think we're going to have to detect this case in the parser unfortunately. The Qt and Nux implementations of Dbusmenu won't correctly view them as separators.

review: Needs Fixing

[lp:~chrisccoulson/libdbusmenu/lp729187]

284. By Chris Coulson on 2011-03-30

Revert the last commit and handle the same problem in the parser instead

285. By Chris Coulson on 2011-03-30

Remove the now unneeded null pointer check on label in construct_dbusmenu_for_widget.
Also, don't use a strcmp in widget_notify_cb for checking if the menuitem is a separator. Just do a null pointer
check on pdata->label instead

[Chris Coulson (chrisccoulson)]

Ok, I've implemented the check in the parser now

review: Needs Resubmitting

[Chris Coulson (chrisccoulson)]

Oops, that was meant to be "Comment only"

[Ted Gould (ted)]

So, while I think this code is fine. I think that client code doesn't
handle changing types well. It could probably be fixed, but I think
that an easier solution might be to just destroy the dbusmenu item and
create a new one if we get a label after making a separator. What do
you think there?

Also, is there a use case for the label getting removed that it'd go
back to being a separator? Seems like *really* bad usage, but I'm
curious if we need to handle that case as well.

[lp:~chrisccoulson/libdbusmenu/lp729187]

286. By Chris Coulson on 2011-03-30

- Don't change the type of existing menu items in the server. This
  isn't handled in the client too well
- Handle a GtkMenuItem's GtkLabel being removed too

[Chris Coulson (chrisccoulson)]

Ok, I've fixed it to destroy the existing menuitem and recreate it, rather than changing the type of the existing one. It also handles being converted back to a separator now (which can happen if someone calls gtk_container_remove to remove the label from the menuitem)

[lp:~chrisccoulson/libdbusmenu/lp729187]

287. By Chris Coulson on 2011-03-30

Remove some code duplication introduced in this branch

=== modified file 'libdbusmenu-glib/defaults.c'
--- libdbusmenu-glib/defaults.c	2011-02-25 14:47:56 +0000
+++ libdbusmenu-glib/defaults.c	2011-03-30 19:01:23 +0000
@@ -81,7 +81,7 @@
 	/* Standard defaults */
 	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_VISIBLE,        G_VARIANT_TYPE_BOOLEAN,   g_variant_new_boolean(TRUE)); 
 	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_ENABLED,        G_VARIANT_TYPE_BOOLEAN,   g_variant_new_boolean(TRUE)); 
-	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_LABEL,          G_VARIANT_TYPE_STRING,    g_variant_new_string(_("Label Empty"))); 
+	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_LABEL,          G_VARIANT_TYPE_STRING,    g_variant_new_string(_("Label Empty")));
 	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_ICON_NAME,      G_VARIANT_TYPE_STRING,    NULL); 
 	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE,    G_VARIANT_TYPE_STRING,    NULL); 
 	dbusmenu_defaults_default_set(self,   DBUSMENU_CLIENT_TYPES_DEFAULT,    DBUSMENU_MENUITEM_PROP_TOGGLE_STATE,   G_VARIANT_TYPE_INT32,     NULL); 

=== modified file 'libdbusmenu-gtk/parser.c'
--- libdbusmenu-gtk/parser.c	2011-03-21 14:00:36 +0000
+++ libdbusmenu-gtk/parser.c	2011-03-30 19:01:23 +0000
@@ -454,7 +454,7 @@
 
       gboolean visible = FALSE;
       gboolean sensitive = FALSE;
-      if (GTK_IS_SEPARATOR_MENU_ITEM (widget))
+      if (GTK_IS_SEPARATOR_MENU_ITEM (widget) || !find_menu_label (widget))
         {
           dbusmenu_menuitem_property_set (mi,
                                           "type",
@@ -512,21 +512,18 @@
 
           GtkWidget *label = find_menu_label (widget);
 
-          if (label)
-            {
-              // Sometimes, an app will directly find and modify the label
-              // (like empathy), so watch the label especially for that.
-              gchar * text = sanitize_label (GTK_LABEL (label));
-              dbusmenu_menuitem_property_set (mi, "label", text);
-              g_free (text);
+          // Sometimes, an app will directly find and modify the label
+          // (like empathy), so watch the label especially for that.
+          gchar * text = sanitize_label (GTK_LABEL (label));
+          dbusmenu_menuitem_property_set (mi, "label", text);
+          g_free (text);
 
-              pdata->label = label;
-              g_signal_connect (G_OBJECT (label),
-                                "notify",
-                                G_CALLBACK (label_notify_cb),
-                                mi);
-              g_object_add_weak_pointer(G_OBJECT (label), (gpointer*)&pdata->label);
-            }
+          pdata->label = label;
+          g_signal_connect (G_OBJECT (label),
+                            "notify",
+                            G_CALLBACK (label_notify_cb),
+                            mi);
+          g_object_add_weak_pointer(G_OBJECT (label), (gpointer*)&pdata->label);
 
           if (GTK_IS_ACTIVATABLE (widget))
             {
@@ -760,11 +757,48 @@
 }
 
 static void
+recreate_menu_item (DbusmenuMenuitem * parent, DbusmenuMenuitem * child)
+{
+  if (parent == NULL)
+    {
+      /* We need a parent */
+      return;
+    }
+  ParserData * pdata = g_object_get_data (G_OBJECT (child), PARSER_DATA);
+  /* Keep a pointer to the GtkMenuItem, as pdata->widget might be
+   * invalidated when we delete the DbusmenuMenuitem
+   */
+  GtkWidget * menuitem = pdata->widget;
+
+  dbusmenu_menuitem_child_delete (parent, child);
+
+  RecurseContext recurse = {0};
+  recurse.toplevel = gtk_widget_get_toplevel(menuitem);
+  recurse.parent = parent;
+
+  parse_menu_structure_helper(menuitem, &recurse);
+}
+
+static gboolean
+recreate_menu_item_in_idle_cb (gpointer data)
+{
+  DbusmenuMenuitem * child = (DbusmenuMenuitem *)data;
+  DbusmenuMenuitem * parent = dbusmenu_menuitem_get_parent (child);
+  g_object_unref (child);
+  recreate_menu_item (parent, child);
+  return FALSE;
+}
+
+static void
 label_notify_cb (GtkWidget  *widget,
                  GParamSpec *pspec,
                  gpointer    data)
 {
   DbusmenuMenuitem *child = (DbusmenuMenuitem *)data;
+  GValue prop_value = {0};
+
+  g_value_init (&prop_value, pspec->value_type); 
+  g_object_get_property (G_OBJECT (widget), pspec->name, &prop_value);
 
   if (pspec->name == g_intern_static_string ("label"))
     {
@@ -774,6 +808,24 @@
                                       text);
       g_free (text);
     }
+  else if (pspec->name == g_intern_static_string ("parent"))
+    {
+      if (GTK_WIDGET (g_value_get_object (&prop_value)) == NULL)
+        {
+          /* This label is being removed from its GtkMenuItem. The
+           * menuitem becomes a separator now. As the client doesn't handle
+           * changing types so well, we remove the current DbusmenuMenuitem
+           * and add a new one.
+           *
+           * Note, we have to defer this to idle, as we are called before
+           * bin->child member of our old parent is invalidated. If we go ahead
+           * and call parse_menu_structure_helper now, the GtkMenuItem will
+           * still appear to have a label and we never convert it to a separator
+           */
+          g_object_ref (child);
+          g_idle_add ((GSourceFunc)recreate_menu_item_in_idle_cb, child);
+        } 
+    }
 }
 
 static void
@@ -891,6 +943,20 @@
     }
   else if (pspec->name == g_intern_static_string ("label"))
     {
+      ParserData *pdata = g_object_get_data (G_OBJECT (child), PARSER_DATA);
+      if (!pdata->label)
+        {
+          /* GtkMenuItem's can start life as a separator if they have no child
+           * GtkLabel. In this case, we need to convert the DbusmenuMenuitem from
+           * a separator to a normal menuitem if the application adds a label.
+           * As changing types isn't handled too well by the client, we delete
+           * this menuitem for now and then recreate it
+           */
+          DbusmenuMenuitem * parent = dbusmenu_menuitem_get_parent (child);
+          recreate_menu_item (parent, child);
+          return;
+        }
+                      
       dbusmenu_menuitem_property_set (child,
                                       DBUSMENU_MENUITEM_PROP_LABEL,
                                       g_value_get_string (&prop_value));