Created attachment 151626 [details] [review]
improved fix for the issue

test both the event and the hash entry for no modifier; change the name of the flag that indicates the outcome of the test.

I think the current code in 2.19 already fixes this issue.

that would be nice, but not true. 

gdk_keymap_map_virtual_modifiers() returns true if the hash entry had no virtual modifiers present, and false otherwise. 

the key test (which if passed, allows a comparison based on keyval) is:

 if (gdk_keymap_map_virtual_modifiers (key_hash->keymap, &modifiers) &&
	      ((modifiers & ~consumed_modifiers & mask & ~vmods) == (state & ~consumed_modifiers & mask & ~vmods) ||
	       (modifiers & ~consumed_modifiers & mask & ~xmods) == (state & ~consumed_modifiers & mask & ~xmods)))

so, we have several possible cases:

1) hash entry has a virtual modifier, key state does not

gdk_keymap_map_virtual_modifiers() will return FALSE; no possible key match will be found. OK.

2) hash entry has no virtual modifiers, key state does

gdk_keymap_map_virtual_modifiers() will return TRUE; the pre-existing conditional will be executed. OK.

3) hash entry has no virtual modifier, key state does

gdk_keymap_map_virtual_modifiers() will return TRUE; the pre-existing conditional will be executed. OK.

4) hash entry has a virtual modifier, key state also has a virtual modifier

gdk_keymap_map_virtual_modifiers() will return FALSE; no possible key match will be found. FAIL

So although adding some generic handling of these virtual modifiers is probably a good thing (15 conditionals to evaluate them though? wow), this doesn't do the basic, simple test: if either the hash entry or the passed in key state have no modifiers and the other has any, then there is no possible match. The code has semi-fixed that, but at the expense of making it impossible to match a hash entry of <meta>-a with a key state of <meta>-s.

-------------------
side note: again, a new function has been added and just left as a stub on non-X11 backends. i'm willing to bet that this would be released in this state.

> 4) hash entry has a virtual modifier, key state also has a virtual modifier
>
> gdk_keymap_map_virtual_modifiers() will return FALSE; no possible key match
> will be found. FAIL

Why ? gdk_keymap_map_virtual_modifiers() is only supposed to return FALSE if there is a conflict between the virtual modifiers.

matthias, i apologize. i misread/missed one word in the comments of gdk_keymap_map_virtual_modifiers(). you are right.

however, that doesn't change the fact that this function doesn't solve the problem that this bug is about. 

if the hash contains entries for "s" and "<meta>-s" AND there are no conflicts between virtual & "real" modifiers, then the test, applied to the "meta" entry will do this:

a) gdk_keymap_map_virtual_modifiers() will return TRUE
b) the virtual modifiers in the entry will have been converted into modN
c) the conditional will remove all virtual modifiers (vmods) in the entry and compare with the passed in modifier state ALSO with virtual modifiers removed. this will now be a no-op, since there should be no such modifiers in the entry anymore, but we'll just gloss over that for now.
d) then it will remove all the "real" modifiers (xmods) in the entry and compare with the passed in modifier state ALSO with the "real" modifiers removed.

so once again, if the passed-in key was "s" and the entry is "<meta>-s", all that has changed in that map_virtual_modifiers() will first convert <meta> to modN. then the same logic will still be applied as before, and that will determine that the entry matches the keyvalue. 

in short: i still no test that if either the passed in state or the entry has no (virtual) modifiers that the other one matches this condition. the same test is implicitly done for ctrl and shift elsewhere.

What map_virtual_modifiers does is that it _adds_ real modifiers corresponding to the virtual ones. So, after that call, your modifier mask from meta-s will have both a virtual (meta) as well as a real (modN) entry. So neither of the comparisons should be a no-op.

