Bug 19297 - Mouse warping master/slave device mixup
Summary: Mouse warping master/slave device mixup
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Peter Hutterer
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-26 09:20 UTC by Pierre Willenbrock
Modified: 2009-01-12 15:26 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Make miPointerMove work on master device directly if called with one (1.70 KB, patch)
2008-12-26 09:20 UTC, Pierre Willenbrock
no flags Details | Splinter Review
0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch (5.20 KB, patch)
2009-01-07 20:26 UTC, Peter Hutterer
no flags Details | Splinter Review
Bits needed for attachment #21784 to work (2.76 KB, patch)
2009-01-09 10:04 UTC, Pierre Willenbrock
no flags Details | Splinter Review
0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch (4.67 KB, patch)
2009-01-11 21:39 UTC, Peter Hutterer
no flags Details | Splinter Review

Description Pierre Willenbrock 2008-12-26 09:20:51 UTC
Created attachment 21496 [details] [review]
Make miPointerMove work on master device directly if called with one

Mouse warping fail if a slave device with a non-identity scaling was the last used device. Depending on the scaling used, the cursor jumps to the top-left corner if the server handles a warp-request.

The above happens because GetPointerEvents does only work on slave devices, but is called from miPointerMove with a master device(I have no idea whether miPointerMove is supposed to cope with master devices).

Patch attached that makes miPointerMove patch up the device manually if it is called with a master device. This works, but i am not sure that is the correct way.
Comment 1 Peter Hutterer 2009-01-04 22:22:02 UTC
who is the caller when this happens? Are you using synergy or something similar?
Do you have a reproduceable test case?

While I think your patch fixes the problem, this is a bit of a sore spot which we currently have to patch around. It seems to be yet another candidate for a XTest virtual slave device.
Comment 2 Pierre Willenbrock 2009-01-05 05:06:00 UTC
I don't use something like synergy, this happens using a plain kde4, and probably using others.

I test with kde4 okular, which tries to move the cursor to the top/bottom of the screen if one drags the document to the other edge, so one can continue moving the document without releasing the mouse button. This works if a regular mouse or trackpoint is used, but fails for synaptics, which uses said non-identity scaling.

This is a backtrace of my current xserver, so miPointerMove is inlined into miPointerWarpCursor.
#0  miPointerWarpCursor (pDev=0x85d68c0, pScreen=0x822b448, x=525, y=1044)
    at mipointer.c:554
#1  0x08167562 in xf86WarpCursor (pDev=0x85d68c0, pScreen=0x822b448, x=525,
    y=1044) at xf86Cursor.c:475
#2  0x080dfb6f in miPointerSetCursorPosition (pDev=0x85d68c0,
    pScreen=0x822b448, x=525, y=1044, generateEvent=1) at mipointer.c:239
#3  0x081a6415 in AnimCurSetCursorPosition (pDev=0x85d68c0,
pScreen=0x822b448,
    x=525, y=1044, generateEvent=1) at animcur.c:277
#4  0x080c4582 in ProcWarpPointer (client=0xaeb9bb0) at events.c:3226
#5  0x080ba0ff in Dispatch () at dispatch.c:437
#6  0x080698dd in main (argc=6, argv=0xbfbf7df4, envp=0x0) at main.c:382

I don't know which of these functions is supposed to work with master or slave devices, so i put my changes at the last position that is not regularly used, on my system at least. And then i added stuff until it worked for me.
Comment 3 Peter Hutterer 2009-01-07 20:26:20 UTC
Created attachment 21784 [details] [review]
0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch

Verified. Unfortunately, your patch can't go in like this as it is against the protocol spec which require that an event is sent after a warp pointer request.

