Merge lp:~attente/unity-gtk-module/gtk-enable-mnemonics into lp:unity-gtk-module/14.04

[William Hua]

Filter out mnemonics when the gtk-enable-mnemonics setting is cleared. Workaround for LP: #1282782.

Filter out mnemonics when the gtk-enable-mnemonics setting is cleared. Workaround for LP: #1282782.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:315
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/23/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/9
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/9/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/23/rebuild

review: Approve (continuous-integration)

[Allison Karlitskaya (desrt)]

trying to dynamically handle the settings changing at runtime is maybe a bit insane.... but assuming that indeed you do want to do this:

unity_gtk_menu_shell_handle_item_notify() only uses the pspec to get the name -- why not just modify this function to take the name directly, removing the need to lookup the pspec...

keeping a gtksettings per menuitem is definitely overkill -- it's exceptionally unlikely that we will ever see more than one gtksettings per program. just use the global one. cache it in a static if you really don't trust that it will never change...

since you will just use the global gtksettings, no need to remove/readd the signal handler every time. also: don't bother with handler ids: they don't really make much of an improvement in performance. just disconnect by funcs/data.

finally: you shouldn't try to do this for every single menu item: only for toplevel ones in menubars.

one unrelated note: your underscore removal algorithm's attempt to be correct on non-ascii utf8 strings is not really necessary. all non-ascii utf8 sequences are composed entirely of non-ascii characters (ie: every byte is > 0x80). it is therefore impossible for '_' to appear in the middle of the sequence. just do the naive thing here and it will be correct.

review: Needs Fixing

[Allison Karlitskaya (desrt)]

about the strdup: the strlen() + 1 on sized new is not necessary with gstring -- it adds the nul internally. maybe skip GString here entirely since you know that the result string will definitely be smaller than the input.

also: with respect to my previous comments, strchr() is your friend.

[lp:~attente/unity-gtk-module/gtk-enable-mnemonics]

316. By William Hua on 2014-02-24

Pass property name directly.

317. By William Hua on 2014-02-24

Simplify mnemonic removal.

318. By William Hua on 2014-02-25

Only honour gtk-enable-mnemonics on top-level shells.

[William Hua (attente)]

I added the signal handler connection/disconnection in the unity_gtk_menu_shell_set_menu_shell () function because sometimes the GtkMenuShell can be set to NULL, and it makes sense to no longer watch once that happens. If you're worried about it being connected/disconnected more than once, in practice that function is never called with a non-NULL GtkMenuShell except at initialization.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:318
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/24/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/10/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/24/rebuild

review: Approve (continuous-integration)

[Allison Karlitskaya (desrt)]

This is better, but still not exactly what I had in mind. I wouldn't block on further review bickering at this point, though.

What I was thinking of (and did a relative poor job of explaining) is one global default GtkSettings watch (using gtk_settings_get_default) for the entire thing. When it fires, then you can iterate the toplevel items and make the tweak.

[lp:~attente/unity-gtk-module/gtk-enable-mnemonics]

319. By William Hua on 2014-02-26

Use only one signal handler for notify::gtk-enable-mnemonics.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:319
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/25/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/11
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/11/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/25/rebuild

review: Approve (continuous-integration)

[lp:~attente/unity-gtk-module/gtk-enable-mnemonics]

320. By William Hua on 2014-02-27

Reset the label in the label handler.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:320
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/26/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/12/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/26/rebuild

review: Approve (continuous-integration)

[Allison Karlitskaya (desrt)]

Keeping the global list is really strange -- particularly considering that in practice there will only ever be exactly one menubar. It would have been better to just hook directly up to each (of the one and only) toplevel shell instead of bothering with all of this register stuff...

This is totally fine as-is, though...

[lp:~attente/unity-gtk-module/gtk-enable-mnemonics]

321. By William Hua on 2014-03-05

Don't use a global linked list.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:321
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/29/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/15/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/29/rebuild

review: Approve (continuous-integration)

[Allison Karlitskaya (desrt)]

Just one more nag: this check will never fail:

