[PATCH] Don't release passive grabs unless all buttons are up

[Peter Hutterer]

On Sun, Dec 21, 2008 at 10:14:53PM +0100, Thomas Jaeger wrote:
> This turned out to be a little bit trickier than I initially thought,
> since buttonsDown counts the number of physical buttons that are down,
> before they are mapped to logical buttons.
> 
> The thing I'm unsure about (as should be evident from the patch), is
> where to stick the declaration for the new function AllButtonsAreUp.
> 
> Thanks,
> Tom
> 
> Thomas Jaeger wrote:
> > I can't see any reason why we would treat buttons > 5 differently.  This
> > patch simplifies client code by eliminating the need to call XGrabDevice
> > after a button has been pressed and prevents race conditions that could
> > result from that.


Following up on that:
Both the core protocol and the XI protocol spec state that a passive grab is
to be released when all buttons are logically up, regardless of modifiers.
This is where the magic number 5 comes in. Core allows for 5 buttons, XI
allows for more.

This puts us in a difficult position. Core requires us to release the grab if,
say, only button 6 is down. XI requires us to maintain the grab.
I think we need a more complex solution than the one you proposed: test
whether the passive grab is a core or an XI grab and release depending on this
condition.
Something like 
if ((grab->coreGrab && !button->state) && ... ||
    (!grab->coreGrab && AllButtonsAreUp(b))

Keith, any opinions on this?

Cheers,
  Peter

> From 86c2f7e8c37532287772aae1c2554da5fd5eda86 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 20 Dec 2008 16:17:02 +0100
> Subject: [PATCH] Don't release grabs unless all buttons are up
> 
> Previously, only buttons <= 5 would count here.
> ---
>  Xi/exevents.c      |    2 +-
>  dix/devices.c      |   18 ++++++++++++++++++
>  dix/events.c       |    2 +-
>  include/inputstr.h |    3 +++
>  4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Xi/exevents.c b/Xi/exevents.c
> index 083bb2f..07b7341 100644
> --- a/Xi/exevents.c
> +++ b/Xi/exevents.c
> @@ -1069,7 +1069,7 @@ ProcessOtherEvent(xEventPtr xE, DeviceIntPtr device, int count)
>  	    xE->u.u.detail = key;
>  	    return;
>  	}
> -        if (!b->state && device->deviceGrab.fromPassiveGrab)
> +        if (device->deviceGrab.fromPassiveGrab && AllButtonsAreUp(b))
>              deactivateDeviceGrab = TRUE;
>      }
>  
> diff --git a/dix/devices.c b/dix/devices.c
> index d89f6c3..10dca2e 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -1996,6 +1996,24 @@ NoteLedState(DeviceIntPtr keybd, int led, Bool on)
>  	ctrl->leds &= ~((Leds)1 << (led - 1));
>  }
>  
> +Bool
> +AllButtonsAreUp(ButtonClassPtr butc)
> +{
> +    int key;
> +    int missingButtons = butc->buttonsDown;
> +
> +    for (key = 0; missingButtons && key < butc->numButtons; key++)
> +    {
> +	int bit = 1 << (key & 7);
> +	if (butc->down[key >> 3] & bit) {
> +	    if (butc->map[key])
> +		return False;
> +	    missingButtons--;
> +	}
> +    }
> +    return True;
> +}
> +
>  _X_EXPORT int
>  Ones(unsigned long mask)             /* HACKMEM 169 */
>  {
> diff --git a/dix/events.c b/dix/events.c
> index d7618c2..0105e82 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -3846,7 +3846,7 @@ ProcessPointerEvent (xEvent *xE, DeviceIntPtr mouse, int count)
>  	    if (xE->u.u.detail == 0)
>  		return;
>              filters[mouse->id][Motion_Filter(butc)] = MotionNotify;
> -	    if (!butc->state && mouse->deviceGrab.fromPassiveGrab)
> +	    if (mouse->deviceGrab.fromPassiveGrab && AllButtonsAreUp(butc))
>  		deactivateGrab = TRUE;
>  	    break;
>  	default:
> diff --git a/include/inputstr.h b/include/inputstr.h
> index 9591d2f..a94ea01 100644
> --- a/include/inputstr.h
> +++ b/include/inputstr.h
> @@ -200,6 +200,9 @@ typedef struct _ButtonClassRec {
>  #endif
>  } ButtonClassRec, *ButtonClassPtr;
>  
> +extern Bool AllButtonsAreUp(
> +    ButtonClassPtr /*butc*/);
> +
>  typedef struct _FocusClassRec {
>      WindowPtr	win; /* May be set to a int constant (e.g. PointerRootWin)! */
>      int		revert;
> -- 
> 1.5.6.3