How about this one. Not perfect either, but we're only breaking XWarpDevicePointer, which is not released yet anyway.
Comment 4 Pierre Willenbrock 2009-01-08 02:58:28 UTC
Is this supposed to get a slave if dev is a master device?
+    if (dev->u.lastSlave)
+        dev = dev->u.lastSlave;
That won't work because u.lastSlave is unionized with u.master.
Otherwise, i think this should rather fail than push a master device down to  GetPointerEvents, or try to find a slave device by other means.
Comment 5 Peter Hutterer 2009-01-08 03:07:48 UTC
(In reply to comment #4)
> Is this supposed to get a slave if dev is a master device?
> +    if (dev->u.lastSlave)
> +        dev = dev->u.lastSlave;
> That won't work because u.lastSlave is unionized with u.master.

PickPointer always returns a master, so u.lastSlave - if not NULL - will always be a slave device. If it isn't then we have to push the master down and hope for the best.

> Otherwise, i think this should rather fail than push a master device down to 
> GetPointerEvents, or try to find a slave device by other means.

If there's no SD, then we don't have the problem of scaling in the first place, so we're fine.
Comment 6 Pierre Willenbrock 2009-01-09 07:32:09 UTC
(In reply to comment #5)
So i think this is fine. I just have one nitpick: it doesn't work ;-). 

The cursor is now warped to the bottom right corner. I will try to split the two parts of the patch, but i suspect the "assume that WarpPointer coordinates are always in screen coordinates"-part is not respected everywhere.
Comment 7 Pierre Willenbrock 2009-01-09 10:04:49 UTC
Created attachment 21835 [details] [review]
Bits needed for attachment #21784 [details] [review] to work

Using this patch together with attachment #21784 [details] [review] works.
Comment 8 Peter Hutterer 2009-01-11 21:39:14 UTC
Created attachment 21890 [details] [review]
0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch

ok, how about this one. If POINTER_SCREEN is set, scale valuators to device coordinates before continuing. Seems to work here, but then again - so did the previous patch.

btw. what's the app that triggers this behaviour? Your patch fixed the behaviour for GPE being called for events with only valuators[2:] set, which is quite unusual for a warp pointer request (if it still is warp pointer).
Comment 9 Pierre Willenbrock 2009-01-12 05:17:12 UTC
(In reply to comment #8)
> Created an attachment (id=21890) [details]
> 0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch
> 
> ok, how about this one. If POINTER_SCREEN is set, scale valuators to device
> coordinates before continuing. Seems to work here, but then again - so did the
> previous patch.

that one works.

> btw. what's the app that triggers this behaviour? Your patch fixed the
> behaviour for GPE being called for events with only valuators[2:] set, which is
> quite unusual for a warp pointer request (if it still is warp pointer).

Still the same as in comment #2: okular from kde4. I don't see where i did anything special that makes you think it helps only for valuators[2:]. The real meat of my patch is 1: disable clipping in moveAbsolute(synaptics coordinates start at around 1400,1400) and 2: convert the screen coordinates to device coordinates after setting the pointer sprite. Fetching the screen coordinates in moveAbsolute is probably never used. Simply disabling clipping is probably not the right thing, though.

Comment 10 Peter Hutterer 2009-01-12 15:26:26 UTC
> (In reply to comment #8)
> > Created an attachment (id=21890)
>  --> (http://bugs.freedesktop.org/attachment.cgi?id=21890) [details]
> > 0001-dix-fix-WarpPointer-calls-for-devices-with-custom-v.patch
> > 
> > ok, how about this one. If POINTER_SCREEN is set, scale valuators to device
> > coordinates before continuing. Seems to work here, but then again - so did the
> > previous patch.
> 
> that one works.

Pushed as d36adf52a2b2711d22b11105f7bd907d4493fb9b. Closing as fixed.

> > btw. what's the app that triggers this behaviour? Your patch fixed the
> > behaviour for GPE being called for events with only valuators[2:] set, which is
> > quite unusual for a warp pointer request (if it still is warp pointer).
> 
> Still the same as in comment #2: okular from kde4. I don't see where i did
> anything special that makes you think it helps only for valuators[2:]. 

The first hunks of the patch threw me off I guess since they only apply for
valuators >= 2. 


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.