GNOME Bugzilla – Bug 663856
Make option-foo accelerators use the right symbol
Last modified: 2011-12-12 10:02:33 UTC
Created attachment 201224 [details] [review] The patch Attached patch does two things: Fix quartz' gdk_keymap_translate_keyboard_state() to return the right consumed_modifiers, just as on other platforms. Fix GTK+ accelerator handling for the case that the group-toggling modifier is in the set of default accel_mods. In this case we do not want to display the symbol from group 1, but the symbol from group 0 (which is printed on the keyboard). For example we want to say "Command-Option-O" and not "Command-Option-Ø". On X11/Win32 this never happens, so the code was simply not handling this case. If there is such an intersection, we simply remove the relevant modifier from "state" before calling translate_keyboard_state() so we get the symbol from group 0, and add it back afterwards so we can use it for accel matching.
Created attachment 201228 [details] [review] Same patch without debug spew
Review of attachment 201228 [details] [review]: ::: gtk/gtkcellrendereraccel.c @@ +471,2 @@ accel_mods |= GDK_SHIFT_MASK; One concern I have is that we look at whether the keyval changed as an indication that we need to put the shift mask back here. Can't stripping the 'group switch' modifier change the keyval too ? And won't that trigger adding the shift mask ? ::: gtk/gtkkeyhash.c @@ +407,3 @@ + * mask, and it is active, disable it for matching + */ + if (mask &state & GTK_TOGGLE_GROUP_MOD_MASK) missing space betweeen & and state ::: gtk/gtkprivate.h @@ +137,3 @@ +#define GTK_TOGGLE_GROUP_MOD_MASK 0 +#else +#define GTK_TOGGLE_GROUP_MOD_MASK GDK_MOD1_MASK 'toggle group' sounds a little odd to me. how about 'switch group' or 'group switch' ?
It does change the resulting keyval, that's the whole purpose of the patch, we want "O" instead of "Ø". I don't have an opinion about the #define, but we must get its gtk3 enum GdkModifierIntent value right.
(In reply to comment #3) > It does change the resulting keyval, that's the whole purpose of the > patch, we want "O" instead of "Ø". Right. Don't you get 'Shift' added to the accel_mods then ?
Understood what you mean now. But we compare before and after tolower(), not before and after translate_keyboard_state() when we add SHIFT back.
Well, it gets <alt>foo accelerators working again, but they work whether or not the option key is down, and I don't think that's what you want. (It's certainly not what I want, given the number of <alt>foo accelerators hard-coded into applications.) As for what's displayed in the menu, I don't see any difference: It looks right both with and without the patch. Perhaps I'm looking at the wrong thing there?
I don't observe that. With the patch, setting, displaying and invoking of Option-foo and Command-Option-foo accelerators work fine, without they are broken. Note that I don't use any global-menu code here, but when I enable the old ige-mac-menu code in GIMP, everything works fine too. I don't know about the new global menu code. Anyway, the objective here is to fix GTK+, and things using GTK+ maybe need to be fixed/updates.
Ah indeed, they work also without Option pressed, that is clearly broken, will look into fixing this. I had the unmodified versions of my test shortcuts bound to actions too, so these were of course preferred.
Created attachment 201438 [details] [review] Correctly match the result of the translation There we go. We must not only change the call to translate_keyboard_state(), we also need to adjust using its result, by explicitly matching for the group modifier in case it is also an accel modifier. I really don't like this patch, it got ugly. I will wrap the magic aroung translate_keyboard_state() into a helper function, but the rest doesn't feel nice either. Anyone with an easier solution?
latest patch tested and confirmed working here (in the sense that Alt-<key> and <key> would activate different actions).
Created attachment 201481 [details] [review] Factor out stuff to a helper function, and port GtkMenu too
Well, it works. I agree that it's a wart, but the only other approach I can think of is to make gdk_keymap_translate_keyboard_state somehow aware of whether it's being queried for text entry. I think Matthias said that that should happen via im-context-simple, but I haven't looked at that yet. I don't see any difference with and without the GtkMenu addition. What should I be looking for?
GtkMenu's accelerator assigning feature, it doesn't work on the mac global menu anyway, but it felt right to port it too.
Fixed in 2-24, 3-2 and master (with proper API).
I've updated my Fedora 16 and the Alt+key doesn't work anymore in GNOME terminal. No more Alt+d shortcut to remove next word or irssi shortcuts (but it works fine with xterm or in console). In the package list of the update, there is: gtk3-immodule-xim-3.2.2-2.fc16.x86_64 and I think it's related to this bug report.