Patchwork [v2,3/4] qemu-char: add support for U-prefixed symbols

login
register
mail settings
Submitter Michael Tokarev
Date Nov. 1, 2013, 9:59 a.m.
Message ID <52737B75.9080004@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/287752/
State New
Headers show

Comments

Michael Tokarev - Nov. 1, 2013, 9:59 a.m.
16.10.2013 16:40, Jan Krupa wrote:
> This patch adds support for Unicode symbols in keymap files. This
> feature was already used in some keyboard layouts in QEMU generated
> from XKB (e.g. Arabic) but it wasn't implemented in QEMU source code.
>
> There is no need for check of validity of the hex string after U character
> because strtol returns 0 in case the conversion was unsuccessful.
>
> Signed-off-by: Jan Krupa <jkrupa@suse.com>
>
> ---
> ui/keymaps.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index f373cc5..426a893 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -33,6 +33,9 @@ static int get_keysym(const name2keysym_t *table,
>           if (!strcmp(p->name, name))
>               return p->keysym;
>       }
> +    if (strlen(name) == 5 && name[0] == 'U') {
> +        return (int)strtol(name + 1, NULL, 16);
> +    }
>       return 0;
>   }
>

I still dislike this.  People already complained that the keysyms
should be case-insensitive.  And there might be many words starting
with "u" and of 5 chars long.

How about this:

Author: Jan Krupa <JKrupa@suse.com>
Date:   Wed Oct 16 14:40:05 2013 +0200

     qemu-char: add support for U-prefixed symbols

     This patch adds support for Unicode symbols in keymap files. This
     feature was already used in some keyboard layouts in QEMU generated
     from XKB (e.g. Arabic) but it wasn't implemented in QEMU source code.

     There is no need for check of validity of the hex string after U character
     because strtol returns 0 in case the conversion was unsuccessful.

     Signed-off-by: Jan Krupa <jkrupa@suse.com>
     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>


?

(I'm not sure about the author here, I just commit --amend'ed your change)


Thanks,

/mjt
Michael Tokarev - Nov. 1, 2013, 11:28 a.m.
01.11.2013 13:59, Michael Tokarev пишет:
> 16.10.2013 16:40, Jan Krupa wrote:
>> This patch adds support for Unicode symbols in keymap files. This
>> feature was already used in some keyboard layouts in QEMU generated
>> from XKB (e.g. Arabic) but it wasn't implemented in QEMU source code.
>>
>> There is no need for check of validity of the hex string after U character
>> because strtol returns 0 in case the conversion was unsuccessful.
>>
>> Signed-off-by: Jan Krupa <jkrupa@suse.com>
>>
>> ---
>> ui/keymaps.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/ui/keymaps.c b/ui/keymaps.c
>> index f373cc5..426a893 100644
>> --- a/ui/keymaps.c
>> +++ b/ui/keymaps.c
>> @@ -33,6 +33,9 @@ static int get_keysym(const name2keysym_t *table,
>>           if (!strcmp(p->name, name))
>>               return p->keysym;
>>       }
>> +    if (strlen(name) == 5 && name[0] == 'U') {
>> +        return (int)strtol(name + 1, NULL, 16);
>> +    }
>>       return 0;
>>   }
>>
>
> I still dislike this.  People already complained that the keysyms
> should be case-insensitive.  And there might be many words starting
> with "u" and of 5 chars long.
>
> How about this:
>
> Author: Jan Krupa <JKrupa@suse.com>
> Date:   Wed Oct 16 14:40:05 2013 +0200
>
>      qemu-char: add support for U-prefixed symbols
>
>      This patch adds support for Unicode symbols in keymap files. This
>      feature was already used in some keyboard layouts in QEMU generated
>      from XKB (e.g. Arabic) but it wasn't implemented in QEMU source code.
>
>      There is no need for check of validity of the hex string after U character
>      because strtol returns 0 in case the conversion was unsuccessful.
>
>      Signed-off-by: Jan Krupa <jkrupa@suse.com>
>      Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index f373cc5..80d658d 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -33,6 +33,12 @@ static int get_keysym(const name2keysym_t *table,
>           if (!strcmp(p->name, name))
>               return p->keysym;
>       }
> +    if (name[0] == 'U' && strlen(name) == 5) { /* try unicode Uxxxx */
> +        char *end;
> +        int ret = (int)strtoul(name + 1, &end, 16);

Maybe strtol() here as in original.. On my system, both work
the same anyway :)

