[PATCH v2 0/5] 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). -- v2: win32u: Remove auto-repeat functionality. server: Implement key auto-repeat. server: Check message target process for raw keyboard device flags. user32: Add tests for cross-process raw keyboard events. server: Pass desktop to get_first_global_hook. https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- server/hook.c | 4 ++-- server/queue.c | 2 +- server/user.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/hook.c b/server/hook.c index 5abdf39ad37..c22e3748b8b 100644 --- a/server/hook.c +++ b/server/hook.c @@ -372,10 +372,10 @@ unsigned int get_active_hooks(void) } /* return the thread that owns the first global hook */ -struct thread *get_first_global_hook( int id ) +struct thread *get_first_global_hook( struct desktop *desktop, int id ) { struct hook *hook; - struct hook_table *global_hooks = get_global_hooks( current ); + struct hook_table *global_hooks = desktop->global_hooks; if (!global_hooks) return NULL; if (!(hook = get_first_valid_hook( global_hooks, id - WH_MINHOOK, EVENT_MIN, 0, 0, 0 ))) return NULL; diff --git a/server/queue.c b/server/queue.c index e881e40271d..2e9460e4c0a 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1745,7 +1745,7 @@ static int send_hook_ll_message( struct desktop *desktop, struct message *hardwa struct message *msg; timeout_t timeout = 2000 * -10000; /* FIXME: load from registry */ - if (!(hook_thread = get_first_global_hook( id ))) return 0; + if (!(hook_thread = get_first_global_hook( desktop, id ))) return 0; if (!(queue = hook_thread->queue)) return 0; if (is_queue_hung( queue )) return 0; diff --git a/server/user.h b/server/user.h index d805a179d16..e3a14466c9d 100644 --- a/server/user.h +++ b/server/user.h @@ -103,7 +103,7 @@ extern void cleanup_clipboard_thread( struct thread *thread ); extern void remove_thread_hooks( struct thread *thread ); extern unsigned int get_active_hooks(void); -extern struct thread *get_first_global_hook( int id ); +extern struct thread *get_first_global_hook( struct desktop *desktop, int id ); /* queue functions */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/user32/tests/input.c | 129 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 9196e67c15b..31d7f1fad35 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -3165,6 +3165,132 @@ static void test_rawinput(const char* argv0) CloseDesktop(params.desk); } +static void rawinput_test_keyboard_process(void) +{ + HANDLE start, done; + INPUT input = {.type = INPUT_KEYBOARD, .ki.wVk = 'A', .ki.wScan = 1}; + int i; + + start = OpenEventA( EVENT_ALL_ACCESS, FALSE, "rawinput_test_keyboard_process_start" ); + ok( start != 0, "OpenEventA failed, error: %lu\n", GetLastError() ); + done = OpenEventA( EVENT_ALL_ACCESS, FALSE, "rawinput_test_keyboard_process_done" ); + ok( done != 0, "OpenEventA failed, error: %lu\n", GetLastError() ); + + for (i = 0; i < 3; ++i) + { + WaitForSingleObject( start, INFINITE ); + input.ki.dwFlags = 0; + ok_ret( 1, SendInput( 1, &input, sizeof(input) ) ); + SetEvent( done ); + + WaitForSingleObject( start, INFINITE ); + input.ki.dwFlags = KEYEVENTF_KEYUP; + ok_ret( 1, SendInput( 1, &input, sizeof(input) ) ); + SetEvent( done ); + } +} + +static void test_rawinput_keyboard( const char* argv0 ) +{ +#define RAW_KEY(s, f, v, m, ...) {.func = RAW_INPUT_KEYBOARD, .raw_input.kbd = {.MakeCode = s, .Flags = f, .VKey = v, .Message = m}, ## __VA_ARGS__} + struct send_input_keyboard_test raw_test[] = + { + {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN), {0, .todo = TRUE}}, .expect_async = {['A'] = 0x80}}, + {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP), {0, .todo = TRUE}}}, + }; + struct send_input_keyboard_test raw_test_no_focus[] = + { + {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN, .todo = TRUE), {0, .todo = TRUE}}, .expect_async = {['A'] = 0x80}}, + {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP, .todo = TRUE), {0, .todo = TRUE}}}, + }; + struct send_input_keyboard_test raw_test_no_fg[] = {{.expect_async = {['A'] = 0x80}}, {}}; +#undef RAW_KEY + + STARTUPINFOA startup_info = {.cb = sizeof(STARTUPINFOA)}; + PROCESS_INFORMATION process_info; + RAWINPUTDEVICE raw_devices[1]; + HANDLE process_start, process_done; + DWORD ret; + HWND hwnd; + int i; + char path[MAX_PATH]; + + process_start = CreateEventA( NULL, FALSE, FALSE, "rawinput_test_keyboard_process_start" ); + ok( process_start != NULL, "CreateEventA failed\n"); + process_done = CreateEventA( NULL, FALSE, FALSE, "rawinput_test_keyboard_process_done" ); + ok( process_done != NULL, "CreateEventA failed\n" ); + + sprintf( path, "%s input rawinput_test_keyboard", argv0 ); + ret = CreateProcessA( NULL, path, NULL, NULL, TRUE, 0, NULL, NULL, &startup_info, &process_info ); + ok( ret, "CreateProcess \"%s\" failed err %lu.\n", path, GetLastError() ); + empty_message_queue(); + + hwnd = CreateWindowA( "static", "static", WS_VISIBLE | WS_POPUP, 100, 100, 100, 100, 0, NULL, NULL, NULL ); + ok( hwnd != 0, "CreateWindow failed\n" ); + SetWindowLongPtrA( hwnd, GWLP_WNDPROC, (LONG_PTR)append_message_wndproc ); + p_accept_message = accept_keyboard_messages_raw; + empty_message_queue(); + + /* FIXME: Try to workaround X11/Win32 focus inconsistencies and + * make the window visible and foreground as hard as possible. */ + ShowWindow( hwnd, SW_SHOW ); + SetWindowPos( hwnd, NULL, 0, 0, 0, 0, SWP_NOSIZE | SWP_NOMOVE ); + SetForegroundWindow( hwnd ); + UpdateWindow( hwnd ); + empty_message_queue(); + + raw_devices[0].usUsagePage = HID_USAGE_PAGE_GENERIC; + raw_devices[0].usUsage = HID_USAGE_GENERIC_KEYBOARD; + raw_devices[0].dwFlags = RIDEV_NOLEGACY; + raw_devices[0].hwndTarget = 0; + + SetLastError( 0xdeadbeef ); + ret = RegisterRawInputDevices( raw_devices, ARRAY_SIZE(raw_devices), sizeof(RAWINPUTDEVICE) ); + ok( ret, "RegisterRawInputDevices failed: %lu\n", GetLastError() ); + + for (i = 0; i < ARRAY_SIZE(raw_test); ++i) + { + SetEvent( process_start ); + WaitForSingleObject( process_done, INFINITE ); + wait_messages( 5, FALSE ); + ok_seq( raw_test[i].expect ); + check_keyboard_async( raw_test[i].expect_async, raw_test[i].todo_async ); + } + + SetFocus( NULL ); + empty_message_queue(); + + for (i = 0; i < ARRAY_SIZE(raw_test_no_focus); ++i) + { + SetEvent( process_start ); + WaitForSingleObject( process_done, INFINITE ); + wait_messages( 5, FALSE ); + ok_seq( raw_test_no_focus[i].expect ); + check_keyboard_async( raw_test_no_focus[i].expect_async, raw_test_no_focus[i].todo_async ); + } + + SetForegroundWindow( GetDesktopWindow() ); + empty_message_queue(); + + for (i = 0; i < ARRAY_SIZE(raw_test_no_fg); ++i) + { + SetEvent( process_start ); + WaitForSingleObject( process_done, INFINITE ); + wait_messages( 5, FALSE ); + ok_seq( raw_test_no_fg[i].expect ); + check_keyboard_async( raw_test_no_fg[i].expect_async, raw_test_no_fg[i].todo_async ); + } + + DestroyWindow( hwnd ); + + winetest_wait_child_process( process_info.hProcess ); + CloseHandle( process_info.hProcess ); + CloseHandle( process_info.hThread ); + CloseHandle( process_done ); + CloseHandle( process_start ); + p_accept_message = NULL; +} + static void test_DefRawInputProc(void) { LRESULT ret; @@ -6155,6 +6281,8 @@ START_TEST(input) argc = winetest_get_mainargs(&argv); if (argc >= 3 && !strcmp( argv[2], "rawinput_test" )) return rawinput_test_process(); + if (argc >= 3 && !strcmp( argv[2], "rawinput_test_keyboard" )) + return rawinput_test_keyboard_process(); if (argc >= 3 && !strcmp( argv[2], "test_GetMouseMovePointsEx_process" )) return test_GetMouseMovePointsEx_process(); if (argc >= 4 && !strcmp( argv[2], "test_EnableMouseInPointer" )) @@ -6182,6 +6310,7 @@ START_TEST(input) test_GetKeyState(); test_OemKeyScan(); test_rawinput(argv[0]); + test_rawinput_keyboard( argv[0] ); test_DefRawInputProc(); if(pGetMouseMovePointsEx) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/user32/tests/input.c | 8 ++++---- server/queue.c | 34 +++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 31d7f1fad35..3052ce6832c 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -3195,13 +3195,13 @@ static void test_rawinput_keyboard( const char* argv0 ) #define RAW_KEY(s, f, v, m, ...) {.func = RAW_INPUT_KEYBOARD, .raw_input.kbd = {.MakeCode = s, .Flags = f, .VKey = v, .Message = m}, ## __VA_ARGS__} struct send_input_keyboard_test raw_test[] = { - {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN), {0, .todo = TRUE}}, .expect_async = {['A'] = 0x80}}, - {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP), {0, .todo = TRUE}}}, + {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN), {0}}, .expect_async = {['A'] = 0x80}}, + {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP), {0}}}, }; struct send_input_keyboard_test raw_test_no_focus[] = { - {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN, .todo = TRUE), {0, .todo = TRUE}}, .expect_async = {['A'] = 0x80}}, - {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP, .todo = TRUE), {0, .todo = TRUE}}}, + {.expect = {RAW_KEY(1, RI_KEY_MAKE, 'A', WM_KEYDOWN, .todo = TRUE), {0}}, .expect_async = {['A'] = 0x80}}, + {.expect = {RAW_KEY(1, RI_KEY_BREAK, 'A', WM_KEYUP, .todo = TRUE), {0}}}, }; struct send_input_keyboard_test raw_test_no_fg[] = {{.expect_async = {['A'] = 0x80}}, {}}; #undef RAW_KEY diff --git a/server/queue.c b/server/queue.c index 2e9460e4c0a..f90ffb58af5 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2086,15 +2086,32 @@ static int queue_mouse_message( struct desktop *desktop, user_handle_t win, cons return wait; } +static struct thread *get_keyboard_message_thread( struct desktop *desktop, user_handle_t win ) +{ + struct thread *thread; + struct thread_input *input; + user_handle_t target = 0; + + if (win && (thread = get_window_thread( win ))) + { + input = thread->queue->input; + release_object( thread ); + } + else input = desktop->foreground_input; + + if (input && !(target = input->focus)) target = input->active; + + return get_window_thread( target ); +} + /* 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 ) { struct hw_msg_source source = { IMDT_KEYBOARD, origin }; - const struct rawinput_device *device; struct hardware_msg_data *msg_data; struct message *msg; - struct thread *foreground; + struct thread *foreground, *msg_thread; unsigned char vkey = input->kbd.vkey, hook_vkey = vkey; unsigned int message_code, time; lparam_t lparam = input->kbd.scan << 16; @@ -2207,10 +2224,17 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c release_object( foreground ); } - if ((device = current->process->rawinput_kbd) && (device->flags & RIDEV_NOLEGACY)) + if ((msg_thread = get_keyboard_message_thread( desktop, win ))) { - update_input_key_state( desktop, desktop->keystate, message_code, vkey ); - return 0; + const struct rawinput_device *device = msg_thread->process->rawinput_kbd; + BOOL nolegacy = device && (device->flags & RIDEV_NOLEGACY); + + release_object( msg_thread ); + if (nolegacy) + { + update_input_key_state( desktop, desktop->keystate, message_code, vkey ); + return 0; + } } if (!(msg = alloc_hardware_message( input->kbd.info, source, time, 0 ))) return 0; -- GitLab 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 | 23 +++++++++- server/protocol.def | 12 +++++ server/queue.c | 58 ++++++++++++++++++++++++- server/request.h | 10 +++++ server/trace.c | 17 ++++++++ server/user.h | 11 +++++ server/winstation.c | 2 + 8 files changed, 138 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 e08f656f882..9363cbf569b 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -5638,6 +5638,24 @@ 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; + int enable; + int delay; + int period; + char __pad_20[4]; +}; + + enum request { REQ_new_process, @@ -5924,6 +5942,7 @@ enum request REQ_suspend_process, REQ_resume_process, REQ_get_next_thread, + REQ_set_keyboard_repeat, REQ_NB_REQUESTS }; @@ -6215,6 +6234,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 { @@ -6504,11 +6524,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 803 +#define SERVER_PROTOCOL_VERSION 804 /* ### protocol_version end ### */ diff --git a/server/protocol.def b/server/protocol.def index ac103156fe4..c84a93a5d69 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3877,3 +3877,15 @@ 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)REPLY + int enable; /* previous state of auto-repeat enable */ + int delay; /* previous auto-repeat delay in ms */ + int period; /* previous auto-repeat period in ms */ +(a)END diff --git a/server/queue.c b/server/queue.c index f90ffb58af5..279984c3d76 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2104,9 +2104,20 @@ static struct thread *get_keyboard_message_thread( struct desktop *desktop, user return get_window_thread( target ); } +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; + queue_keyboard_message( desktop, desktop->key_repeat.win, &desktop->key_repeat.input, IMO_HARDWARE, NULL, 1 ); +} + /* 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 }; struct hardware_msg_data *msg_data; @@ -2209,6 +2220,29 @@ static int queue_keyboard_message( struct desktop *desktop, user_handle_t win, c } } + if (origin == IMO_HARDWARE) + { + /* 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 key is down, start or continue auto-repeating */ + if (!(input->kbd.flags & KEYEVENTF_KEYUP) && desktop->key_repeat.enable) + { + 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 = 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 ); + } + } + if (!unicode && (foreground = get_foreground_thread( desktop, win ))) { struct rawinput_message raw_msg = {0}; @@ -2983,7 +3017,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 ); @@ -3857,3 +3891,23 @@ DECL_HANDLER(update_rawinput_devices) release_object( desktop ); } } + +DECL_HANDLER(set_keyboard_repeat) +{ + struct desktop *desktop; + + if (!(desktop = get_hardware_input_desktop( 0 ))) return; + + /* report previous values */ + reply->enable = desktop->key_repeat.enable; + reply->delay = -desktop->key_repeat.delay / 10000; + reply->period = -desktop->key_repeat.period / 10000; + + /* 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 327c6168241..07cae3188fb 100644 --- a/server/request.h +++ b/server/request.h @@ -403,6 +403,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 @@ -693,6 +694,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 ); @@ -2335,6 +2337,14 @@ 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 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_reply, enable) == 8 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_reply, delay) == 12 ); +C_ASSERT( FIELD_OFFSET(struct set_keyboard_repeat_reply, period) == 16 ); +C_ASSERT( sizeof(struct set_keyboard_repeat_reply) == 24 ); #endif /* WANT_REQUEST_HANDLERS */ diff --git a/server/trace.c b/server/trace.c index a68577763e7..1dc3ac31b27 100644 --- a/server/trace.c +++ b/server/trace.c @@ -4590,6 +4590,20 @@ 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 void dump_set_keyboard_repeat_reply( const struct set_keyboard_repeat_reply *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, @@ -4875,6 +4889,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] = { @@ -5162,6 +5177,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { NULL, NULL, (dump_func)dump_get_next_thread_reply, + (dump_func)dump_set_keyboard_repeat_reply, }; static const char * const req_names[REQ_NB_REQUESTS] = { @@ -5449,6 +5465,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 e3a14466c9d..0a79897916d 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
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/win32u/message.c | 58 ------------------------------------ dlls/win32u/ntuser_private.h | 5 ---- dlls/win32u/sysparams.c | 3 -- include/ntuser.h | 1 - 4 files changed, 67 deletions(-) diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index 877be545810..6b89b873f37 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -288,7 +288,6 @@ struct send_message_info }; static const INPUT_MESSAGE_SOURCE msg_source_unavailable = { IMDT_UNAVAILABLE, IMO_UNAVAILABLE }; -static BOOL keyboard_auto_repeat_enabled; /* flag for messages that contain pointers */ /* 32 messages per entry, messages 0..31 map to bits 0..31 */ @@ -516,13 +515,6 @@ static inline BOOL check_hwnd_filter( const MSG *msg, HWND hwnd_filter ) return (msg->hwnd == hwnd_filter || is_child( hwnd_filter, msg->hwnd )); } -BOOL set_keyboard_auto_repeat( BOOL enable ) -{ - BOOL enabled = keyboard_auto_repeat_enabled; - keyboard_auto_repeat_enabled = enable; - return enabled; -} - /*********************************************************************** * unpack_message * @@ -2309,21 +2301,6 @@ static void send_parent_notify( HWND hwnd, WORD event, WORD idChild, POINT pt ) } } - -static void handle_keyboard_repeat_message( HWND hwnd ) -{ - struct user_thread_info *thread_info = get_user_thread_info(); - MSG *msg = &thread_info->key_repeat_msg; - UINT speed; - - msg->lParam = (msg->lParam & ~(LPARAM)0xffff) + ((msg->lParam + 1) & 0xffff); - - if (NtUserSystemParametersInfo( SPI_GETKEYBOARDSPEED, 0, &speed, 0 )) - NtUserSetSystemTimer( hwnd, SYSTEM_TIMER_KEY_REPEAT, 400 / (speed + 1) ); - - NtUserPostMessage( hwnd, msg->message, msg->wParam, msg->lParam ); -} - /*********************************************************************** * process_pointer_message * @@ -2416,37 +2393,6 @@ static BOOL process_keyboard_message( MSG *msg, UINT hw_id, HWND hwnd_filter, if (ImmProcessKey( msg->hwnd, NtUserGetKeyboardLayout(0), msg->wParam, msg->lParam, 0 )) msg->wParam = VK_PROCESSKEY; - /* set/kill timers for key auto-repeat */ - if (remove && keyboard_auto_repeat_enabled) - { - struct user_thread_info *thread_info = get_user_thread_info(); - - switch (msg->message) - { - case WM_KEYDOWN: - case WM_SYSKEYDOWN: - { - UINT delay; - - if (msg->wParam == VK_PROCESSKEY) break; - - if (thread_info->key_repeat_msg.hwnd != msg->hwnd) - kill_system_timer( thread_info->key_repeat_msg.hwnd, SYSTEM_TIMER_KEY_REPEAT ); - thread_info->key_repeat_msg = *msg; - if (NtUserSystemParametersInfo( SPI_GETKEYBOARDDELAY, 0, &delay, 0 )) - NtUserSetSystemTimer( msg->hwnd, SYSTEM_TIMER_KEY_REPEAT, (delay + 1) * 250 ); - break; - } - - case WM_KEYUP: - case WM_SYSKEYUP: - /* Only stop repeat if the scan codes match. */ - if ((thread_info->key_repeat_msg.lParam & 0x01ff0000) == (msg->lParam & 0x01ff0000)) - kill_system_timer( thread_info->key_repeat_msg.hwnd, SYSTEM_TIMER_KEY_REPEAT ); - break; - } - } - return TRUE; } @@ -3655,10 +3601,6 @@ LRESULT WINAPI NtUserDispatchMessage( const MSG *msg ) case SYSTEM_TIMER_TRACK_MOUSE: update_mouse_tracking_info( msg->hwnd ); return 0; - - case SYSTEM_TIMER_KEY_REPEAT: - handle_keyboard_repeat_message( msg->hwnd ); - return 0; } } diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index f0bd14179d4..020c98005df 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -33,9 +33,6 @@ enum system_timer_id { SYSTEM_TIMER_TRACK_MOUSE = 0xfffa, SYSTEM_TIMER_CARET = 0xffff, - - /* not compatible with native */ - SYSTEM_TIMER_KEY_REPEAT = 0xfff0, }; struct user_object @@ -120,7 +117,6 @@ struct user_thread_info struct received_message_info *receive_info; /* Message being currently received */ struct user_key_state_info *key_state; /* Cache of global key state */ struct imm_thread_data *imm_thread_data; /* IMM thread data */ - MSG key_repeat_msg; /* Last WM_KEYDOWN message to repeat */ HKL kbd_layout; /* Current keyboard layout */ UINT kbd_layout_id; /* Current keyboard layout ID */ struct hardware_msg_data *rawinput; /* Current rawinput message data */ @@ -249,7 +245,6 @@ struct peek_message_filter }; extern int peek_message( MSG *msg, const struct peek_message_filter *filter ); -extern BOOL set_keyboard_auto_repeat( BOOL enable ); /* systray.c */ extern LRESULT system_tray_call( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam, void *data ); diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 49bf26214e7..517913fca94 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -6401,9 +6401,6 @@ ULONG_PTR WINAPI NtUserCallOneParam( ULONG_PTR arg, ULONG code ) process_layout = arg; return TRUE; - case NtUserCallOneParam_SetKeyboardAutoRepeat: - return set_keyboard_auto_repeat( arg ); - case NtUserCallOneParam_SetThreadDpiAwarenessContext: return set_thread_dpi_awareness_context( arg ); diff --git a/include/ntuser.h b/include/ntuser.h index ff45ffa2bc8..1a6e62a89d9 100644 --- a/include/ntuser.h +++ b/include/ntuser.h @@ -909,7 +909,6 @@ enum NtUserCallOneParam_ReplyMessage, NtUserCallOneParam_SetCaretBlinkTime, NtUserCallOneParam_SetProcessDefaultLayout, - NtUserCallOneParam_SetKeyboardAutoRepeat, NtUserCallOneParam_SetThreadDpiAwarenessContext, /* temporary exports */ NtUserGetDeskPattern, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5741
On Tue May 28 10:15:27 2024 +0000, Alexandros Frantzis wrote:
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. Fixed in v2.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72324
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?
Done in v2.
I don't think RIDEV_NOLEGACY handling is really correct anyway...
In v2 I added some tests to exercise this behavior between processes and a few focus scenarios. Based on these tests I updated the handling to use window thread/process which is going to be the target of the legacy message, instead of the current thread, to check RIDEV_NOLEGACY. This makes more sense to me from what I have understood of raw devices: it's not up to the sender, but rather the raw device registration in the target application to determine whether to emit legacy events for the hardware events. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72326
(Still in draft since I haven't yet added integration with SPI values) v2: * Rebased on latest master, to pull in recent `server/queue.c` keyboard changes. * Autorepeat all keys, don't make exceptions for modifiers. * Autorepeat even if RIDEV_NOLEGACY. * Remove all uses of `current` thread from `queue_keyboard_messages`, so we can call it from the auto-repeat callback more easily. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72327
On Tue Jun 4 13:52:38 2024 +0000, Alexandros Frantzis wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/5741/diffs?diff_id=116514&start_sha=8755054e6533453da7c9746aed4b311868e22344#14b74bf2d7f44f52a31e4ee1baa51371f14df769_2269_2297) Move key repeat logic before raw device handling to ensure we repeat keys even with RIDEV_NOLEGACY.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72325
Rémi Bernon (@rbernon) commented about server/queue.c:
release_object( foreground ); }
- if ((device = current->process->rawinput_kbd) && (device->flags & RIDEV_NOLEGACY))
I wouldn't bother with this NOLEGACY thing, but if you really want to fix it I think it should probably be done in `queue_hardware_message`: as far as I can tell, ll-hooks should still called in all cases. And in that queue_hardware_message function you have a call to `find_hardware_target_window` which does what you're doing here and returns the actual thread which should normally receive the message. You get its process there and check for that flag there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72343
Rémi Bernon (@rbernon) commented about dlls/user32/tests/input.c:
+ ok( ret, "CreateProcess \"%s\" failed err %lu.\n", path, GetLastError() ); + empty_message_queue(); + + hwnd = CreateWindowA( "static", "static", WS_VISIBLE | WS_POPUP, 100, 100, 100, 100, 0, NULL, NULL, NULL ); + ok( hwnd != 0, "CreateWindow failed\n" ); + SetWindowLongPtrA( hwnd, GWLP_WNDPROC, (LONG_PTR)append_message_wndproc ); + p_accept_message = accept_keyboard_messages_raw; + empty_message_queue(); + + /* FIXME: Try to workaround X11/Win32 focus inconsistencies and + * make the window visible and foreground as hard as possible. */ + ShowWindow( hwnd, SW_SHOW ); + SetWindowPos( hwnd, NULL, 0, 0, 0, 0, SWP_NOSIZE | SWP_NOMOVE ); + SetForegroundWindow( hwnd ); + UpdateWindow( hwnd ); + empty_message_queue(); I'm happy to see tests but fwiw playing with focus is the best way to create spuriously failing tests, especially with Wine having all sorts of related race conditions.
I started changing these tests to use `create_foreground_window` instead of these hacks, although I didn't do it for all the tests as I wanted to see how it was going. I think it's more reliable, and it gives a meaningful failure when it fails to get the window foreground. Then still I'm not sure it's worth adding more cross process tests, especially as dispatching is already better and more reliably tested in `dinput`. If the only purpose is to exhibit NOLEGACY being checked in the target process, I think we can just assume that it is, without adding tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72344
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_keyboard.c:
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);
For SPI integration, why not keep these calls and do the server request from win32u? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72345
On Tue Jun 4 14:16:57 2024 +0000, Alexandros Frantzis wrote:
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? Done in v2. I don't think RIDEV_NOLEGACY handling is really correct anyway... In v2 I added some tests to exercise this behavior between processes and a few focus scenarios. Based on these tests I updated the handling to use window thread/process which is going to be the target of the legacy message, instead of the current thread, to check RIDEV_NOLEGACY. This makes more sense to me from what I have understood of raw devices: it's not up to the sender, but rather the raw device registration in the target application to determine whether to emit legacy events for the hardware events. Yeah, we can probably consider the sender to be irrelevant here. It works very differently in Wine vs in Windows.
In most tests (except `dinput` tests which inject input from a virtual driver), we use `SendInput` in which case the test process is the sender, but that's just because that function happens to behave similarly enough to user input, and thus usable for testing purposes. But we want to test how Windows behaves on actual user input, not so much what SendInput does (well that's also interesting but only to some extent), and this normally doesn't involve a sender process and input probably comes directly from some in-kernel hardware driver queue. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72347
On Tue Jun 4 15:36:24 2024 +0000, Rémi Bernon wrote:
For SPI integration, why not keep these calls and do the server request from win32u? That's one of the options I have been considering. The downside is that we lose some timing granularity that we otherwise could now support with the direct server request, but probably not a big deal.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72436
If the only purpose is to exhibit NOLEGACY being checked in the target process, I think we can just assume that it is, without adding tests.
That's the primary reason I added them, first to investigate what's actually expected and then validate subsequent changes. I am fine dropping the tests from the MR, and especially if the solution we decide on doesn't actually lead to any improved behavior on this front. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72438
I wouldn't bother with this NOLEGACY thing, but if you really want to fix it I think it should probably be done in queue_hardware_message: as far as I can tell, ll-hooks should still called in all cases.
My main goal is to remove the dependency of `queue_keyboard_message` on `current` in a way that at least doesn't make anything worse (and ideally improves things if the work is not too much of tangent). I am fine with going with the simplest approach possible, but I am not sure what trade-off (in terms of correctness and/or complexity) would be preferable. Some notes about a few options discussed before: 1. Adding `!current` check: it will lead to legacy messages during key repetition (assuming we don't set `current` when repeating) 2. Saving and restoring `current` for key repeat: we will need to handle `current` being destroyed, but also checking `current`/sender is not correct as we discussed. 3. Using `foreground` instead of `current`: Possibly good enough, although still not strictly correct. 4. NOLEGACY logic in queue_hardware_message: Not straightforward due to interactions with the hotkey and message altering logic earlier in the function (but, complexity aside, indeed seems to be a good way forward for a proper fix). What would be the simplest approach that you would find acceptable in the context of this MR? Perhaps we could store the `(no)legacy` flag as part of the keyrepeat info, and consult that for all repeats instead of checking the device again? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72437
NOLEGACY logic in queue_hardware_message: Not straightforward due to interactions with the hotkey and message altering logic earlier in the function (but, complexity aside, indeed seems to be a good way forward for a proper fix).
Well, don't assume that the way NOLEGACY is implemented right now is correct. It's apparently not and it was rather just part of a wider rawinput patch series and got upstreamed with it without much testing. I'm not sure it's used by many applications, or rather not in a way that its interactions with other mechanisms is important: applications which use NOLEGACY then just don't care about most window messages, they are *only* interested in rawinput messages and use that flag to optimize the message processing. A manual test shows that hotkeys messages are supposed to be received even with that flag, so basically moving the NOLEGACY check right after `find_hardware_message_window` looks like a simple fix and goes in the right direction. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5741#note_72440
participants (3)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Rémi Bernon