[PATCH] user32: Implement GetMouseMovePointsEx().

[Arkadiusz Hiler]

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 
---
 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 )

[Marvin]

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

[Arkadiusz Hiler]

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

[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,

[Myah Caron]

> I saw some other patches earlier from Myah Caron, isn't it overlapping?
> Or maybe yours supersede theirs?

Arkadiusz's tests are more thorough and also avoid the awful race condition I had in mine, so I think their tests should supersede mine :)

[Arkadiusz Hiler]

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(&current, 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

[Rémi Bernon]

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(&current, 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.
>
> 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!

[Arkadiusz Hiler]

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!