[PATCH libxkbcommon v4 2/2] Add xkb_keysym_from_casename() helper for case-insensitive search

[David Herrmann]

Hi Ran

On Mon, Oct 8, 2012 at 10:00 AM, Ran Benita <ran234 at gmail.com> wrote:
> On Sun, Oct 07, 2012 at 03:59:09PM +0200, David Herrmann wrote:
>>
>>  makekeys.py           |  6 ++++
>>  src/keysym.c          | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  test/keysym.c         | 29 ++++++++++++++++
>>  xkbcommon/xkbcommon.h | 15 ++++++++
>>  4 files changed, 144 insertions(+)
>>
>> diff --git a/makekeys.py b/makekeys.py
>> index 94885c0..ab75cce 100644
>> --- a/makekeys.py
>> +++ b/makekeys.py
>> @@ -5,6 +5,7 @@ import re, sys, itertools
>>  pattern = re.compile(r'^#define\s+XKB_KEY_(?P<name>\w+)\s+(?P<value>0x[0-9a-fA-F]+)\s')
>>  matches = [pattern.match(line) for line in open(sys.argv[1])]
>>  entries = [(m.group("name"), int(m.group("value"), 16)) for m in matches if m]
>> +lentries = [(m.group("name").lower(), m.group("name")) for m in matches if m]
>>
>>  print('''struct name_keysym {
>>      const char *name;
>> @@ -16,6 +17,11 @@ for (name, _) in sorted(entries, key=lambda e: e[0]):
>>      print('    {{ "{name}", XKB_KEY_{name} }},'.format(name=name))
>>  print('};\n')
>>
>> +print('static const struct name_keysym case_to_keysym[] = {');
>> +for (lname, name) in sorted(lentries, key=lambda e: e[0]):
>> +    print('    {{ "{lname}", XKB_KEY_{name} }},'.format(lname=lname, name=name))
>> +print('};\n')
>
> I think it would be more space efficient if we don't use a new 'lentries'
> list like above, but reuse the (original-cased) 'entries' list and
> change the sort key from 'lambda e: e[0]' to 'lambda e: e[0].lower()'.
> This will result in a table with the same strings being generated but
> ordered by lowercase. If I'm reading your code correctly, it would work
> nonetheless, but any half-decent compiler/linker would only store the
> strings once, e.g. instead of this:
>     $ strings libxkbcommon.so | grep -i lambda
>     Greek_LAMBDA
>     Greek_lambda
>     greek_lambda
> You get this:
>     $ strings libxkbcommon.so | grep -i lambda
>     Greek_LAMBDA
>     Greek_lambda
>
> Also maybe we want to stick a:
>     locale.setlocle(locale.LC_ALL, "C")
> or "en_us.UTF-8" there? I don't know if the python2/3 functions are
> locale sensitive, but why not to be safe I guess.

Both changes make sense, I will fix them.

>> +
>>  # *.sort() is stable so we always get the first keysym for duplicate
>>  print('static const struct name_keysym keysym_to_name[] = {');
>>  for (name, _) in (next(g[1]) for g in itertools.groupby(sorted(entries, key=lambda e: e[1]), key=lambda e: e[1])):
>> diff --git a/src/keysym.c b/src/keysym.c
>> index 85d4386..d320055 100644
>> --- a/src/keysym.c
>> +++ b/src/keysym.c
>> @@ -65,6 +65,12 @@ static int compare_by_name(const void *a, const void *b)
>>      return strcmp(key->name, entry->name);
>>  }
>>
>> +static int compare_by_casename(const void *a, const void *b)
>> +{
>> +    const struct name_keysym *key = a, *entry = b;
>> +    return strcasecmp(key->name, entry->name);
>> +}
>> +
>>  XKB_EXPORT int
>>  xkb_keysym_get_name(xkb_keysym_t ks, char *buffer, size_t size)
>>  {
>> @@ -144,6 +150,94 @@ xkb_keysym_from_name(const char *s)
>>      return XKB_KEY_NoSymbol;
>>  }
>>
>> +/*
>> + * Find the lower-case keysym of any keysym entry.
>> + * If we use bsearch() to find a keysym based on a case-insensitive search, we
>> + * may get _any_ of all possible duplicates back. This function looks at all
>> + * entries before and after @entry which have the same name (case insensitive)
>> + * and then returns the _best_ match of all.
>> + * The _best_ match is the lower-case keysym which we find with the help of
>> + * xkb_keysym_is_lower().
>> + */
>> +static const struct name_keysym *find_lcase(const struct name_keysym *entry)
>> +{
>> +    const struct name_keysym *iter, *last;
>> +    size_t len = sizeof(case_to_keysym) / sizeof(*case_to_keysym);
>> +
>> +    if (xkb_keysym_is_lower(entry->keysym))
>> +        return entry;
>
> xkb_keysym_is_lower is pretty lousy right now, it doesn't handle unicode
> keysysm at all. But I will fix this later (I think we're going to have
> to copy XConvertCase and UCSConvertCase from libX11:src/KeyBind.c - kind
> of ugly).

That's actually no problem at all. The Unicode keysyms have different
keysym-names so I will never call is_lower() for them. I only call
this for keysyms that have the same name (case-insensitive) and these
are all handled pretty well I think.
Nevertheless, it would still be nice to have is_lower() handle them, too.

I will wait until Daniel comments on these patches before sending the
final revision (I will try to catch him on IRC).

Thanks for the review!
David