after further reflection and a brief chat with matthias on #gtk+, i think the real issue is that in fact it is mod1...mod5 that are the problematic modifiers in the sense that they are actually backend specific (in this case, to X11). GTK should be working with modifiers that correspond to keys, and leave mod1..mod5 as something X11-specific. this is quite an extensive problem to solve, and probably is better left as another bug and/or proposal. 

and, as matthias noted, the immediate problem is that stubbed-in versions of gdk_keymap_map_virtual_modifiers() (as on win32 and osx at least) will cause gtk_key_hash_lookup function to malfunction, because they always return TRUE.

after wrestling with an attempt to implement the two backend GDK functions intended to handle virtual modifiers in the quartz backend, i have reinforced my opinion that vmods have no role in GTK itself. they are a purely X11-based concept, and do not map to anything in the quartz or win32 backends. I feel fairly strongly that this code needs to be moved out of GTK. otherwise some variant of my patch needs to be applied to _gtk_key_hash_lookup() to make vmods no-ops for backends that cannot support the fundamental concept that vmods represent in the X11 backend.

either that or an explanation of what quartz and win32 should be doing with HYPER, SUPER etc. 

notice that one of the modifiers (physical or virtual) as defined by the X11 backend is actually required to map to a non-vmod on quartz, since the NSCommand modifier is a real modifier, not a virtual one. Using one of Mod1..Mod5 for this seems problematic, because it means that keybindings using these modifiers will work on X11 and be "wierd" on Quartz, since one of them will correspond to NSCommand and not some other "less common" key. This is why the quartz backend (as used by Ardour) steals META for this purpose, but it doesn't treat META as a vmod - its a real modifier corresponding to a real key and a real modifier defined by the windowing system.

comments please.

another way to think about this is how the GTK/X11 backend would handle a keyboard that actually had physical keys that generate Super, Hyper etc. Treating them as vmods under these circumstances appears to me to be the wrong thing to do. They are only vmods because most people do not have keyboards that ever generate these modifiers, or if they do, they want them treated as one of Mod1..Mod5 simply because so much other software can't deal with the idea of Super, Hyper or Meta.

ISTM that you're looking at this the wrong way. OSX has no concept of Mod1, Meta, or Super, or any of the others. It knows about NSAlternate, NSCommand, and NSControl. Win32 similarly knows about its key modifier symbols, not GDK's. That GDK's names happen to map to X11's names is a convenience for the X11 programmers but is harmless (for a change!) to the other back ends. 

gdkkeys-quartz.c maps those OSX modifier keys to whatever we want them to be. The present arrangement maps NSCommand to Mod1 and doesn't map NSAlternate to anything. NSControl gets mapped to GDK_CONTROL. This makes sense to me, but one could argue that NSCommand should be mapped to GDK_Control and NSControl to GDK_MODFoo (doesn't really matter which one). I don't think that it really matters; once everything is working right it will all be configurable with an accelerator map and key bindings, and sophisticated users will be able to do whatever they like -- which is as it should be. (A cool addition to GtkOSXApplication would read the key mappings out of System Preferences. Maybe some day.)

The catch seems to be getting everything working right...

I think for this problem the answer is to map NSAlternate and NSCommand to Mod1 and Mod5, and to use gtk_key_add_virtual_modifiers() to make whichever one NSCommand is attached to to also map Meta.

There are of course complications, so step one is to write a test program... unless one of you have one lying around somewhere. ;)

the thing is that i am using a different arrangement that does map NSAlternate to Mod1, and maps NSCommand to Meta. NSCommand cannot reasonably be mapped to Control without huge amounts of confusion all round. This arrangement works fine for us (Ardour), and means that the GDK_CONTROL_MASK continues to mean "the control key". 

i think you're simplifying the issue too: the change to the key hash lookup introduced the idea of virtual modifiers, an idea that makes no sense in an OS X context (and probably not in a Windows one either). so we have code in the heart of GDK right now that is really doing something that is totally X specific. more than this, it also screws up the non-X case.

