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.