+ if (gtk_enable_mnemonics_name == NULL)
282 + gtk_enable_mnemonics_name = g_intern_static_string ("gtk-enable-mnemonics");
283 +
284 + if (g_param_spec_get_name (pspec) == gtk_enable_mnemonics_name)
285 + {

because you already have a detailed signal connection.

[lp:~attente/unity-gtk-module/gtk-enable-mnemonics]

322. By William Hua on 2014-03-05

Don't check pspec name in detailed signal handler.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:322
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/30/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/16/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-gtk-module-ci/30/rebuild

review: Approve (continuous-integration)

[Sebastien Bacher (seb128)]

Seems like that's ready to land, thanks guys for the review/iterations!

review: Approve

=== modified file 'lib/unity-gtk-menu-item-private.h'
--- lib/unity-gtk-menu-item-private.h	2013-11-28 18:29:27 +0000
+++ lib/unity-gtk-menu-item-private.h	2014-03-05 17:18:00 +0000
@@ -53,6 +53,7 @@
   guchar             child_shell_valid : 1;
   guint              item_index;
   UnityGtkAction    *action;
+  gchar             *label;
 };
 
 GType               unity_gtk_menu_item_get_type          (void)                            G_GNUC_INTERNAL;
 
=== modified file 'lib/unity-gtk-menu-item.c'
--- lib/unity-gtk-menu-item.c	2013-11-28 18:29:27 +0000
+++ lib/unity-gtk-menu-item.c	2014-03-05 17:18:00 +0000
@@ -19,6 +19,7 @@
 
 #include "unity-gtk-menu-item-private.h"
 #include "unity-gtk-action-group-private.h"
+#include <string.h>
 
 G_DEFINE_TYPE (UnityGtkMenuItem,
                unity_gtk_menu_item,
@@ -307,7 +308,7 @@
   g_return_if_fail (parent_shell != NULL);
   g_warn_if_fail (object == menu_item);
 
-  unity_gtk_menu_shell_handle_item_notify (parent_shell, item, pspec);
+  unity_gtk_menu_shell_handle_item_notify (parent_shell, item, g_param_spec_get_name (pspec));
 }
 
 static void
@@ -368,11 +369,27 @@
 }
 
 static void
+unity_gtk_menu_item_finalize (GObject *object)
+{
+  UnityGtkMenuItem *item;
+
+  g_return_if_fail (UNITY_GTK_IS_MENU_ITEM (object));
+
+  item = UNITY_GTK_MENU_ITEM (object);
+
+  g_free (item->label);
+  item->label = NULL;
+
+  G_OBJECT_CLASS (unity_gtk_menu_item_parent_class)->finalize (object);
+}
+
+static void
 unity_gtk_menu_item_class_init (UnityGtkMenuItemClass *klass)
 {
   GObjectClass *object_class = G_OBJECT_CLASS (klass);
 
   object_class->dispose = unity_gtk_menu_item_dispose;
+  object_class->finalize = unity_gtk_menu_item_finalize;
 }
 
 static void
@@ -416,7 +433,7 @@
           GtkWidget *submenu = gtk_menu_item_get_submenu (menu_item);
 
           if (submenu != NULL)
-            item->child_shell = unity_gtk_menu_shell_new (GTK_MENU_SHELL (submenu));
+            item->child_shell = unity_gtk_menu_shell_new_internal (GTK_MENU_SHELL (submenu));
         }
 
       item->child_shell_valid = TRUE;
@@ -461,36 +478,92 @@
     }
 }
 
