[PATCH 3/3] xkbcomp: Add missing flag support and correct modifier handling of ISOLock

[Ran Benita]

On Thu, Jan 23, 2014 at 08:40:00PM +0100, Andreas Wettstein wrote:
> Add missing support for "affect" flag to selectively affect locking or
> unlocking for for modifier locking, control locking, and ISOLock.
> Fix some incorrect masking and modifier handling for ISOLock.

Some small comments below you might want to address, but:

Reviewed-By: Ran Benita <ran234 at gmail.com>

> Signed-off-by: Andreas Wettstein <wettstein509 at solnet.ch>
> ---
>  action.c | 102 ++++++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 62 insertions(+), 40 deletions(-)
> 
> diff --git a/action.c b/action.c
> index 4623c0c..7188d6b 100644
> --- a/action.c
> +++ b/action.c
> @@ -436,33 +436,13 @@ HandleSetLatchMods(XkbDescPtr xkb,
>      return ReportIllegal(action->type, field);
>  }
>  
> -static Bool
> -HandleLockMods(XkbDescPtr xkb,
> -               XkbAnyAction * action,
> -               unsigned field, ExprDef * array_ndx, ExprDef * value)

Why did you move this function down? It messes with the diff and I think
the current position is better (with the other Mods actions). You can
move the lockWhich up instead.

