[PATCH 3/3] xkbcomp: Add missing flag support and correct modifier handling of ISOLock
Ran Benita
ran234 at gmail.com
Fri Feb 14 02:22:31 PST 2014
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
More information about the xorg-devel
mailing list