[PATCH 1/4] xkb: LockMods can lock another group on key release #865

[Peter Hutterer]

On Sat, Nov 10, 2012 at 05:08:20PM +0100, Andreas Wettstein wrote:
> Two unused bytes in 'XkbModAction' are used to allow 'LockMods' to
> perform group locks as well.  These two bytes are interpreted as the
> 'flags' and the 'group_XXX' bytes in 'XkbGroupAction'.  When the key
> that 'LockMods' is assigned to is pressed and released before any
> other key is pressed, a group lock as specified by these two bytes is
> performed.  Otherwise, 'LockMods' operates according to the XKB
> protocol specification.  If the two bytes are zero, which is the case
> by default, also no group change happens, so 'LockMods' operates
> according to the specification.
> 
> Signed-off-by: Andreas Wettstein <wettstein509 at solnet.ch>

ok, I looked at all four and they do look good and look like they do what
they should. The only two comments I have at this point are:
* where are the bytes guaranteed to be zero? Not questioning, I just want to
  double-check and it's outside the patch contexts
* XkbModAction increases by 2 bytes. this is really my main worry here,
  it is semi-public API. from a protocol POV it's fine because XKbAction
  forces it to 8 bytes anyway so none of the protocol parsers should be
  affected (fwiw, xcb would need an update here as well)

So really, the only danger we have here is potentially breaking clients that
expect XkbModAction to be 6 bytes, because of whatever reason. The safest
approach would be to add a new struct here (XkbModAction2) and use that
everywhere to avoid this issue. Daniel, any opinion?

Cheers,
   Peter

> ---
>  include/xkbstr.h |  3 +++
>  xkb/xkbActions.c |  7 +++++++
>  xkb/xkbtext.c    | 26 ++++++++++++++++++++++++--
>  3 Dateien geändert, 34 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
> 
> diff --git a/include/xkbstr.h b/include/xkbstr.h
> index 935330c..8fd7daf 100644
> --- a/include/xkbstr.h
> +++ b/include/xkbstr.h
> @@ -127,6 +127,9 @@ typedef struct _XkbModAction {
>      /* FIXME: Make this an int. */
>      unsigned char vmods1;
>      unsigned char vmods2;
> +    /* The effect on groups is an extension beyond the XKB specification */
> +    unsigned char group_flags;
> +    char	  group_XXX;
>  } XkbModAction;
>  
>  #define	XkbModActionVMods(a) ((short) (((a)->vmods1 << 8) | (a)->vmods2))
> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
> index 1adb389..8cd5f5c 100644
> --- a/xkb/xkbActions.c
> +++ b/xkb/xkbActions.c
> @@ -366,6 +366,13 @@ _XkbFilterLockState(XkbSrvInfoPtr xkbi,
>          xkbi->clearMods = filter->upAction.mods.mask;
>          if (!(filter->upAction.mods.flags & XkbSA_LockNoUnlock))
>              xkbi->state.locked_mods &= ~filter->priv;
> +	if (filter->upAction.mods.group_flags & XkbSA_GroupAbsolute)
> +	     xkbi->state.locked_group = XkbSAGroup(&filter->upAction.mods);
> +	else xkbi->state.locked_group += XkbSAGroup(&filter->upAction.mods);
> +    }else if(pAction){
> +	/* Another key has been pressed: do not change group on release */
> +	XkbSASetGroup(&filter->upAction.mods, 0);
> +	filter->upAction.mods.group_flags &= ~XkbSA_GroupAbsolute;
>      }
>      return 1;
>  }
> diff --git a/xkb/xkbtext.c b/xkb/xkbtext.c
> index fdf1d17..a10639f 100644
> --- a/xkb/xkbtext.c
> +++ b/xkb/xkbtext.c
> @@ -737,6 +737,7 @@ CopyModActionArgs(XkbDescPtr xkb, XkbAction *action, char *buf, int *sz)
>  {
>      XkbModAction *act;
>      unsigned tmp;
> +    char tbuf[32];
>  
>      act = &action->mods;
>      tmp = XkbModActionVMods(act);
> @@ -749,8 +750,29 @@ CopyModActionArgs(XkbDescPtr xkb, XkbAction *action, char *buf, int *sz)
>      }
>      else
>          TryCopyStr(buf, "none", sz);
> -    if (act->type == XkbSA_LockMods)
> -        return TRUE;
> +    if (act->type == XkbSA_LockMods){
> +	switch (act->flags & (XkbSA_LockNoUnlock | XkbSA_LockNoLock)) {
> +	case XkbSA_LockNoLock:
> +	    sprintf(tbuf, ",affect=unlock"); break;
> +	case XkbSA_LockNoUnlock:
> +	    sprintf(tbuf, ",affect=lock"); break;
> +	case XkbSA_LockNoUnlock | XkbSA_LockNoLock:
> +	    sprintf(tbuf, ",affect=neither"); break;
> +	default:
> +	    sprintf(tbuf, ""); break;
> +	}
> +	TryCopyStr(buf, tbuf, sz);
> +	if(XkbSAGroup(act) != 0 || (act->group_flags & XkbSA_GroupAbsolute)){
> +	    TryCopyStr(buf, ",group=", sz);
> +	    if (act->group_flags & XkbSA_GroupAbsolute)
> +		sprintf(tbuf, "%d", XkbSAGroup(act)+1);
> +	    else if (XkbSAGroup(act) < 0)
> +		sprintf(tbuf, "%d", XkbSAGroup(act));
> +	    else sprintf(tbuf, "+%d", XkbSAGroup(act));
> +	    TryCopyStr(buf, tbuf, sz);
> +	}
> +	return TRUE;
> +    }
>      if (act->flags & XkbSA_ClearLocks)
>          TryCopyStr(buf, ",clearLocks", sz);
>      if (act->flags & XkbSA_LatchToLock)
> -- 
> 1.7.11.3
> 
> _______________________________________________
> 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
>