Merge lp:~ted/libdbusmenu/lp934574 into lp:libdbusmenu/0.6

[Ted Gould]

Fixes a rather nasty little bug where icons don't update. There's actually a couple of factors. One, is that we don't resetup the signals when we get a new image. The second is that we'll send a clear even if the value was never set.

[Charles Kerr (charlesk)]

menuitem.c:

  > 9+ if ((!inhash || replaced) &&
  > 10+ !(!inhash && value == NULL)) {

  This is the only item that needs fixing... this really hurts.

  After staring at the code block above this for a bit,
  I /think/ this can be changed to:

  > 9+ if (replaced)

parser.c:

  Looks fine. I have two trivial suggestions, feel free to ignore:

  Since the patch does the same thing three times, it might be
  cleaner to have a n update_icon_from_item() wrapper function that
  extracts the ParserData from the Menuitem and calls update_icon().

  gpointer is a void*, so an explicit cast is never required.
  "ParserData * pdata = g_object_get_data()" will always work --
  "ParserData * pdata = (ParserData*)g_object_get_data()" is unnecessary.

review: Needs Fixing

[Ted Gould (ted)]

Okay, so I think I agree on the logic :-) Took a while to figure out, but I think I'm at the same place now. r384

[lp:~ted/libdbusmenu/lp934574]

384. By Ted Gould on 2012-03-08

Switching out logic, we're using replaced now because it gets set everytime that the hashtable is modified, and if we weren't modifying the hash table in some way, we don't want to signal. And, conversely, no one cares if we didn't modify the hash table.

=== modified file 'libdbusmenu-glib/menuitem.c'
--- libdbusmenu-glib/menuitem.c	2012-02-09 18:44:09 +0000
+++ libdbusmenu-glib/menuitem.c	2012-03-08 15:03:20 +0000
@@ -1253,7 +1253,7 @@
 	   becuse it has been unref'd when replaced in the hash
 	   table.  But the fact that there was a value is
 	   the imporant part. */
-	if (!inhash || replaced) {
+	if (replaced) {
 		GVariant * signalval = value;
 
 		if (signalval == NULL) {

=== modified file 'libdbusmenu-gtk/parser.c'
--- libdbusmenu-gtk/parser.c	2012-02-15 17:18:15 +0000
+++ libdbusmenu-gtk/parser.c	2012-03-08 15:03:20 +0000
@@ -44,6 +44,8 @@
   GtkWidget *shell;
   GtkWidget *image;
   AtkObject *accessible;
+
+  guint theme_changed_sig;
 } ParserData;
 
 typedef struct _RecurseContext
@@ -59,6 +61,8 @@
 static void           checkbox_toggled         (GtkWidget *         widget,
                                                 DbusmenuMenuitem *  mi);
 static void           update_icon              (DbusmenuMenuitem *  menuitem,
+                                                ParserData *        pdata,
+                                                GtkImageMenuItem *  gmenuitem,
                                                 GtkImage *          image);
 static GtkWidget *    find_menu_label          (GtkWidget *         widget);
 static void           label_notify_cb          (GtkWidget *         widget,
@@ -212,6 +216,11 @@
                 g_object_remove_weak_pointer(G_OBJECT(pdata->accessible), (gpointer*)&pdata->accessible);
         }
 
+	if (pdata != NULL && pdata->theme_changed_sig != 0) {
+		g_signal_handler_disconnect(gtk_icon_theme_get_default(), pdata->theme_changed_sig);
+		pdata->theme_changed_sig = 0;
+	}
+
 	g_free(pdata);
 
 	return;
@@ -568,19 +577,7 @@
 
               if (GTK_IS_IMAGE (image))
                 {
-                  update_icon (mi, GTK_IMAGE (image));
-
-                  /* Watch for theme changes because if gicon changes, we want to send a
-                     different pixbuf. */
-                  g_signal_connect(G_OBJECT(gtk_icon_theme_get_default()),
-                                   "changed", G_CALLBACK(theme_changed_cb), widget);
-
-                  pdata->image = image;
-                  g_signal_connect (G_OBJECT (image),
-                                    "notify",
-                                    G_CALLBACK (image_notify_cb),
-                                    mi);
-                  g_object_add_weak_pointer(G_OBJECT (image), (gpointer*)&pdata->image);
+                  update_icon (mi, pdata, GTK_IMAGE_MENU_ITEM(widget), GTK_IMAGE (image));
                 }
             }
 
@@ -737,7 +734,7 @@
 }
 
 static void
