[PATCH v3 0/1] MR1977: comctl32: Fix PathWordBreakProc.
Currently it doesn't work as expected, namely gets stuck on delimiter and caret is moved before the delimiter. Unnecessary more robust than Windows, which feels like doesn't do any checks at all. EDIT1: v2: return length in `WB_RIGHT` if its bigger than default return value of 0 on invalid values -- v3: comctl32: Fix PathWordBreakProc. https://gitlab.winehq.org/wine/wine/-/merge_requests/1977
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/comctl32/commctrl.c | 53 ++++++++++-- dlls/comctl32/tests/combo.c | 155 ++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 8 deletions(-) diff --git a/dlls/comctl32/commctrl.c b/dlls/comctl32/commctrl.c index c5910b40869..18ff9e41bff 100644 --- a/dlls/comctl32/commctrl.c +++ b/dlls/comctl32/commctrl.c @@ -1628,15 +1628,52 @@ static inline BOOL IsDelimiter(WCHAR c) static int CALLBACK PathWordBreakProc(LPCWSTR lpch, int ichCurrent, int cch, int code) { - if (code == WB_ISDELIMITER) - return IsDelimiter(lpch[ichCurrent]); - else - { - int dir = (code == WB_LEFT) ? -1 : 1; - for(; 0 <= ichCurrent && ichCurrent < cch; ichCurrent += dir) - if (IsDelimiter(lpch[ichCurrent])) return ichCurrent; + INT ret = 0; + + switch (code) { + case WB_LEFT: + if (!lpch || ichCurrent < 0 || ichCurrent > cch) + break; + + /* skip trailing delimiters */ + while (ichCurrent && IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + /* skip word */ + while (ichCurrent && !IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + /* special case: if there's a single leading delimiter, skip it */ + if (ichCurrent == 1 && IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + + ret = ichCurrent; + break; + case WB_RIGHT: + if (!lpch || ichCurrent < 0 || ichCurrent > cch) + { + if (cch > ret) + ret = cch; + break; + } + + /* skip word */ + while (ichCurrent < cch && lpch[ichCurrent] && !IsDelimiter(lpch[ichCurrent])) + ichCurrent++; + /* move caret after the delimiters */ + while (ichCurrent < cch && lpch[ichCurrent] && IsDelimiter(lpch[ichCurrent])) + ichCurrent++; + + ret = ichCurrent; + break; + case WB_ISDELIMITER: + if (lpch && ichCurrent >= 0 && ichCurrent < cch) + ret = IsDelimiter(lpch[ichCurrent]); + break; + default: + ERR("unexpected action code\n"); + break; } - return ichCurrent; + + return ret; } /*********************************************************************** diff --git a/dlls/comctl32/tests/combo.c b/dlls/comctl32/tests/combo.c index 3eceeb55bd5..24868160970 100644 --- a/dlls/comctl32/tests/combo.c +++ b/dlls/comctl32/tests/combo.c @@ -1514,6 +1514,160 @@ static void test_comboex_CBEN_GETDISPINFO(void) DestroyWindow(combo); } +static BOOL hold_key(int vk) +{ + BYTE key_state[256]; + BOOL result; + + result = GetKeyboardState(key_state); + ok(result, "GetKeyboardState failed.\n"); + if (!result) return FALSE; + key_state[vk] |= 0x80; + result = SetKeyboardState(key_state); + ok(result, "SetKeyboardState failed.\n"); + return result != 0; +} + +static BOOL release_key(int vk) +{ + BYTE key_state[256]; + BOOL result; + + result = GetKeyboardState(key_state); + ok(result, "GetKeyboardState failed.\n"); + if (!result) return FALSE; + key_state[vk] &= ~0x80; + result = SetKeyboardState(key_state); + ok(result, "SetKeyboardState failed.\n"); + return result != 0; +} + +static LRESULT send_ctrl_key(HWND hwnd, UINT key) +{ + LRESULT result; + hold_key(VK_CONTROL); + result = SendMessageA(hwnd, WM_KEYDOWN, key, 1); + release_key(VK_CONTROL); + return result; +} + +static void move_word(INT line, HWND edit, UINT direction, INT expected_caret_position) +{ + INT selection_start = -1, selection_end = -1; + + send_ctrl_key(edit, direction); + SendMessageA(edit, EM_GETSEL, (WPARAM)&selection_start, (WPARAM)&selection_end); + ok_(__FILE__, line)(selection_start == expected_caret_position, "Unexpected caret position %i.\n", selection_start); +} + +static void test_comboex_CBES_EX_PATHWORDBREAKPROC(void) +{ + HWND combo, edit; + INT start, end; + WCHAR some_pathW[] = L"some/path"; + WCHAR delimitersW[] = L"a .\\/"; + EDITWORDBREAKPROCW work_break_proc; + int ret; + + combo = createComboEx(WS_VISIBLE | WS_CHILD | CBS_DROPDOWN); + SendMessageA(combo, CBEM_SETEXTENDEDSTYLE, 0, CBES_EX_PATHWORDBREAKPROC); + edit = (HWND)SendMessageA(combo, CBEM_GETEDITCONTROL, 0, 0); + + /* multiple delimiters */ + SetWindowTextA(edit, "C:\\aa bb\\cc.dd/ee\\ff\\/ ."); + /* from left to right */ + SendMessageA(edit, EM_GETSEL, (WPARAM)&start, (WPARAM)&end); + ok(start == 0, "Unexpected caret position %i.\n", start); /* |C:\aa bb\cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 3); /* C:\|aa bb\cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 6); /* C:\aa |bb\cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 9); /* C:\aa bb\|cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 12); /* C:\aa bb\cc.|dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 15); /* C:\aa bb\cc.dd/|ee\ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 18); /* C:\aa bb\cc.dd/ee\|ff\/ . */ + move_word(__LINE__, edit, VK_RIGHT, 24); /* C:\aa bb\cc.dd/ee\ff\/ .| */ + /* from right to left */ + SendMessageA(edit, EM_SETSEL, GetWindowTextLengthA(edit), GetWindowTextLengthA(edit)); + SendMessageA(edit, EM_GETSEL, (WPARAM)&start, (WPARAM)&end); + ok(start == 24, "Unexpected caret position %i.\n", start); /* C:\aa bb\cc.dd/ee\ff\/ .| */ + move_word(__LINE__, edit, VK_LEFT, 18); /* C:\aa bb\cc.dd/ee\|ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 15); /* C:\aa bb\cc.dd/|ee\ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 12); /* C:\aa bb\cc.|dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 9); /* C:\aa bb\|cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 6); /* C:\aa |bb\cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 3); /* C:\|aa bb\cc.dd/ee\ff\/ . */ + move_word(__LINE__, edit, VK_LEFT, 0); /* |C:\aa bb\cc.dd/ee\ff\/ . */ + + /* no trailing delimiter and leading delimiter */ + SetWindowTextA(edit, "\\\\some\\path"); + SendMessageA(edit, EM_SETSEL, GetWindowTextLengthA(edit), GetWindowTextLengthA(edit)); + SendMessageA(edit, EM_GETSEL, (WPARAM)&start, (WPARAM)&end); + ok(start == 11, "Unexpected caret position %i.\n", start); /* \\some\path| */ + move_word(__LINE__, edit, VK_LEFT, 7); /* \\some\|path */ + move_word(__LINE__, edit, VK_LEFT, 2); /* \\|some\path*/ + move_word(__LINE__, edit, VK_LEFT, 0); /* |\\some\path*/ + + /* single leading delimiter */ + SetWindowTextA(edit, "/some/path"); + SendMessageA(edit, EM_SETSEL, GetWindowTextLengthA(edit), GetWindowTextLengthA(edit)); + SendMessageA(edit, EM_GETSEL, (WPARAM)&start, (WPARAM)&end); + ok(start == 10, "Unexpected caret position %i.\n", start); /* /some/path|*/ + move_word(__LINE__, edit, VK_LEFT, 6); /* /some/|path*/ + move_word(__LINE__, edit, VK_LEFT, 0); /* |/some/path*/ + + work_break_proc = (EDITWORDBREAKPROCW)SendMessageA(edit, EM_GETWORDBREAKPROC, 0, 0); + + /* null text */ + ret = work_break_proc(NULL, 0, 0, WB_LEFT); + ok(ret == 0, "Unexpected return value %i.\n", ret); + if (!strcmp(winetest_platform, "wine")) /* crashes on Windows */ + { + ret = work_break_proc(NULL, 0, 0, WB_RIGHT); + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(NULL, 0, 0, WB_ISDELIMITER); + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(NULL, 10, 10, WB_LEFT); + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(NULL, 10, 10, WB_RIGHT); + ok(ret == 10, "Unexpected return value %i.\n", ret); + ret = work_break_proc(NULL, 10, 10, WB_ISDELIMITER); + ok(ret == 0, "Unexpected return value %i.\n", ret); + } + + ret = work_break_proc(some_pathW, 3, 0, WB_LEFT); + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(some_pathW, lstrlenW(some_pathW), lstrlenW(some_pathW), WB_LEFT); + ok(ret == 5, "Unexpected return value %i.\n", ret); + ret = work_break_proc(some_pathW, lstrlenW(some_pathW), lstrlenW(some_pathW), WB_RIGHT); + ok(ret == lstrlenW(some_pathW), "Unexpected return value %i.\n", ret); + if (!strcmp(winetest_platform, "wine")) + { + ret = work_break_proc(some_pathW, 15, 5, WB_LEFT); /* doesn't always return the same value */ + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(some_pathW, -10, 0, WB_LEFT); /* gets stuck in a loop */ + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(some_pathW, 15, 5, WB_RIGHT); /* crash or unexpected value */ + ok(ret == 5, "Unexpected return value %i.\n", ret); + ret = work_break_proc(some_pathW, 0, -5, WB_RIGHT); /* crash or unexpected value */ + ok(ret == 0, "Unexpected return value %i.\n", ret); + } + + /* invalid action code */ + ret = work_break_proc(NULL, 5, 5, 0xFFFF); + ok(ret == 0, "Unexpected return value %i.\n", ret); + + /* delimiter */ + ret = work_break_proc(delimitersW, 0, lstrlenW(delimitersW), WB_ISDELIMITER); + ok(ret == 0, "Unexpected return value %i.\n", ret); + ret = work_break_proc(delimitersW, 1, lstrlenW(delimitersW), WB_ISDELIMITER); + ok(ret == 1, "Unexpected return value %i.\n", ret); + ret = work_break_proc(delimitersW, 2, lstrlenW(delimitersW), WB_ISDELIMITER); + ok(ret == 1, "Unexpected return value %i.\n", ret); + ret = work_break_proc(delimitersW, 3, lstrlenW(delimitersW), WB_ISDELIMITER); + ok(ret == 1, "Unexpected return value %i.\n", ret); + ret = work_break_proc(delimitersW, 4, lstrlenW(delimitersW), WB_ISDELIMITER); + ok(ret == 1, "Unexpected return value %i.\n", ret); +} + START_TEST(combo) { ULONG_PTR ctx_cookie; @@ -1534,6 +1688,7 @@ START_TEST(combo) test_comboex_subclass(); test_comboex_get_set_item(); test_comboex_CBEN_GETDISPINFO(); + test_comboex_CBES_EX_PATHWORDBREAKPROC(); if (!load_v6_module(&ctx_cookie, &hCtx)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1977
Jinoh Kang (@iamahuman) commented about dlls/comctl32/commctrl.c:
static int CALLBACK PathWordBreakProc(LPCWSTR lpch, int ichCurrent, int cch, int code) { - if (code == WB_ISDELIMITER) - return IsDelimiter(lpch[ichCurrent]); - else - { - int dir = (code == WB_LEFT) ? -1 : 1; - for(; 0 <= ichCurrent && ichCurrent < cch; ichCurrent += dir) - if (IsDelimiter(lpch[ichCurrent])) return ichCurrent; + INT ret = 0; + + switch (code) { + case WB_LEFT: + if (!lpch || ichCurrent < 0 || ichCurrent > cch)
The `ichCurrent < 0` case is not tested. Assuming it triggers undefined behaviour on Windows, it's hard to add working tests for it. Since this code is untested, you should just remove the `ichCurrent < 0`. Maybe add `assert(ichCurrent < 0);` if you're genuinely worried. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21123
Jinoh Kang (@iamahuman) commented about dlls/comctl32/commctrl.c:
+ while (ichCurrent && IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + /* skip word */ + while (ichCurrent && !IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + /* special case: if there's a single leading delimiter, skip it */ + if (ichCurrent == 1 && IsDelimiter(lpch[ichCurrent - 1])) + ichCurrent--; + + ret = ichCurrent; + break; + case WB_RIGHT: + if (!lpch || ichCurrent < 0 || ichCurrent > cch) + { + if (cch > ret) + ret = cch;
ret = cch;
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21124
It's better to test this directly by calling SetPathWordBreakProc() on some test window. This way you don't need a combo window. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21126
On Wed Jan 18 12:49:43 2023 +0000, Nikolay Sivov wrote:
It's better to test this directly by calling SetPathWordBreakProc() on some test window. This way you don't need a combo window. To explicitly test `SetPathWordBreakProc` I guess? I was using an edit control previously, but then changed it to combobox to cover `CBES_EX_PATHWORDBREAKPROC` along the way.
Should I move the test to edit tests instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21127
On Wed Jan 18 12:49:43 2023 +0000, Vladislav Timonin wrote:
To explicitly test `SetPathWordBreakProc` I guess? I was using an edit control previously, but then changed it to combobox to cover `CBES_EX_PATHWORDBREAKPROC` along the way. Should I move the test to edit tests instead? You can keep both tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21128
On Wed Jan 18 11:11:53 2023 +0000, Jinoh Kang wrote:
The `ichCurrent < 0` case is not tested. Assuming it triggers undefined behaviour on Windows, it's hard to add working tests for it. Since this code is untested, you should just remove the `ichCurrent < 0`. Maybe add `assert(ichCurrent < 0);` if you're genuinely worried. Tests that are broken on Windows are ran on Wine for what it's worth, `ichCurrent < 0` in particular is tested by `work_break_proc(some_pathW, -10, 0, WB_LEFT)`, which on Windows gets stuck in a loop.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21129
On Wed Jan 18 12:56:58 2023 +0000, Vladislav Timonin wrote:
Tests that are broken on Windows are ran on Wine for what it's worth, `ichCurrent < 0` in particular is tested by `work_break_proc(some_pathW, -10, 0, WB_LEFT)`, which on Windows gets stuck in a loop. I didn't see the `winetest_platform` test. My apologies.
While we don't necessarily put extra effort into cloning Windows behaviour that no known apps rely upon, we shouldn't put extra effort into _deviating_ from Windows behaviour, either. In general, subtle differences from Windows behavior tend to decrease the chance of arbitrary Windows apps running successfully on Wine, since the appljcation may be relying (even possibly unknowingly by the author) on undocumented Windows behaviour. If Windows crashes or hangs, Wine should crash or hang too instead of masking the error and continuing silently; otherwise, it makes harder to spot e.g. bugs in Wine that made the application misbehave and supply invalid arguments in the first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1977#note_21130
participants (4)
-
Jinoh Kang (＠iamahuman) -
Nikolay Sivov (＠nsivov)
-
Vladislav Timonin
-
Vladislav Timonin (＠vt)