[PATCH] xkb: Fix repeat behaviour of redirect and message actions
Peter Hutterer
peter.hutterer at who-t.net
Tue Jan 22 22:52:35 PST 2013
On Tue, Jan 22, 2013 at 09:24:53PM +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 halved.
>
> Signed-off-by: Andreas Wettstein <wettstein509 at solnet.ch>
> ---
> xkb/xkbActions.c | 145 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 89 insertions(+), 56 deletions(-)
>
> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
> index 8cd5f5c..f19505d 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((const void *) pMsg, (const void *) pAction, 8) == 0) {
no need for the casts.
> + /* 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;
> + /* 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;
> }
are you sure about removing that last return statement? it breaks
compilation, so maybe you missed a hunk here?
the rest looks ok, but I admit I've only given this a cursory review.
Cheers,
Peter
>
> 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
>
More information about the xorg-devel
mailing list