From: "Rémi Bernon" <rbernon@codeweavers.com>
Subject: Re: [PATCH v2] user32: Implement GetMouseMovePointsEx().
Message-Id: <af0252d0-3eff-6550-b11a-9ae89dd057bb@codeweavers.com>
Date: Wed, 16 Sep 2020 10:57:00 +0200
In-Reply-To: <20200915154654.534933-1-ahiler@codeweavers.com>
References: <20200915154654.534933-1-ahiler@codeweavers.com>

A few nitpicks:

On 2020-09-15 17:46, Arkadiusz Hiler wrote:
> With this patch we start storing history of last 64 mouse positions on the
> server side which can be retrieved by the newly introduced get_cursor_history
> request.
> 
> The history is kept in reverse chronological order in an array that acts as
> circular buffer - this makes it easy to iterate over it in
> GetMouseMovePointsEx().
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=36873
> Signed-off-by: Arkadiusz Hiler <ahiler@codeweavers.com>
> ---
> 
> I've re-tested this revision with Mount & Blade II: Bannerlord - the mouse
> works and feels right.
> 
> v2: (as suggested by Rémi)
>   * don't include autogenerated changes to server_protocol.h and trace.c
>   * introduce get_cursor_history request instead of re-using set_cursor
>   * C_ASSERT on structure sizes
>   * check GetMouseMovePointsEx(count) against ARRAY_SIZE(history.points)
>   * register points in reverse and iterate forward in GMMPEx
>   * use wine_server_call_err
> 
>   dlls/user32/input.c       | 57 +++++++++++++++++++++++++---
>   dlls/user32/tests/input.c | 79 ++++++++++++++++++++++++++++++++++++++-
>   server/protocol.def       | 22 +++++++++++
>   server/queue.c            | 29 ++++++++++++++
>   server/trace.c            | 28 ++++++++++++++
>   5 files changed, 208 insertions(+), 7 deletions(-)
> 
> diff --git a/dlls/user32/input.c b/dlls/user32/input.c
> index 1dd43a36a11..f7233015d22 100644
> --- a/dlls/user32/input.c
> +++ b/dlls/user32/input.c
> @@ -1271,22 +1271,67 @@ 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_history_t history;
> +    cursor_pos_t *pos;
> +    int copied;
> +    unsigned int i;
>   
> -    if((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > 64)) {
> +    C_ASSERT(ARRAY_SIZE(history.positions) == 64);
> +
> +    TRACE("(%d %p %p %d %d)\n", size, ptin, ptout, count, res);
> +
> +    if ((size != sizeof(MOUSEMOVEPOINT)) || (count < 0) || (count > ARRAY_SIZE(history.positions)))
> +    {
>           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;
> +    }

Looking at it now, I believe we should probably rename "res" to 
"resolution", as documented on MSDN, to make it less confusing as "res" 
is in general used as a short for "result".

Also, maybe you could add a GMMP_USE_HIGH_RESOLUTION_POINTS test case 
with todo_wine, to make it more clear that we don't implement it yet?

>   
> -    SetLastError(ERROR_POINT_NOT_FOUND);
> -    return -1;
> +    SERVER_START_REQ( get_cursor_history )
> +    {
> +        wine_server_set_reply( req, &history, sizeof(history) );
> +        if (wine_server_call_err( req ))

Here "( ... )" is used .../...

> +            return -1;
> +    }
> +    SERVER_END_REQ;
> +
> +    for (i = 0; i < ARRAY_SIZE(history.positions); i++)
> +    {
> +        pos = &history.positions[(i + history.newest) % ARRAY_SIZE(history.positions)];
> +        if (ptin->x == pos->x && ptin->y == pos->y && (!ptin->time || ptin->time == pos->time))
> +            break;
> +    }
> +
> +    if (i == ARRAY_SIZE(history.positions))
> +    {
> +        SetLastError(ERROR_POINT_NOT_FOUND);

.../... but not there.

> +        return -1;
> +    }
> +
> +    for (copied = 0; copied < count && i < ARRAY_SIZE(history.positions); copied++, i++)
> +    {
> +        pos = &history.positions[(i + history.newest) % ARRAY_SIZE(history.positions)];
> +        ptout[copied].x = pos->x;
> +        ptout[copied].y = pos->y;
> +        ptout[copied].time = pos->time;
> +        ptout[copied].dwExtraInfo = pos->info;
> +    }
> +
> +    return copied;
>   }
>   

