[patch] xkbcomp: fixing an age-old warning
Ran Benita
ran234 at gmail.com
Thu Sep 11 04:01:01 PDT 2014
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
>
More information about the xorg-devel
mailing list