symbols: fix real/alias key merge ordering bug

Background:

The CopySymbolsDef has a comment on a couple of lines which supposedly
fixed a bug:

    /*
     * kt_index[i] may have been set by a previous run (if we have two
     * layouts specified). Let's not overwrite it with the ONE_LEVEL
     * default group if we dont even have keys for this group anyway.
     *
     * FIXME: There should be a better fix for this.
     */
    if (!darray_empty(groupi->levels))
        key->kt_index[i] = types[i];

But neither the comment nor the fix make any sense, because the kt_index
is indexed per group, i.e. each group gets its own type.
The original xkbcomp commit which added this (36fecff58) points to this
bug: https://bugzilla.redhat.com/show_bug.cgi?id=436626
which complains about -layout "ru,us" -variant "phonetic," not working
properly. And indeed when we try:
    sudo ./test/interactive -l ru,us -v
the first group doesn't get any syms for the main keys.

The problem (Clearly the fix above is useless):

The ru(phonetic) map is specified using aliases, e.g. LatQ, LatW instead
of AD01, AD02, etc. When combined with another layout which uses the
real names (AD01, AD02), the symbols code should recognize they are the
same key and merge them into one KeyInfo. The current code does that,
but it doesn't catch the case where the alias was processes *before* the
real one; so we get two KeyInfo's and the later one wins. So e.g. the
ru(phonetic) symbols are ignored.

The fix:

Before adding a new KeyInfo to the keys array, always replace its name
by the real name, which avoids the entire issue. Luckily this is done
pretty late so most error messages should still show the alias name.

Signed-off-by: Ran Benita <ran234@gmail.com>

diff --git a/src/xkbcomp/symbols.c b/src/xkbcomp/symbols.c
index 9cdd911..67b4471 100644 (file)
--- a/src/xkbcomp/symbols.c
+++ b/src/xkbcomp/symbols.c
@@ -493,15 +493,19 @@ AddKeySymbols(SymbolsInfo *info, KeyInfo *keyi)
     unsigned long real_name;
     KeyInfo *iter, *new;
 
+    /*
+     * Don't keep aliases in the keys array; this guarantees that
+     * searching for keys to merge with by straight comparison (see the
+     * following loop) is enough, and we won't get multiple KeyInfo's
+     * for the same key because of aliases.
+     */
+    if (FindKeyNameForAlias(info->keymap, keyi->name, &real_name))
+        keyi->name = real_name;
+
     darray_foreach(iter, info->keys)
         if (iter->name == keyi->name)
             return MergeKeys(info, iter, keyi);
 
-    if (FindKeyNameForAlias(info->keymap, keyi->name, &real_name))
-        darray_foreach(iter, info->keys)
-            if (iter->name == real_name)
-                return MergeKeys(info, iter, keyi);
-
     darray_resize0(info->keys, darray_size(info->keys) + 1);
     new = &darray_item(info->keys, darray_size(info->keys) - 1);
     return CopyKeyInfo(keyi, new, true);
@@ -1499,11 +1503,14 @@ CopySymbolsDef(SymbolsInfo *info, KeyInfo *keyi)
     xkb_group_index_t i, nGroups;
     xkb_level_index_t width, tmp;
     struct xkb_key_type * type;
-    bool haveActions, autoType;
-    unsigned types[XKB_NUM_GROUPS];
+    bool haveActions;
     unsigned int symIndex = 0;
 