> -{
> -    XkbModAction *act;
> -    unsigned t1, t2;
> -
> -    act = (XkbModAction *) action;
> -    if ((array_ndx != NULL) && (field == F_Modifiers))
> -        return ReportActionNotArray(action->type, field);
> -    switch (field)
> -    {
> -    case F_Modifiers:
> -        t1 = act->flags;
> -        if (CheckModifierField(xkb, action->type, value, &t1, &t2))
> -        {
> -            act->flags = t1;
> -            act->real_mods = act->mask = (t2 & 0xff);
> -            t2 = (t2 >> 8) & 0xffff;
> -            XkbSetModActionVMods(act, t2);
> -            return True;
> -        }
> -        return False;
> -    }
> -    return ReportIllegal(action->type, field);
> -}
> +static LookupEntry lockWhich[] = {
> +    {"both", 0},
> +    {"lock", XkbSA_LockNoUnlock},
> +    {"neither", (XkbSA_LockNoLock | XkbSA_LockNoUnlock)},

You didn't add it, but I do wonder what "neither" is good for, it
renders the action rather useless, no?

> +    {"unlock", XkbSA_LockNoLock},
> +    {NULL, 0}
> +};
>  
>  static LookupEntry groupNames[] = {
>      {"group1", 1},
> @@ -514,6 +494,41 @@ CheckGroupField(unsigned action,
>  }
>  
>  static Bool
> +HandleLockMods(XkbDescPtr xkb,
> +               XkbAnyAction * action,
> +               unsigned field, ExprDef * array_ndx, ExprDef * value)
> +{
> +    XkbModAction *act;
> +    unsigned t1, t2;
> +    ExprResult rtrn;
> +
> +    act = (XkbModAction *) action;
> +    if ((array_ndx != NULL) && (field == F_Modifiers || field == F_Affect))
> +        return ReportActionNotArray(action->type, field);
> +    switch (field)
> +    {
> +    case F_Affect:
> +	if (!ExprResolveEnum(value, &rtrn, lockWhich))
> +            return ReportMismatch(action->type, field, "lock or unlock");
> +        act->flags &= ~(XkbSA_LockNoLock | XkbSA_LockNoUnlock);

Would you consider adding a CheckAffectField() function, to unify to
unify the handling of this for all the actions who use it? I think the
handling is the same, except for ISOLock, where you'd need to set
act->affect as well.

> +        act->flags |= rtrn.ival;

This should be rtrn.uval, the current code got it wrong.

> +        return True;
> +    case F_Modifiers:
> +        t1 = act->flags;
> +        if (CheckModifierField(xkb, action->type, value, &t1, &t2))
> +        {
> +            act->flags = t1;
> +            act->real_mods = act->mask = (t2 & 0xff);
> +            t2 = (t2 >> 8) & 0xffff;
> +            XkbSetModActionVMods(act, t2);
> +            return True;
> +        }
> +        return False;
> +    }
> +    return ReportIllegal(action->type, field);
> +}
> +
> +static Bool
>  HandleSetLatchGroup(XkbDescPtr xkb,
>                      XkbAnyAction * action,
>                      unsigned field, ExprDef * array_ndx, ExprDef * value)
> @@ -641,14 +656,6 @@ static LookupEntry btnNames[] = {
>      {NULL, 0}
>  };
>  
> -static LookupEntry lockWhich[] = {
> -    {"both", 0},
> -    {"lock", XkbSA_LockNoUnlock},
> -    {"neither", (XkbSA_LockNoLock | XkbSA_LockNoUnlock)},
> -    {"unlock", XkbSA_LockNoLock},
> -    {NULL, 0}
> -};
> -
>  static Bool
>  HandlePtrBtn(XkbDescPtr xkb,
>               XkbAnyAction * action,
> @@ -779,8 +786,12 @@ static LookupEntry isoNames[] = {
>      {"pointer", XkbSA_ISONoAffectPtr},
>      {"ctrls", XkbSA_ISONoAffectCtrls},
>      {"controls", XkbSA_ISONoAffectCtrls},
> -    {"all", ~((unsigned) 0)},
> +    {"all", XkbSA_ISOAffectMask},

Yep this makes sense.

>      {"none", 0},
> +    {"both", 0},
> +    {"lock", XkbSA_LockNoUnlock},
> +    {"neither", (XkbSA_LockNoLock | XkbSA_LockNoUnlock)},
> +    {"unlock", XkbSA_LockNoLock},
>      {NULL, 0},
>  };
>  
> @@ -804,8 +815,8 @@ HandleISOLock(XkbDescPtr xkb,
>          if (CheckModifierField(xkb, action->type, value, &flags, &mods))
>          {
>              act->flags = flags & (~XkbSA_ISODfltIsGroup);
> -            act->real_mods = mods & 0xff;
> -            mods = (mods >> 8) & 0xff;
> +            act->real_mods = act->mask = (mods & 0xff);
> +            mods = (mods >> 8) & 0xffff;

Nice catch.

>              XkbSetModActionVMods(act, mods);
>              return True;
>          }
> @@ -826,7 +837,9 @@ HandleISOLock(XkbDescPtr xkb,
>              return ReportActionNotArray(action->type, field);
>          if (!ExprResolveMask(value, &rtrn, SimpleLookup, (XPointer) isoNames))
>              return ReportMismatch(action->type, field, "keyboard component");
> -        act->affect = (~rtrn.uval) & XkbSA_ISOAffectMask;
> +	act->affect = (~rtrn.uval) & XkbSA_ISOAffectMask;

Added a tab here. Also might as well change this to ival (strangely this
is what ResolveMask uses).

> +        act->flags &= ~(XkbSA_LockNoLock | XkbSA_LockNoUnlock);
> +        act->flags |= rtrn.ival & (XkbSA_LockNoLock | XkbSA_LockNoUnlock);
>          return True;
>      }
>      return ReportIllegal(action->type, field);
> @@ -943,6 +956,15 @@ HandleSetLockControls(XkbDescPtr xkb,
>          XkbActionSetCtrls(act, rtrn.uval);
>          return True;
>      }
> +    else if (field == F_Affect && action->type == XkbSA_LockControls) {
> +        if (array_ndx != NULL)
> +            return ReportActionNotArray(action->type, field);
> +        if (!ExprResolveEnum(value, &rtrn, lockWhich))
> +            return ReportMismatch(action->type, field, "lock or unlock");
> +        act->flags &= ~(XkbSA_LockNoLock | XkbSA_LockNoUnlock);
> +        act->flags |= rtrn.ival;
> +        return True;
> +    }
>      return ReportIllegal(action->type, field);
>  }
>  
> @@ -1289,7 +1311,7 @@ ApplyActionFactoryDefaults(XkbAction * action)
>      }
>      else if (action->type == XkbSA_ISOLock)
>      {
> -        action->iso.real_mods = LockMask;
> +        action->iso.real_mods = action->iso.mask = LockMask;
>      }
>      return;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> 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