[patch] xkbcomp: fixing an age-old warning

[Ran Benita]

On Thu, Sep 11, 2014 at 10:18:52PM +0200, Benno Schulenberg wrote:
> On Thu, Sep 11, 2014, at 13:01, Ran Benita wrote:
> > 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,
> 
> Hmm.  I wouldn't call that more intuitive, I would call it plain wrong.  :)
> If I wanted an overridden key to retain a possible fourth symbol, I would
> write "[ any, B, C, any ]" instead of "[ any, B, C ]".

If you want to outright replace the key, you should use Replace (see the
beginning of MergeKeys() - haven't tried it though). Override in this
case means to do the merge per-level. So if I put [ NoSymbol, B, C ] it
shouldn't touch the other levels IMO.

But if you override the *type* then it obviously affects the levels.

> > and we should not
> > change it.  [...]
> 
> Fair enough... let's assume that some definitions depend on the current
> behaviour.
> 
> > 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))
> 
> Implemented in the revised patch, attached.

Looks good.

> > 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.
> 
> I would like to propose that solution for this warning:
> Warning:          Symbol map for key <RALT> redefined
>                   Using last definition for conflicting fields

If you send a patch I will review it :) (but you will have to get
someone to apply it).

Ran

> Thanks for the detailed review.
> 
> Benno