-update_icon (DbusmenuMenuitem *menuitem, GtkImage *image)
+update_icon (DbusmenuMenuitem *menuitem, ParserData * pdata, GtkImageMenuItem * gmenuitem, GtkImage *image)
 {
   GdkPixbuf * pixbuf = NULL;
   const gchar * icon_name = NULL;
@@ -746,6 +743,35 @@
   GtkIconInfo * info;
   gint width;
 
+  /* Check to see if we're changing the image.  If so, we need to track
+     that little bugger */
+  /* Why check for gmenuitem being NULL?  Because there are some cases where
+     we can't get it easily, and those mean it's not changed just the icon
+     underneith, so we can ignore these larger impacts */
+  if (image != GTK_IMAGE(pdata->image) && gmenuitem != NULL) {
+    if (pdata->theme_changed_sig != 0) {
+      g_signal_handler_disconnect(gtk_icon_theme_get_default(), pdata->theme_changed_sig);
+      pdata->theme_changed_sig = 0;
+    }
+
+    if (pdata->image != NULL) {
+      g_signal_handlers_disconnect_by_func(pdata->image, G_CALLBACK(image_notify_cb), menuitem);
+      g_object_remove_weak_pointer(G_OBJECT(pdata->image), (gpointer*)&pdata->image);
+    }
+
+    pdata->image = GTK_WIDGET(image);
+
+    if (pdata->image != NULL) {
+      pdata->theme_changed_sig = g_signal_connect(G_OBJECT(gtk_icon_theme_get_default()),
+                                                  "changed", G_CALLBACK(theme_changed_cb), gmenuitem);
+      g_signal_connect (G_OBJECT (pdata->image),
+                        "notify",
+                        G_CALLBACK (image_notify_cb),
+                        menuitem);
+      g_object_add_weak_pointer(G_OBJECT (pdata->image), (gpointer*)&pdata->image);
+    }
+  }
+
   if (image != NULL && should_show_image (image)) {
     switch (gtk_image_get_storage_type (image)) {
     case GTK_IMAGE_EMPTY:
@@ -946,7 +972,8 @@
       pspec->name == g_intern_static_string ("stock") ||
       pspec->name == g_intern_static_string ("storage-type"))
     {
-      update_icon (mi, GTK_IMAGE (widget));
+      ParserData *pdata = (ParserData *)g_object_get_data(G_OBJECT(mi), PARSER_DATA);
+      update_icon (mi, pdata, NULL, GTK_IMAGE (widget));
     }
 }
 
@@ -1134,13 +1161,15 @@
     {
       GtkWidget *image = NULL;
       g_object_get(widget, "image", &image, NULL);
-      update_icon (child, GTK_IMAGE(image));
+      ParserData *pdata = (ParserData *)g_object_get_data(G_OBJECT(child), PARSER_DATA);
+      update_icon (child, pdata, GTK_IMAGE_MENU_ITEM(widget), GTK_IMAGE(image));
     }
   else if (pspec->name == g_intern_static_string ("image"))
     {
       GtkWidget *image;
       image = GTK_WIDGET (g_value_get_object (&prop_value));
-      update_icon (child, GTK_IMAGE (image));
+      ParserData *pdata = (ParserData *)g_object_get_data(G_OBJECT(child), PARSER_DATA);
+      update_icon (child, pdata, GTK_IMAGE_MENU_ITEM(widget), GTK_IMAGE (image));
     }
   else if (pspec->name == g_intern_static_string ("parent"))
     {
@@ -1259,7 +1288,8 @@
 
   gpointer pmi = g_object_get_data(G_OBJECT(data), CACHED_MENUITEM);
   if (pmi != NULL) {
-    update_icon(DBUSMENU_MENUITEM(pmi), GTK_IMAGE(image));
+    ParserData *pdata = (ParserData *)g_object_get_data(G_OBJECT(pmi), PARSER_DATA);
+    update_icon(DBUSMENU_MENUITEM(pmi), pdata, NULL, GTK_IMAGE(image));
   }
 
   /* Switch signal to new theme */