-    key = FindNamedKey(keymap, keyi->name, true);
+    /*
+     * The name is guaranteed to be real and not an alias (see
+     * AddKeySymbols), so 'false' is safe here.
+     */
+    key = FindNamedKey(keymap, keyi->name, false);
     if (!key) {
         log_vrb(info->keymap->ctx, 5,
                 "Key %s not found in keycodes; Symbols ignored\n",
@@ -1515,12 +1522,11 @@ CopySymbolsDef(SymbolsInfo *info, KeyInfo *keyi)
     width = 0;
     for (i = nGroups = 0; i < XKB_NUM_GROUPS; i++) {
         GroupInfo *groupi = &keyi->groups[i];
+        bool autoType = false;
 
         if (i + 1 > nGroups && groupi->defined)
             nGroups = i + 1;
 
-        autoType = false;
-
         if (!haveActions) {
             LevelInfo *leveli;
             darray_foreach(leveli, groupi->levels) {
@@ -1547,7 +1553,7 @@ CopySymbolsDef(SymbolsInfo *info, KeyInfo *keyi)
                         LongKeyNameText(keyi->name));
         }
 
-        if (FindNamedType(keymap, groupi->type, &types[i])) {
+        if (FindNamedType(keymap, groupi->type, &key->kt_index[i])) {
             if (!autoType || darray_size(groupi->levels) > 2)
                 key->explicit_groups |= (1 << i);
         }
@@ -1561,11 +1567,11 @@ CopySymbolsDef(SymbolsInfo *info, KeyInfo *keyi)
              * Index 0 is guaranteed to contain something, usually
              * ONE_LEVEL or at least some default one-level type.
              */
-            types[i] = 0;
+            key->kt_index[i] = 0;
         }
 
         /* if the type specifies fewer levels than the key has, shrink the key */
-        type = &keymap->types[types[i]];
+        type = &keymap->types[key->kt_index[i]];
         if (type->num_levels < darray_size(groupi->levels)) {
             log_vrb(info->keymap->ctx, 1,
                     "Type \"%s\" has %d levels, but %s has %d levels; "
@@ -1598,16 +1604,6 @@ CopySymbolsDef(SymbolsInfo *info, KeyInfo *keyi)
     for (i = 0; i < nGroups; i++) {
         GroupInfo *groupi = &keyi->groups[i];
 
-        /* assign kt_index[i] to the index of the type in map->types.
-         * kt_index[i] may have been set by a previous run (if we have two
-         * layouts specified). Let's not overwrite it with the ONE_LEVEL
-         * default group if we dont even have keys for this group anyway.
-         *
-         * FIXME: There should be a better fix for this.
-         */
-        if (!darray_empty(groupi->levels))
-            key->kt_index[i] = types[i];
-
         if (!darray_empty(groupi->syms)) {
             /* fill key to "width" symbols*/
             for (tmp = 0; tmp < width; tmp++) {

diff --git a/test/keyseq.c b/test/keyseq.c
index d7b7178..5539799 100644 (file)
--- a/test/keyseq.c
+++ b/test/keyseq.c
@@ -137,7 +137,8 @@ main(void)
     struct xkb_keymap *keymap;
 
     assert(ctx);
-    keymap = test_compile_rules(ctx, "evdev", "evdev", "us,il", NULL,
+    keymap = test_compile_rules(ctx, "evdev", "evdev",
+                                "us,il,ru", ",,phonetic",
                                 "grp:alt_shift_toggle,grp:menu_toggle");
     assert(keymap);
 
@@ -200,6 +201,7 @@ main(void)
                         KEY_K,        BOTH,  XKB_KEY_hebrew_lamed,    NEXT,
                         KEY_F,        BOTH,  XKB_KEY_hebrew_kaph,     NEXT,
                         KEY_COMPOSE,  BOTH,  XKB_KEY_ISO_Next_Group,  NEXT,
+                        KEY_COMPOSE,  BOTH,  XKB_KEY_ISO_Next_Group,  NEXT,
                         KEY_O,        BOTH,  XKB_KEY_o,               FINISH));
 
     assert(test_key_seq(keymap,
@@ -272,6 +274,25 @@ main(void)
                         KEY_NUMLOCK,  BOTH,  XKB_KEY_Num_Lock,  NEXT,
                         KEY_KP2,      BOTH,  XKB_KEY_KP_Down,   FINISH));
 
+    /* Test that the aliases in the ru(phonetic) symbols map work. */
+    assert(test_key_seq(keymap,
+                        KEY_COMPOSE,     BOTH,  XKB_KEY_ISO_Next_Group,  NEXT,
+                        KEY_COMPOSE,     BOTH,  XKB_KEY_ISO_Next_Group,  NEXT,
+                        KEY_1,           BOTH,  XKB_KEY_1,               NEXT,
+                        KEY_Q,           BOTH,  XKB_KEY_Cyrillic_ya,     NEXT,
+                        KEY_LEFTSHIFT,   DOWN,  XKB_KEY_Shift_L,         NEXT,
+                        KEY_1,           BOTH,  XKB_KEY_exclam,          NEXT,
+                        KEY_Q,           BOTH,  XKB_KEY_Cyrillic_YA,     NEXT,
+                        KEY_LEFTSHIFT,   UP,    XKB_KEY_Shift_L,         NEXT,
+                        KEY_V,           BOTH,  XKB_KEY_Cyrillic_zhe,    NEXT,
+                        KEY_CAPSLOCK,    BOTH,  XKB_KEY_Caps_Lock,       NEXT,
+                        KEY_1,           BOTH,  XKB_KEY_1,               NEXT,
+                        KEY_V,           BOTH,  XKB_KEY_Cyrillic_ZHE,    NEXT,
+                        KEY_RIGHTSHIFT,  DOWN,  XKB_KEY_Shift_R,         NEXT,
+                        KEY_V,           BOTH,  XKB_KEY_Cyrillic_zhe,    NEXT,
+                        KEY_RIGHTSHIFT,  UP,    XKB_KEY_Shift_R,         NEXT,
+                        KEY_V,           BOTH,  XKB_KEY_Cyrillic_ZHE,    FINISH));
+
     xkb_map_unref(keymap);
     xkb_context_unref(ctx);
     return 0;