[PATCH] user32: Implement GetMouseMovePointsEx().
Rémi Bernon
rbernon at codeweavers.com
Tue Sep 15 08:35:03 CDT 2020
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(¤t, 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.
>
> [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.
>
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!
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list