Re: winex11.drv: Added missing mouse position clipping. (try 2)

[Vitaliy Margolen]

Lauri Kenttä wrote:
> When the cursor position is clipped with ClipCursor, the cursor should be
> confined to that area. This patch fixes cursor position clipping in
> X11DRV_ClipCursor, X11DRV_GetCursorPos, X11DRV_SetCursorPos and
> X11DRV_send_mouse_input and makes the mouse warp to the new position if
> moved outside the clip rect.
>
> The previous try fixed only clipping (not warping) and only in
> GetCursorPos, but the problem wasn't that small after all.
> ---
>  dlls/winex11.drv/mouse.c |   52
> ++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 41 insertions(+), 11 deletions(-)

You changing way too many things without a single test. Lots of those areas
you touch will break oh so many apps.

For starters write some tests and verify what you doing is correct. You
should be able to write tests for all injected messages (send_mouse_event).
If you can't make an automated test, write a visual test for the rest and
send to wine-devel. Especially pay attention to all messages sent, and order
of calling hook, sending messages, modifying values returned from
GetCursorPos(). You breaking that.

And don't try to keep pointer inside Wine's window - this won't get past AJ.

Vitaliy.

winex11.drv: Added missing mouse position clipping. (try 2)

[Lauri Kenttä]

Thanks for your comments, Vitaliy.

At first, I must point out that you're commenting on a patch that has
already been superseded. Not that there's much difference, but the new
series of patches has some related changes (including a new test) applied
before this one.

Vitaliy Margolen wrote:
> Lots of those areas you touch will break oh so many apps.

Could you please elaborate that "so many"?

I'm aware that my version causes hooks to be called with clipped
coordinates on mouse move, when they should in that case be called with
the unclipped ones. Still, please note that the current version calls all
hooks (not just on mouse move) with the unclipped coordinates, returns
unclipped coordinates from GetCursorPos and even sends incorrect WM_*
messages. This is clearly wrong and causes many problems (example: border
scrolling is next to impossible in many full-screen games when played on
Wine virtual desktop).

I sincerely think my version is less likely to break anything. Surely
there are more applications using WM_* and GetCursorPos than hooks.

Maybe I will have time to fix the remaining bugs some day. I will also try
to write some Wine tests so that these problems (be they in my version or
the old one) are not forgotten.

Vitaliy Margolen wrote:
> And don't try to keep pointer inside Wine's window

Anyone could do exactly the same thing "manually" by calling SetCursorPos,
so why shouldn't ClipCursor do it automatically? Also, apparently Wine
won't (or can't) warp the mouse if it lies outside the virtual desktop.

As I see it, clipping should first be implemented correctly, because
that's what Windows applications expect. Then, if this causes problems, we
could use a registry key similar to DXGrab to disable (or restrict)
XWarpPointer usage.

-- 
Lauri Kenttä

winex11.drv: Added missing mouse position clipping. (try 2)

[Vitaliy Margolen]

Lauri Kenttä wrote:
> The new series of patches has some related changes (including a new test) applied before this one.
You have some issues in your test as well. The SetCursorPos() calls hooks on
Wine because Wine has big issue - it doesn't filter pointer move messages
that were caused by Wine itself. What you did in your patch is a half fix
the wrong way.

> Could you please elaborate that "so many"?
I can't give you all the programs/games that depend on particular behavior -
there are big number of them.

> I'm aware that my version causes hooks to be called with clipped
> coordinates on mouse move, when they should in that case be called with
> the unclipped ones.
This particular change will break all games using dinput when mouse pointer
goes outside of virtual desktop.

> returns unclipped coordinates from GetCursorPos
This is what native will do inside hook handler. Add some tests.

> and even sends incorrect WM_* messages.
Tests please.

> This is clearly wrong and causes many problems (example: border
> scrolling is next to impossible in many full-screen games when played on
> Wine virtual desktop).
What exactly is wrong with them? And what part are you trying to fix?

> I sincerely think my version is less likely to break anything. Surely
> there are more applications using WM_* and GetCursorPos than hooks.
This is not a reason to break anything. You either fix things without
breaking anything else. Or you don't touch the code at all.

> Anyone could do exactly the same thing "manually" by calling SetCursorPos,
> so why shouldn't ClipCursor do it automatically? Also, apparently Wine
> won't (or can't) warp the mouse if it lies outside the virtual desktop.
Number of reasons:
- Who said that Wine should keep the pointer inside virtual desktop? It's
there for the exact reason so users can have other apps running outside of
Wine at the same time as full-screen game inside Wine.
- You doing it wrong. Warping pointer is not the right way. If you really
must do it, then you should be using pointer grab. Also you are not dealing
with all the extra pointer move messages generated as a result of
XWarpPointer().


Vitaliy.