Created attachment 104916 [details] [review]
Patch fixing the issue

brrr, i unfortunately see a nasty issue with this:

+      accel_groups = gtk_accel_groups_from_object (G_OBJECT (toplevel));
+      for (list = accel_groups; list; list = list->next)
+        {
+          GtkAccelGroup *accel_group = list->data;
+          if (gtk_accel_group_query (accel_group, accel_key, accel_mods, NULL))

here, the accel group sorting doesn't take the gtkkeyhash lookups into account that GtkWindow does, so we can miss out on accelerators.

+                {
+                  gtk_menu_shell_cancel (GTK_MENU_SHELL (widget));
+                  gtk_window_activate_key (GTK_WINDOW (toplevel), event);

and alternative would be to collect the involved GtkWindows from the list of accel_groups and then just call gtk_window_activate_key() on those until one returns TRUE. but that'd also invoke non-accel keys like mnemonics, so needs to be worked around by seperating accel activation into a new function gtk_window_activate_accelerator(). that's unless Owen has another idea regarding the key hashes. putting him on CC.

What about simply making gtk_window_get_key_hash() internal-public
and look up the key event ourselves, then call gtk_window_activate_key()
if a non-mnemonic match was returned?

Besides: why does gtk_window_activate_key() not simply use
gtk_window_get_key_hash()? Its way to get to the hash looks awfully
complicated.

- I think the complex way of getting the key hash got there because of an add-revert cycle that reverted a bit to much, just calling get_key_hash() should be fine.

- Making get_key_hash() internally exported seems OK to me

Created attachment 105248 [details] [review]
Updated patch

I just figured that accessing the window's key hash won't help a bit
since that would require the GtkWindowKeyEntry struct to be internally
public too (which would be really ugly).

Instead, I added _gtk_window_has_accel() and _gtk_window_activate_accel()
and factored out the common code they need to a static utility function
gtk_window_get_key_entry().

Comment on attachment 105248 [details] [review]
Updated patch

>+++ gtk/gtkwindow.h	(working copy)
>@@ -425,6 +425,11 @@ void _gtk_window_keys_foreach (GtkWindow
> 			       GtkWindowKeysForeachFunc func,
> 			       gpointer                 func_data);
> 
>+gboolean        _gtk_window_has_accel           (GtkWindow      *window,
>+                                                 GdkEventKey    *event);
>+gboolean        _gtk_window_activate_accel      (GtkWindow      *window,
>+                                                 GdkEventKey    *event);

i don't quite like the idea of introducing window_has_accel(). it may give the impression that you can query whether a specific accel will be consumed by accel activation or not. that is not the case, accelerators can be hooked up into an accel group but have handlers that simply return FALSE upon activation.

>+++ gtk/gtkmenu.c	(working copy)
>@@ -2883,7 +2883,18 @@ gtk_menu_key_press (GtkWidget	*widget,
> 	    }
> 	}
>     }
>-  
>+  else if (!can_change_accels)
>+    {
>+      GtkWidget *toplevel = gtk_menu_get_toplevel (widget);
>+
>+      if (GTK_IS_WINDOW (toplevel) &&
>+          _gtk_window_has_accel (GTK_WINDOW (toplevel), event))
>+        {
>+          gtk_menu_shell_cancel (GTK_MENU_SHELL (widget));
>+          _gtk_window_activate_accel (GTK_WINDOW (toplevel), event);

with the problem outlined above, this code can lead to:
- Ctrl+A: pop down the menu, and perform an associated accel action.
- Ctrl+B: simply being ignored, because it's not used in any accel group.
- Ctrl+C: pop down the menu, nothing else happens. this is possible if Ctrl+C is part of an accel group but has a currently disabled accel handler.

the discrepancy between Ctrl+B and Ctrl+C is likely to be confusing for the user.

>+        }
>+    }
>+
>   return TRUE;
> }

The discrepancy between Ctrl+B and Ctrl+C in your example might be
confusing, but is it more confusing that just doing nothing in the
common case?

Do you see any way around this problem?

We're moving to gitlab! As part of this move, we are closing bugs that haven't seen activity in more than 5 years. If this issue is still imporant to you and
still relevant with GTK+ 3.22 or master, please consider creating a gitlab issue
for it.