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

[Peter Hutterer]

On Sun, Feb 19, 2012 at 08:32:26PM -0800, Jeremy Huddleston wrote:
> 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:

first, and that may help understanding this issue: calling IsFloating() on a
master device returned FALSE because a master is always paired with
keyboard/pointer. However, there is a short window where a pointer device is
not yet paired but the pointer is about to be displayed. During that window,
IsFloating(master) will return TRUE and that causes the issues we've seen.
This patch simply closes that window, it has no affect otherwise.

I'll figure out some tests though.

Cheers,
  Peter

> 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
> > 
>