[patch] xkbcomp: fixing an age-old warning

[Ran Benita]

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>

(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