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!