[patch] xkbcomp: fixing an age-old warning

[Ran Benita]

On Thu, Sep 11, 2014 at 01:36:24AM +0300, Ran Benita wrote:
> On Tue, Sep 09, 2014 at 10:37:18PM +0200, Benno Schulenberg wrote:
> > 
> > Hi,
> > 
> > For many keyboard layouts, starting X produces the following warning:
> > 
> > Warning:          Type "ONE_LEVEL" has 1 levels, but <RALT> has 2 symbols
> >                   Ignoring extra symbols
> > 
> > It is easily reproced with this command:
> > 
> > $ setxkbmap -print  |  xkbcomp -w 1  -  $DISPLAY
> > 
> > The warning is given because in the loads of overlapping data that
> > setxkbmap provides to xkbcomp, the <RALT> key first gets defined
> > with two symbols assigned to it [1], which automatically sets its
> > number of levels to 2, and then it gets redefined to the type of
> > ONE_LEVEL and with just one symbol assigned [2], but this redefining
> > mistakenly does not adjust the number of levels to 1.  Attached patch
> > fixes this.  The patch looks invasive, because it also inverts the if,
> > to make the condition more readable, but the basic change is this:
> > 
> > -    if (into->numLevels[group] >= from->numLevels[group])
> > +    if ((into->numLevels[group] >= from->numLevels[group]) && (from->defs.merge != MergeOverride))
> > 
> > This causes the number of levels to be forced to that of the new definition
> > when the merge is an Override (that is: a redefinition and not a real merge).
> > 
> > The patch is a fix for bug #57242
> > (https://bugs.freedesktop.org/show_bug.cgi?id=57242).
> 
> Nice fix. You're right - MergeKeyGroups is called after MergeKeys had
> already picked the into-key's key type, so it is silly for
> MergeKeyGroups to add levels beyond the key type's width, only for them
> to be truncated later. It also makes no sense to use one's key type with
> the other's symbols... So In general this makes things more consistent
> with MergeKeys.
> 
> One comment below.
> 
> > Benno
> > 
> > 
> > [1]  altwin:    key <RALT> { [ Alt_R, Meta_R ] };
> > [2]  level3:  key <RALT> { type[Group1]="ONE_LEVEL", symbols[Group1] = [ ISO_Level3_Shift ] };
> > 
> > -- 
> > http://www.fastmail.fm - The professional email service
> > 
> 
> > From 3f06401794569fe6619a4eac107e9421a23326ba Mon Sep 17 00:00:00 2001
> > From: Benno Schulenberg <bensberg at justemail.net>
> > Date: Sat, 21 Sep 2013 10:32:54 +0200
> > Subject: [PATCH] When overriding a key, also adjust its number of levels.
> > 
> > This gets rid of the age-old warning of the right Alt key being
> > ONE_LEVEL but having two symbols assigned.  Reducing the number
> > of levels to that of the later definition takes away the cause
> > of the warning.
> > 
> > Tested-by: Ren?? Herman <rene.herman at gmail.com>
> > Tested-by: Knut Petersen <Knut_Petersen at t-online.de>
> > Signed-off-by: Benno Schulenberg <bensberg at justemail.net>
> > ---
> >  symbols.c |   15 ++++++++-------
> >  1 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/symbols.c b/symbols.c
> > index d43ba9f..e32423d 100644
> > --- a/symbols.c
> > +++ b/symbols.c
> > @@ -339,18 +339,19 @@ MergeKeyGroups(SymbolsInfo * info,
> >      clobber = (from->defs.merge != MergeAugment);
> >      report = (warningLevel > 9) ||
> >          ((into->defs.fileID == from->defs.fileID) && (warningLevel > 0));
> > -    if (into->numLevels[group] >= from->numLevels[group])
> > -    {
> > -        resultSyms = into->syms[group];
> > -        resultActs = into->acts[group];
> > -        resultWidth = into->numLevels[group];
> > -    }
> > -    else
> > +    if ((from->numLevels[group] > into->numLevels[group])
> > +	|| (from->defs.merge == MergeOverride))
> 
> You should use `clobber` here, so it also works for MergeReplace, and be
> consistent with MergeKeys (which uses the same condition as `clobber` to
> pick the key type). (And while you're here, a tab slipped in).
> 
> With that you can add:
> 
> Reviewed-by: Ran Benita <ran234 at gmail.com>

Actually I'm not so sure. The current behavior of a key-group override
is per-symbol, e.g.

    override key <FOO> {
        [ NoSymbol, B, C ];
    };

Means: replace whatever is in the 2nd and 3rd levels with the symbols B
and C (create them if they do not exist). "override" is almost always
the default. Often, the key type for both `from` and `into` is not
specified, so an automatic type is assigned (FindAutomaticType) at a
later point.

Now, if we have this:

    key <FOO> {
        [1, 2, 3, 4];
    }
    [...]
    override key <FOO> {
        [ NoSymbol, B, C ];
    };

With current code you get: [1, B, C, 4], with your patch: [1, B, C].
I think the current behavior is a bit more intuitive, and we should not
change it. However, if you have an explicit type (which is the case for
RALT with e.g. layout=us,variant=euro):

    key <FOO> {
        [1, 2, 3, 4];
    }
    [...]
    override key <FOO> {
        type="THREE_LEVEL";
        [ NoSymbol, B, C ];
    };

You get [ 1, B, C ] also with the current code. However the "4" is only
lost at a later point, which produces the warning. But eventually, you
get what was intended.

So if you want to get rid of the warning, I'd suggest either:

1. Change the test to

        if ((from->numLevels[group] > into->numLevels[group])
            || (override && from->types[group] != None))

   This means: if an overriding key-group specifies an explicit key
   type, always use its numLevels (as it will happen later anyway).

2. Just make the warning require a verbosity>0. It is not necessarily
   a problem if the truncation kicks in, so it should not be displayed
   to the user by default. However a keymap author would probably like
   to see it.

Ran

> 
> (and hope it ever gets applied :)
> 
> Ran
> 
> >      {
> >          resultSyms = from->syms[group];
> >          resultActs = from->acts[group];
> >          resultWidth = from->numLevels[group];
> >      }
> > +    else
> > +    {
> > +        resultSyms = into->syms[group];
> > +        resultActs = into->acts[group];
> > +        resultWidth = into->numLevels[group];
> > +    }
> >      if (resultSyms == NULL)
> >      {
> >          resultSyms = uTypedCalloc(resultWidth, KeySym);
> > -- 
> > 1.7.0.4
> > 
> 
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
>