wrong layouts number returned with lv3:ralt_alt option

[bam80]

The behavior produces a crash of KWin on Wayland currently, making it impossible to start a session:
https://bugs.kde.org/show_bug.cgi?id=440027

Here is the relevant part of /usr/share/X11/xkb/symbols/level3:

// The right Alt key never chooses the third level.
// This option attempts to undo the effect of a layout's inclusion of
// 'ralt_switch'.  You may want to also select another level3 option
// to map the level3 shift to some other key.
partial modifier_keys
xkb_symbols "ralt_alt" {
  key <RALT> {
    type[Group1]="TWO_LEVEL",
    type[Group2]="TWO_LEVEL",
    type[Group3]="TWO_LEVEL",
    type[Group4]="TWO_LEVEL",
    symbols[Group1] = [ Alt_R, Meta_R ],
    symbols[Group2] = [ Alt_R, Meta_R ],
    symbols[Group3] = [ Alt_R, Meta_R ],
    symbols[Group4] = [ Alt_R, Meta_R ]
  };
  modifier_map Mod1 { <RALT> };
};

This option causing xkb_keymap_num_layouts() to return 4, despite the fact we requested only 2 layouts when creating the context with xkb_keymap_new_from_names(). The problem is we currently assume the former can't increase the latter.

The same configuration produces different internal keymap for KWin on X11 and Wayland (which uses libxkbcommon):
X11 (taken as xkbcomp $DISPLAY out.xkb):

key <RALT> {
        type= "TWO_LEVEL",
        symbols[Group1]= [           Alt_R,          Meta_R ]
    };

vs
xkbcommon (Wayland):

key <RALT>               {
    type= "TWO_LEVEL",
    symbols[Group1]= [           Alt_R,          Meta_R ],
    symbols[Group2]= [           Alt_R,          Meta_R ],
    symbols[Group3]= [           Alt_R,          Meta_R ],
    symbols[Group4]= [           Alt_R,          Meta_R ]
  };

We don't understand yet which one is right, but probably these two should be the same?

[bam80]

@whot @bluetech We are doubt if this is a bug of xkbcommon or not.
As result, an attempt to workaround it on KDE Plasma side didn't get a support:
https://invent.kde.org/plasma/kwin/-/merge_requests/1508#note_318470
That will affect our users experience.

Could you help us to find a best way to go?

[whot]

verified with xkbcli-compile-keymap --layout us,ru --options=grp:caps_toggle,grp_led:caps,lv3:ralt_alt. This certainly looks like a bug in libxkbcommon, a key with extra groups should not expand the keymap to have those groups too. @bluetech, any idea where to start looking?

[bam80]

Thanks for looking!
I'm not familiar with the code, but if you need help/have ideas where to start - please, let us know! :)

[bam80]

What I can see it's by design, no?

keymap.h

    /* Number of groups in the key with the most groups. */
    xkb_layout_index_t num_groups;

keymap.c

/**
 * Return the total number of active groups in the keymap.
 */
XKB_EXPORT xkb_layout_index_t
xkb_keymap_num_layouts(struct xkb_keymap *keymap)
{
    return keymap->num_groups;
}

xkbcomp/keymap.c

    /* Find maximum number of groups out of all keys in the keymap. */
    xkb_keys_foreach(key, keymap)
        keymap->num_groups = MAX(keymap->num_groups, key->num_groups);

[whot]

The way keymaps works is that you may have multiple groups but a key can have a single group only - because it's the same across all groups (or only defined in one group). For example, the space key usually works like the former:

$  setxkbmap -print -layout us,de | xkbcomp -xkb - - | grep "key <SPCE>"
    key <SPCE> {         [           space ] };

so those comments don't necessarily agree or disagree with the behaviour in this bug (the first two at least).

[bam80]

Would it be correct fix then to define Group1 only for the key in config?:

partial modifier_keys
xkb_symbols "ralt_alt" {
  key <RALT> {
    type[Group1]="TWO_LEVEL",
    symbols[Group1] = [ Alt_R, Meta_R ],
  };
  modifier_map Mod1 { <RALT> };
};

[fooishbar]

IIRC the issue was that the XKB rules syntax lacked any notion of applying an option to all groups. So selecting ralt:alt only applies it to the first group, and then the key has to manually override all four groups in order to get the default behaviour.

[bam80]

Any thoughts about the proper fix then?
Maybe we could squash multiple groups for a key inside xkbcommon into a single one if they all have the same info?

PS:
Essentially, when reading out the config, don't replace existing group encountered, but just delete it if some other group with the same type and symbols already exists?

[whot]

So, best I can tell from a short run through the code, this was added in 428c6f3 though as part of a larger rework. The key comment here is /* If @from has extra groups, just move them to @into. */ in MergeGroups() and the subsequent few lines of code were added. Based on the code I assume that is what adds the extra groups from the added key to the existing map.

xkbcomp only calls MergeGroups if the added key and the destination key have symbols defined for a group. This would skip the loop for groups 3 and 4 on the RALT definition here, resulting in 2 groups in total. libxkbcommon adds those two extra groups so we end up with 4 groups on this key and across the keymap.

My guess right now is that this behavior is incorrect, it just didn't trigger any real bug so far. From a short skim through xkeyboard-config that particular option seems to be the only one that relies on this particular behaviour - defining four groups and hoping the excessive ones get silently dropped.

IRC the issue was that the XKB rules syntax lacked any notion of applying an option to all groups.

A possible solution but I'm running out of time to check this further today:
I changed my symbols file to the bit from #262 (comment) and added this to the rules/evdev file. Note the lv3:ralt_alt lines:

! layout        option  =       symbols
  $threelevellayouts    grp:alts_toggle = +level3(ralt_switch_for_alts_toggle)
  *                     lv3:ralt_alt    = +level3(ralt_alt)              // added
  *                     misc:typo       = +typo(base)
  *                     misc:apl        = +apl(level3)

! layout[1]     option  =       symbols
  $threelevellayouts    grp:alts_toggle = +level3(ralt_switch_for_alts_toggle):1
  *                     lv3:ralt_alt    = +level3(ralt_alt):1             // added
  *                     misc:typo       = +typo(base):1
  *                     misc:apl        = +apl(level3):1

! layout[2]     option  =       symbols
  $threelevellayouts    grp:alts_toggle = +level3(ralt_switch_for_alts_toggle):2
  *                     lv3:ralt_alt    = +level3(ralt_alt):2              // added
  *                     misc:typo       = +typo(base):2
  *                     misc:apl        = +apl(level3):2

! layout[3]     option  =       symbols
  $threelevellayouts    grp:alts_toggle = +level3(ralt_switch_for_alts_toggle):3
  *                     lv3:ralt_alt    = +level3(ralt_alt):3              // added
  *                     misc:typo       = +typo(base):3
  *                     misc:apl        = +apl(level3):3

! layout[4]     option  =       symbols
  $threelevellayouts    grp:alts_toggle = +level3(ralt_switch_for_alts_toggle):4
  *                     lv3:ralt_alt    = +level3(ralt_alt):4              // added
  *                     misc:typo       = +typo(base):4
  *                     misc:apl        = +apl(level3):4

as well as commenting out the later line for lv3:ralt_alt

! option = symbols
....
//  lv3:ralt_alt                =       +level3(ralt_alt)

So basically: special case that option to it applies to the groups one-by-one. This now gives me the correct keymap with libxkbcommon and xkbcomp.

I think this may be the correct solution, regardless of any libxkbcommon fix. But I need to page a lot more code back in to be sure. @fooishbar?

[whot]

I've filed this in https://gitlab.freedesktop.org/xkeyboard-config/xkeyboard-config/-/merge_requests/253. I think libxkbcommon still needs to be fixed too though but this will no longer trigger the bug.

[bam80]

Many thanks from KDE community! :) 🎉

[bam80]

Seems I faced similar issue again, but now it's that xkb_keymap_num_layouts() can only return 4 max, even if source RMLVO contained more layouts.
Is it a bug?

PS: might be relevant:
xkbcomp needs :all syntax for rules
@bluetech

I opened a separate issue about this:
#311

[wismill]

From comment above:

xkbcomp only calls MergeGroups if the added key and the destination key have symbols defined for a group. This would skip the loop for groups 3 and 4 on the RALT definition here, resulting in 2 groups in total. libxkbcommon adds those two extra groups so we end up with 4 groups on this key and across the keymap.

My guess right now is that this behavior is incorrect, it just didn't trigger any real bug so far. From a short skim through xkeyboard-config that particular option seems to be the only one that relies on this particular behaviour - defining four groups and hoping the excessive ones get silently dropped.

@whot I observed that xkbcomp has the exact same behavior, but you need to have different symbols for each group to trigger it, else xkbcomp removes identical groups:

xkb_symbols "ralt_alt" {
  key <RALT> {
    type[Group1]="TWO_LEVEL",
    type[Group2]="TWO_LEVEL",
    type[Group3]="TWO_LEVEL",
    type[Group4]="TWO_LEVEL",
    symbols[Group1] = [ Alt_R, Meta_R ],
-   symbols[Group2] = [ Alt_R, Meta_R ],
+   symbols[Group2] = [ Alt_R, Alt_R ],
-   symbols[Group3] = [ Alt_R, Meta_R ],
+   symbols[Group3] = [ Alt_R, Meta_L ],
-   symbols[Group4] = [ Alt_R, Meta_R ]
+   symbols[Group4] = [ Alt_L, Meta_R ]
  };
  modifier_map Mod1 { <RALT> };
};

So we could add this extra step to mimic the xkbcomp behavior (and it would work in this issue’s case), but that would not prevent in general libxkbcommon nor xkbcomp to add more than 1 group per key from an included symbol file.

There are 2 properties at our disposal to detect if the parsed file is a whole keymap or an included file for a `specific* group:

1. IncludeStmt::modifier: this gives us what is the target group. So if set, we can force only one group per key. However, this is usually not set for the first group, so this is not reliable.
2. SymbolsInfo::include_depth: detect if currently using an included file. There is no include statement in resulting keymaps compiled by xkbcommon, so if this is set we could force to drop the extra groups. But in principle, an include statement at keymap level to import keys with various groups is also legitimate.

We could use 2) to raise a warning though, and add this warning detection to test/xkeyboard-config-test.py.in. This script is also run on xkeyboard-config, so we could ensure we never ship a symbol file with multiple groups there.

Also, when compiling a keymap from RMLVO names, we do know exactly how many groups are expected, so we could pass this info to the parser and discard:

• Extra groups after the first one for each included symbols file;
• Extra groups after the last expected one, in case the rules resolution added those.

This solution with RMLVO names seems robust and can be combined with the warning of the previous section.

[wismill]

Also, when compiling a keymap from RMLVO names, we do know exactly how many groups are expected, so we could pass this info to the parser and discard:

* Extra groups after the first one for each included symbols file;

* Extra groups after the last expected one, in case the rules resolution added those.

I implemented a fix for the first item in #518; it turns out that the second item does have legit use case, such as legacy rules inserting an US layout before a non-Latin one.