[PATCH] user32: Implement GetMouseMovePointsEx().
Rémi Bernon
rbernon at codeweavers.com
Mon Sep 14 12:28:45 CDT 2020
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 at codeweavers.com>
More information about the wine-devel
mailing list