Created attachment 232142 [details] [review]
x11: Ignore num lock / scroll lock for event state

This will do nothing but confuse.

it seems that GDK is not doing this either; is there any particular reason why this should happen, except "do nothing but confuse"? why is it that Metacity/Mutter requires this code?

the patch ooks good to me otherwise, but I'd rather have a detailed rationale in the commit message.

Hmm, do you actually have Scroll Lock mapped to a modifier? It will set a virtual modifier, but you ideally want to ignore those and only look at the core modifiers, since there's a bunch of really esoteric virtual modifiers.  You'll still need to ignore Num Lock from the real modifiers though; doing so is entirely sensible.

(In reply to comment #2)
> it seems that GDK is not doing this either; is there any particular reason why
> this should happen, except "do nothing but confuse"? why is it that
> Metacity/Mutter requires this code?

I assume that GDK doesn't do it out of legacy concerns, but I'm unsure.

The answer is that the state of Num Lock and Scroll Lock shouldn't affect code like this:

    if (clutter_event_get_state (event) == CLUTTER_CONTROL_MASK)
      /* do a thing */
    else
      /* do another thing */

(In reply to comment #4)
> Hmm, do you actually have Scroll Lock mapped to a modifier? It will set a
> virtual modifier, but you ideally want to ignore those and only look at the
> core modifiers, since there's a bunch of really esoteric virtual modifiers. 
> You'll still need to ignore Num Lock from the real modifiers though; doing so
> is entirely sensible.

I don't know. All I know is that mutter does this, and apparently has done this since the keybindings code was introduced in 2002:

http://git.gnome.org/browse/mutter/commit/?id=e4e200a1dc6ee6e459e5c68916669a48eda33d20

If we don't need it, we don't need it.

Created attachment 232922 [details] [review]
x11: Ignore num lock / scroll lock for event state

As x11 considers num lock and scroll lock to be modifiers, code that
checks for an exact modifier combination will fail if naively done when
num lock or scroll lock are turned on. Applications that want to ignore
these modifiers will need to use XKB to manually mask out the modifier
state.

As it is very unlikely that applications will want to care about the
state of num lock or scroll lock for key press/key release events, mask
out the num lock and scroll lock keys automatically.

Review of attachment 232922 [details] [review]:

looks good. it would be nice to have a notice in the "Release notes" section of the README.in file that notes that on X11 we mask out these locks from the modifiers starting from the 1.14 release.

Attachment 232922 [details] pushed as 4691878 - x11: Ignore num lock / scroll lock for event state


I've done that, thanks!