+static gchar *
+g_strdup_no_mnemonics (const gchar *str)
+{
+  if (str != NULL)
+    {
+      gchar *string;
+      gchar *out;
+      const gchar *in;
+      gboolean underscore;
+
+      string = g_malloc (strlen (str) + 1);
+      out = string;
+      underscore = FALSE;
+
+      for (in = str; *in != '\0'; in++)
+        {
+          if (*in != '_')
+            {
+              underscore = FALSE;
+              *out++ = *in;
+            }
+          else
+            {
+              if (!underscore)
+                underscore = TRUE;
+              else
+                {
+                  /* double underscores are not accelerator markers */
+                  underscore = FALSE;
+                  *out++ = '_';
+                  *out++ = '_';
+                }
+            }
+        }
+
+      /* trailing underscores are not accelerator markers */
+      if (underscore)
+        *out++ = '_';
+
+      *out++ = '\0';
+
+      return string;
+    }
+
+  return NULL;
+}
+
 const gchar *
 unity_gtk_menu_item_get_label (UnityGtkMenuItem *item)
 {
-  const gchar *label;
-
   g_return_val_if_fail (UNITY_GTK_IS_MENU_ITEM (item), NULL);
   g_return_val_if_fail (item->menu_item != NULL, NULL);
 
-  label = gtk_menu_item_get_label (item->menu_item);
-
-  if (label != NULL && label[0] != '\0')
+  if (item->label == NULL)
     {
-      if (GTK_IS_IMAGE_MENU_ITEM (item->menu_item))
+      const gchar *label = gtk_menu_item_get_label (item->menu_item);
+
+      if (label != NULL && label[0] != '\0')
         {
-          GtkImageMenuItem *image_menu_item = GTK_IMAGE_MENU_ITEM (item->menu_item);
-
-          if (gtk_image_menu_item_get_use_stock (image_menu_item))
+          if (GTK_IS_IMAGE_MENU_ITEM (item->menu_item))
             {
-              GtkStockItem stock_item;
-
-              if (gtk_stock_lookup (label, &stock_item))
-                label = stock_item.label;
+              GtkImageMenuItem *image_menu_item = GTK_IMAGE_MENU_ITEM (item->menu_item);
+
+              if (gtk_image_menu_item_get_use_stock (image_menu_item))
+                {
+                  GtkStockItem stock_item;
+
+                  if (gtk_stock_lookup (label, &stock_item))
+                    label = stock_item.label;
+                }
             }
         }
+
+      if (label == NULL || label[0] == '\0')
+        label = gtk_menu_item_get_nth_label (item->menu_item, 0);
+
+      if (label != NULL && label[0] != '\0')
+        {
+          if (item->parent_shell == NULL || item->parent_shell->has_mnemonics)
+            item->label = g_strdup (label);
+          else
+            item->label = g_strdup_no_mnemonics (label);
+        }
     }
 
-  if (label == NULL || label[0] == '\0')
-    label = gtk_menu_item_get_nth_label (item->menu_item, 0);
-
-  return label != NULL && label[0] != '\0' ? label : NULL;
+  return item->label;
 }
 
 GIcon *
 
=== modified file 'lib/unity-gtk-menu-shell-private.h'
--- lib/unity-gtk-menu-shell-private.h	2013-11-28 18:29:27 +0000
+++ lib/unity-gtk-menu-shell-private.h	2014-03-05 17:18:00 +0000
@@ -26,24 +26,26 @@
 
 G_BEGIN_DECLS
 
-UnityGtkMenuItem * unity_gtk_menu_shell_get_item              (UnityGtkMenuShell *shell,
-                                                               guint              index)  G_GNUC_INTERNAL;
-
-GSequence *        unity_gtk_menu_shell_get_visible_indices   (UnityGtkMenuShell *shell)  G_GNUC_INTERNAL;
-
-GSequence *        unity_gtk_menu_shell_get_separator_indices (UnityGtkMenuShell *shell)  G_GNUC_INTERNAL;
-
-void               unity_gtk_menu_shell_handle_item_notify    (UnityGtkMenuShell *shell,
-                                                               UnityGtkMenuItem  *item,
-                                                               GParamSpec        *pspec)  G_GNUC_INTERNAL;
-
-void               unity_gtk_menu_shell_activate_item         (UnityGtkMenuShell *shell,
-                                                               UnityGtkMenuItem  *item)   G_GNUC_INTERNAL;
-
-void               unity_gtk_menu_shell_print                 (UnityGtkMenuShell *shell,
-                                                               guint              indent) G_GNUC_INTERNAL;
-
-gboolean           unity_gtk_menu_shell_is_debug              (void)                      G_GNUC_INTERNAL;
+UnityGtkMenuShell * unity_gtk_menu_shell_new_internal          (GtkMenuShell      *menu_shell) G_GNUC_INTERNAL;
+
+UnityGtkMenuItem *  unity_gtk_menu_shell_get_item              (UnityGtkMenuShell *shell,
+                                                                guint              index)      G_GNUC_INTERNAL;
+
+GSequence *         unity_gtk_menu_shell_get_visible_indices   (UnityGtkMenuShell *shell)      G_GNUC_INTERNAL;
+
+GSequence *         unity_gtk_menu_shell_get_separator_indices (UnityGtkMenuShell *shell)      G_GNUC_INTERNAL;
+
+void                unity_gtk_menu_shell_handle_item_notify    (UnityGtkMenuShell *shell,
+                                                                UnityGtkMenuItem  *item,
+                                                                const gchar       *property)   G_GNUC_INTERNAL;
+
+void                unity_gtk_menu_shell_activate_item         (UnityGtkMenuShell *shell,
+                                                                UnityGtkMenuItem  *item)       G_GNUC_INTERNAL;
+
+void                unity_gtk_menu_shell_print                 (UnityGtkMenuShell *shell,
+                                                                guint              indent)     G_GNUC_INTERNAL;
+
+gboolean            unity_gtk_menu_shell_is_debug              (void)                          G_GNUC_INTERNAL;
 
 G_END_DECLS
 
 