/mjt
Jan Krupa - Nov. 5, 2013, 3:24 p.m.
On 11/01/2013 12:28 PM, Michael Tokarev wrote:
> 01.11.2013 13:59, Michael Tokarev пишет:
>> 16.10.2013 16:40, Jan Krupa wrote:
>>> This patch adds support for Unicode symbols in keymap files. This
>>> feature was already used in some keyboard layouts in QEMU generated
>>> from XKB (e.g. Arabic) but it wasn't implemented in QEMU source code.
>>>
>>> There is no need for check of validity of the hex string after U
>>> character
>>> because strtol returns 0 in case the conversion was unsuccessful.
>>>
>>> Signed-off-by: Jan Krupa <jkrupa@suse.com>
>>>
>>> ---
>>> ui/keymaps.c |    3 +++
>>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/ui/keymaps.c b/ui/keymaps.c
>>> index f373cc5..426a893 100644
>>> --- a/ui/keymaps.c
>>> +++ b/ui/keymaps.c
>>> @@ -33,6 +33,9 @@ static int get_keysym(const name2keysym_t *table,
>>>           if (!strcmp(p->name, name))
>>>               return p->keysym;
>>>       }
>>> +    if (strlen(name) == 5 && name[0] == 'U') {
>>> +        return (int)strtol(name + 1, NULL, 16);
>>> +    }
>>>       return 0;
>>>   }
>>>
>>
>> I still dislike this.  People already complained that the keysyms
>> should be case-insensitive.  And there might be many words starting
>> with "u" and of 5 chars long.
>>
>> How about this:
>>
>> Author: Jan Krupa <JKrupa@suse.com>
>> Date:   Wed Oct 16 14:40:05 2013 +0200
>>
>>      qemu-char: add support for U-prefixed symbols
>>
>>      This patch adds support for Unicode symbols in keymap files. This
>>      feature was already used in some keyboard layouts in QEMU generated
>>      from XKB (e.g. Arabic) but it wasn't implemented in QEMU source
>> code.
>>
>>      There is no need for check of validity of the hex string after U
>> character
>>      because strtol returns 0 in case the conversion was unsuccessful.
>>
>>      Signed-off-by: Jan Krupa <jkrupa@suse.com>
>>      Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>
>> diff --git a/ui/keymaps.c b/ui/keymaps.c
>> index f373cc5..80d658d 100644
>> --- a/ui/keymaps.c
>> +++ b/ui/keymaps.c
>> @@ -33,6 +33,12 @@ static int get_keysym(const name2keysym_t *table,
>>           if (!strcmp(p->name, name))
>>               return p->keysym;
>>       }
>> +    if (name[0] == 'U' && strlen(name) == 5) { /* try unicode Uxxxx */
>> +        char *end;
>> +        int ret = (int)strtoul(name + 1, &end, 16);
> 
> Maybe strtol() here as in original.. On my system, both work
> the same anyway :)

Hi Michael,

Thanks for the review! Your change looks good. Agree with adding it this
way.

Jan

Patch

diff --git a/ui/keymaps.c b/ui/keymaps.c
index f373cc5..80d658d 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -33,6 +33,12 @@  static int get_keysym(const name2keysym_t *table,
          if (!strcmp(p->name, name))
              return p->keysym;
      }
+    if (name[0] == 'U' && strlen(name) == 5) { /* try unicode Uxxxx */
+        char *end;
+        int ret = (int)strtoul(name + 1, &end, 16);
+        if (*end == '\0' && ret > 0)
+          return ret;
+    }
      return 0;
  }