Ok thanks. Reviewed-by: Jeremy Huddleston <jerem...@apple.com>
On Feb 20, 2012, at 5:14 PM, Peter Hutterer <peter.hutte...@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 <jerem...@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.hutte...@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 >>> >> > _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel