[PATCH v9 0/2] MR5761: comclt32: Allow ctrl-tab and tab hotkeys for propsheets
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56103 -- v9: comclt32: Allow hotkeys for propsheet navigation. comctl32/tests: Add test for propsheet hotkey navigation. https://gitlab.winehq.org/wine/wine/-/merge_requests/5761
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comctl32/tests/propsheet.c | 185 ++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c index 9bd1c08bd6a..a7953135992 100644 --- a/dlls/comctl32/tests/propsheet.c +++ b/dlls/comctl32/tests/propsheet.c @@ -302,6 +302,8 @@ static void test_disableowner(void) DestroyWindow(parenthwnd); } +HANDLE active_page_changed; + static INT_PTR CALLBACK nav_page_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) { switch(msg){ @@ -312,6 +314,8 @@ static INT_PTR CALLBACK nav_page_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM case PSN_SETACTIVE: active_page = /* PropSheet_HwndToIndex(hdr->hwndFrom, hwnd); */ (int)SendMessageA(hdr->hwndFrom, PSM_HWNDTOINDEX, (WPARAM)hwnd, 0); + if (active_page_changed) + SetEvent(active_page_changed); return TRUE; case PSN_KILLACTIVE: /* prevent navigation away from the fourth page */ @@ -1388,6 +1392,186 @@ static void test_invalid_hpropsheetpage(void) DestroyWindow(hdlg); } +#define WM_DIALOG_IDLE (WM_USER + 0x99) + +static WNDPROC old_hotkeys_dialog_proc; +static HWND hotkey_dialog; +static HANDLE hotkey_dialog_idle; + +static LRESULT CALLBACK new_hotkeys_dialog_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) +{ + switch(msg) + { + case WM_INITDIALOG: + hotkey_dialog = hwnd; + PostMessageW(hwnd, WM_DIALOG_IDLE, 0, 0); + break; + + case WM_DIALOG_IDLE: + SetEvent(hotkey_dialog_idle); + return TRUE; + } + + return CallWindowProcW(old_hotkeys_dialog_proc, hwnd, msg, wparam, lparam); +} + + +static LRESULT CALLBACK hook_proc_hotkeys(int code, WPARAM wparam, LPARAM lparam) +{ + static BOOL done; + if (code == HCBT_CREATEWND) + { + CBT_CREATEWNDW *c = (CBT_CREATEWNDW *)lparam; + + /* The first dialog created will be the parent dialog */ + if (!done && c->lpcs->lpszClass == (LPWSTR)WC_DIALOG) + { + old_hotkeys_dialog_proc = (WNDPROC)SetWindowLongPtrW((HWND)wparam, GWLP_WNDPROC, (LONG_PTR)new_hotkeys_dialog_proc); + done = TRUE; + } + } + + return CallNextHookEx(NULL, code, wparam, lparam); +} + +static DWORD WINAPI hotkey_dialog_thread(void *arg) +{ + HPROPSHEETPAGE hpsp[2]; + PROPSHEETPAGEA psp[2]; + PROPSHEETHEADERA psh; + HHOOK hook; + int i; + + /* set up a hook proc in order to subclass the main dialog early on */ + hook = SetWindowsHookExW(WH_CBT, hook_proc_hotkeys, NULL, GetCurrentThreadId()); + + /* create the property sheet pages */ + memset(psp, 0, sizeof(PROPSHEETPAGEA) * ARRAY_SIZE(psp)); + + for (i = 0; i < ARRAY_SIZE(psp); i++) + { + psp[i].dwSize = sizeof(PROPSHEETPAGEA); + psp[i].hInstance = GetModuleHandleA(NULL); + psp[i].pszTemplate = (LPCSTR)MAKEINTRESOURCE(IDD_PROP_PAGE_RADIO); + psp[i].pfnDlgProc = nav_page_proc; + hpsp[i] = pCreatePropertySheetPageA(&psp[i]); + } + + /* set up the property sheet dialog */ + memset(&psh, 0, sizeof(psh)); + psh.dwSize = PROPSHEETHEADERA_V1_SIZE; + psh.dwFlags = PSH_WIZARD; + psh.pszCaption = "A Wizard"; + psh.nPages = ARRAY_SIZE(psp); + psh.hwndParent = GetDesktopWindow(); + psh.phpage = hpsp; + pPropertySheetA(&psh); + + UnhookWindowsHookEx(hook); + + return 0; +} + +static void test_hotkey_navigation(void) +{ + HWND control; + HANDLE thread; + int i; + INPUT inputs[6] = {0}; + BOOL success; + + for (i = 0; i < ARRAY_SIZE(inputs); i++) + { + inputs[i].type = INPUT_KEYBOARD; + } + + active_page_changed = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(active_page_changed != NULL, "CreateEvent failed, error %lu\n", GetLastError()); + hotkey_dialog_idle = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(hotkey_dialog_idle != NULL, "CreateEvent failed, error %lu\n", GetLastError()); + + thread = CreateThread(NULL, 0, hotkey_dialog_thread, NULL, 0, NULL); + + WaitForSingleObject(hotkey_dialog_idle, INFINITE); + ResetEvent(active_page_changed); + success = SetForegroundWindow(hotkey_dialog); + ok(success, "SetForegroundWindow failed\n"); + SetFocus(hotkey_dialog); + + control = (HWND)SendMessageA(hotkey_dialog, PSM_GETCURRENTPAGEHWND, 0, 0); + active_page = (LONG)SendMessageA(hotkey_dialog, PSM_HWNDTOINDEX, (WPARAM)control, 0); + ok(active_page == 0, "Got %ld\n", active_page); + + inputs[0].ki.wVk = VK_CONTROL; + inputs[0].ki.dwFlags = 0; + inputs[1].ki.wVk = VK_NEXT; + inputs[1].ki.dwFlags = 0; + inputs[2].ki.wVk = VK_NEXT; + inputs[2].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[3].ki.wVk = VK_CONTROL; + inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; + SendInput(4, inputs, sizeof(INPUT)); + WaitForSingleObject(active_page_changed, 1000); + todo_wine + ok(active_page == 1, "Got %ld\n", active_page); + PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); + WaitForSingleObject(hotkey_dialog_idle, INFINITE); + + inputs[0].ki.wVk = VK_CONTROL; + inputs[0].ki.dwFlags = 0; + inputs[1].ki.wVk = VK_PRIOR; + inputs[1].ki.dwFlags = 0; + inputs[2].ki.wVk = VK_PRIOR; + inputs[2].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[3].ki.wVk = VK_CONTROL; + inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; + SendInput(4, inputs, sizeof(INPUT)); + WaitForSingleObject(active_page_changed, 1000); + ok(active_page == 0, "Got %ld\n", active_page); + PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); + WaitForSingleObject(hotkey_dialog_idle, INFINITE); + + inputs[0].ki.wVk = VK_CONTROL; + inputs[0].ki.dwFlags = 0; + inputs[1].ki.wVk = VK_TAB; + inputs[1].ki.dwFlags = 0; + inputs[2].ki.wVk = VK_TAB; + inputs[2].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[3].ki.wVk = VK_CONTROL; + inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; + SendInput(4, inputs, sizeof(INPUT)); + WaitForSingleObject(active_page_changed, 1000); + todo_wine + ok(active_page == 1, "Got %ld\n", active_page); + PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); + WaitForSingleObject(hotkey_dialog_idle, INFINITE); + + inputs[0].ki.wVk = VK_CONTROL; + inputs[0].ki.dwFlags = 0; + inputs[1].ki.wVk = VK_SHIFT; + inputs[1].ki.dwFlags = 0; + inputs[2].ki.wVk = VK_TAB; + inputs[2].ki.dwFlags = 0; + inputs[3].ki.wVk = VK_TAB; + inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[4].ki.wVk = VK_SHIFT; + inputs[4].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[5].ki.wVk = VK_CONTROL; + inputs[5].ki.dwFlags = KEYEVENTF_KEYUP; + SendInput(6, inputs, sizeof(INPUT)); + WaitForSingleObject(active_page_changed, 1000); + ok(active_page == 0, "Got %ld\n", active_page); + PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); + WaitForSingleObject(hotkey_dialog_idle, INFINITE); + + PostMessageA(hotkey_dialog, WM_CLOSE, 0, 0); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + CloseHandle(hotkey_dialog_idle); + CloseHandle(active_page_changed); + active_page_changed = NULL; +} + static void init_comctl32_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -1442,6 +1626,7 @@ START_TEST(propsheet) test_CreatePropertySheetPage(); test_page_dialog_texture(); test_invalid_hpropsheetpage(); + test_hotkey_navigation(); if (!load_v6_module(&ctx_cookie, &ctx)) return; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5761
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56103 --- dlls/comctl32/propsheet.c | 108 ++++++++++++++++---------------- dlls/comctl32/tests/propsheet.c | 2 - 2 files changed, 54 insertions(+), 56 deletions(-) diff --git a/dlls/comctl32/propsheet.c b/dlls/comctl32/propsheet.c index b530a1a8094..fe8a4c68333 100644 --- a/dlls/comctl32/propsheet.c +++ b/dlls/comctl32/propsheet.c @@ -2884,6 +2884,59 @@ static void PROPSHEET_CleanUp(HWND hwndDlg) GlobalFree(psInfo); } +/****************************************************************************** + * PROPSHEET_IsDialogMessage + */ +static BOOL PROPSHEET_IsDialogMessage(HWND hwnd, LPMSG lpMsg) +{ + PropSheetInfo* psInfo = GetPropW(hwnd, PropSheetInfoStr); + + TRACE("\n"); + if (!psInfo || (hwnd != lpMsg->hwnd && !IsChild(hwnd, lpMsg->hwnd))) + return FALSE; + + if (lpMsg->message == WM_KEYDOWN && (GetKeyState(VK_CONTROL) & 0x8000)) + { + int new_page = 0; + INT dlgCode = SendMessageW(lpMsg->hwnd, WM_GETDLGCODE, 0, (LPARAM)lpMsg); + + if (!(dlgCode & DLGC_WANTMESSAGE)) + { + switch (lpMsg->wParam) + { + case VK_TAB: + if (GetKeyState(VK_SHIFT) & 0x8000) + new_page = -1; + else + new_page = 1; + break; + + case VK_NEXT: new_page = 1; break; + case VK_PRIOR: new_page = -1; break; + } + } + + if (new_page) + { + if (PROPSHEET_CanSetCurSel(hwnd) != FALSE) + { + new_page += psInfo->active_page; + + if (new_page < 0) + new_page = psInfo->nPages - 1; + else if (new_page >= psInfo->nPages) + new_page = 0; + + PROPSHEET_SetCurSel(hwnd, new_page, 1, 0); + } + + return TRUE; + } + } + + return IsDialogMessageW(hwnd, lpMsg); +} + static INT do_loop(const PropSheetInfo *psInfo) { MSG msg = { 0 }; @@ -2896,7 +2949,7 @@ static INT do_loop(const PropSheetInfo *psInfo) if(ret == -1) break; - if(!IsDialogMessageW(hwnd, &msg)) + if(!PROPSHEET_IsDialogMessage(hwnd, &msg)) { TranslateMessage(&msg); DispatchMessageW(&msg); @@ -3234,59 +3287,6 @@ BOOL WINAPI DestroyPropertySheetPage(HPROPSHEETPAGE hpsp) return TRUE; } -/****************************************************************************** - * PROPSHEET_IsDialogMessage - */ -static BOOL PROPSHEET_IsDialogMessage(HWND hwnd, LPMSG lpMsg) -{ - PropSheetInfo* psInfo = GetPropW(hwnd, PropSheetInfoStr); - - TRACE("\n"); - if (!psInfo || (hwnd != lpMsg->hwnd && !IsChild(hwnd, lpMsg->hwnd))) - return FALSE; - - if (lpMsg->message == WM_KEYDOWN && (GetKeyState(VK_CONTROL) & 0x8000)) - { - int new_page = 0; - INT dlgCode = SendMessageW(lpMsg->hwnd, WM_GETDLGCODE, 0, (LPARAM)lpMsg); - - if (!(dlgCode & DLGC_WANTMESSAGE)) - { - switch (lpMsg->wParam) - { - case VK_TAB: - if (GetKeyState(VK_SHIFT) & 0x8000) - new_page = -1; - else - new_page = 1; - break; - - case VK_NEXT: new_page = 1; break; - case VK_PRIOR: new_page = -1; break; - } - } - - if (new_page) - { - if (PROPSHEET_CanSetCurSel(hwnd) != FALSE) - { - new_page += psInfo->active_page; - - if (new_page < 0) - new_page = psInfo->nPages - 1; - else if (new_page >= psInfo->nPages) - new_page = 0; - - PROPSHEET_SetCurSel(hwnd, new_page, 1, 0); - } - - return TRUE; - } - } - - return IsDialogMessageW(hwnd, lpMsg); -} - /****************************************************************************** * PROPSHEET_DoCommand */ diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c index a7953135992..41205660231 100644 --- a/dlls/comctl32/tests/propsheet.c +++ b/dlls/comctl32/tests/propsheet.c @@ -1512,7 +1512,6 @@ static void test_hotkey_navigation(void) inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(4, inputs, sizeof(INPUT)); WaitForSingleObject(active_page_changed, 1000); - todo_wine ok(active_page == 1, "Got %ld\n", active_page); PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); WaitForSingleObject(hotkey_dialog_idle, INFINITE); @@ -1541,7 +1540,6 @@ static void test_hotkey_navigation(void) inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(4, inputs, sizeof(INPUT)); WaitForSingleObject(active_page_changed, 1000); - todo_wine ok(active_page == 1, "Got %ld\n", active_page); PostMessageW(hotkey_dialog, WM_DIALOG_IDLE, 0, 0); WaitForSingleObject(hotkey_dialog_idle, INFINITE); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5761
On Thu Jun 27 23:08:13 2024 +0000, Jinoh Kang wrote:
I don't see the reason behind this, isn't this what my WM_DIALOG_LOADED is for? No, I mean **after** `SendInput` (and `WaitForSingleObject`). While waiting for the event, the message loop thread might still be processing other queued messages and not have dequeued ours from SendInput. Are you saying that message loop has no more work once the window has been initialized? Because that's not the case. Ah you mean it might not have processed the keyup yet and we already continue execution?
I pushed an update, is this better? I used my own message over WM_NULL though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74629
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/propsheet.c:
+ SetFocus(hotkey_dialog); + + control = (HWND)SendMessageA(hotkey_dialog, PSM_GETCURRENTPAGEHWND, 0, 0); + active_page = (LONG)SendMessageA(hotkey_dialog, PSM_HWNDTOINDEX, (WPARAM)control, 0); + ok(active_page == 0, "Got %ld\n", active_page); + + inputs[0].ki.wVk = VK_CONTROL; + inputs[0].ki.dwFlags = 0; + inputs[1].ki.wVk = VK_NEXT; + inputs[1].ki.dwFlags = 0; + inputs[2].ki.wVk = VK_NEXT; + inputs[2].ki.dwFlags = KEYEVENTF_KEYUP; + inputs[3].ki.wVk = VK_CONTROL; + inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; + SendInput(4, inputs, sizeof(INPUT)); + WaitForSingleObject(active_page_changed, 1000); I would send a custom message to the window and call flush_events(). Then you can remove active_page_changed and WaitForSingleObject(hotkey_dialog_idle).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74645
flush\_events()
From which thread?
1. From main thread: useless unless we're pumping messages 2. From dialog thread: it's already pumping messages
Then you can remove active_page_changed and WaitForSingleObject(hotkey_dialog_idle).
Good idea. We should use SendMessageTimeout() specifically, to: 1. Wait until the WndProc has (1) received the message, (2) processed the messave, *and* (3) returned the LRESULT. (@DarkShadow44: you used PostMessage, which doesn't wait for the WndProc to receive it. This is why I said you should *send* the message, not post it.) 2. We want to preserve the timeout. Otherwise winetest runs might get stuck. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74651
On Thu Jun 27 23:42:56 2024 +0000, Fabian Maurer wrote:
Ah you mean it might not have processed the keyup yet and we already continue execution? I pushed an update, is this better? I used my own message over WM_NULL though. PostMessage doesn't work for this purpose. Post is not Send. See the other thread.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74652
From which thread?
1. From main thread: useless unless we're pumping messages 2. From dialog thread: it's already pumping messages
From the dialog thread of course. Now I think of it, sending a WM_NULL should be enough.
We want to preserve the timeout. Otherwise winetest runs might get stuck.
I don't think it can get stuck with the SendMessage() approach. As you said, the dialog thread is pumping messages. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74653
As you said, the dialog thread is pumping messages.
I thought it would be a good idea to avoid blocking forever as soon as possible, but now that I think about it, message pump hang is much rarer. Thanks for the heads up :') -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74654
On Fri Jun 28 11:15:21 2024 +0000, Jinoh Kang wrote:
As you said, the dialog thread is pumping messages. I thought it would be a good idea to avoid blocking forever as soon as possible, but now that I think about it, message pump hang is much rarer. Thanks for the heads up :') You mean something like I did in https://gitlab.winehq.org/DarkShadow44/wine/-/commit/3bf01926aae11ce3aa70b67...
That doesn't work, I used PostMessage intentionally to queue the message at the end of the message queue. Then I know that, once the message is processed, the others are as well. SendMessage should be handles immediately, without going through the message queue like that. Although even this breaks the tests: https://gitlab.winehq.org/DarkShadow44/wine/-/commit/2f00ffeaf532fafe233a594..., and I'm not sure why that doesn't work. It seems to process the data from SendInput after the posted message is processed. Seems like I don't really understand how exactly SendInput works. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74739
On Sat Jun 29 01:12:58 2024 +0000, Fabian Maurer wrote:
You mean something like I did in https://gitlab.winehq.org/DarkShadow44/wine/-/commit/3bf01926aae11ce3aa70b67... That doesn't work, I used PostMessage intentionally to queue the message at the end of the message queue. Then I know that, once the message is processed, the others are as well. SendMessage should be handles immediately, without going through the message queue like that. Although even this breaks the tests: https://gitlab.winehq.org/DarkShadow44/wine/-/commit/2f00ffeaf532fafe233a594..., and I'm not sure why that doesn't work. It seems to process the data from SendInput after the posted message is processed. Seems like I don't really understand how exactly SendInput works. Eh, you are sending a message to the window created by another thread, the message should be queued in such a case. I wonder why it doesn't work.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74742
SendMessage should be handles immediately, without going through the message queue like that.
Not if the window belongs to another thread. Pay special attention to the 4<sup>th</sup> paragraph of https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-sendm...:
If the specified window was created by the calling thread, the window procedure is called immediately as a subroutine. **If the specified window was created by a different thread, the system switches to that thread and calls the appropriate window procedure.** Messages sent between threads are processed only when the receiving thread executes message retrieval code.
[emphasis mine] ---
I wonder why it doesn't work.
Because sent messages (QS\_SENDMESSAGE) take priority over hardware (QS\_INPUT) messages. See server/queue.c, get\_messages handler. Posted messages (QS\_POSTMESSAGE) have the same problem. That is, we still need the WaitForSingleObject. My original proposal was to use SendMessage *in addition to* WaitForSingleObject, not replace it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74743
participants (4)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Jinoh Kang (＠iamahuman)
-
Zhiyi Zhang (＠zhiyi)