[PATCH] dix: IsFloating() on master devices is always false

[Jeremy Huddleston]

This one makes me squirm a bit.  It *looks* right, but whenever I put one tidbit of input handling into my brain, another bit falls out, and it's usually that fallen-out bit that was the key to understanding why something won't work.

AIUI, this change will affect use cases like these:

xkb/xkbAccessX.c
694:    dev = IsFloating(mouse) ? mouse : GetMaster(mouse, MASTER_KEYBOARD);

IsMaster(mouse) can be true because mouse->type == MASTER_POINTER
IsFloating(mouse) was true because GetMaster(mouse, MASTER_KEYBOARD) == NULL
dev was being set to mouse
IsFloating(mouse) is now false, so dev is now NULL.

This particular case is probably fine because we check (dev && dev->key), but I wonder if there are other cases this will bring up.

In dix/events.c, ScreenRestructured():

    for (pDev = inputInfo.devices; pDev; pDev = pDev->next)
    {
        if (!IsFloating(pDev) && !DevHasCursor(pDev))
            continue;

IsFloating(pDev) was previously true for (pDev->type == MASTER_POINTER, GetMaster(pDev, MASTER_KEYBOARD) == NULL).  Now it is false.
IsFloating(pDev) is now false in this case, so !IsFloating(pDev) is now true.  If !DevHasCursor(pDev) is also true, we now skip this device whereas before we would've adjusted ConfineCursorToWindow(pDev).

I think this is actually ok that we're skipping it, and perhaps it was a bug that we were were doing ConfineCursorToWindow() previously, so I *think* this case is fine ...

I've given up on looking at other uses of IsFloating() because it is starting to hurt.  Can you please expand the tests in test/input.c to test floating devices.

Reviewed-by: Jeremy Huddleston <jeremyhu at apple.com>

--Jeremy

On Feb 19, 2012, at 19:23, Peter Hutterer wrote:

> There are a few subtle bugs during startup where IsFloating() returns true
> if the device is a master device that is not yet paired with its keyboard
> device.
> 
> Force IsFloating() to always return FALSE for master devices, that was the
> intent after all and any code that relies on the other behaviour should be
> fixed instead.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Turned out the vesa test case was another occurence of this same issue, so I
> think that instaead of the previous two patches (revert + fix) it's better
> to just push this one instead. After all, there may be more lingering bugs
> here so we might as well squash all of them preventively. 
> 
> It's a bit more bigger than I'd like at this point in the release but given
> that this is how the API was intended it should have few repercussions.
> 
> dix/events.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/dix/events.c b/dix/events.c
> index 3c7d5d0..96e3f9c 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -343,7 +343,7 @@ IsMaster(DeviceIntPtr dev)
> Bool
> IsFloating(DeviceIntPtr dev)
> {
> -    return GetMaster(dev, MASTER_KEYBOARD) == NULL;
> +    return !IsMaster(dev) && GetMaster(dev, MASTER_KEYBOARD) == NULL;
> }
> 
> 
> -- 
> 1.7.7.5
>