Well, unfortunately there's lots of code in the heart of Gdk that's totally X11 specific: Selections, for example. 

If Mathias is willing to move his modifier code into X11, great. He doesn't seem to have warmed much to the idea, though.(I suppose one of us could do it for him, but it seems a bit rude.) That means that the X11 and Win32 code has to work around it. 

I may indeed be simplifying. I don't know. That's why I'm going to write some tests and figure out what's really going on before I do anything.

> If Mathias is willing to move his modifier code into X11, great. He doesn't
> seem to have warmed much to the idea, though.(I suppose one of us could do it
> for him, but it seems a bit rude.) That means that the X11 and Win32 code has
> to work around it. 

I have no huge problem with that, except for the detail that e.g. vte has started using the function. We can certainly do it for gtk3

Um, started using which function? add_virtual_mod and/or map_virtual_mod? The one that really causes the trouble, hash_lookup, is supposed to be private, isn't it?
Anyway, add|map_virtual_mod are no-ops on win32 and quartz, so that isn't really a problem. Just document that they don't do anything except on X11.

_gkt_key_hash_lookup is the real problem. If it was declared in a general header but implemented in platform code, then each platform could handle it the way that works best on that platform. It makes a certain amount of sense to do that, since keymaps are already handled that way. I think we could do that pretty easily for both 2 and 3 without causing any ABI issues. 

We should get Tor to chime in before we do anything, just to make sure we don't do something dumb and to get a clean commit that doesn't break anybody's build.

Yes, map_virtual_modifiers is the function I was talking about. The key hash code is not in gdk at all. So ti didn't occur to me that you were talking about that.

Sorry. 

What do you think about making all or part of _gtk_key_hash_lookup per-platform?

The cleanest (and probably most correct) fix would have a new per-platform function in gdk (gdk_key_apply_modifiers, perhaps?) which _gtk_key_hash_lookup could call -- but that would be an API change. 

What I was thinking of when I wrote comment 12 was to have separate gtkkeyhash-platform.c files which implement _gtk_key_hash_lookup. That serves to keep everything private, but might be overdoing it a bit for a single function.

Created attachment 170521 [details] [review]
Map NSAlternateKey to GDK_MOD5_MASK and implement gdk_keymap virtual modifier functions

This patch seems to fix things up pretty well, both in my test program and in Gnucash. Paul, could you test it in Ardour to see if it helps out there?

Accelerator maps and bindings can use either <Mod1> or <Meta> for NSCommandKeyMask and either <Super> or <Mod5> for NSAlternateKeyMask.

Due to the way that gdkkeys-quartz handles Option, using letter accelerators with option requires using the group 1 keysymbol. For example, to use Cmd-Opt-t you'd have to specify the accelerator as <Meta><Super>þ (that's GDK_KEY_thorn); If you want Cmd-Opt-Shift-t, then the Gtk spec will be <Meta><Super>Þ (GDK_KEY_Thorn). That doesn't apply to function keys, which return the same value regardless of shift and option state.

i'll try to test that, but it will take a while - i'm not in a development cycle on OS X right now. 

we totally "fake up" option key handling. the docs i've read strongly suggest that apple really, really want option-<alphanumeric> to be used for character entry, so i hacked up some code that pulls out the un-optioned key when necessary. it works well for us. i'm not sure its advisable for it to be in GTK itself, though it does make <Mod1><actual-key-name> work :)

Apple discourages using Option-alphanumeric, but they don't discourage e.g. Command-Option-alphanumeric for a binding, nor Option-Function (where function includes arrow keys and I think keypad keys). Amazingly Gtk with this patch handles all of those cases with the caveat noted about having to use the keyvalue that's actually produced rather than the base key when Option and Shift are involved.

One thing it doesn't handle is dead keys. That's a separate issue, and I'll look at it sometime soon.

Created attachment 175074 [details]
Test for gdk-quartz key mapping and key-hash retrieval