=== modified file 'lib/unity-gtk-menu-shell.c'
--- lib/unity-gtk-menu-shell.c	2014-01-13 15:41:42 +0000
+++ lib/unity-gtk-menu-shell.c	2014-03-05 17:18:00 +0000
@@ -435,6 +435,13 @@
 unity_gtk_menu_shell_handle_item_label (UnityGtkMenuShell *shell,
                                         UnityGtkMenuItem  *item)
 {
+  g_return_if_fail (UNITY_GTK_IS_MENU_SHELL (shell));
+  g_return_if_fail (UNITY_GTK_IS_MENU_ITEM (item));
+  g_warn_if_fail (item->parent_shell == shell);
+
+  g_free (item->label);
+  item->label = NULL;
+
   unity_gtk_menu_shell_update_item (shell, item);
 }
 
@@ -654,6 +661,41 @@
     }
 }
 
+static void
+unity_gtk_menu_shell_set_has_mnemonics (UnityGtkMenuShell *shell,
+                                        gboolean           has_mnemonics)
+{
+  g_return_if_fail (UNITY_GTK_IS_MENU_SHELL (shell));
+
+  if (has_mnemonics != shell->has_mnemonics)
+    {
+      shell->has_mnemonics = has_mnemonics;
+
+      if (shell->items != NULL)
+        {
+          guint i;
+
+          for (i = 0; i < shell->items->len; i++)
+            unity_gtk_menu_shell_handle_item_label (shell, g_ptr_array_index (shell->items, i));
+        }
+    }
+}
+
+static void
+unity_gtk_menu_shell_handle_settings_notify (GObject    *object,
+                                             GParamSpec *pspec,
+                                             gpointer    user_data)
+{
+  gboolean has_mnemonics;
+
+  g_return_if_fail (GTK_IS_SETTINGS (object));
+  g_return_if_fail (UNITY_GTK_IS_MENU_SHELL (user_data));
+
+  g_object_get (GTK_SETTINGS (object), "gtk-enable-mnemonics", &has_mnemonics, NULL);
+
+  unity_gtk_menu_shell_set_has_mnemonics (UNITY_GTK_MENU_SHELL (user_data), has_mnemonics);
+}
+
 static void unity_gtk_menu_shell_clear_menu_shell (UnityGtkMenuShell *shell);
 
 static void
@@ -731,13 +773,18 @@
 unity_gtk_menu_shell_dispose (GObject *object)
 {
   UnityGtkMenuShell *shell;
+  GtkSettings *settings;
 
   g_return_if_fail (UNITY_GTK_IS_MENU_SHELL (object));
 
   shell = UNITY_GTK_MENU_SHELL (object);
+  settings = gtk_settings_get_default ();
 
   unity_gtk_menu_shell_set_menu_shell (shell, NULL);
 
+  if (settings != NULL)
+    g_signal_handlers_disconnect_by_func (settings, unity_gtk_menu_shell_handle_settings_notify, shell);
+
   G_OBJECT_CLASS (unity_gtk_menu_shell_parent_class)->dispose (object);
 }
 