I think you could use "( ... )" style everywhere in the function, to 
make it more consistent with the general user32 style (including on 
SetLastError lines you didn't touch).

>   /***********************************************************************
> 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 */
               ^ sure

> +    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");

s/positon/position/

> +    ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n");

s/moue_event/mouse_event/

> +    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");

s/positon/position/

> +    ok(in.x != point.x && in.y != point.y, "cursor didn't change position after moue_event()\n");

s/moue_event/mouse_event/

> +    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
>   }

Same here regarding spaces within paren, it's not very consistent 
already but I believe it's the preferred style. I don't know the exact 
rule yet but I've seen Alexandre adding the spaces when committing 
patches, so we can probably help upfront.

> diff --git a/server/protocol.def b/server/protocol.def
> index 92290af701c..80e2896a311 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 newest;
> +    int __pad;
> +    cursor_pos_t positions[64];
> +} cursor_history_t;
> +
>   /****************************************************************/
>   /* Request declarations */
>   
> @@ -3613,6 +3629,12 @@ struct handle_info
>   #define SET_CURSOR_CLIP   0x08
>   #define SET_CURSOR_NOCLIP 0x10
>   
> +/* Get the history of the 64 last cursor positions */
> +@REQ(get_cursor_history)
> +@REPLY
> +    VARARG(history,cursor_history);
> +@END
> +
>   
>   /* Batch read rawinput message data */
>   @REQ(get_rawinput_buffer)
> diff --git a/server/queue.c b/server/queue.c
> index c1016016051..d05f06c8f45 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_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,32 @@ static void update_rawinput_device(const struct rawinput_device *device)
>       e->device.target = get_user_full_handle( e->device.target );
>   }
>   
> +static void prepend_cursor_history( int x, int y, unsigned int time, lparam_t info )
> +{
> +    const size_t positions_len = ARRAY_SIZE( cursor_history.positions );
> +    cursor_pos_t *pos;
> +    cursor_history.newest = (cursor_history.newest + positions_len - 1) % positions_len;
> +    pos = &cursor_history.positions[cursor_history.newest];
> +
> +    pos->x = x;
> +    pos->y = y;
> +    pos->time = time;
> +    pos->info = info;
> +}
> +
>   /* 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)
> +        prepend_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;
> @@ -3220,6 +3240,15 @@ DECL_HANDLER(set_cursor)
>       reply->last_change = input->desktop->cursor.last_change;
>   }
>   
> +/* Get the history of the 64 last cursor positions */
> +DECL_HANDLER(get_cursor_history)
> +{
> +    if (sizeof(cursor_history) <= get_reply_max_size())
> +        set_reply_data( &cursor_history, min( sizeof(cursor_history), get_reply_max_size() ) );
> +    else
> +        set_error( STATUS_BUFFER_TOO_SMALL );
> +}
> +
>   DECL_HANDLER(get_rawinput_buffer)
>   {
>       struct thread_input *input = current->queue->input;
> diff --git a/server/trace.c b/server/trace.c
> index c93e4fe3b4e..3f040c138b7 100644
> --- a/server/trace.c
> +++ b/server/trace.c
> @@ -885,6 +885,34 @@ static void dump_varargs_rectangles( const char *prefix, data_size_t size )
>       remove_data( size );
>   }
>   
> +static void dump_varargs_cursor_history( const char *prefix, data_size_t size )
> +{
> +    const cursor_history_t *history = cur_data;
> +    const cursor_pos_t *pos;
> +    data_size_t len;
> +    if (size != sizeof(*history))
> +    {
> +            fprintf( stderr, "%s{}", prefix );
> +            return;
> +    }

Too many spaces in indentation here.

> +
> +    fprintf( stderr, "%s{", prefix );
> +    fprintf( stderr, "newest=%u,", history->newest);

Missing space before closing paren here              ^

> +    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 */
> 

Cheers!
-- 
Rémi Bernon <rbernon@codeweavers.com>