> 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?
Also, is there a reason why I shouldn't write a test for this to make the
issue better known?
>> 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.)
>> returns unclipped coordinates from GetCursorPos
> This is what native will do inside hook handler. Add some tests.
That's not true. (See below.)
>> and even sends incorrect WM_* messages.
> Tests please.
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
- extra WM_MOUSEMOVE after zero-length moves
- some other extra hook calls and WM:s due to XWarpPointer
The next snippets show the WM bug and how GetCursorPos works inside hook
handlers. This first one is from Win2k8 Server. First, the pointer is at
(34,34) and clipping is set to (30,30)-(40,40). Then the program calls
mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 6, 0, 0).
11: hookproc: WM_MOUSEMOVE: (34,40), get = (34,34), injected = 1
11: hookproc: WM_LBUTTONUP: (34,39), get = (34,39), injected = 1
11: wndproc: WM_MOUSEMOVE: (34,39), get = (34,39)
11: wndproc: WM_LBUTTONUP: (34,39), get = (34,39)
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
Compare to current Wine output:
11: hookproc: WM_MOUSEMOVE: (34,40), get = (34,34), injected = 1
11: hookproc: WM_LBUTTONUP: (34,40), get = (34,39), injected = 1
11: hookproc: WM_MOUSEMOVE: (34,39), get = (34,39), injected = 0
11: wndproc: WM_MOUSEMOVE: (34,40), get = (34,39)
11: wndproc: WM_LBUTTONUP: (34,40), get = (34,39)
11: wndproc: WM_MOUSEMOVE: (34,39), get = (34,39)
Aside of the extra move caused by XWarpPointer, the second hook and both
injected messages have incorrect (unclipped) coordinates.
> 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.
> 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?
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. First, even small positive comments are considered
psychologically very important, but you have given none. Second, most of
your negative feedback has only stated that the patches are bad and wrong,
often without giving much details or any better ideas. That is, the
comments haven't been very constructive. Currently your messages look a
bit like "f*** off, noob", which hopefully is not what you had in mind.
Anyway, this is certainly not a good way to encourage spending time to
Wine development. Some (luckily not me) would take it badly and just rm
-rf wine-git and never try again, even if they could be a great help with
some guidance. So let's all be nice to each other, and everyone will be
happier.
--
Lauri Kenttä
#define _WIN32_WINNT 0x0500
#include <windows.h>
#include <stdio.h>
int stage = 0;
RECT *tmp_rect(int x0, int y0, int x1, int y1) {
static RECT r;
SetRect(&r, x0, y0, x1, y1);
return &r;
}
INPUT *tmp_input(DWORD flags, int x, int y) {
static INPUT input;
input.type = INPUT_MOUSE;
input.mi.dwFlags = flags;
input.mi.dx = x;
input.mi.dy = y;
return &input;
}
const char *tmp_pos_str(void) {
static char buf[100];
POINT pt;
GetCursorPos(&pt);
sprintf(buf, "(%d,%d)", (int) pt.x, (int) pt.y);
return buf;
}
const char *msg_name(UINT msg) {
switch (msg) {
#define CASE(x) case x: return # x
CASE(WM_MOUSEMOVE);
CASE(WM_LBUTTONDOWN);
CASE(WM_LBUTTONUP);
#undef CASE
}
return 0;
}
void events(HWND hwnd) {
MSG msg;
Sleep(100); /* Wait for XWarpCursor */
while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
TranslateMessage(&msg);
DispatchMessage(&msg);
}
}
LRESULT CALLBACK wndproc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam)
{
if (msg_name(message) && stage) {
printf("%d: wndproc: %s: (%d,%d), get = %s\n", stage, msg_name(message), LOWORD(lparam), HIWORD(lparam), tmp_pos_str());
return 0;
}
return DefWindowProc(hwnd, message, wparam, lparam);
}
LRESULT CALLBACK hook_proc(int code, WPARAM message, LPARAM lparam) {
MSLLHOOKSTRUCT *data = (MSLLHOOKSTRUCT*) lparam;
if (msg_name(message) && stage) {
printf("%d: hookproc: %s: (%d,%d), get = %s, injected = %d\n", stage, msg_name(message), (int) data->pt.x, (int) data->pt.y, tmp_pos_str(), (int) (data->flags & 1));
}
return CallNextHookEx(0, code, message, lparam);
}
int WINAPI WinMain(HINSTANCE hinst, HINSTANCE _1, LPSTR _2, int _3) {
HWND hwnd;
HHOOK hook;
WNDCLASSEX wincl = {0};
wincl.hInstance = hinst;
wincl.lpszClassName = "CursorTest";
wincl.lpfnWndProc = wndproc;
wincl.cbSize = sizeof (WNDCLASSEX);
wincl.hbrBackground = (HBRUSH) COLOR_BACKGROUND;
if (!RegisterClassEx(&wincl)) return 1;
/* The class is registered, let's create the program*/
hwnd = CreateWindowEx(
0, "CursorTest", "CursorTest",
WS_POPUP | WS_VISIBLE,
0, 0, 300, 300,
HWND_DESKTOP, NULL, hinst, NULL
);
/* reset */
SetCursorPos(0, 0);
events(hwnd);
hook = SetWindowsHookEx(WH_MOUSE_LL, hook_proc, hinst, 0);
if (!hook) return 1;
#define DO(x) do {\
printf("%d: %s\n", ++stage, # x); \
x; \
events(hwnd); \
printf("%d: GetCursorPos: %s\n\n", stage, tmp_pos_str()); \
} while (0)
/* first move after hooking */
DO(SetCursorPos(1, 1));
/* clip twice; the cursor should move and produce messages but not call hooks. */
DO(ClipCursor(tmp_rect(10, 10, 20, 20)));
DO(ClipCursor(tmp_rect(30, 30, 40, 40)));
/* move cursor; again, messages but not hooks */
DO(SetCursorPos(15, 15));
DO(SetCursorPos(15, 15));
DO(SetCursorPos(35, 35));
DO(SetCursorPos(35, 35));
DO(SetCursorPos(55, 55));
DO(SetCursorPos(55, 55));
/* inject input; notice coords (passed and GetCursorPos) in hooks and messages */
DO(mouse_event(MOUSEEVENTF_LEFTDOWN | MOUSEEVENTF_MOVE, -5, -5, 0, 0));
DO(mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 6, 0, 0));
DO(SendInput(1, tmp_input(MOUSEEVENTF_MOVE, -1, -1), sizeof(INPUT)));
DO(SendInput(1, tmp_input(MOUSEEVENTF_MOVE, 0, 0), sizeof(INPUT)));
/* move cursor; notice that message count and hook count don't match */
DO(SetCursorPos(35, 35));
DO(mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 0, 0, 0));
DO(mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 0, 0, 0));
DO(SetCursorPos(35, 35));
DO(mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 0, 0, 0));
DO(mouse_event(MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE, 0, 0, 0, 0));
DO(mouse_event(MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE, 0, 0, 0, 0));
#undef DO
UnhookWindowsHookEx(hook);
return 0;
}
