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

[Jeremy Huddleston]

Ok thanks.

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


On Feb 20, 2012, at 5:14 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:

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