[PATCH] user32: Implement GetMouseMovePointsEx().
I've tested this change with Mount & Blade II: Bannerlord - the mouse works and feels right. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=36873 Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/user32/input.c | 74 ++++++++++++++++++++++++++++--- dlls/user32/tests/input.c | 79 +++++++++++++++++++++++++++++++++- include/wine/server_protocol.h | 30 ++++++++++--- server/protocol.def | 28 +++++++++--- server/queue.c | 27 ++++++++++++ server/trace.c | 30 +++++++++++++ 6 files changed, 250 insertions(+), 18 deletions(-) diff --git a/dlls/user32/input.c b/dlls/user32/input.c index 1dd43a36a11..b8c6ef6d682 100644 --- a/dlls/user32/input.c +++ b/dlls/user32/input.c @@ -1271,22 +1271,84 @@ TrackMouseEvent (TRACKMOUSEEVENT *ptme) * Success: count of point set in the buffer * Failure: -1 */ -int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) { +int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) +{ + cursor_pos_history_t history; + cursor_pos_t *pos; + int copied, current; + NTSTATUS ret; + BOOL found; - if((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) { + TRACE("(%d %p %p %d %d)\n", size, ptin, ptout, count, res); + + if ((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) + { SetLastError(ERROR_INVALID_PARAMETER); return -1; } - if(!ptin || (!ptout && count)) { + if (!ptin || (!ptout && count)) + { SetLastError(ERROR_NOACCESS); return -1; } - FIXME("(%d %p %p %d %d) stub\n", size, ptin, ptout, count, res); + if (res != GMMP_USE_DISPLAY_POINTS) + { + FIXME("only GMMP_USE_DISPLAY_POINTS is supported for now\n"); + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + } - SetLastError(ERROR_POINT_NOT_FOUND); - return -1; + SERVER_START_REQ( set_cursor ) + { + req->flags = SET_CURSOR_GET_HISTORY; + wine_server_set_reply( req, &history, sizeof(history) ); + ret = wine_server_call( req ); + } + SERVER_END_REQ; + + if (ret) + { + SetLastError(ret); + return -1; + } + + found = FALSE; + current = history.last; + while (history.size) + { + pos = &history.positions[current]; + + if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) + { + found = TRUE; + break; + } + + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + history.size--; + } + + if (!found) + { + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + } + + copied = 0; + while (history.size-- && copied < count) + { + cursor_pos_t *pos = &history.positions[current]; + ptout[copied].x = pos->x; + ptout[copied].y = pos->y; + ptout[copied].time = pos->time; + ptout[copied].dwExtraInfo = pos->info; + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + copied++; + } + + return copied; } /*********************************************************************** diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 1809c147cbd..998e0d585fe 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -1481,9 +1481,11 @@ static void test_GetMouseMovePointsEx(void) MOUSEMOVEPOINT in; MOUSEMOVEPOINT out[200]; POINT point; + TEST_INPUT input; /* Get a valid content for the input struct */ - if(!GetCursorPos(&point)) { + if (!GetCursorPos(&point)) + { win_skip("GetCursorPos() failed with error %u\n", GetLastError()); return; } @@ -1605,6 +1607,81 @@ static void test_GetMouseMovePointsEx(void) ok(GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == MYERROR, "expected error ERROR_INVALID_PARAMETER, got %u\n", GetLastError()); + /* more than 64 to be sure we wrap around */ + for (int i = 0; i < 67; i++) + { + in.x = i; + in.y = i*2; + SetCursorPos(in.x, in.y); + } + + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + + for (int i = 0; i < retval; i++) + { + ok(out[i].x == in.x && out[i].y == in.y, "wrong position %d, expected %dx%d got %dx%d\n", i, in.x, in.y, out[i].x, out[i].y); + in.x--; + in.y -= 2; + } + + /* make there's no deduplication */ + SetCursorPos(6, 6); + SetCursorPos(6, 6); + in.x = 6; + in.y = 6; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].x == 6 && out[0].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 6 && out[1].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[1].x, out[1].y); + + /* make sure 2 events are distinguishable by their timestamps */ + in.x = 150; + in.y = 75; + SetCursorPos(30, 30); + SetCursorPos(in.x, in.y); + SetCursorPos(150, 150); + Sleep(3); + SetCursorPos(in.x, in.y); + + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].x == 150 && out[0].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 150 && out[1].y == 150, "expected cursor position to be 150x150 but got %d %d\n", out[1].x, out[1].y); + ok(out[2].x == 150 && out[2].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[2].x, out[2].y); + + in.time = out[2].time; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 62, "expected to get 62 mouse move points but got %d\n", retval); + ok(out[0].x == 150 && out[0].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 30 && out[1].y == 30, "expected cursor position to be 30x30 but got %d %d\n", out[1].x, out[1].y); + + /* events created through other means should also be on the list with correct extra info */ + mouse_event(MOUSEEVENTF_MOVE, -13, 17, 0, 0xcafecafe); + ok(GetCursorPos(&point), "failed to get cursor positon\n"); + ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n"); + in.time = 0; + in.x = point.x; + in.y = point.y; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].dwExtraInfo == 0xcafecafe, "wrong extra info, got 0x%lx expected 0xcafecafe\n", out[0].dwExtraInfo); + + input.type = INPUT_MOUSE; + memset(&input, 0, sizeof(input)); + input.u.mi.dwFlags = MOUSEEVENTF_MOVE; + input.u.mi.dwExtraInfo = 0xdeadbeef; + input.u.mi.dx = -17; + input.u.mi.dy = 13; + SendInput(1, (INPUT*) &input, sizeof(INPUT)); + ok(GetCursorPos(&point), "failed to get cursor positon\n"); + ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n"); + in.time = 0; + in.x = point.x; + in.y = point.y; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].dwExtraInfo == 0xdeadbeef, "wrong extra info, got 0x%lx expected 0xdeadbeef\n", out[0].dwExtraInfo); #undef BUFLIM #undef MYERROR } diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index 5259b5a60ce..e7cb6a2518c 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -766,6 +766,22 @@ struct rawinput_device user_handle_t target; }; +typedef struct +{ + int x; + int y; + unsigned int time; + int __pad; + lparam_t info; +} cursor_pos_t; + +typedef struct +{ + unsigned int size; + unsigned int last; + cursor_pos_t positions[64]; +} cursor_pos_history_t; + @@ -5264,13 +5280,15 @@ struct set_cursor_reply int new_y; rectangle_t new_clip; unsigned int last_change; + /* VARARG(history,cursor_pos_history); */ char __pad_52[4]; }; -#define SET_CURSOR_HANDLE 0x01 -#define SET_CURSOR_COUNT 0x02 -#define SET_CURSOR_POS 0x04 -#define SET_CURSOR_CLIP 0x08 -#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_HANDLE 0x01 +#define SET_CURSOR_COUNT 0x02 +#define SET_CURSOR_POS 0x04 +#define SET_CURSOR_CLIP 0x08 +#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_GET_HISTORY 0x20 @@ -6320,7 +6338,7 @@ union generic_reply /* ### protocol_version begin ### */ -#define SERVER_PROTOCOL_VERSION 641 +#define SERVER_PROTOCOL_VERSION 642 /* ### protocol_version end ### */ diff --git a/server/protocol.def b/server/protocol.def index 92290af701c..7d278653414 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -782,6 +782,22 @@ struct rawinput_device user_handle_t target; }; +typedef struct +{ + int x; + int y; + unsigned int time; + int __pad; + lparam_t info; +} cursor_pos_t; + +typedef struct +{ + unsigned int size; + unsigned int last; + cursor_pos_t positions[64]; +} cursor_pos_history_t; + /****************************************************************/ /* Request declarations */ @@ -3606,12 +3622,14 @@ struct handle_info int new_y; rectangle_t new_clip; /* new clip rectangle */ unsigned int last_change; /* time of last position change */ + VARARG(history,cursor_pos_history); /* history of cursor positions */ @END -#define SET_CURSOR_HANDLE 0x01 -#define SET_CURSOR_COUNT 0x02 -#define SET_CURSOR_POS 0x04 -#define SET_CURSOR_CLIP 0x08 -#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_HANDLE 0x01 +#define SET_CURSOR_COUNT 0x02 +#define SET_CURSOR_POS 0x04 +#define SET_CURSOR_CLIP 0x08 +#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_GET_HISTORY 0x20 /* Batch read rawinput message data */ diff --git a/server/queue.c b/server/queue.c index c1016016051..7afb1f22bae 100644 --- a/server/queue.c +++ b/server/queue.c @@ -225,6 +225,8 @@ static const struct object_ops thread_input_ops = /* pointer to input structure of foreground thread */ static unsigned int last_input_time; +static cursor_pos_history_t cursor_history; + static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ); static void free_message( struct message *msg ); @@ -1518,14 +1520,34 @@ static void update_rawinput_device(const struct rawinput_device *device) e->device.target = get_user_full_handle( e->device.target ); } +static void append_cursor_history( int x, int y, unsigned int time, lparam_t info ) +{ + cursor_pos_t *pos; + cursor_history.last = (cursor_history.last + 1) % ARRAY_SIZE( cursor_history.positions ); + pos = &cursor_history.positions[cursor_history.last]; + + pos->x = x; + pos->y = y; + pos->time = time; + pos->info = info; + + if (cursor_history.size < ARRAY_SIZE( cursor_history.positions )) + cursor_history.size++; +} + /* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { user_handle_t win; struct thread *thread; struct thread_input *input; + struct hardware_msg_data *msg_data; unsigned int msg_code; + msg_data = msg->data; + if (msg->msg == WM_MOUSEMOVE) + append_cursor_history( msg->x, msg->y, msg->time, msg_data->info ); + update_input_key_state( desktop, desktop->keystate, msg->msg, msg->wparam ); last_input_time = get_tick_count(); if (msg->msg != WM_MOUSEMOVE) always_queue = 1; @@ -3213,6 +3235,11 @@ DECL_HANDLER(set_cursor) set_clip_rectangle( desktop, (req->flags & SET_CURSOR_NOCLIP) ? NULL : &req->clip, 0 ); } + if (req->flags & SET_CURSOR_GET_HISTORY) + { + if (sizeof(cursor_history) <= get_reply_max_size()) + set_reply_data( &cursor_history, min( sizeof(cursor_history), get_reply_max_size() ) ); + } reply->new_x = input->desktop->cursor.x; reply->new_y = input->desktop->cursor.y; diff --git a/server/trace.c b/server/trace.c index c93e4fe3b4e..9bb76163c52 100644 --- a/server/trace.c +++ b/server/trace.c @@ -885,6 +885,35 @@ static void dump_varargs_rectangles( const char *prefix, data_size_t size ) remove_data( size ); } +static void dump_varargs_cursor_pos_history( const char *prefix, data_size_t size) +{ + data_size_t len; + const cursor_pos_history_t *history = cur_data; + const cursor_pos_t *pos; + if (size != sizeof(*history)) + { + fprintf( stderr, "%s{}", prefix ); + return; + } + + fprintf( stderr, "%s{", prefix ); + fprintf( stderr, "size=%u,", history->size); + fprintf( stderr, "last=%u,", history->last); + fprintf( stderr, "positions={" ); + len = ARRAY_SIZE( history->positions ); + pos = history->positions; + while (len > 0) + { + fprintf( stderr, "{x=%d,y=%d,time=%u", pos->x, pos->y, pos->time ); + dump_uint64( ",info=", &pos->info ); + fputc( '}', stderr ); + pos++; + if (--len) fputc( ',', stderr ); + } + fputc( '}', stderr ); + fputc( '}', stderr ); +} + static void dump_varargs_message_data( const char *prefix, data_size_t size ) { /* FIXME: dump the structured data */ @@ -4321,6 +4350,7 @@ static void dump_set_cursor_reply( const struct set_cursor_reply *req ) fprintf( stderr, ", new_y=%d", req->new_y ); dump_rectangle( ", new_clip=", &req->new_clip ); fprintf( stderr, ", last_change=%08x", req->last_change ); + dump_varargs_cursor_pos_history( ", history=", cur_size ); } static void dump_get_rawinput_buffer_request( const struct get_rawinput_buffer_request *req ) -- 2.28.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=78681 Your paranoid android. === w10pro64_zh_CN (32 bit report) === user32: input.c:2188: Test failed: Spurious WM_INPUT messages === w10pro64_he (64 bit report) === user32: input.c:1368: Test failed: Wrong new pos: (103,103) input.c:1292: Test failed: GetCursorPos: (100,98) === w10pro64_ja (64 bit report) === user32: input.c:1302: Test failed: Wrong set pos: (99,100) input.c:1322: Test failed: GetCursorPos: (99,100) === debiant (32 bit report) === user32: monitor: Timeout === debiant (32 bit Chinese:China report) === user32: monitor: Timeout === debiant (32 bit WoW report) === user32: monitor: Timeout === debiant (64 bit WoW report) === user32: monitor: Timeout
On Mon, Sep 14, 2020 at 09:19:17AM -0500, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=78681
Your paranoid android.
=== w10pro64_zh_CN (32 bit report) ===
user32: input.c:2188: Test failed: Spurious WM_INPUT messages
=== w10pro64_he (64 bit report) ===
user32: input.c:1368: Test failed: Wrong new pos: (103,103) input.c:1292: Test failed: GetCursorPos: (100,98)
=== w10pro64_ja (64 bit report) ===
user32: input.c:1302: Test failed: Wrong set pos: (99,100) input.c:1322: Test failed: GetCursorPos: (99,100)
=== debiant (32 bit report) ===
user32: monitor: Timeout
=== debiant (32 bit Chinese:China report) ===
user32: monitor: Timeout
=== debiant (32 bit WoW report) ===
user32: monitor: Timeout
=== debiant (64 bit WoW report) ===
user32: monitor: Timeout
Doesn't look like it's caused by anything in this patch. User32 input tests seem to be rather volatile. I've seen some faliures happening here and there randomly both on Windows and under Wine, even without any of my changes. But I hven't seen anything like this. I've also been running that patch through testbot (+/- some cosmetic changes), and no complaints there: https://testbot.winehq.org/JobDetails.pl?Key=78677 https://testbot.winehq.org/JobDetails.pl?Key=78689 -- Cheers, Arek
Hi!
--- include/wine/server_protocol.h | 30 ++++++++++--- server/trace.c | 30 +++++++++++++
You don't need to and actually shouldn't include the changes to these generated files in the patches you send to wine-devel, it'll be added when they get accepted, to be sure it's always consistent. Marvin also does it before building and running the tests.
6 files changed, 250 insertions(+), 18 deletions(-)
diff --git a/dlls/user32/input.c b/dlls/user32/input.c index 1dd43a36a11..b8c6ef6d682 100644 --- a/dlls/user32/input.c +++ b/dlls/user32/input.c @@ -1271,22 +1271,84 @@ TrackMouseEvent (TRACKMOUSEEVENT *ptme) * Success: count of point set in the buffer * Failure: -1 */ -int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) { +int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) +{ + cursor_pos_history_t history; + cursor_pos_t *pos; + int copied, current; + NTSTATUS ret; + BOOL found;
- if((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) { + TRACE("(%d %p %p %d %d)\n", size, ptin, ptout, count, res); + + if ((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) + {
Shouldn't it be ARRAY_SIZE(history.positions) instead of 64?
SetLastError(ERROR_INVALID_PARAMETER); return -1; }
- if(!ptin || (!ptout && count)) { + if (!ptin || (!ptout && count)) + {
I think the general rule is "don't fix white space or code style on lines you don't touch". Although it's probably a bit flexible :)
SetLastError(ERROR_NOACCESS); return -1; }
- FIXME("(%d %p %p %d %d) stub\n", size, ptin, ptout, count, res); + if (res != GMMP_USE_DISPLAY_POINTS) + { + FIXME("only GMMP_USE_DISPLAY_POINTS is supported for now\n"); + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + }
- SetLastError(ERROR_POINT_NOT_FOUND); - return -1; + SERVER_START_REQ( set_cursor ) + { + req->flags = SET_CURSOR_GET_HISTORY; + wine_server_set_reply( req, &history, sizeof(history) );
So if you don't use any of the existing set_cursor reply here, right? Maybe it's better having a dedicated request?
+ ret = wine_server_call( req ); + } + SERVER_END_REQ; + + if (ret) + { + SetLastError(ret); + return -1; + }
There's a wine_server_call_err that does most of it for you.
+ + found = FALSE; + current = history.last; + while (history.size) + { + pos = &history.positions[current]; + + if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) + { + found = TRUE; + break; + } + + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + history.size--; + } +
I understand it's just iterating the history in reverse order, but if feels hard to read and a bit verbose, wouldn't something like with that be enough? unsigned int i; for( i = history.last; history.size > 0; i--, history.size-- ) { pos = &history.positions[i % ARRAY_SIZE(history.positions)]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break; } if (!history.size) /* ... */
+ if (!found) + { + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + } + + copied = 0; + while (history.size-- && copied < count) + { + cursor_pos_t *pos = &history.positions[current]; + ptout[copied].x = pos->x; + ptout[copied].y = pos->y; + ptout[copied].time = pos->time; + ptout[copied].dwExtraInfo = pos->info; + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + copied++; + } + + return copied; }
Same here.
/*********************************************************************** diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 1809c147cbd..998e0d585fe 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -1481,9 +1481,11 @@ static void test_GetMouseMovePointsEx(void) MOUSEMOVEPOINT in; MOUSEMOVEPOINT out[200]; POINT point; + TEST_INPUT input;
/* Get a valid content for the input struct */ - if(!GetCursorPos(&point)) { + if (!GetCursorPos(&point)) + { win_skip("GetCursorPos() failed with error %u\n", GetLastError()); return; } @@ -1605,6 +1607,81 @@ static void test_GetMouseMovePointsEx(void) ok(GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == MYERROR, "expected error ERROR_INVALID_PARAMETER, got %u\n", GetLastError());
+ /* more than 64 to be sure we wrap around */ + for (int i = 0; i < 67; i++) + { + in.x = i; + in.y = i*2; + SetCursorPos(in.x, in.y); + } + + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + + for (int i = 0; i < retval; i++) + { + ok(out[i].x == in.x && out[i].y == in.y, "wrong position %d, expected %dx%d got %dx%d\n", i, in.x, in.y, out[i].x, out[i].y); + in.x--; + in.y -= 2; + } + + /* make there's no deduplication */ + SetCursorPos(6, 6); + SetCursorPos(6, 6); + in.x = 6; + in.y = 6; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].x == 6 && out[0].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 6 && out[1].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[1].x, out[1].y); + + /* make sure 2 events are distinguishable by their timestamps */ + in.x = 150; + in.y = 75; + SetCursorPos(30, 30); + SetCursorPos(in.x, in.y); + SetCursorPos(150, 150); + Sleep(3); + SetCursorPos(in.x, in.y); + + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].x == 150 && out[0].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 150 && out[1].y == 150, "expected cursor position to be 150x150 but got %d %d\n", out[1].x, out[1].y); + ok(out[2].x == 150 && out[2].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[2].x, out[2].y); + + in.time = out[2].time; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 62, "expected to get 62 mouse move points but got %d\n", retval); + ok(out[0].x == 150 && out[0].y == 75, "expected cursor position to be 150x75 but got %d %d\n", out[0].x, out[0].y); + ok(out[1].x == 30 && out[1].y == 30, "expected cursor position to be 30x30 but got %d %d\n", out[1].x, out[1].y); + + /* events created through other means should also be on the list with correct extra info */ + mouse_event(MOUSEEVENTF_MOVE, -13, 17, 0, 0xcafecafe); + ok(GetCursorPos(&point), "failed to get cursor positon\n"); + ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n"); + in.time = 0; + in.x = point.x; + in.y = point.y; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].dwExtraInfo == 0xcafecafe, "wrong extra info, got 0x%lx expected 0xcafecafe\n", out[0].dwExtraInfo); + + input.type = INPUT_MOUSE; + memset(&input, 0, sizeof(input)); + input.u.mi.dwFlags = MOUSEEVENTF_MOVE; + input.u.mi.dwExtraInfo = 0xdeadbeef; + input.u.mi.dx = -17; + input.u.mi.dy = 13; + SendInput(1, (INPUT*) &input, sizeof(INPUT)); + ok(GetCursorPos(&point), "failed to get cursor positon\n"); + ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n"); + in.time = 0; + in.x = point.x; + in.y = point.y; + retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); + ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); + ok(out[0].dwExtraInfo == 0xdeadbeef, "wrong extra info, got 0x%lx expected 0xdeadbeef\n", out[0].dwExtraInfo); #undef BUFLIM #undef MYERROR }
I saw some other patches earlier from Myah Caron, isn't it overlapping? Or maybe yours supersede theirs?
diff --git a/server/protocol.def b/server/protocol.def index 92290af701c..7d278653414 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -782,6 +782,22 @@ struct rawinput_device user_handle_t target; };
+typedef struct +{ + int x; + int y; + unsigned int time; + int __pad; + lparam_t info; +} cursor_pos_t; + +typedef struct +{ + unsigned int size; + unsigned int last; + cursor_pos_t positions[64]; +} cursor_pos_history_t; + /****************************************************************/ /* Request declarations */
@@ -3606,12 +3622,14 @@ struct handle_info int new_y; rectangle_t new_clip; /* new clip rectangle */ unsigned int last_change; /* time of last position change */ + VARARG(history,cursor_pos_history); /* history of cursor positions */ @END -#define SET_CURSOR_HANDLE 0x01 -#define SET_CURSOR_COUNT 0x02 -#define SET_CURSOR_POS 0x04 -#define SET_CURSOR_CLIP 0x08 -#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_HANDLE 0x01 +#define SET_CURSOR_COUNT 0x02 +#define SET_CURSOR_POS 0x04 +#define SET_CURSOR_CLIP 0x08 +#define SET_CURSOR_NOCLIP 0x10 +#define SET_CURSOR_GET_HISTORY 0x20
/* Batch read rawinput message data */ diff --git a/server/queue.c b/server/queue.c index c1016016051..7afb1f22bae 100644 --- a/server/queue.c +++ b/server/queue.c @@ -225,6 +225,8 @@ static const struct object_ops thread_input_ops = /* pointer to input structure of foreground thread */ static unsigned int last_input_time;
+static cursor_pos_history_t cursor_history; + static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ); static void free_message( struct message *msg );
@@ -1518,14 +1520,34 @@ static void update_rawinput_device(const struct rawinput_device *device) e->device.target = get_user_full_handle( e->device.target ); }
+static void append_cursor_history( int x, int y, unsigned int time, lparam_t info ) +{ + cursor_pos_t *pos; + cursor_history.last = (cursor_history.last + 1) % ARRAY_SIZE( cursor_history.positions ); + pos = &cursor_history.positions[cursor_history.last]; + + pos->x = x; + pos->y = y; + pos->time = time; + pos->info = info; + + if (cursor_history.size < ARRAY_SIZE( cursor_history.positions )) + cursor_history.size++; +} + /* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { user_handle_t win; struct thread *thread; struct thread_input *input; + struct hardware_msg_data *msg_data; unsigned int msg_code;
+ msg_data = msg->data; + if (msg->msg == WM_MOUSEMOVE) + append_cursor_history( msg->x, msg->y, msg->time, msg_data->info ); + update_input_key_state( desktop, desktop->keystate, msg->msg, msg->wparam ); last_input_time = get_tick_count(); if (msg->msg != WM_MOUSEMOVE) always_queue = 1;
Did you look that the message merging below and how it was interacting with the position history? Maybe it could be worth adding a test to see if the history should match individual messages, or if it doesn't matter? I believe the merging is mainly there to avoid duplicate messages that could be produced when X11 XI2 rawevents are enabled. This is currently the case in upstream Wine when ClipCursor is used. In this case, both MotionNotify and RawMotion events may be received, and generate WM_MOUSEMOVE message, one of which gets merged into the other IIRC. It's not always the case, though, as for instance, when the cursor is blocked by the clipping boundaries, only RawMotion events are received.
@@ -3213,6 +3235,11 @@ DECL_HANDLER(set_cursor)
set_clip_rectangle( desktop, (req->flags & SET_CURSOR_NOCLIP) ? NULL : &req->clip, 0 ); } + if (req->flags & SET_CURSOR_GET_HISTORY) + { + if (sizeof(cursor_history) <= get_reply_max_size()) + set_reply_data( &cursor_history, min( sizeof(cursor_history), get_reply_max_size() ) ); + }
reply->new_x = input->desktop->cursor.x; reply->new_y = input->desktop->cursor.y;
Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On Mon, Sep 14, 2020 at 07:28:45PM +0200, Rémi Bernon wrote:
Hi!
--- include/wine/server_protocol.h | 30 ++++++++++--- server/trace.c | 30 +++++++++++++
You don't need to and actually shouldn't include the changes to these generated files in the patches you send to wine-devel, it'll be added when they get accepted, to be sure it's always consistent. Marvin also does it before building and running the tests.
I thought that the make_requests is called only to assure consistency, i.e. check after the fact that everything but the protocol version matches. I'll take it out but I'll have to keep the dump_varargs_cursor_pos_history() in trace.c - VARARGS printing is used but the function doesn't get generated automatically, not even a stub.
6 files changed, 250 insertions(+), 18 deletions(-)
diff --git a/dlls/user32/input.c b/dlls/user32/input.c index 1dd43a36a11..b8c6ef6d682 100644 --- a/dlls/user32/input.c +++ b/dlls/user32/input.c @@ -1271,22 +1271,84 @@ TrackMouseEvent (TRACKMOUSEEVENT *ptme) * Success: count of point set in the buffer * Failure: -1 */ -int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) { +int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) +{ + cursor_pos_history_t history; + cursor_pos_t *pos; + int copied, current; + NTSTATUS ret; + BOOL found; - if((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) { + TRACE("(%d %p %p %d %d)\n", size, ptin, ptout, count, res); + + if ((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) + {
Shouldn't it be ARRAY_SIZE(history.positions) instead of 64?
It could be, but... IIUC the server requests are somewhat separate from the Windows API and often more generalized version of the function. This means that the server call can change its size any time, e.g. by increasing it's buffer size to back some other function (very unlikely), but GMMPEx is documented to return up to 64, so I've kept the original check. I would just size = min(64, history.size) down in the code and static_assert(ARRAY_SIZE(history.positions) >= 64, "..."), but that's C11 and _Static_assert is a GCC extension. Also I see no precedent for that in Wine... So ARRAY_SIZE(history.positions) it is, but it's more error-prone that I would like.
SetLastError(ERROR_INVALID_PARAMETER); return -1; } - if(!ptin || (!ptout && count)) { + if (!ptin || (!ptout && count)) + {
I think the general rule is "don't fix white space or code style on lines you don't touch". Although it's probably a bit flexible :)
I think it will slide, the function gets a complete overhaul :-P While we are at it - what's the recommended style here? I see about the same number of call( a, b ) as call(a, b) in input.c.
SetLastError(ERROR_NOACCESS); return -1; } - FIXME("(%d %p %p %d %d) stub\n", size, ptin, ptout, count, res); + if (res != GMMP_USE_DISPLAY_POINTS) + { + FIXME("only GMMP_USE_DISPLAY_POINTS is supported for now\n"); + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + } - SetLastError(ERROR_POINT_NOT_FOUND); - return -1; + SERVER_START_REQ( set_cursor ) + { + req->flags = SET_CURSOR_GET_HISTORY; + wine_server_set_reply( req, &history, sizeof(history) );
So if you don't use any of the existing set_cursor reply here, right? Maybe it's better having a dedicated request?
I was thinking of this initially, but set_cursor is also a 'get_cursor' and the whole structure is not used for each kind of request... For me it looked like all cursors calls are bundled together for a reason. I'll happily add get_cursor_history :-)
+ ret = wine_server_call( req ); + } + SERVER_END_REQ; + + if (ret) + { + SetLastError(ret); + return -1; + }
There's a wine_server_call_err that does most of it for you.
Oh, neat!
+ + found = FALSE; + current = history.last; + while (history.size) + { + pos = &history.positions[current]; + + if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) + { + found = TRUE; + break; + } + + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + history.size--; + } +
I understand it's just iterating the history in reverse order, but if feels hard to read and a bit verbose, wouldn't something like with that be enough?
unsigned int i;
for( i = history.last; history.size > 0; i--, history.size-- ) { pos = &history.positions[i % ARRAY_SIZE(history.positions)]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break; }
Also too error-prone for my taste - I don't like the implicit assumption that ARRAY_SIZE(history.positions) is a power of 2 lower than UINT_MAX. Documenting it with static asserts would be nice. How about making the current implementation a bit less verbose: for (current = history.last; history.size > 0; history.size--) { pos = &history.positions[current]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break; current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); } if (history.size == 0) { SetLastError(ERROR_POINT_NOT_FOUND); return -1; } copied = 0; for (copied = 0; history.size && copied < count; copied++, history.size--) { pos = &history.positions[current]; ptout[copied].x = pos->x; ptout[copied].y = pos->y; ptout[copied].time = pos->time; ptout[copied].dwExtraInfo = pos->info; current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); } return copied; It a middle ground between both version - not as pretty as your suggestion but a bit more robust. If we want to keep all operations in for() we can introduce: static void ring_dec(int *val, const int ring_size) { *val = (*val + ring_size - 1) % ring_size; } ... int ring_size = ARRAY_SIZE(history.positions); for (current = history.last; history.size > 0; history.size--, ring_dec(¤t, ring_size)) ... <SNIP>
/* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { user_handle_t win; struct thread *thread; struct thread_input *input; + struct hardware_msg_data *msg_data; unsigned int msg_code; + msg_data = msg->data; + if (msg->msg == WM_MOUSEMOVE) + append_cursor_history( msg->x, msg->y, msg->time, msg_data->info ); + update_input_key_state( desktop, desktop->keystate, msg->msg, msg->wparam ); last_input_time = get_tick_count(); if (msg->msg != WM_MOUSEMOVE) always_queue = 1;
Did you look that the message merging below and how it was interacting with the position history? Maybe it could be worth adding a test to see if the history should match individual messages, or if it doesn't matter?
Yes, I was pondering on it for a bit. That's why I have added those tests: /* make there's no deduplication */ SetCursorPos(6, 6); SetCursorPos(6, 6); in.x = 6; in.y = 6; retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); ok(out[0].x == 6 && out[0].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[0].x, out[0].y); ok(out[1].x == 6 && out[1].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[1].x, out[1].y); ... to see if the history can have duplicates on Windows.
I believe the merging is mainly there to avoid duplicate messages that could be produced when X11 XI2 rawevents are enabled. This is currently the case in upstream Wine when ClipCursor is used. In this case, both MotionNotify and RawMotion events may be received, and generate WM_MOUSEMOVE message, one of which gets merged into the other IIRC.
It's not always the case, though, as for instance, when the cursor is blocked by the clipping boundaries, only RawMotion events are received.
Oh, that's much needed context. Now I see that the code snippet above, although a good test on its own, is not covering the merging we are doing. The current merge_message() cares only about the messages that are in the thread_input queue of the thread we would post the message to. GetMouseMovePointsEx() tracks the cursor globally, even outside of any window, and it would need to implement its own version of the deduplication code. I would say that since we have a long history of 64 positions it's not worth complicating the code to avoid a few duplicates in an uncommon case. Additionally, while overwriting x, y and timestamp is safe for an unprocessed event, doing that for entries in the cursor_history is quite risky - all of those values are used for lookup (ptin) - this has higher chance of breaking things than fixing anything. Apparently the GetMouseMovePointsEx() can have more points that the number of WM_MOUSEMOVE messages the window has received[0], but each WM_MOUSEMOVE has to have a corresponding entry in the history[1]. So the current implementation should be fine. [0]: https://github.com/opentk/opentk/pull/91/ [1]: https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmou... If you really think that we should add deduplication it's possible in the form of: typedef struct { ... int used; } cursor_pos_t; DECL_HANDLER(get_cursor_history) { ... cursor_history.positions[cursor_history.last].used = 1; } int append_cursor_history(..., bool can_merge) { cursor_pos_t *pos = &cursor_history.positions[cursor_history.last]; if (can_merge && !pos.used) { /* merge */ } } And then we would need call append_cursor_history() few times depending on the context: win = find_hardware_message_window( desktop, input, msg, &msg_code, &thread ); if (!win || !thread) { append_cursor_history(..., false); if (input) update_input_key_state( input->desktop, input->keystate, msg->msg, msg->wparam ); free_message( msg ); return; } ... if (!always_queue || merge_message( input, msg )) { append_cursor_history(..., true); free_message( msg ); } else { append_cursor_history(..., false); msg->unique_id = 0; /* will be set once we return it to the app */ list_add_tail( &input->msg_list, &msg->entry ); set_queue_bits( thread->queue, get_hardware_msg_bit(msg) ); } release_object( thread ); But that's still racy and a lot can go wrong. Maybe we would even need to keep track of associated thread_input for the purpose of merging instead of just having a boolean 'used'... So again IMO not worth it. Thanks for the review! :-) Cheers, Arek
On 2020-09-15 13:37, Arkadiusz Hiler wrote:
On Mon, Sep 14, 2020 at 07:28:45PM +0200, Rémi Bernon wrote:
Hi!
--- include/wine/server_protocol.h | 30 ++++++++++--- server/trace.c | 30 +++++++++++++
You don't need to and actually shouldn't include the changes to these generated files in the patches you send to wine-devel, it'll be added when they get accepted, to be sure it's always consistent. Marvin also does it before building and running the tests.
I thought that the make_requests is called only to assure consistency, i.e. check after the fact that everything but the protocol version matches.
I'll take it out but I'll have to keep the dump_varargs_cursor_pos_history() in trace.c - VARARGS printing is used but the function doesn't get generated automatically, not even a stub.
Yes, some patches need to update trace.c, but only in the non-generated parts.
6 files changed, 250 insertions(+), 18 deletions(-)
diff --git a/dlls/user32/input.c b/dlls/user32/input.c index 1dd43a36a11..b8c6ef6d682 100644 --- a/dlls/user32/input.c +++ b/dlls/user32/input.c @@ -1271,22 +1271,84 @@ TrackMouseEvent (TRACKMOUSEEVENT *ptme) * Success: count of point set in the buffer * Failure: -1 */ -int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) { +int WINAPI GetMouseMovePointsEx(UINT size, LPMOUSEMOVEPOINT ptin, LPMOUSEMOVEPOINT ptout, int count, DWORD res) +{ + cursor_pos_history_t history; + cursor_pos_t *pos; + int copied, current; + NTSTATUS ret; + BOOL found; - if((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) { + TRACE("(%d %p %p %d %d)\n", size, ptin, ptout, count, res); + + if ((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) + {
Shouldn't it be ARRAY_SIZE(history.positions) instead of 64?
It could be, but... IIUC the server requests are somewhat separate from the Windows API and often more generalized version of the function. This means that the server call can change its size any time, e.g. by increasing it's buffer size to back some other function (very unlikely), but GMMPEx is documented to return up to 64, so I've kept the original check.
I would just size = min(64, history.size) down in the code and static_assert(ARRAY_SIZE(history.positions) >= 64, "..."), but that's C11 and _Static_assert is a GCC extension. Also I see no precedent for that in Wine...
So ARRAY_SIZE(history.positions) it is, but it's more error-prone that I would like.
We have C_ASSERT for such assertions. As you said, it's very unlikely the size will be anything else than 64, so maybe just assert it is, and use that assumption where it can simplify the code? I don't think we want to support the most generic cases before it is actually needed, especially when it's unlikely.
SetLastError(ERROR_INVALID_PARAMETER); return -1; } - if(!ptin || (!ptout && count)) { + if (!ptin || (!ptout && count)) + {
I think the general rule is "don't fix white space or code style on lines you don't touch". Although it's probably a bit flexible :)
I think it will slide, the function gets a complete overhaul :-P
While we are at it - what's the recommended style here? I see about the same number of call( a, b ) as call(a, b) in input.c.
AFAICS user32 is usually using the former, but it's not entirely consistent. From what I could see the maintainer like it best, so it's easier to get patches accepted if they follow that style (just kidding, it's equally hard).
SetLastError(ERROR_NOACCESS); return -1; } - FIXME("(%d %p %p %d %d) stub\n", size, ptin, ptout, count, res); + if (res != GMMP_USE_DISPLAY_POINTS) + { + FIXME("only GMMP_USE_DISPLAY_POINTS is supported for now\n"); + SetLastError(ERROR_POINT_NOT_FOUND); + return -1; + } - SetLastError(ERROR_POINT_NOT_FOUND); - return -1; + SERVER_START_REQ( set_cursor ) + { + req->flags = SET_CURSOR_GET_HISTORY; + wine_server_set_reply( req, &history, sizeof(history) );
So if you don't use any of the existing set_cursor reply here, right? Maybe it's better having a dedicated request?
I was thinking of this initially, but set_cursor is also a 'get_cursor' and the whole structure is not used for each kind of request... For me it looked like all cursors calls are bundled together for a reason.
I'll happily add get_cursor_history :-)
It's probably used for convenience as the request already needs to return the values, so it wasn't necessary to have the get_cursor counterpart. But in this case, the new flag is used to control returned values instead (where current flags indicate which input are provided), and none of the current returned values are used. So IMHO it deserves a dedicated request.
+ ret = wine_server_call( req ); + } + SERVER_END_REQ; + + if (ret) + { + SetLastError(ret); + return -1; + }
There's a wine_server_call_err that does most of it for you.
Oh, neat!
+ + found = FALSE; + current = history.last; + while (history.size) + { + pos = &history.positions[current]; + + if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) + { + found = TRUE; + break; + } + + current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); + history.size--; + } +
I understand it's just iterating the history in reverse order, but if feels hard to read and a bit verbose, wouldn't something like with that be enough?
unsigned int i;
for( i = history.last; history.size > 0; i--, history.size-- ) { pos = &history.positions[i % ARRAY_SIZE(history.positions)]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break; }
Also too error-prone for my taste - I don't like the implicit assumption that ARRAY_SIZE(history.positions) is a power of 2 lower than UINT_MAX. Documenting it with static asserts would be nice.
Sure, it could also just start at history.last + ARRAY_SIZE. As above, I think we can just assert a few things and keep the code simple. Now, having a closer look I see the points are then also copied in reverse order. What about recording the history on the server side in reverse instead, with a signed index and handling the < 0 case there, and doing the whole search and copy forward in user32? On the simplification side, it also doesn't seem very likely that there will be an history with less than 64 points (maybe right after boot, but even then we probably don't care). What about dropping the history.size too and use static size everywhere?
How about making the current implementation a bit less verbose:
for (current = history.last; history.size > 0; history.size--) { pos = &history.positions[current]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break;
current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); }
if (history.size == 0) { SetLastError(ERROR_POINT_NOT_FOUND); return -1; }
copied = 0; for (copied = 0; history.size && copied < count; copied++, history.size--) { pos = &history.positions[current]; ptout[copied].x = pos->x; ptout[copied].y = pos->y; ptout[copied].time = pos->time; ptout[copied].dwExtraInfo = pos->info; current = (current + ARRAY_SIZE(history.positions) - 1) % ARRAY_SIZE(history.positions); }
return copied;
It a middle ground between both version - not as pretty as your suggestion but a bit more robust.
If we want to keep all operations in for() we can introduce:
static void ring_dec(int *val, const int ring_size) { *val = (*val + ring_size - 1) % ring_size; }
...
int ring_size = ARRAY_SIZE(history.positions);
for (current = history.last; history.size > 0; history.size--, ring_dec(¤t, ring_size)) ...
<SNIP>
Well... My personal taste is that it's still overly verbose and having a helper just for that loop isn't really helping readability. Also, as another matter of taste, I feel that "current" could just be named i as any random index variable. When first reading the code I wondered for a minute if it was anything special. Using "copied" for the output index is probably fine as we need to count the copied values anyway.
/* queue a hardware message into a given thread input */ static void queue_hardware_message( struct desktop *desktop, struct message *msg, int always_queue ) { user_handle_t win; struct thread *thread; struct thread_input *input; + struct hardware_msg_data *msg_data; unsigned int msg_code; + msg_data = msg->data; + if (msg->msg == WM_MOUSEMOVE) + append_cursor_history( msg->x, msg->y, msg->time, msg_data->info ); + update_input_key_state( desktop, desktop->keystate, msg->msg, msg->wparam ); last_input_time = get_tick_count(); if (msg->msg != WM_MOUSEMOVE) always_queue = 1;
Did you look that the message merging below and how it was interacting with the position history? Maybe it could be worth adding a test to see if the history should match individual messages, or if it doesn't matter?
Yes, I was pondering on it for a bit. That's why I have added those tests:
/* make there's no deduplication */ SetCursorPos(6, 6); SetCursorPos(6, 6); in.x = 6; in.y = 6; retval = pGetMouseMovePointsEx(sizeof(MOUSEMOVEPOINT), &in, out, BUFLIM, GMMP_USE_DISPLAY_POINTS); ok(retval == 64, "expected to get 64 mouse move points but got %d\n", retval); ok(out[0].x == 6 && out[0].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[0].x, out[0].y); ok(out[1].x == 6 && out[1].y == 6, "expected cursor position to be 6x6 but got %d %d\n", out[1].x, out[1].y);
... to see if the history can have duplicates on Windows.
Alright!
I believe the merging is mainly there to avoid duplicate messages that could be produced when X11 XI2 rawevents are enabled. This is currently the case in upstream Wine when ClipCursor is used. In this case, both MotionNotify and RawMotion events may be received, and generate WM_MOUSEMOVE message, one of which gets merged into the other IIRC.
It's not always the case, though, as for instance, when the cursor is blocked by the clipping boundaries, only RawMotion events are received.
Oh, that's much needed context. Now I see that the code snippet above, although a good test on its own, is not covering the merging we are doing.
The current merge_message() cares only about the messages that are in the thread_input queue of the thread we would post the message to.
GetMouseMovePointsEx() tracks the cursor globally, even outside of any window, and it would need to implement its own version of the deduplication code. I would say that since we have a long history of 64 positions it's not worth complicating the code to avoid a few duplicates in an uncommon case.
Additionally, while overwriting x, y and timestamp is safe for an unprocessed event, doing that for entries in the cursor_history is quite risky - all of those values are used for lookup (ptin) - this has higher chance of breaking things than fixing anything.
Apparently the GetMouseMovePointsEx() can have more points that the number of WM_MOUSEMOVE messages the window has received[0], but each WM_MOUSEMOVE has to have a corresponding entry in the history[1]. So the current implementation should be fine.
[0]: https://github.com/opentk/opentk/pull/91/ [1]: https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmou...
If you really think that we should add deduplication it's possible in the form of:
typedef struct { ... int used; } cursor_pos_t;
DECL_HANDLER(get_cursor_history) { ... cursor_history.positions[cursor_history.last].used = 1; }
int append_cursor_history(..., bool can_merge) { cursor_pos_t *pos = &cursor_history.positions[cursor_history.last]; if (can_merge && !pos.used) { /* merge */ } }
And then we would need call append_cursor_history() few times depending on the context:
win = find_hardware_message_window( desktop, input, msg, &msg_code, &thread ); if (!win || !thread) { append_cursor_history(..., false); if (input) update_input_key_state( input->desktop, input->keystate, msg->msg, msg->wparam ); free_message( msg ); return; }
... if (!always_queue || merge_message( input, msg )) { append_cursor_history(..., true); free_message( msg ); } else { append_cursor_history(..., false); msg->unique_id = 0; /* will be set once we return it to the app */ list_add_tail( &input->msg_list, &msg->entry ); set_queue_bits( thread->queue, get_hardware_msg_bit(msg) ); } release_object( thread );
But that's still racy and a lot can go wrong. Maybe we would even need to keep track of associated thread_input for the purpose of merging instead of just having a boolean 'used'...
So again IMO not worth it.
Yes, I don't think we need to worry too much about it, especially since the history can contain duplicate or additional values.
Thanks for the review! :-)
Cheers, Arek
Cheers! -- Rémi Bernon <rbernon(a)codeweavers.com>
On Tue, Sep 15, 2020 at 03:35:03PM +0200, Rémi Bernon wrote: <SNIP>
Shouldn't it be ARRAY_SIZE(history.positions) instead of 64?
It could be, but... IIUC the server requests are somewhat separate from the Windows API and often more generalized version of the function. This means that the server call can change its size any time, e.g. by increasing it's buffer size to back some other function (very unlikely), but GMMPEx is documented to return up to 64, so I've kept the original check.
I would just size = min(64, history.size) down in the code and static_assert(ARRAY_SIZE(history.positions) >= 64, "..."), but that's C11 and _Static_assert is a GCC extension. Also I see no precedent for that in Wine...
So ARRAY_SIZE(history.positions) it is, but it's more error-prone that I would like.
We have C_ASSERT for such assertions. As you said, it's very unlikely the size will be anything else than 64, so maybe just assert it is, and use that assumption where it can simplify the code?
I don't think we want to support the most generic cases before it is actually needed, especially when it's unlikely.
I was grepping -i for static.*assert and assert.*static - that's not the first time win32 surprises me with naming conventions :-) C_ASSERT(ARRAY_SIZE(...) == 64) it is then.
So if you don't use any of the existing set_cursor reply here, right? Maybe it's better having a dedicated request?
I was thinking of this initially, but set_cursor is also a 'get_cursor' and the whole structure is not used for each kind of request... For me it looked like all cursors calls are bundled together for a reason.
I'll happily add get_cursor_history :-)
It's probably used for convenience as the request already needs to return the values, so it wasn't necessary to have the get_cursor counterpart.
But in this case, the new flag is used to control returned values instead (where current flags indicate which input are provided), and none of the current returned values are used. So IMHO it deserves a dedicated request.
This makes sense - I've already changed that in my local copy.
I understand it's just iterating the history in reverse order, but if feels hard to read and a bit verbose, wouldn't something like with that be enough?
unsigned int i;
for( i = history.last; history.size > 0; i--, history.size-- ) { pos = &history.positions[i % ARRAY_SIZE(history.positions)]; if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time)) break; }
Also too error-prone for my taste - I don't like the implicit assumption that ARRAY_SIZE(history.positions) is a power of 2 lower than UINT_MAX. Documenting it with static asserts would be nice.
Sure, it could also just start at history.last + ARRAY_SIZE. As above, I think we can just assert a few things and keep the code simple.
Now, having a closer look I see the points are then also copied in reverse order. What about recording the history on the server side in reverse instead, with a signed index and handling the < 0 case there, and doing the whole search and copy forward in user32?
That's a good idea, we can move that lengthy modulo decrement there and we will need only one instance of it. I'll do some renaming too: append_cursor_history() -> prepend_cursor_history() history.last -> history.newest
On the simplification side, it also doesn't seem very likely that there will be an history with less than 64 points (maybe right after boot, but even then we probably don't care). What about dropping the history.size too and use static size everywhere?
I've based this on what windows does - after a reboot if you have only 20 points in your history GMMPEx will return only 20 points, but you are right this will simplify things and is very unlikely to cause problems. I already did all the changes and the code looks much nicer :D I'll retest with M&BII and resend the patch, thanks! -- Cheers, Arek
participants (4)
-
Arkadiusz Hiler -
Marvin -
Myah Caron -
Rémi Bernon