[PATCH] user32: Implement GetMouseMovePointsEx().

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

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