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 <[email protected]> > Signed-off-by: Andreas Wettstein <[email protected]> > --- > 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 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
