[PATCH v3 0/1] MR6708: win32u: Sync cursor position on window position change.
-- v3: win32u: Sync cursor position on window position change. https://gitlab.winehq.org/wine/wine/-/merge_requests/6708
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/user32/tests/msg.c | 51 +++++++++++++++++++++++++++++++++++- dlls/win32u/input.c | 13 +++++++++ dlls/win32u/win32u_private.h | 1 + dlls/win32u/window.c | 1 + server/protocol.def | 1 + server/queue.c | 9 ++++--- 6 files changed, 71 insertions(+), 5 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 0347f0d73b0..590f50aa6b7 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -73,6 +73,8 @@ #define ARCH "none" #endif +static void pump_msg_loop(HWND hwnd, HACCEL hAccel); + /* encoded DRAWITEMSTRUCT into an LPARAM */ typedef struct { @@ -108,6 +110,7 @@ typedef struct static BOOL test_DestroyWindow_flag; static BOOL test_context_menu; +static BOOL ignore_mouse_messages = TRUE; static HWINEVENTHOOK hEvent_hook; static HHOOK hKBD_hook; static HHOOK hCBT_hook; @@ -6160,6 +6163,30 @@ static const struct message WmFrameChanged_move[] = { { 0 } }; +static const struct message WmMove_mouse[] = { + { WM_WINDOWPOSCHANGING, sent|wparam, SWP_NOACTIVATE|SWP_NOSIZE }, + { WM_WINDOWPOSCHANGED, sent|wparam, SWP_NOACTIVATE|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOCLIENTSIZE }, + { WM_MOVE, sent|defwinproc, 0 }, + { WM_GETTEXT, sent|optional }, + { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam, 0, 0 }, + { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|optional, 0, 0 }, /* Win7 seems to send this twice. */ + { WM_SETCURSOR, sent|lparam, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent }, + /* WM_MOUSEMOVE with accompanying WM_SETCURSOR may sometimes appear a few extra times on Windows 7 with the + * same parameters. */ + { WM_SETCURSOR, sent|lparam|optional, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent|optional }, + { WM_SETCURSOR, sent|lparam|optional, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent|optional }, + { WM_SETCURSOR, sent|lparam|optional, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent|optional }, + { WM_SETCURSOR, sent|lparam|optional, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent|optional }, + { WM_SETCURSOR, sent|lparam|optional, 0, HTCLIENT | (WM_MOUSEMOVE << 16) }, + { WM_MOUSEMOVE, sent|optional }, + { 0 } +}; + static void test_setwindowpos(void) { HWND hwnd; @@ -6198,6 +6225,7 @@ static void test_setwindowpos(void) expect(sysX + X, rc.right); expect(winY + Y, rc.bottom); + GetWindowRect(hwnd, &rc); res = SetWindowPos( hwnd, 0, 0, 0, 0, 0, SWP_NOSIZE | SWP_NOZORDER | SWP_NOACTIVATE | SWP_FRAMECHANGED); ok_sequence(WmFrameChanged_move, "FrameChanged", FALSE); @@ -6207,6 +6235,26 @@ static void test_setwindowpos(void) expect(sysX, rc.right); expect(winY, rc.bottom); + /* get away from possible menu bar to avoid spurious position changed induced by WM. */ + res = SetWindowPos( hwnd, HWND_TOPMOST, 200, 200, 200, 200, SWP_SHOWWINDOW ); + ok(res == TRUE, "SetWindowPos expected TRUE, got %Id.\n", res); + SetForegroundWindow( hwnd ); + SetActiveWindow( hwnd ); + flush_events(); + pump_msg_loop( hwnd, 0 ); + flush_sequence(); + GetWindowRect( hwnd, &rc ); + SetCursorPos( rc.left + 100, rc.top + 100 ); + flush_events(); + pump_msg_loop( hwnd, 0 ); + flush_sequence(); + ignore_mouse_messages = FALSE; + res = SetWindowPos( hwnd, 0, 205, 205, 0, 0, SWP_NOSIZE | SWP_NOZORDER | SWP_NOACTIVATE ); + ok(res == TRUE, "SetWindowPos expected TRUE, got %Id.\n", res); + flush_events(); + ok_sequence(WmMove_mouse, "MouseMove", FALSE); + ignore_mouse_messages = TRUE; + DestroyWindow(hwnd); } @@ -10853,7 +10901,8 @@ static LRESULT MsgCheckProc (BOOL unicode, HWND hwnd, UINT message, case WM_NCMOUSEMOVE: case WM_SETCURSOR: case WM_IME_SELECT: - return 0; + if (ignore_mouse_messages) return 0; + break; } msg.hwnd = hwnd; diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 0640b34ac2e..0577f460601 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -732,6 +732,19 @@ BOOL WINAPI NtUserSetCursorPos( INT x, INT y ) return ret; } +/*********************************************************************** + * update_window_cursor_pos + */ +void update_window_cursor_pos(void) +{ + SERVER_START_REQ( set_cursor ) + { + req->flags = SET_CURSOR_UPDATE; + wine_server_call( req ); + } + SERVER_END_REQ; +} + /*********************************************************************** * get_cursor_pos */ diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index 8956f83af75..06f9919267d 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -94,6 +94,7 @@ extern BOOL destroy_caret(void); extern HWND get_active_window(void); extern HWND get_capture(void); extern BOOL get_cursor_pos( POINT *pt ); +extern void update_window_cursor_pos(void); extern HWND get_focus(void); extern DWORD get_input_state(void); extern BOOL get_async_keyboard_state( BYTE state[256] ); diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 48a7304b6b0..928287b15ee 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -3795,6 +3795,7 @@ BOOL set_window_pos( WINDOWPOS *winpos, int parent_x, int parent_y ) winpos->cx = new_rects.window.right - new_rects.window.left; winpos->cy = new_rects.window.bottom - new_rects.window.top; send_message( winpos->hwnd, WM_WINDOWPOSCHANGED, 0, (LPARAM)winpos ); + update_window_cursor_pos(); } if ((winpos->flags & (SWP_NOSIZE|SWP_NOMOVE|SWP_FRAMECHANGED)) != (SWP_NOSIZE|SWP_NOMOVE)) diff --git a/server/protocol.def b/server/protocol.def index a4f25e805f8..c0306113220 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3916,6 +3916,7 @@ typedef union #define SET_CURSOR_CLIP 0x08 #define SET_CURSOR_NOCLIP 0x10 #define SET_CURSOR_FSCLIP 0x20 +#define SET_CURSOR_UPDATE 0x40 /* Get the history of the 64 last cursor positions */ @REQ(get_cursor_history) diff --git a/server/queue.c b/server/queue.c index c99c8d8661b..bfda0cfb2e4 100644 --- a/server/queue.c +++ b/server/queue.c @@ -564,7 +564,7 @@ static void update_desktop_cursor_handle( struct desktop *desktop, struct thread } /* set the cursor position and queue the corresponding mouse message */ -static void set_cursor_pos( struct desktop *desktop, int x, int y ) +static void set_cursor_pos( struct desktop *desktop, int x, int y, int update_window_only ) { static const struct hw_msg_source source = { IMDT_UNAVAILABLE, IMO_SYSTEM }; const struct rawinput_device *device; @@ -572,7 +572,7 @@ static void set_cursor_pos( struct desktop *desktop, int x, int y ) if ((device = current->process->rawinput_mouse) && (device->flags & RIDEV_NOLEGACY)) { - update_desktop_cursor_pos( desktop, 0, x, y ); + if (!update_window_only) update_desktop_cursor_pos( desktop, 0, x, y ); return; } @@ -627,7 +627,7 @@ void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsig /* warp the mouse to be inside the clip rect */ x = max( min( desktop_shm->cursor.x, new_rect.right - 1 ), new_rect.left ); y = max( min( desktop_shm->cursor.y, new_rect.bottom - 1 ), new_rect.top ); - if (x != desktop_shm->cursor.x || y != desktop_shm->cursor.y) set_cursor_pos( desktop, x, y ); + if (x != desktop_shm->cursor.x || y != desktop_shm->cursor.y) set_cursor_pos( desktop, x, y, 0 ); /* request clip cursor rectangle reset to the desktop thread */ if (reset) post_desktop_message( desktop, WM_WINE_CLIPCURSOR, flags, FALSE ); @@ -4009,7 +4009,8 @@ DECL_HANDLER(set_cursor) } SHARED_WRITE_END; - if (req->flags & SET_CURSOR_POS) set_cursor_pos( desktop, req->x, req->y ); + if (req->flags & SET_CURSOR_UPDATE) set_cursor_pos( desktop, reply->prev_x, reply->prev_y, 1 ); + if (req->flags & SET_CURSOR_POS) set_cursor_pos( desktop, req->x, req->y, 0 ); if (req->flags & SET_CURSOR_CLIP) set_clip_rectangle( desktop, &req->clip, req->flags, 0 ); if (req->flags & SET_CURSOR_NOCLIP) set_clip_rectangle( desktop, NULL, SET_CURSOR_NOCLIP, 0 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6708
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149236 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000023100E2, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
v3: - mind raw input. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85741
On Tue Oct 22 15:35:03 2024 +0000, Rémi Bernon wrote:
Could this be done without a new server request? Every window position change goes through the `apply_window_pos` request already, wineserver should be able to update the cursor pos accordingly, or is there something I'm missing? The mouse message should go after WM_WINDOWPOSCHANGED (and obviously other messages induced by that or anything between apply_window_pos server call and this place). I think that was good to additionally verify that it is not an effect of SendMessage vs PostMessage but no, message posted in WM_WINDOWPOSCHANGED handling arrives before this WM_MOUSEMOVE.
So I think coalescing that with some earlier server call requires complete overhaul of set_window_pos() with moving all the handling there. That might be problematic (or nontrivial at least) because of multiple sent and waited messages on the way. In the current way it works there is a lot of server calls on a way of set_window_pos(), it seems to me an extra (essentially) posted message won't change much. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85743
On Tue Oct 22 15:47:50 2024 +0000, Paul Gofman wrote:
The mouse message should go after WM_WINDOWPOSCHANGED (and obviously other messages induced by that or anything between apply_window_pos server call and this place). I think that was good to additionally verify that it is not an effect of SendMessage vs PostMessage but no, message posted in WM_WINDOWPOSCHANGED handling arrives before this WM_MOUSEMOVE. So I think coalescing that with some earlier server call requires complete overhaul of set_window_pos() with moving all the handling there. That might be problematic (or nontrivial at least) because of multiple sent and waited messages on the way. In the current way it works there is a lot of server calls on a way of set_window_pos(), it seems to me an extra (essentially) posted message won't change much. Also mind that the new message is only sent if the window position actually changes, that is probably not the most hot path, not sanely possible for an app to change some window position each frame without inducing much bigger issues.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85744
On Tue Oct 22 15:51:49 2024 +0000, Paul Gofman wrote:
Also mind that the new message is only sent if the window position actually changes, that is probably not the most hot path, not sanely possible for an app to change some window position each frame without inducing much bigger issues. Hmm okay, but still, making a dedicated request and flag feels a bit too much like a ad-hoc solution to workaround an incorrect design. I know fixing the design would potentially require more changes, but I think it's the right thing to do. Adding ad-hoc workarounds will just make it more difficult to fix it later. For instance it's described in https://devblogs.microsoft.com/oldnewthing/20111219-00/?p=8863 that WM_MOUSEMOVE are generated on-demand, probably this is the case where it happens.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85746
On Tue Oct 22 16:00:21 2024 +0000, Rémi Bernon wrote:
Hmm okay, but still, making a dedicated request and flag feels a bit too much like a ad-hoc solution to workaround an incorrect design. I know fixing the design would potentially require more changes, but I think it's the right thing to do. Adding ad-hoc workarounds will just make it more difficult to fix it later. For instance it's described in https://devblogs.microsoft.com/oldnewthing/20111219-00/?p=8863 that WM_MOUSEMOVE are generated on-demand, probably this is the case where it happens. But it stems from the fact that we don't have a single monolithic win32u kernel which has all in place, the messaging handling is split between the client Unix part and the server. That is the case for virtually any non-trivial window configuration operation: some parts are performed through server, messages to be sent are generated mostly on the client. Do you have some specific considerations in mind why this cursor update is a hack while the rest of set_window_pos isn't, or what kind of redesign that could be?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85747
On Tue Oct 22 16:05:42 2024 +0000, Paul Gofman wrote:
But it stems from the fact that we don't have a single monolithic win32u kernel which has all in place, the messaging handling is split between the client Unix part and the server. That is the case for virtually any non-trivial window configuration operation: some parts are performed through server, messages to be sent are generated mostly on the client. Do you have some specific considerations in mind why this cursor update is a hack while the rest of set_window_pos isn't, or what kind of redesign that could be? Also, currently the whole handling of the mouse input is performed by sending hardware messages from the client now. I know you have thoughts about moving all of that to server, but even then I don't immediately see how that could help this case. Here it is induced by window move and the place to do it is stipulated by the other messages performed on client. Do you have in mind this whole system (including set_window_pos, various message handling on repaint) to be moved to the server somehow?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85748
On Tue Oct 22 16:11:17 2024 +0000, Paul Gofman wrote:
Also, currently the whole handling of the mouse input is performed by sending hardware messages from the client now. I know you have thoughts about moving all of that to server, but even then I don't immediately see how that could help this case. Here it is induced by window move and the place to do it is stipulated by the other messages performed on client. Do you have in mind this whole system (including set_window_pos, various message handling on repaint) to be moved to the server somehow? As a separate note it seems to me that in principle all we are doing here is NtSetCursorPos (as I did initially), the generated message stems from that and is not some special case. That what was done in the first patch version (while I totally agree with your concern with undesirable side effects).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85749
Rémi Bernon (@rbernon) commented about server/queue.c:
if ((device = current->process->rawinput_mouse) && (device->flags & RIDEV_NOLEGACY)) { - update_desktop_cursor_pos( desktop, 0, x, y ); + if (!update_window_only) update_desktop_cursor_pos( desktop, 0, x, y );
With SET_CURSOR_UPDATE `update_desktop_cursor_pos` won't get called, is there any reason why? I don't think it has much side effect, could we get rid of the condition? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6708#note_85884
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon