[PATCH 0/1] MR5741: Draft: server: Implement key auto-repeat.
This is an early draft of an implementation of key auto-repeat in wineserver to get some feedback. Some open questions: 1. queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code). 2. This draft introduces a new server request to configure auto-repeat (`enable/delay/period`). I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves). Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/winewayland.drv/wayland_keyboard.c | 21 +++---- include/wine/server_protocol.h | 19 ++++++- server/protocol.def | 8 +++ server/queue.c | 73 ++++++++++++++++++++++++- server/request.h | 6 ++ server/trace.c | 10 ++++ server/user.h | 11 ++++ server/winstation.c | 2 + 8 files changed, 134 insertions(+), 16 deletions(-) diff --git a/dlls/winewayland.drv/wayland_keyboard.c b/dlls/winewayland.drv/wayland_keyboard.c index bdef56e8f0c..4d10567cfea 100644 --- a/dlls/winewayland.drv/wayland_keyboard.c +++ b/dlls/winewayland.drv/wayland_keyboard.c @@ -870,20 +870,16 @@ static void keyboard_handle_modifiers(void *data, struct wl_keyboard *wl_keyboar static void keyboard_handle_repeat_info(void *data, struct wl_keyboard *wl_keyboard, int rate, int delay) { - UINT speed; - TRACE("rate=%d delay=%d\n", rate, delay); - /* Handle non-negative rate values, ignore invalid (negative) values. A - * rate of 0 disables repeat. */ - if (rate >= 80) speed = 31; - else if (rate >= 5) speed = rate * 400 / 1000 - 1; - else speed = 0; - - delay = max(0, min(3, round(delay / 250.0) - 1)); - NtUserSystemParametersInfo(SPI_SETKEYBOARDSPEED, speed, NULL, 0); - NtUserSystemParametersInfo(SPI_SETKEYBOARDDELAY, delay, NULL, 0); - NtUserCallOneParam(rate > 0, NtUserCallOneParam_SetKeyboardAutoRepeat); + SERVER_START_REQ(set_keyboard_repeat) + { + req->enable = rate > 0; + req->delay = delay; + req->period = rate > 0 ? max(1000 / rate, 1) : 0; + wine_server_call(req); + } + SERVER_END_REQ; } static const struct wl_keyboard_listener keyboard_listener = { @@ -916,7 +912,6 @@ void wayland_keyboard_init(struct wl_keyboard *wl_keyboard) return; } - NtUserCallOneParam(TRUE, NtUserCallOneParam_SetKeyboardAutoRepeat); pthread_mutex_lock(&keyboard->mutex); keyboard->wl_keyboard = wl_keyboard; keyboard->xkb_context = xkb_context; diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index d17f0936c3f..c4f0404bf38 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -5651,6 +5651,20 @@ struct get_next_thread_reply }; + +struct set_keyboard_repeat_request +{ + struct request_header __header; + int enable; + int delay; + int period; +}; +struct set_keyboard_repeat_reply +{ + struct reply_header __header; +}; + + enum request { REQ_new_process, @@ -5938,6 +5952,7 @@ enum request REQ_suspend_process, REQ_resume_process, REQ_get_next_thread, + REQ_set_keyboard_repeat, REQ_NB_REQUESTS }; @@ -6230,6 +6245,7 @@ union generic_request struct suspend_process_request suspend_process_request; struct resume_process_request resume_process_request; struct get_next_thread_request get_next_thread_request; + struct set_keyboard_repeat_request set_keyboard_repeat_request; }; union generic_reply { @@ -6520,11 +6536,12 @@ union generic_reply struct suspend_process_reply suspend_process_reply; struct resume_process_reply resume_process_reply; struct get_next_thread_reply get_next_thread_reply; + struct set_keyboard_repeat_reply set_keyboard_repeat_reply; }; /* ### protocol_version begin ### */ -#define SERVER_PROTOCOL_VERSION 801 +#define SERVER_PROTOCOL_VERSION 802 /* ### protocol_version end ### */ diff --git a/server/protocol.def b/server/protocol.def index 37a39206ca4..1ab42d9d170 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3889,3 +3889,11 @@ struct handle_info @REPLY obj_handle_t handle; /* next thread handle */ @END + + +/* Setup keyboard auto-repeat */ +(a)REQ(set_keyboard_repeat) + int enable; /* whether to enable auto-repeat */ + int delay; /* auto-repeat delay in ms */ + int period; /* auto-repeat period in ms */ +(a)END diff --git a/server/queue.c b/server/queue.c index 6d4396234f2..067db9c60d6 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2086,9 +2086,38 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons return wait; } +static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, const hw_input_t *input, + unsigned int origin, struct msg_queue *sender, int repeat ); + +static void key_repeat_timeout( void *private ) +{ + struct desktop *desktop = private; + + desktop->key_repeat.timeout = NULL; + + /* TODO: queue_keyboard_message depends on having a valid 'current' thread. + * Since the timeout callback is called without any active current thread, + * we use the foreground thread as current. In the future we might want to + * remove the dependency on 'current' and pass all required information + * explicitly to queue_keyboard_message. */ + if ((current = get_foreground_thread( desktop, desktop->key_repeat.win ))) + { + queue_keyboard_message( desktop, 0, &desktop->key_repeat.input, IMO_HARDWARE, NULL, 1 ); + current = NULL; + } +} + +static int is_vkey_repeatable( unsigned char vkey ) +{ + return vkey != VK_LSHIFT && vkey != VK_RSHIFT && + vkey != VK_LCONTROL && vkey != VK_RCONTROL && + vkey != VK_LMENU && vkey != VK_RMENU && + vkey != VK_LWIN && vkey != VK_RWIN; +} + /* queue a hardware message for a keyboard event */ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, const hw_input_t *input, - unsigned int origin, struct msg_queue *sender ) + unsigned int origin, struct msg_queue *sender, int repeat ) { struct hw_msg_source source = { IMDT_KEYBOARD, origin }; const struct rawinput_device *device; @@ -2235,7 +2264,32 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c msg->lparam = (flags << 16) | lparam | 1u /* repeat count */; if (!(wait = send_hook_ll_message( desktop, msg, WH_KEYBOARD_LL, lparam | hook_vkey, sender ))) + { queue_hardware_message( desktop, msg, 1 ); + if (origin == IMO_HARDWARE && vkey != VK_PACKET) + { + /* if the repeat key is released, stop auto-repeating */ + if (((input->kbd.flags & KEYEVENTF_KEYUP) && + (input->kbd.scan == desktop->key_repeat.input.kbd.scan))) + { + if (desktop->key_repeat.timeout) remove_timeout_user( desktop->key_repeat.timeout ); + desktop->key_repeat.timeout = NULL; + desktop->key_repeat.win = 0; + memset( &desktop->key_repeat.input, 0, sizeof(desktop->key_repeat.input) ); + } + /* if a repeatable key is down, start or continue auto-repeating */ + if (!(input->kbd.flags & KEYEVENTF_KEYUP) && desktop->key_repeat.enable && + is_vkey_repeatable( vkey )) + { + timeout_t timeout = repeat ? desktop->key_repeat.period : desktop->key_repeat.delay; + desktop->key_repeat.input = *input; + desktop->key_repeat.input.kbd.time = 0; + desktop->key_repeat.win = msg->win; + if (desktop->key_repeat.timeout) remove_timeout_user( desktop->key_repeat.timeout ); + desktop->key_repeat.timeout = add_timeout_user( timeout, key_repeat_timeout, desktop ); + } + } + } return wait; } @@ -2959,7 +3013,7 @@ DECL_HANDLER(send_hardware_message) wait = queue_mouse_message( desktop, req->win, &req->input, origin, sender ); break; case INPUT_KEYBOARD: - wait = queue_keyboard_message( desktop, req->win, &req->input, origin, sender ); + wait = queue_keyboard_message( desktop, req->win, &req->input, origin, sender, 0 ); break; case INPUT_HARDWARE: queue_custom_hardware_message( desktop, req->win, origin, &req->input ); @@ -3833,3 +3887,18 @@ DECL_HANDLER(update_rawinput_devices) release_object( desktop ); } } + +DECL_HANDLER(set_keyboard_repeat) +{ + struct desktop *desktop; + + if (!(desktop = get_hardware_input_desktop( 0 ))) return; + + /* ignore negative values to allow partial updates */ + if (req->enable >= 0) desktop->key_repeat.enable = req->enable; + if (req->delay >= 0) desktop->key_repeat.delay = -req->delay * 10000; + if (req->period >= 0) desktop->key_repeat.period = -req->period * 10000; + + if (desktop->key_repeat.timeout && !desktop->key_repeat.enable) + remove_timeout_user( desktop->key_repeat.timeout ); +} diff --git a/server/request.h b/server/request.h index 5061e0ed399..68eddcec620 100644 --- a/server/request.h +++ b/server/request.h @@ -404,6 +404,7 @@ DECL_HANDLER(terminate_job); DECL_HANDLER(suspend_process); DECL_HANDLER(resume_process); DECL_HANDLER(get_next_thread); +DECL_HANDLER(set_keyboard_repeat); #ifdef WANT_REQUEST_HANDLERS @@ -695,6 +696,7 @@ static const req_handler req_handlers[REQ_NB_REQUESTS] = (req_handler)req_suspend_process, (req_handler)req_resume_process, (req_handler)req_get_next_thread, + (req_handler)req_set_keyboard_repeat, }; C_ASSERT( sizeof(abstime_t) == 8 ); @@ -2344,6 +2346,10 @@ C_ASSERT( FIELD_OFFSET(struct get_next_thread_request, flags) == 28 ); C_ASSERT( sizeof(struct get_next_thread_request) == 32 ); C_ASSERT( FIELD_OFFSET(struct get_next_thread_reply, handle) == 8 ); C_ASSERT( sizeof(struct get_next_thread_reply) == 16 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_request, enable) == 12 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_request, delay) == 16 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_request, period) == 20 ); +C_ASSERT( sizeof(struct set_keyboard_repeat_request) == 24 ); #endif /* WANT_REQUEST_HANDLERS */ diff --git a/server/trace.c b/server/trace.c index f9c78ade989..35093176c9e 100644 --- a/server/trace.c +++ b/server/trace.c @@ -4604,6 +4604,13 @@ static void dump_get_next_thread_reply( const struct get_next_thread_reply *req fprintf( stderr, " handle=%04x", req->handle ); } +static void dump_set_keyboard_repeat_request( const struct set_keyboard_repeat_request *req ) +{ + fprintf( stderr, " enable=%d", req->enable ); + fprintf( stderr, ", delay=%d", req->delay ); + fprintf( stderr, ", period=%d", req->period ); +} + static const dump_func req_dumpers[REQ_NB_REQUESTS] = { (dump_func)dump_new_process_request, (dump_func)dump_get_new_process_info_request, @@ -4890,6 +4897,7 @@ static const dump_func req_dumpers[REQ_NB_REQUESTS] = { (dump_func)dump_suspend_process_request, (dump_func)dump_resume_process_request, (dump_func)dump_get_next_thread_request, + (dump_func)dump_set_keyboard_repeat_request, }; static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { @@ -5178,6 +5186,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { NULL, NULL, (dump_func)dump_get_next_thread_reply, + NULL, }; static const char * const req_names[REQ_NB_REQUESTS] = { @@ -5466,6 +5475,7 @@ static const char * const req_names[REQ_NB_REQUESTS] = { "suspend_process", "resume_process", "get_next_thread", + "set_keyboard_repeat", }; static const struct diff --git a/server/user.h b/server/user.h index d805a179d16..caf9254b1f7 100644 --- a/server/user.h +++ b/server/user.h @@ -64,6 +64,16 @@ struct global_cursor user_handle_t win; /* window that contains the cursor */ }; +struct key_repeat +{ + int enable; /* enable auto-repeat */ + timeout_t delay; /* auto-repeat delay */ + timeout_t period; /* auto-repeat period */ + hw_input_t input; /* the input to repeat */ + user_handle_t win; /* target window for input event */ + struct timeout_user *timeout; /* timeout for repeat */ +}; + struct desktop { struct object obj; /* object header */ @@ -82,6 +92,7 @@ struct desktop unsigned int users; /* processes and threads using this desktop */ struct global_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ + struct key_repeat key_repeat; /* key auto-repeat */ }; /* user handles functions */ diff --git a/server/winstation.c b/server/winstation.c index 80126ad5d60..66fb6ad1e35 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -294,6 +294,7 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned list_init( &desktop->threads ); memset( &desktop->cursor, 0, sizeof(desktop->cursor) ); memset( desktop->keystate, 0, sizeof(desktop->keystate) ); + memset( &desktop->key_repeat, 0, sizeof(desktop->key_repeat) ); list_add_tail( &winstation->desktops, &desktop->entry ); list_init( &desktop->hotkeys ); list_init( &desktop->pointers ); @@ -365,6 +366,7 @@ static void desktop_destroy( struct object *obj ) if (desktop->msg_window) free_window_handle( desktop->msg_window ); if (desktop->global_hooks) release_object( desktop->global_hooks ); if (desktop->close_timeout) remove_timeout_user( desktop->close_timeout ); + if (desktop->key_repeat.timeout) remove_timeout_user( desktop->key_repeat.timeout ); release_object( desktop->winstation ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=145871 Your paranoid android. === debian11b (build log) === error: patch failed: include/wine/server_protocol.h:6520 Task: Patch failed to apply
Rémi Bernon (@rbernon) commented about server/queue.c:
+ * remove the dependency on 'current' and pass all required information + * explicitly to queue_keyboard_message. */ + if ((current = get_foreground_thread( desktop, desktop->key_repeat.win ))) + { + queue_keyboard_message( desktop, 0, &desktop->key_repeat.input, IMO_HARDWARE, NULL, 1 ); + current = NULL; + } +} + +static int is_vkey_repeatable( unsigned char vkey ) +{ + return vkey != VK_LSHIFT && vkey != VK_RSHIFT && + vkey != VK_LCONTROL && vkey != VK_RCONTROL && + vkey != VK_LMENU && vkey != VK_RMENU && + vkey != VK_LWIN && vkey != VK_RWIN; +} Is manually excluding some vkeys necessary?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71539
Rémi Bernon (@rbernon) commented about server/queue.c:
msg->lparam = (flags << 16) | lparam | 1u /* repeat count */;
if (!(wait = send_hook_ll_message( desktop, msg, WH_KEYBOARD_LL, lparam | hook_vkey, sender ))) + { queue_hardware_message( desktop, msg, 1 ); + if (origin == IMO_HARDWARE && vkey != VK_PACKET)
Doing this inside the `!wait`, you will only trigger repetition when there is no LL-hook registered. I'm not completely sure how this should work but maybe you can ignore LL-hooks processing entirely for this, and handle the repetition unconditionally at the end of `queue_keyboard_message`. Otherwise, and if we need it to be conditioned to LL-hook processing, it would need to be moved somehow inside `queue_hardware_message`, which gets called from `store_message_result` when the last LL-hook have completed their processing. The latter seems more complicated, and I'm not sure it's necessary. That could be subject to some testing, but LL-hook on modern Windows are very unreliable and are often bypassed (for instance when they are taking "too long" to process). For that reason I think we can probably ignore them for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71540
queue_keyboard_message requires a `current` thread, but we don't get one in timeout callbacks. At the moment I am manually setting `current` to the foreground thread, but I am wondering if that's acceptable or we should explore other ways forward (also see TODO in code).
You can probably replace `current` with `foreground` in `queue_keyboard_message`, similarly in `queue_mouse_message`.
I am thinking that for more straightforward integration with the keyboard repeat SPI parameters, the request should only support the `enable` flag and the server should query the SPI registry values to get `delay` and `period` when needed. I am wondering if there any caveats here since I don't see other code in the server querying registry values (well, except to implement the registry requests themselves). Also, I would hope that opening and caching the `HKCU\Control Panel\Keyboard` hkey would remove most of the cost of performing this operation (if that's even a concern at all).
Idk, the request looks good as it is. Is there any concern about keeping it like this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71541
On Tue May 28 08:25:46 2024 +0000, Rémi Bernon wrote:
Is manually excluding some vkeys necessary? Hmm, actually no. I was trying this in a Win10 VM installation and thought that this matched the behavior I was seeing, but this seems to have beenan artifact of the host system not repeating the key. I tried again on real hardware and I get repetitions for all keys, so I will remove this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71553
On Tue May 28 08:25:46 2024 +0000, Rémi Bernon wrote:
Doing this inside the `!wait`, you will only trigger repetition when there is no LL-hook registered. I'm not completely sure how this should work but maybe you can ignore LL-hooks processing entirely for this, and handle the repetition unconditionally at the end of `queue_keyboard_message`. Otherwise, and if we need it to be conditioned to LL-hook processing, it would need to be moved somehow inside `queue_hardware_message`, which gets called from `store_message_result` when the last LL-hook have completed their processing. The latter seems more complicated, and I'm not sure it's necessary. That could be subject to some testing, but LL-hook on modern Windows are very unreliable and are often bypassed (for instance when they are taking "too long" to process). For that reason I think we can probably ignore them for now. Ack.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71554
You can probably replace current with foreground in queue_keyboard_message, similarly in queue_mouse_message.
So, that's one instance of `current`, the other is more hidden `send_hook_ll_message(...) -> get_first_global_hook(id) -> get_global_hooks(current)`. Perhaps we can also change this to `get_first_global_hook(thread, id) -> get_global_hooks(thread)` and also pass foreground? I am not familiar with all the interactions and assumptions in this part of the code. Is it safe to use `foreground` in these cases? Can it happen in practice that `foreground->process != current->process` (for the first case) or `foreground->desktop != current->desktop` (for the second)?
Idk, the request looks good as it is. Is there any concern about keeping it like this?
No concerns, I was just considering what options we have to best integrate with `SPI_(GET|SET)KEYBOARD(SPEED|DELAY)`. The request works fine otherwise, and for the GET part I can change the request to return the current values, so `GET => set_keyboard_repeat(-1, -1, -1) => (enable, delay, period)`, and then map these to the SPI values in a best effort manner (e.g., based on your formulas, and similarly for SET). An alternative would be to express the request in terms of the more limited SPI values, but I think I'd rather keep the higher granularity. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71555
I am not familiar with all the interactions and assumptions in this part of the code. Is it safe to use `foreground` in these cases?
Looks like `get_global_hooks` only uses the thread to get its desktop. Maybe pass the desktop to `get_first_global_hook` and remove then unnecessary use of `get_global_hooks`?
Can it happen in practice that `foreground->process != current->process` (for the first case)
It can happen when injecting input, but this won't trigger a repeat, or when mouse is moved over a window that's in the background, but here as well there's no repeat mechanism (then maybe changing the mouse message handling isn't appropriate yet). I don't think `RIDEV_NOLEGACY` handling is really correct anyway, and ideally, "legacy" input messages should probably be generated from the rawinput messages, instead of in parallel, and `RIDEV_NOLEGACY` would be then handled per-process. This is a larger change though. As other options, you can probably also check whether `current` is set, and ignore `RIDEV_NOLEGACY` handling if it's not, or maybe pass `current` as a `thread` repeat parameter to restore it when the timer fires. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_71559
participants (4)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Marvin -
Rémi Bernon