Hi Ran On Tue, Oct 2, 2012 at 9:37 AM, Ran Benita <[email protected]> wrote: > Hi David, > > On Mon, Oct 01, 2012 at 07:29:58PM +0200, David Herrmann wrote: >> xkb_keysym_from_name() uses a big lookup table generated by "makekeys" >> to find keysyms. It does this case-sensitive because we have keys like >> XKB_KEY_A and XKB_KEY_a. So if a user searches for "a" we must always >> return the case-sensitive match, which is XKB_KEY_a. >> However, lets look at XKB_KEY_XF86HomePage. The user currently _must_ >> enter "XF86HomePage" correctly to get this keysym. If they enter >> "Xf86HomePage" or "XF86Homepage" or anything else, xkb_keysym_from_name() >> will fail. This is unreasonable and bogus. Instead we should allow an >> application to have a case-insensitive search as fallback. This removes >> policy from xkbcommon and moves it into the application. They can now >> decide, whether they use a possibly unsafe fallback or stay with the >> case-sensitive search. >> >> Therefore, this patch modifies "makekeys" to convert all symbols to >> lower-case and add them into the lookup table, too. The case-sensitive >> symbols are unchanged and stay in the table, but a user-application can >> now fall back to lower-case search if a symbol cannot be resolved. >> >> This patch does take care that any case-sensitive key overwrites any >> lower-case key. That is, if "makekeys" adds XKB_KEY_A, it will add it as >> "A" and "a". If XKB_KEY_a is added later, it will overwrite the previous >> "a" because this is a case-sensitive match. >> This guarantees that looking up for "a" always returns KEY_a. >> >> This patch also makes sure that case-sensitive keys are inserted into the >> lookup table first. This guarantees that xkb_keysym_get_name() always >> returns the first match which is the case-sensitive entry. >> >> There are several theoretical pitfalls here: >> - Assume there is a key Abcd and abcd. If the user searches for ABCD and >> does not find anything, the fallback will return "abcd", although The >> user might have preferred "Abcd". >> An application can circumwent it by simply not using the fallback >> lower-case search, so this does not affect existing applications. >> - Assume there is a key AbCd but no other key "abcd" in any case. If the >> user searches for "abcd", it will return "AbCd" even though they didn't >> request a case-insensitive match. This _does_ affect existing >> applications. >> However, there is currently no key where a user wouldn't be happy about >> this, because every key where case really changes semantics, is always >> available in both, upper-case and lower-case and so this doesn't >> happen. >> An application can catch this by converting the result back to a name >> and checking whether the names match. However, I really don't know why >> anyone would bother? >> >> I haven't found any other cases where this changes semantics. There are >> some pitfalls when doing the case-insensitive fallback match, however, >> this is optional! >> The only case were existing application's behavior changes are listed >> above. However, considering that nearly everyone wants lower-case keys, >> anyway, this is in my eyes the best solution. >> >> Furthermore, new keysyms should be avoided and users should use Unicode >> symbols to identify keys. This is always unambigious and isn't affected at >> all by this patch. >> >> Signed-off-by: David Herrmann <[email protected]> >> --- >> makekeys/makekeys.c | 71 >> ++++++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 67 insertions(+), 4 deletions(-) > > I like the idea, and it seems to work. > > First, one thing that's easy to miss, this should work: > assert(test_string("xf86_switch_vt_5", XKB_KEY_XF86Switch_VT_5)); > Have a look at src/keysym.c, it treats the "XF86_" case specially.
Argh, I overlooked that. Anyway, that's as easy as trying "xf86", too. I will add it. > Second, I think that in the keymap files themselves (xkb_symbols etc.) > we should always do case-sensitive lookup, this would otherwise lead to > ambiguity, and the original xkbcomp won't be able to parse these files, > for no particularly good reason. So LookupKeysym() in xkbcomp/symbols.c > needs to adapt as well, e.g. by comparing to keysym_get_name. If it can > be made more efficient such that it doesn't need the extra lookup + > compare, that's even better. That was my main concern, too. The current approach just makes any lower-case lookup succeed. So I was wondering whether converting this whole mess to "gperf" would be acceptable? I would then create two tables, a case-sensitive and a lower-case table. We can then decide, whether we add a flag to xkb_keysym_from_name() or whether we introduce a separate xkb_keysym_from_casename() or something like that (adj. strcasecmp()). This would preserve the semantics of xkb_keysym_from_name(), which is probably what we want(?). > Also, this probably warrants a note in the xkb_keysym_from_name() > documentation, explaining how to use it for a case-sensitive and > case-insesitive lookup. I actually think that adding a flag to control > this would be best (which would break ABI/API if done directly), but > don't mind much. I prefer the flags argument, too. > Finally, some minor comments below. Thanks for the code-review. But I think I might just try gperf to replace that unmaintainable "makekeys" program. Any objections? Thanks for the review! David _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
