Re: [PATCH xkbcomp] keycodes: Ignore high keycodes

[Ran Benita]

On Thu, Apr 06, 2017 at 08:14:35PM +0100, Daniel Stone wrote:
> On 6 April 2017 at 19:57, Ran Benita  wrote:
> > I always hoped that Wayland systems could switch to a new keycode file,
> > which uses the full evdev key names (xkbcommon no longer has the 4
> > character limit), and the evdev codes (without the + 8 offset), and
> > aliases for backward compat with existing keymaps. But I guess that
> > would be a lot of pain for little gain.
> 
> Yeah, me too. But by the time we'd encoded the +8 offset in enough
> clients, that boat had already sailed, unfortunately.

Yeah.

> > So this seems like a good idea to me, to move things forward.
> >
> > My only concern about the patch is that I think that it will issue a
> > million warnings if keymaps start to use this? I looked at utils.c (a
> > true relic) and it seems that there is no filtering on these ERROR2 and
> > ACTION2, they just print. Add to that other files which would start to
> > use the new keycodes, and you get more warnings (hopefully not fatal
> > errors).
> >
> > I think a reasonable test plan would be:
> >
> > - Add ~15 new >255 keycodes.
> > - Use them in some symbols file.
> > - Try to load this with a patched xkbcomp.
> > - See that:
> >   - There's not a huge log spam.
> >   - That it actually doesn't fail.
> >
> > Note that I say ~15, because I vaguely remember that there are "allow 10
> > errors then give up" checks in some places.
> 
> Funnily, that's exactly what I did. And yeah, it does spam a lot more
> warnings, but there are already 46 lines of warnings spat out by
> xkbcomp just building a plain evdev/us keymap. I made sure that
> they're not errors though, so the number doesn't matter. Just to be
> doubly safe, I bumped it up to 16 new keycodes, and it worked fine,
> warnings notwithstanding. I could tune the WARNs down to INFO, but it
> already warns for things like no symbols defined for a given key, so
> ...

Good enough for me!

I think we should first hear if Sergey would even accept >255 keycodes
in xkeyboard-config keymaps which are shared with X11 (once this patch
is applied and sufficient time has passed). But regardless:

Reviewed-by: Ran Benita 

Ran
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xkbcomp] keycodes: Ignore high keycodes

[Daniel Stone]

Hi Ran,

On 6 April 2017 at 19:57, Ran Benita  wrote:
> On Thu, Apr 06, 2017 at 05:37:28PM +0100, Daniel Stone wrote:
>> Rather than throwing a fatal error when a keycode definition exceeds the
>> declared maximum (i.e. 255), just ignore the definition and continue.
>>
>> This allows xkeyboard-config to start shipping datasets including high
>> keycodes, which will work in xkbcommon as it ignores explicit range
>> declarations.
>
> I always hoped that Wayland systems could switch to a new keycode file,
> which uses the full evdev key names (xkbcommon no longer has the 4
> character limit), and the evdev codes (without the + 8 offset), and
> aliases for backward compat with existing keymaps. But I guess that
> would be a lot of pain for little gain.

Yeah, me too. But by the time we'd encoded the +8 offset in enough
clients, that boat had already sailed, unfortunately.

> So this seems like a good idea to me, to move things forward.
>
> My only concern about the patch is that I think that it will issue a
> million warnings if keymaps start to use this? I looked at utils.c (a
> true relic) and it seems that there is no filtering on these ERROR2 and
> ACTION2, they just print. Add to that other files which would start to
> use the new keycodes, and you get more warnings (hopefully not fatal
> errors).
>
> I think a reasonable test plan would be:
>
> - Add ~15 new >255 keycodes.
> - Use them in some symbols file.
> - Try to load this with a patched xkbcomp.
> - See that:
>   - There's not a huge log spam.
>   - That it actually doesn't fail.
>
> Note that I say ~15, because I vaguely remember that there are "allow 10
> errors then give up" checks in some places.

Funnily, that's exactly what I did. And yeah, it does spam a lot more
warnings, but there are already 46 lines of warnings spat out by
xkbcomp just building a plain evdev/us keymap. I made sure that
they're not errors though, so the number doesn't matter. Just to be
doubly safe, I bumped it up to 16 new keycodes, and it worked fine,
warnings notwithstanding. I could tune the WARNs down to INFO, but it
already warns for things like no symbols defined for a given key, so
...

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xkbcomp] keycodes: Ignore high keycodes

[Ran Benita]

On Thu, Apr 06, 2017 at 05:37:28PM +0100, Daniel Stone wrote:
> Rather than throwing a fatal error when a keycode definition exceeds the
> declared maximum (i.e. 255), just ignore the definition and continue.
> 
> This allows xkeyboard-config to start shipping datasets including high
> keycodes, which will work in xkbcommon as it ignores explicit range
> declarations.

I always hoped that Wayland systems could switch to a new keycode file,
which uses the full evdev key names (xkbcommon no longer has the 4
character limit), and the evdev codes (without the + 8 offset), and
aliases for backward compat with existing keymaps. But I guess that
would be a lot of pain for little gain.

So this seems like a good idea to me, to move things forward.

My only concern about the patch is that I think that it will issue a
million warnings if keymaps start to use this? I looked at utils.c (a
true relic) and it seems that there is no filtering on these ERROR2 and
ACTION2, they just print. Add to that other files which would start to
use the new keycodes, and you get more warnings (hopefully not fatal
errors).

I think a reasonable test plan would be:

- Add ~15 new >255 keycodes.
- Use them in some symbols file.
- Try to load this with a patched xkbcomp.
- See that:
  - There's not a huge log spam.
  - That it actually doesn't fail.

Note that I say ~15, because I vaguely remember that there are "allow 10
errors then give up" checks in some places.

Ran
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xkbcomp] keycodes: Ignore high keycodes

[Daniel Stone]

Rather than throwing a fatal error when a keycode definition exceeds the
declared maximum (i.e. 255), just ignore the definition and continue.

This allows xkeyboard-config to start shipping datasets including high
keycodes, which will work in xkbcommon as it ignores explicit range
declarations.

Signed-off-by: Daniel Stone 
Reported-by: Christian Kellner 
---
 keycodes.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/keycodes.c b/keycodes.c
index 22d9eae..e692bf7 100644
--- a/keycodes.c
+++ b/keycodes.c
@@ -330,10 +330,10 @@ AddKeyName(KeyNamesInfo * info,
 
 if ((kc < info->effectiveMin) || (kc > info->effectiveMax))
 {
-ERROR2("Illegal keycode %d for name <%s>\n", kc, name);
+WARN2("Illegal keycode %d for name <%s>\n", kc, name);
 ACTION2("Must be in the range %d-%d inclusive\n",
 info->effectiveMin, info->effectiveMax);
-return False;
+return True;
 }
 if (kc < info->computedMin)
 info->computedMin = kc;
@@ -589,10 +589,10 @@ HandleKeycodeDef(KeycodeDef * stmt, unsigned merge, 
KeyNamesInfo * info)
 code = result.ival;
 if ((code < info->effectiveMin) || (code > info->effectiveMax))
 {
-ERROR2("Illegal keycode %d for name <%s>\n", code, stmt->name);
+WARN2("Illegal keycode %d for name <%s>\n", code, stmt->name);
 ACTION2("Must be in the range %d-%d inclusive\n",
 info->effectiveMin, info->effectiveMax);
-return 0;
+return 1;
 }
 if (stmt->merge != MergeDefault)
 {
-- 
2.12.2

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel