[PATCH (revised)] xkb: Fix repeat behaviour of redirect and message actions

[Peter Hutterer]

On Tue, Jan 29, 2013 at 09:49:20PM +0100, Andreas Wettstein wrote:
> The redirect and the message action filter functions implicitly assumed that
> when they receive an event for the same keycode they were activated for, that
> this is the a release of the key that activated the filter.  This is not true
> if the key autorepeats.  Due to the incorrect assumption, the effective key
> repeat rate was effectively halved.
> 
> Signed-off-by: Andreas Wettstein <wettstein509 at solnet.ch>

applied, thanks

Cheers,
   Peter

> ---
>  xkb/xkbActions.c | 147 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 90 insertions(+), 57 deletions(-)
> 
> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
> index 8cd5f5c..da727e6 100644
> --- a/xkb/xkbActions.c
> +++ b/xkb/xkbActions.c
> @@ -754,6 +754,15 @@ _XkbFilterActionMessage(XkbSrvInfoPtr xkbi,
>      XkbMessageAction *pMsg;
>      DeviceIntPtr kbd;
>  
> +    if ((filter->keycode != 0) && (filter->keycode != keycode))
> +	return 1;
> +
> +    /* This can happen if the key repeats, and the state (modifiers or group)
> +       changes meanwhile. */
> +    if ((filter->keycode == keycode) && pAction &&
> +	(pAction->type != XkbSA_ActionMessage))
> +	return 1;
> +
>      kbd = xkbi->device;
>      if (filter->keycode == 0) { /* initial press */
>          pMsg = &pAction->msg;
> @@ -781,20 +790,27 @@ _XkbFilterActionMessage(XkbSrvInfoPtr xkbi,
>      }
>      else if (filter->keycode == keycode) {
>          pMsg = &filter->upAction.msg;
> -        if (pMsg->flags & XkbSA_MessageOnRelease) {
> -            xkbActionMessage msg;
> -
> -            msg.keycode = keycode;
> -            msg.press = 0;
> -            msg.keyEventFollows =
> -                ((pMsg->flags & XkbSA_MessageGenKeyEvent) != 0);
> -            memcpy((char *) msg.message, (char *) pMsg->message,
> -                   XkbActionMessageLength);
> -            XkbSendActionMessage(kbd, &msg);
> +	if (pAction == NULL) {
> +	    if (pMsg->flags & XkbSA_MessageOnRelease) {
> +		xkbActionMessage msg;
> +
> +		msg.keycode = keycode;
> +		msg.press = 0;
> +		msg.keyEventFollows =
> +		    ((pMsg->flags & XkbSA_MessageGenKeyEvent) != 0);
> +		memcpy((char *) msg.message, (char *) pMsg->message,
> +		       XkbActionMessageLength);
> +		XkbSendActionMessage(kbd, &msg);
> +	    }
> +	    filter->keycode = 0;
> +	    filter->active = 0;
> +	    return ((pMsg->flags & XkbSA_MessageGenKeyEvent) != 0);
> +	} else if (memcmp(pMsg, pAction, 8) == 0) {
> +	    /* Repeat: If we send the same message, avoid multiple messages
> +	       on release from piling up. */
> +	    filter->keycode = 0;
> +	    filter->active = 0;
>          }
> -        filter->keycode = 0;
> -        filter->active = 0;
> -        return ((pMsg->flags & XkbSA_MessageGenKeyEvent) != 0);
>      }
>      return 1;
>  }
> @@ -810,15 +826,21 @@ _XkbFilterRedirectKey(XkbSrvInfoPtr xkbi,
>      xkbDeviceInfoPtr xkbPrivPtr = XKBDEVICEINFO(xkbi->device);
>      ProcessInputProc backupproc;
>  
> +    if ((filter->keycode != 0) && (filter->keycode != keycode))
> +        return 1;
> +
> +    /* This can happen if the key repeats, and the state (modifiers or group)
> +       changes meanwhile. */
> +    if ((filter->keycode == keycode) && pAction &&
> +	(pAction->type != XkbSA_RedirectKey))
> +	return 1;
> +
>      /* never actually used uninitialised, but gcc isn't smart enough
>       * to work that out. */
>      memset(&old, 0, sizeof(old));
>      memset(&old_prev, 0, sizeof(old_prev));
>      memset(&ev, 0, sizeof(ev));
>  
> -    if ((filter->keycode != 0) && (filter->keycode != keycode))
> -        return 1;
> -
>      GetSpritePosition(xkbi->device, &x, &y);
>      ev.header = ET_Internal;
>      ev.length = sizeof(DeviceEvent);
> @@ -877,49 +899,60 @@ _XkbFilterRedirectKey(XkbSrvInfoPtr xkbi,
>              xkbi->state = old;
>              xkbi->prev_state = old_prev;
>          }
> +	return 0;
>      }
> -    else if (filter->keycode == keycode) {
> -
> -        ev.type = ET_KeyRelease;
> -        ev.detail.key = filter->upAction.redirect.new_key;
> -
> -        mask = XkbSARedirectVModsMask(&filter->upAction.redirect);
> -        mods = XkbSARedirectVMods(&filter->upAction.redirect);
> -        if (mask)
> -            XkbVirtualModsToReal(xkbi->desc, mask, &mask);
> -        if (mods)
> -            XkbVirtualModsToReal(xkbi->desc, mods, &mods);
> -        mask |= filter->upAction.redirect.mods_mask;
> -        mods |= filter->upAction.redirect.mods;
> -
> -        if (mask || mods) {
> -            old = xkbi->state;
> -            old_prev = xkbi->prev_state;
> -            xkbi->state.base_mods &= ~mask;
> -            xkbi->state.base_mods |= (mods & mask);
> -            xkbi->state.latched_mods &= ~mask;
> -            xkbi->state.latched_mods |= (mods & mask);
> -            xkbi->state.locked_mods &= ~mask;
> -            xkbi->state.locked_mods |= (mods & mask);
> -            XkbComputeDerivedState(xkbi);
> -            xkbi->prev_state = xkbi->state;
> -        }
> -
> -        UNWRAP_PROCESS_INPUT_PROC(xkbi->device, xkbPrivPtr, backupproc);
> -        xkbi->device->public.processInputProc((InternalEvent *) &ev,
> -                                              xkbi->device);
> -        COND_WRAP_PROCESS_INPUT_PROC(xkbi->device, xkbPrivPtr, backupproc,
> -                                     xkbUnwrapProc);
> -
> -        if (mask || mods) {
> -            xkbi->state = old;
> -            xkbi->prev_state = old_prev;
> -        }
> -
> -        filter->keycode = 0;
> -        filter->active = 0;
> +    else {
> +	/* If it is a key release, or we redirect to another key, release the
> +	   previous new_key.  Otherwise, repeat. */
> +	ev.detail.key = filter->upAction.redirect.new_key;
> +	if (pAction == NULL ||  ev.detail.key != pAction->redirect.new_key) {
> +	    ev.type = ET_KeyRelease;
> +	    filter->active = 0;
> +	}
> +	else {
> +	    ev.type = ET_KeyPress;
> +	    ev.key_repeat = TRUE;
> +	}
> +
> +	mask = XkbSARedirectVModsMask(&filter->upAction.redirect);
> +	mods = XkbSARedirectVMods(&filter->upAction.redirect);
> +	if (mask)
> +	    XkbVirtualModsToReal(xkbi->desc, mask, &mask);
> +	if (mods)
> +	    XkbVirtualModsToReal(xkbi->desc, mods, &mods);
> +	mask |= filter->upAction.redirect.mods_mask;
> +	mods |= filter->upAction.redirect.mods;
> +
> +	if (mask || mods) {
> +	    old = xkbi->state;
> +	    old_prev = xkbi->prev_state;
> +	    xkbi->state.base_mods &= ~mask;
> +	    xkbi->state.base_mods |= (mods & mask);
> +	    xkbi->state.latched_mods &= ~mask;
> +	    xkbi->state.latched_mods |= (mods & mask);
> +	    xkbi->state.locked_mods &= ~mask;
> +	    xkbi->state.locked_mods |= (mods & mask);
> +	    XkbComputeDerivedState(xkbi);
> +	    xkbi->prev_state = xkbi->state;
> +	}
> +
> +	UNWRAP_PROCESS_INPUT_PROC(xkbi->device, xkbPrivPtr, backupproc);
> +	xkbi->device->public.processInputProc((InternalEvent *) &ev,
> +					      xkbi->device);
> +	COND_WRAP_PROCESS_INPUT_PROC(xkbi->device, xkbPrivPtr, backupproc,
> +				     xkbUnwrapProc);
> +
> +	if (mask || mods) {
> +	    xkbi->state = old;
> +	    xkbi->prev_state = old_prev;
> +	}
> +
> +	/* We return 1 in case we have sent a release event because the new_key
> +	   has changed.  Then, subsequently, we will call this function again
> +	   with the same pAction, which will create the press for the new
> +	   new_key. */
> +	return (pAction && ev.detail.key != pAction->redirect.new_key);
>      }
> -    return 0;
>  }
>  
>  static int
> -- 
> 1.8.0
> 
> _______________________________________________
> 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
>