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).
From: Alexandros Frantzis alexandros.frantzis@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 */ +@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 */ +@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 ); }
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?
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.
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?
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.
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.
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.
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.
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Marvin -
Rémi Bernon (＠rbernon)