@@ -806,6 +853,7 @@
 static void
 unity_gtk_menu_shell_init (UnityGtkMenuShell *self)
 {
+  self->has_mnemonics = TRUE;
 }
 
 /**
@@ -822,6 +870,23 @@
 unity_gtk_menu_shell_new (GtkMenuShell *menu_shell)
 {
   UnityGtkMenuShell *shell = g_object_new (UNITY_GTK_TYPE_MENU_SHELL, NULL);
+  GtkSettings *settings = gtk_settings_get_default ();
+
+  if (settings != NULL)
+    {
+      g_signal_connect (settings, "notify::gtk-enable-mnemonics", G_CALLBACK (unity_gtk_menu_shell_handle_settings_notify), shell);
+      g_object_get (settings, "gtk-enable-mnemonics", &shell->has_mnemonics, NULL);
+    }
+
+  unity_gtk_menu_shell_set_menu_shell (shell, menu_shell);
+
+  return shell;
+}
+
+UnityGtkMenuShell *
+unity_gtk_menu_shell_new_internal (GtkMenuShell *menu_shell)
+{
+  UnityGtkMenuShell *shell = g_object_new (UNITY_GTK_TYPE_MENU_SHELL, NULL);
 
   unity_gtk_menu_shell_set_menu_shell (shell, menu_shell);
 
@@ -899,7 +964,7 @@
 void
 unity_gtk_menu_shell_handle_item_notify (UnityGtkMenuShell *shell,
                                          UnityGtkMenuItem  *item,
-                                         GParamSpec        *pspec)
+                                         const gchar       *property)
 {
   static const gchar *visible_name;
   static const gchar *sensitive_name;
@@ -909,7 +974,7 @@
   static const gchar *parent_name;
   static const gchar *submenu_name;
 
-  const gchar *pspec_name;
+  const gchar *name;
 
   g_return_if_fail (UNITY_GTK_IS_MENU_SHELL (shell));
   g_return_if_fail (UNITY_GTK_IS_MENU_ITEM (item));
@@ -929,24 +994,24 @@
   if (submenu_name == NULL)
     submenu_name = g_intern_static_string ("submenu");
 
-  pspec_name = g_param_spec_get_name (pspec);
+  name = g_intern_string (property);
 
   if (unity_gtk_menu_shell_is_debug ())
-    g_print ("%s ((%s *) %p, (%s *) %p { \"%s\" }, %s)\n", G_STRFUNC, G_OBJECT_TYPE_NAME (shell), shell, G_OBJECT_TYPE_NAME (item), item, unity_gtk_menu_item_get_label (item), pspec_name);
+    g_print ("%s ((%s *) %p, (%s *) %p { \"%s\" }, %s)\n", G_STRFUNC, G_OBJECT_TYPE_NAME (shell), shell, G_OBJECT_TYPE_NAME (item), item, unity_gtk_menu_item_get_label (item), name);
 
-  if (pspec_name == visible_name)
+  if (name == visible_name)
     unity_gtk_menu_shell_handle_item_visible (shell, item);
-  else if (pspec_name == sensitive_name)
+  else if (name == sensitive_name)
     unity_gtk_menu_shell_handle_item_sensitive (shell, item);
-  else if (pspec_name == label_name)
+  else if (name == label_name)
     unity_gtk_menu_shell_handle_item_label (shell, item);
-  else if (pspec_name == accel_path_name)
+  else if (name == accel_path_name)
     unity_gtk_menu_shell_handle_item_accel_path (shell, item);
-  else if (pspec_name == active_name)
+  else if (name == active_name)
     unity_gtk_menu_shell_handle_item_active (shell, item);
-  else if (pspec_name == parent_name)
+  else if (name == parent_name)
     unity_gtk_menu_shell_handle_item_parent (shell, item);
-  else if (pspec_name == submenu_name)
+  else if (name == submenu_name)
     unity_gtk_menu_shell_handle_item_submenu (shell, item);
 }
 
 
=== modified file 'lib/unity-gtk-menu-shell.h'
--- lib/unity-gtk-menu-shell.h	2013-02-19 11:55:57 +0000
+++ lib/unity-gtk-menu-shell.h	2014-03-05 17:18:00 +0000
@@ -52,6 +52,7 @@
   /*< private >*/
   GtkMenuShell        *menu_shell;
   gulong               menu_shell_insert_handler_id;
+  gboolean             has_mnemonics;
   GPtrArray           *items;
   GPtrArray           *sections;
   GSequence           *visible_indices;