Bug 56866

Created attachment 69691 [details]
test case

Your test case only looks at the first level of the first layout of each key, while I assume XCB looks at all of them.

The xkb_state type represents the state of a keyboard at a given point in time (i.e. after keys were pressed and released, layouts were switched, etc.). So when you use xkb_state_key_get_syms(), you are asking to get the correct keysym for the current active layout and modifiers in the state that you passed it.

What your function really wants to do is independent of a keyboard state, it just wants to enumerate all of the keysyms the can ever be generated by a given keycode (don't know why you'd want that, though). So what you want is to only look at the keymap. For this, you should do something like this:

num_layouts = xkb_keymap_num_layouts_for_key(...)
for each layout:
    num_levels = xkb_keymap_num_levels_for_key(...)
    for each level:
       num_syms = xkb_keymap_key_get_syms_by_level(..., &syms)
       if num_syms == 1
           add the keysym syms[0]

Note that in xkbcommon, each key press (or more accurately a single shift level) can produce multiple keysyms, like pressing <A> and getting 3 keysyms F O O. X doesn't do that, and you probably want to ignore those for now (hence the num_syms == 1).

Try this and see if you get what you expect.

Sorry for late reply,

I realized what I actually need is a function that looks at all shift levels for a given key (similar to XkbKeyGroupWidth) and i need to do this only for the current base(active) layout. This is used in an algorithm to set modifier masks.

Later I listen for a state changes and update modifiers masks when the layout changes.

Today i encountered a crash in:
XkbKeyGroupWidth(const struct xkb_key *key, xkb_layout_index_t layout)

Crash happens only for keys which don't have a keysyms, in my case it crashes when i pass a key with code 93 to xkb_keymap_num_levels_for_key()

You can see how its used in (qxcbkeyboard.cpp):
https://codereview.qt-project.org/#change,39226

Output from xmodmop -pke:

keycode  92 = ISO_Level3_Shift NoSymbol ISO_Level3_Shift NoSymbol ISO_Level3_Shift ISO_Level3_Shift
keycode  93 =
keycode  94 = less greater backslash bar bar brokenbar bar brokenbar less greater onehalf threequarters
keycode  95 = F11 F11 F11 F11 F11 F11 XF86Switch_VT_11 F11 F11 XF86Switch_VT_11 F11 F11 F11 F11 XF86Switch_VT_11 F11 F11 F11 F11 XF86Switch_VT_11
keycode  96 = F12 F12 F12 F12 F12 F12 XF86Switch_VT_12 F12 F12 XF86Switch_VT_12 F12 F12 F12 F12 XF86Switch_VT_12 F12 F12 F12 F12 XF86Switch_VT_12
keycode  97 =
keycode  98 = Katakana NoSymbol Katakana NoSymbol Katakana Katakana

XkbKeyGroupWidth isn't published API, or hasn't been for months.  The release equivalent is xkb_keymap_num_keylevels_for_key.  Are you using 0.2.0?

By "similar to XkbKeyGroupWidth" i mean the Xlib version.
Yes, I am using 0.2.0 version and the public API function xkb_keymap_num_keylevels_for_key which internally calls xkbcommon function with signature XkbKeyGroupWidth(const struct xkb_key *key, xkb_layout_index_t layout).

The problem you are facing is this: suppose the keymap consists of 3 layouts (say us, in, ru). This means the the effective layout can be 0, 1 or 2. The the keycode for <A> is likely to be interpreted differently for each layout, so this key will internally have different keysyms for each layout (0, 1, 2). Now consider a keycode like <ESC>, this is very unlikely to be interpreted differently in each layout. So internally it only has one layout (0).

If the effective layout is 1, and you press <ESC>, it doesn't have that, so the layout is wrapped to fit the range of layouts that <ESC> knows about, and then it gets the keysym from there (in that case from layout 0). Same if the effective layout is 3, and you have a key which only defines 2, etc.

The crash happens when you call xkb_keymap_num_levels_for_key() with a layout that the keycode doesn't know about - the code assumes that the layout is find for the key. keycode 92 doesn't have any keysyms defined for it at all, so effectively this function will always crash for it.

In the little iteration code I described above, I call xkb_keymap_num_layouts_for_key() - this would return 1 for <ESC>, and 0 for keycode 92 - and so the num_levels_for_key part is never reached. What you are doing, is calling this function with the effective layout of the entire keymap, which is out of range for keycode 92.

I think your use case sound somewhat strange (maybe tell us what it is?), and seems to be with the X core protocol in mind, not XKB. But it is sort of legitimate (i.e. always passing the effective layout). We definitely don't want to crash... 

The way the layout wrapping is done is internal, and we don't want to expose this. This leaves us with 2 options, as far as i can see:

1. Always return 0 if the layout is out of range for this key - this implies the user should always call num_layouts_for_key() in some way before.
2. Wrap the layout into the legal range, like xkb_state_key_get_layout() would do.

The second option sounds sensible to me, and ordinary code which iterates on num_layouts_for_key() won't be effected. Daniel, what do you think?

I had a look at the code review page you linked to; nice work! It's a nice way to integrate X with libxkbcommon in the client, and cool to see it get done.

I understand now why you need this code. In order to determine which values to mask in for the Hyper, Super, Meta etc. modifiers in the Qt modifier mask, you look at the modifiers which are bound to keys which have the keysysm Hyper_L etc.

This kind of trickery might have been needed with plain xcb, but the entire setupModifiers function is really not the proper way to do this, with either xcb-xkb or xkbcommon. With XKB, what you want is to look at the (virtual) modifiers called Hyper, Super, etc, which at least with xkeyboard-config are properly defined. See here:
http://cgit.freedesktop.org/xkeyboard-config/tree/compat/misc
Pretty much always included.

Unfortunately, with xkbcommon we don't report these modifiers in the effective modifier mask currently (only real mods). Until we fix this, you should do this with xcb-xkb. You would need the XkbGetNames request to get the virtual modifier indexes associated with the names, and then XkbGetMap request to get the real modifiers associated with the virtual modifier, which you can then finally test for. A bit annoying, but quite better than the current hack you have. With xkbcommon it would be easier.

[ Also, an unrelated note: since you listen to XkbStateNotify events and update the xkb_state with update_mask accordingly, you are effectively acting as a slave for the master state in the server. So you don't really want to use xkb_state_update_key(); in fact, it might cause some problems. So try removing it and see what happens.
I also don't see where you free the keymap, context or state! ]

Hmmm.  I'm almost tempted to say we should introduce another flag, but I
think the best option is probably to do the right thing by default and
wrap.  I didn't think of this usecase at all when I wrote those two getters.

(Also, yes, you should be using EFFECTIVE rather than DEPRESSED for the
state lookup, and not using xkb_state_update_key.)

I've added a commit that fixes it in my tree. It also wraps the layout in xkb_keymap_key_get_syms_by_level().

Created attachment 70119 [details]
Find the real mods for Hyper, Super, etc

Here is a program that does what I described in comment 7, adapted from some code I already had lying around. Beyond the XCB verbosity and GCCisms, it should be clear what it does. This is the "proper" way to do this with XKB, and should alleviate all of the hacks having to do with looking at keycodes and keysyms.

As I already mentioned, it should all become unnecessary in the future with xkbcommon, which would just allow you to do xkb_state_mod_name_is_active("Super") etc. But currently it doesn't.

The mentioned issues are fixed in master, so I'm closing this.