Commit 3abfe83e113e8abe28b044026cbd1fb34e9a4818
2012-09-12T23:51:19
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>
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149
diff --git a/src/xkbcomp/symbols.c b/src/xkbcomp/symbols.c
index 9cdd911..67b4471 100644
--- 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
--- 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;