Re: [PATCH] user32: Implement GetMouseMovePointsEx().

[Rémi Bernon]

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,