This is the test program I used to study and then verify the patch. Kristian might find it useful when he gets around to looking at the patch.

To build it, first modify gdk/quartz/gdkevents-quartz.c as follows and rebuild gtk:
--- a/gdk/quartz/gdkevents-quartz.c
+++ b/gdk/quartz/gdkevents-quartz.c
@@ -934,7 +934,7 @@ fill_scroll_event (GdkWindow          *window,
   event->scroll.device = _gdk_display->core_pointer;
 }
 
-static void
+void
 fill_key_event (GdkWindow    *window,
                 GdkEvent     *event,
                 NSEvent      *nsevent,

Then build with:
gcc $CFLAGS -xobjective-c -framework Carbon -framework Cocoa -DGDK_COMPILATION -DGTK_COMPILATION -I.. -I../gdk -I$PREFIX/include -I$PREFIX/include/glib-2.0 -I$PREFIX/lib/glib-2.0/include -I$PREFIX/include/pango-1.0 -I$PREFIX/include/cairo -I$PREFIX/include/gdk-pixbuf-2.0 -I$PREFIX/include/atk-1.0 -L$PREFIX/lib -lglib-2.0 -lgdk-quartz-3.0 -lgdk_pixbuf-2.0 -lgtk-quartz-3.0 -lgobject-2.0  -o testkeymapping testkeymapping.c  ../gtk/gtkkeyhash.c

If you're using a gtk-osx environment (as of course you should be! ;-) ) then be sure to be in a jhbuild shell. Otherwise you'll have to set CFLAGS and PREFIX to match whatever you used to build Gtk.

I should have mentioned that the build above is from the "tests" directory; if you put testkeymapping.c somewhere else you may have to adjust the include paths and path to gtkkeyhash.c

my objection to using the Alternate=>MOD5 mapping is simply the disjunction it creates with X11 in terms of sharing key bindings. Pressing the Alt key on X11 sets GDK_MOD1_MASK in the event state. Since OS X also has Ctrl as well (handled with the obvious GDK_CONTROL_MASK), that means that it is the Command key that is the extra that needs handling. I'd prefer Alt to continue generating MOD1 as it does on X11 and Command to generate something else (eg. META_MASK or MOD5_MASK or whatever).

that said, its all pretty much just one convention over another. Note that I am not discussing which keyval values are used, just modifiers. 

as a side note, i continue to think that this key hash lookup thing is crazy, but that's really not up for discussion, it would appear.

In a perfectly abstracted system, it shouldn't make any difference what's MOD_1 and what's MOD_5... but gtkaccelgroup.c maps the <alt> tag to GDK_MOD1_MASK, and several default bindings in a variety of widgets hard-code it. The problem here is that NSAlternate's primary use is as a modifier for extending the character set, and assigning it to MOD1 means that either all of those bindings are broken or key-handling has to be broken (making the extended characters and dead keys inaccessible)... and the latter is unacceptable to a lot of Mac users. Maybe that's not an issue for Ardour, but I had a lot of complaints from Gnucash users as well as on the gtk-osx list until I started shipping with this patch and the deadkeys patch.

The unfortunate result is that accelerators or bindings involving NSAlternate are a bit funky, and you can't call it <alt> (because that's hard-coded to MOD1, which is NSCommand. In fact, you don't call it anything at all; instead of saying <ctrl><alt>d, you say <ctrl>ð. That's annoying, and not what users expect. Non-Scandinavians will be baffled by a ð showing up as a shortcut on a menu. I need to work on that some more.

NSCommand isn't "extra", it's the most-used modifier (e.g., Cut, Copy, and Paste  map to cmd-x, cmd-c, and cmd-v); control is used less often, mostly for editing (ctrl-a/e for moving to the beginning or end of the line, ctrl-d to delete forward, etc.) or in combination with command. Option is used mostly with arrow keys for line editing (to move forward and back a word), to extend the keyboard when typing text, or in menu context to display a alternate menu entries (Quit turns into Force Quit on a dock menu, for example.)

Key hash lookup is a good thing: It abstracts the names used in accelerator maps and bindings from the hardware masks (ModFoo). It just needs to be finished by removing all of the hard-coded GDK_MOD1_MASK bindings and extended by adding two more virtual masks so that there are the same number as hardware masks. GDK_ALT_MASK could be one of them.

Comment on attachment 170521 [details] [review]
Map NSAlternateKey to GDK_MOD5_MASK and implement gdk_keymap virtual modifier functions

I don't think is makes sense to map the key labeled "Alt" to *anything* but MOD1 because that's what a lot of code out there expects.

IMO the only question here is what to map command to.

Only after we mapped the Mac modifiers to sane stuff we can start talking about how to change GTK+ accelerators.

To be a devil's advocate for a moment, the issue is that Alt-<foo> has a fairly different semantic on the Mac than it does on X Window systems, and that difference appears to have propagated quite well up into user knowledge. So although from a development perspective there are good reasons to want Alt == MOD1 everywhere, I think that these reasons somewhat hide the deeper issue which is that applications shouldn't be using MOD1 as if it had particular semantics.

What I have the biggest problem with is the notion of meta-modifiers unless its the only type of modifier there is. Keys should either generate literal modifiers (e.g. GDK_CTRL_MASK, GDK_SHIFT_MASK) or meta-modifiers (e.g. GDK_META_MASK, GDK_COMMAND_MASK), but not both. Having them generate both creates headaches for application developers, and for GTK internals that have to decide which "level" of modifiers (literal or meta) to use for what purpose. Applications that actually need to know about key presses can track press/release events quite easily. I personally prefer a meta-ized modifier design, so that x-platform development is easier, which is why Ardour uses Primary/Secondary/TertiaryModifier rather than MOD1, META or anything like that. At some point though, the association from one to the other has to be made.

I'd also note that changing these things around is pretty hairy for application developers. We've already caused bugs in a release as a result of using two different GTK stacks, one which has John's preferred key<=>modifier bindings and one that has mine. Existing user keybinding files end up not working anymore, and there's no way to identify how to map from the old to the new (or vice versa).

To amplify Paul's "playing devil's advocate", the semantics of the option key aren't fairly different, they're completely different in text-entry context. In that context the option key is a character modifier and is *not available as a shortcut modifier except in combination with command or control*. It is precisely *because* MOD1 is hard-coded in so many places, including in Gtk itself, that MOD1 cannot be mapped to option without breaking the character modifier function. Users outside the US and UK depend on the Mac keyboard semantics to be able to type in both their own languages and in english and consider hijacking the option key for shortcuts to be a significant bug.

As for virtual vs. physical modifiers, I agree that only virtual modifiers should be exposed to developers. I think the current architecture arises from too closely copying the X11 system, but compared to some other places (like selections) where it makes porting very difficult it's a minor matter here.

(In reply to comment #24)
> (From update of attachment 170521 [details] [review])
> I don't think is makes sense to map the key labeled "Alt" to *anything* but
> MOD1 because that's what a lot of code out there expects.
> 
> IMO the only question here is what to map command to.
> 
> Only after we mapped the Mac modifiers to sane stuff we can start talking about
> how to change GTK+ accelerators.

Sorry, that argument is absurd on its face. Code has no way of telling what key produces the MOD1 bit.

Created attachment 196282 [details] [review]
Updated NSAlternateKey to GDK_MOD5_MASK to apply to master of 11Sep2011

Comment on attachment 151507 [details] [review]
fix handling modifiers in _gtk_key_hash_lookup

There is an improved patch attached

I just attached a patch to bug #605799 that makes both modified and
virtual-unmodified acceletors of the same key work without any change
to gtkkeyhash.c

I think all the issues here have either been fixed in the meantime, or are
handled in more specific bugs. I'm resolving this one as obsolete now.