[PATCH v6 0/2] MR5761: comclt32: Allow ctrl-tab and tab hotkeys for propsheets
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56103 -- v6: comclt32: Allow hotkeys for propsheet navigation. https://gitlab.winehq.org/wine/wine/-/merge_requests/5761
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comctl32/tests/propsheet.c | 174 ++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c index 9bd1c08bd6a..599583007ec 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,175 @@ static void test_invalid_hpropsheetpage(void) DestroyWindow(hdlg); } +#define WM_DIALOG_LOADED (WM_USER + 0x99) + +static WNDPROC old_hotkeys_dialog_proc; +static HWND hotkey_dialog; +static HANDLE hotkey_dialog_loaded; + +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_LOADED, 0, 0); + break; + + case WM_DIALOG_LOADED: + SetEvent(hotkey_dialog_loaded); + 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}; + + 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_loaded = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(hotkey_dialog_loaded != NULL, "CreateEvent failed, error %lu\n", GetLastError()); + + thread = CreateThread(NULL, 0, hotkey_dialog_thread, NULL, 0, NULL); + + WaitForSingleObject(hotkey_dialog_loaded, INFINITE); + ResetEvent(active_page_changed); + 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, 200); + todo_wine + ok(active_page == 1, "Got %ld\n", active_page); + + 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, 200); + 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_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, 200); + todo_wine + ok(active_page == 1, "Got %ld\n", active_page); + + 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, 200); + ok(active_page == 0, "Got %ld\n", active_page); + + PostMessageA(hotkey_dialog, WM_CLOSE, 0, 0); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + CloseHandle(hotkey_dialog_loaded); + CloseHandle(active_page_changed); + active_page_changed = NULL; +} + static void init_comctl32_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -1442,6 +1615,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 | 10 ++- 2 files changed, 58 insertions(+), 60 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 599583007ec..a44909f420f 100644 --- a/dlls/comctl32/tests/propsheet.c +++ b/dlls/comctl32/tests/propsheet.c @@ -1508,8 +1508,7 @@ static void test_hotkey_navigation(void) inputs[3].ki.wVk = VK_CONTROL; inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(4, inputs, sizeof(INPUT)); - WaitForSingleObject(active_page_changed, 200); - todo_wine + WaitForSingleObject(active_page_changed, 150); ok(active_page == 1, "Got %ld\n", active_page); inputs[0].ki.wVk = VK_CONTROL; @@ -1521,7 +1520,7 @@ static void test_hotkey_navigation(void) inputs[3].ki.wVk = VK_CONTROL; inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(4, inputs, sizeof(INPUT)); - WaitForSingleObject(active_page_changed, 200); + WaitForSingleObject(active_page_changed, 150); ok(active_page == 0, "Got %ld\n", active_page); inputs[0].ki.wVk = VK_CONTROL; @@ -1533,8 +1532,7 @@ static void test_hotkey_navigation(void) inputs[3].ki.wVk = VK_CONTROL; inputs[3].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(4, inputs, sizeof(INPUT)); - WaitForSingleObject(active_page_changed, 200); - todo_wine + WaitForSingleObject(active_page_changed, 150); ok(active_page == 1, "Got %ld\n", active_page); inputs[0].ki.wVk = VK_CONTROL; @@ -1550,7 +1548,7 @@ static void test_hotkey_navigation(void) inputs[5].ki.wVk = VK_CONTROL; inputs[5].ki.dwFlags = KEYEVENTF_KEYUP; SendInput(6, inputs, sizeof(INPUT)); - WaitForSingleObject(active_page_changed, 200); + WaitForSingleObject(active_page_changed, 150); ok(active_page == 0, "Got %ld\n", active_page); PostMessageA(hotkey_dialog, WM_CLOSE, 0, 0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5761
On Mon Jun 24 22:24:39 2024 +0000, Zhiyi Zhang wrote:
There is a test failure on Windows 10 21H1 Arabic. See https://testbot.winehq.org/JobDetails.pl?Key=146448#k309. Please take a look at how it should work on Arabic locale on Windows. I reran the tests manually by uploading comctl32_test to the testbot, and there were no issues. I also restarted the pipeline by making a simple change, and now all is green.. I think that test failure isn't related to Arabic, it looks more like a spurious failure.
Either the test I made is inherently flaky or it just ran into the timeout, although 200ms seemed plenty to me... I can't reproduce it locally either, even when running the test hundreds of times. I'd like to fix the test though, can you advice me on how to approach that? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74291
On Mon Jun 24 22:25:05 2024 +0000, Fabian Maurer wrote:
I reran the tests manually by uploading comctl32_test to the testbot, and there were no issues. I also restarted the pipeline by making a simple change, and now all is green.. I think that test failure isn't related to Arabic, it looks more like a spurious failure. Either the test I made is inherently flaky or it just ran into the timeout, although 200ms seemed plenty to me... I can't reproduce it locally either, even when running the test hundreds of times. I'd like to fix the test though, can you advice me on how to approach that? Did you try with a larger timeout or Windows 10 21H1 in Arabic (Egypt) locale?
I would also try making the dialog foreground to ensure the input is not sent to other windows. If all that fails, you can add a flaky_if(in_arabic_locale). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74312
On Tue Jun 25 01:02:26 2024 +0000, Zhiyi Zhang wrote:
Did you try with a larger timeout or Windows 10 21H1 in Arabic (Egypt) locale? I would also try making the dialog foreground to ensure the input is not sent to other windows. If all that fails, you can add a flaky_if(in_arabic_locale). I reduced the timeout to 150ms, and the tests work everywhere: https://testbot.winehq.org/JobDetails.pl?Key=146528
As noted, I don't think it's Arabic only. But I don't feel comfortable merging it with a higher timeout unless I know what caused it. Problem is that I can't really reproduce the issue, so I don't know how to fix it either. Maybe making it foreground would help, but how would I know? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74397
On Tue Jun 25 17:55:32 2024 +0000, Fabian Maurer wrote:
I reduced the timeout to 150ms, and the tests work everywhere: https://testbot.winehq.org/JobDetails.pl?Key=146528 As noted, I don't think it's Arabic only. But I don't feel comfortable merging it with a higher timeout unless I know what caused it. Problem is that I can't really reproduce the issue, so I don't know how to fix it either. Maybe making it foreground would help, but how would I know? If you run it on TestBots multiple times ( >=5 ) and can't reproduce the issue anymore, let's consider it fixed. If it still appears in future runs, let's add a flaky.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5761#note_74426
participants (3)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Zhiyi Zhang (＠zhiyi)