From 3abfe83e113e8abe28b044026cbd1fb34e9a4818 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Wed, 12 Sep 2012 23:51:19 +0300 Subject: [PATCH] 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 --- src/xkbcomp/symbols.c | 42 +++++++++++++++++++----------------------- test/keyseq.c | 23 ++++++++++++++++++++++- 2 files changed, 41 insertions(+), 24 deletions(-) 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; -- 2.7.4