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

[Vitaliy Margolen]

On 01/22/2010 03:42 PM, Lauri Kenttä wrote:
>> The SetCursorPos() calls hooks on Wine because Wine has big issue - it
>> doesn't filter pointer move messages that were caused by Wine itself.
> 
> The first patch (http://source.winehq.org/patches/data/57569) tries to
> address exactly this. I think XWarpCursor is not a problem, as the event
> comes asynchronously with some delay. So my patch fixes the easier but
> worse case, calling hooks directly from SetCursorPos.
> 
> What's wrong with this, why is it wrong, and do you have a better idea?
> What else causes Wine to produce extra messages?
I'm fine with this patch. According to your test SetCursorPos() indeed
doesn't generate ll hook. Was an oversight on my part when I added that
extra queue_raw_mouse_message() call.

Just Keep the formatting the same (curly brace on it's own line). And remove
FIXME comment - it's not the only place that calls XWarpPointer(). Oh and
make it a series with your test patch. This patch being #1 and the test #2.

> Also, is there a reason why I shouldn't write a test for this to make the
> issue better known?
By all means. The way things work in Wine - if tests fail on Wine you should
use "todo_wine" at front of each failing ok() call. So 'make test' always
succeeds.

>>> 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.
> 
> (This was the elaboration I was asking for.) I didn't know that, and I
> don't have any such games. Feel free to reject the patch, I'll rewrite it
> without this bug and in smaller chunks. (It would be helpful if the first
> two patches were also either accepted or rejected.)
I'm not the one accepting or rejecting patches - Alexandre is. I'm just one
of the developers who made lots of changes to that particular area of the
code to match what native does.

In this case I'm worrying about mouse movements on the edge of the screen.
Remember that ll hook is supposed to be called with whatever came straight
from the mouse driver. Not the pointer. And to test it you'll need to use
injected events (mouse_event). SetCursorPos() won't do for this, it only
alters pointer position.

>>> returns unclipped coordinates from GetCursorPos
>> This is what native will do inside hook handler. Add some tests.
> That's not true. (See below.)
Yes, you right here. I was thinking about something else - GetCursorPos()
returns previous position while inside hook handler.

> I've attached a program that outputs coordinates from hookproc and
> wndproc, both the passed ones and the ones from GetCursorPos. If you try
> it, you can see that Wine has the following bugs:
> - incorrect coordinates in some WM:s
> - incorrect coordinates in some hook calls
> - incorrect coordinates and missing WM:s after clipping
> - extra hook calls on SetCursorPos, even if the pos doesn't change
All these can be fixed. But will need Wine tests to verify and keep it from
breaking in the future. And need one patch for each problem unless it's a
same fix for all of them. If things break separate patch per issue will help
identify the change during regression testing.

> - extra WM_MOUSEMOVE after zero-length moves
Careful with this. Some games do require it. That's why it's there with the
comment (in SetCursorPos). Or is there another source of them?

> - some other extra hook calls and WM:s due to XWarpPointer
To fix this Wine have to keep track of all XWarpPointer calls and all
generated X events so it can filter ones that came from warping pointer.

> As you can see, the correct order would be:
> - call hooks for moving, return old pos from GetCursorPos
> - clip and store the new position
> - send the move message using the new position
> - call hooks and send the button message, both using the clipped position
Looks right.

>> What exactly is wrong with [border scrolling]?
> Many games (for example, Baldur's Gate) check for mouse coordinates. If
> the pointer is outside the virtual desktop, Wine gives negative
> coordinates and the game doesn't scroll left/up, because x != 0 and y !=
> 0. The same thing happens to right/bottom, of course. Correct clipping
> fixes it.
That explains why some games have this issue.

>> Who said that Wine should keep the pointer inside virtual desktop?
> Ok, I'll not do it. But don't you think it's a bit confusing if the
> pointer moves freely but the program thinks it's clipped to an area?
I've had that discussion with Alexandre. His point was - don't confine
pointer to the virtual desktop because Wine has it exactly for the reason of
letting you run full-screen games in the window, as a work around for games
not having windowed mode. And to let you use your system when Wine
crashes/locks up.

The question of what to do when cursor is outside of VD - I personally thing
we should just clip coordinates to VD and pass them to the app. Might need
to do something different when Wine looses focus...

> My apologies for bringing up something that is actually none of my
> business, but I think you should pay more attention to the way you write
> your comments.
I'm sorry if some of my comments came extra harsh. Didn't intended that way.
All I wanted to do is to review your patches and make sure you are aware of
all the hidden gotchas in that piece of code. And of course help you get all
your work past Alexandre.

Vitaliy.