[PATCH v6 0/6] MR4140: uxtheme/tests: Add some tests for OpenThemeData() with various app/class names.
Themed Delphi applications use "explorer::listview" and "explorer::treeview": https://gitlab.com/freepascal.org/lazarus/lazarus/-/blob/main/lcl/interfaces... -- v6: uxtheme: If the application class is already set then OpenThemeData() should fail. uxtheme: Move fall back to default class to MSSTYLES_OpenThemeClass(). uxtheme/tests: Add a test for SetWindowTheme/OpenThemeData sequence. uxtheme: Parse app/class name in OpenThemeData(). uxtheme/tests: Add a test for OpenThemeData("explorer::treeview"). uxtheme/tests: Move the IsThemePartDefined() test before hTheme handle is closed. https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Also fix some hTheme handle leaks. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/tests/system.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index c5fc19dd86a..d119b68d4fd 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -562,12 +562,14 @@ static void test_OpenThemeData(void) hTheme = OpenThemeData(hWnd, szButtonClassList); ok( hTheme != NULL, "got NULL, expected a HTHEME handle\n"); ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); + CloseThemeData(hTheme); /* Test with bUtToN instead of Button */ SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, szButtonClassList2); ok( hTheme != NULL, "got NULL, expected a HTHEME handle\n"); ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); + CloseThemeData(hTheme); SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, szClassList); @@ -583,6 +585,12 @@ static void test_OpenThemeData(void) "Expected 0xdeadbeef, got 0x%08lx\n", GetLastError()); + SetLastError(0xdeadbeef); + bTPDefined = IsThemePartDefined(hTheme, 0, 0); + todo_wine + ok( bTPDefined == FALSE, "Expected FALSE\n" ); + ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); + hRes = CloseThemeData(hTheme); ok( hRes == S_OK, "Expected S_OK, got 0x%08lx\n", hRes); @@ -600,12 +608,6 @@ static void test_OpenThemeData(void) "Expected 0xdeadbeef, got 0x%08lx\n", GetLastError()); - SetLastError(0xdeadbeef); - bTPDefined = IsThemePartDefined(hTheme, 0 , 0); - todo_wine - ok( bTPDefined == FALSE, "Expected FALSE\n" ); - ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); - DestroyWindow(hWnd); } @@ -2591,7 +2593,7 @@ static void test_theme(void) /* > XP use opaque scrollbar arrow parts, but TMT_TRANSPARENT is TRUE */ else { - ok(hr == S_OK, "Got unexpected hr %#lx,\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); ok(transparent, "Expected transparent.\n"); transparent = IsThemeBackgroundPartiallyTransparent(htheme, SBP_ARROWBTN, 0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/tests/system.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index d119b68d4fd..c9fdf30820d 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -558,6 +558,27 @@ static void test_OpenThemeData(void) /* Only do the next checks if we have an active theme */ + SetLastError(0xdeadbeef); + hTheme = OpenThemeData(hWnd, L"dead::beef;explorer::treeview"); + ok(!hTheme, "OpenThemeData() should fail\n"); + ok(GetLastError() == E_PROP_ID_UNSUPPORTED, "Got unexpected %#lx.\n", GetLastError()); + + SetLastError(0xdeadbeef); + hTheme = OpenThemeData(hWnd, L"explorer::treeview"); + todo_wine + ok(hTheme != NULL, "OpenThemeData() failed\n"); + todo_wine + ok(GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError()); + CloseThemeData(hTheme); + + SetLastError(0xdeadbeef); + hTheme = OpenThemeData(hWnd, L"deadbeef::treeview;dead::beef"); + todo_wine + ok(hTheme != NULL, "OpenThemeData() failed\n"); + todo_wine + ok(GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError()); + CloseThemeData(hTheme); + SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, szButtonClassList); ok( hTheme != NULL, "got NULL, expected a HTHEME handle\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Themed Delphi applications use "explorer::listview" and "explorer::treeview": https://gitlab.com/freepascal.org/lazarus/lazarus/-/blob/main/lcl/interfaces... Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/msstyles.c | 35 +++++++++++++++++++++++++++++++---- dlls/uxtheme/tests/system.c | 4 ++-- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index cfd21a48989..a9b46f8ef22 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -1022,6 +1022,23 @@ static void MSSTYLES_ParseThemeIni(PTHEME_FILE tf, BOOL setMetrics) } } +static void parse_app_class_name(LPCWSTR name, LPWSTR app_name, LPWSTR class_name) +{ + LPCWSTR p; + + app_name[0] = class_name[0] = 0; + + p = wcsstr(name, L"::"); + if (p) + { + lstrcpynW(app_name, name, min(p - name + 1, MAX_THEME_APP_NAME)); + p += 2; + lstrcpynW(class_name, p, min(wcslen(p) + 1, MAX_THEME_CLASS_NAME)); + } + else + lstrcpynW(class_name, name, MAX_THEME_CLASS_NAME); +} + /*********************************************************************** * MSSTYLES_OpenThemeClass * @@ -1036,6 +1053,8 @@ static void MSSTYLES_ParseThemeIni(PTHEME_FILE tf, BOOL setMetrics) PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, UINT dpi) { PTHEME_CLASS cls = NULL; + WCHAR buf[MAX_THEME_APP_NAME + MAX_THEME_CLASS_NAME]; + WCHAR szAppName[MAX_THEME_APP_NAME]; WCHAR szClassName[MAX_THEME_CLASS_NAME]; LPCWSTR start; LPCWSTR end; @@ -1052,14 +1071,22 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U start = pszClassList; while((end = wcschr(start, ';'))) { len = end-start; - lstrcpynW(szClassName, start, min(len+1, ARRAY_SIZE(szClassName))); + lstrcpynW(buf, start, min(len+1, ARRAY_SIZE(buf))); start = end+1; - cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); + + parse_app_class_name(buf, szAppName, szClassName); + if (szAppName[0]) + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); if(cls) break; } if(!cls && *start) { - lstrcpynW(szClassName, start, ARRAY_SIZE(szClassName)); - cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); + parse_app_class_name(start, szAppName, szClassName); + if (szAppName[0]) + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); } if(cls) { TRACE("Opened app %s, class %s from list %s\n", debugstr_w(cls->szAppName), debugstr_w(cls->szClassName), debugstr_w(pszClassList)); diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index c9fdf30820d..eca609b7971 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -560,14 +560,14 @@ static void test_OpenThemeData(void) SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, L"dead::beef;explorer::treeview"); + todo_wine ok(!hTheme, "OpenThemeData() should fail\n"); + todo_wine ok(GetLastError() == E_PROP_ID_UNSUPPORTED, "Got unexpected %#lx.\n", GetLastError()); SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, L"explorer::treeview"); - todo_wine ok(hTheme != NULL, "OpenThemeData() failed\n"); - todo_wine ok(GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError()); CloseThemeData(hTheme); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/tests/system.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index eca609b7971..de9419a032a 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -558,6 +558,16 @@ static void test_OpenThemeData(void) /* Only do the next checks if we have an active theme */ + hRes = SetWindowTheme(hWnd, L"explorer", NULL); + ok(hRes == S_OK, "Got unexpected hr %#lx.\n", hRes); + SetLastError(0xdeadbeef); + hTheme = OpenThemeData(hWnd, L"explorer::treeview"); + todo_wine + ok(!hTheme, "OpenThemeData() should fail\n"); + todo_wine + ok(GetLastError() == E_PROP_ID_UNSUPPORTED, "Got unexpected %#lx.\n", GetLastError()); + SetWindowTheme(hWnd, NULL, NULL); + SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, L"dead::beef;explorer::treeview"); todo_wine -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/msstyles.c | 7 +++++++ dlls/uxtheme/system.c | 4 ---- dlls/uxtheme/tests/system.c | 2 -- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index a9b46f8ef22..4c67e3b15c5 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -1079,6 +1079,10 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); else cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); + + /* Fall back to default class if the specified subclass is not found */ + if (!cls) cls = MSSTYLES_FindClass(tfActiveTheme, NULL, szClassName); + if(cls) break; } if(!cls && *start) { @@ -1087,6 +1091,9 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); else cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); + + /* Fall back to default class if the specified subclass is not found */ + if (!cls) cls = MSSTYLES_FindClass(tfActiveTheme, NULL, szClassName); } if(cls) { TRACE("Opened app %s, class %s from list %s\n", debugstr_w(cls->szAppName), debugstr_w(cls->szClassName), debugstr_w(pszClassList)); diff --git a/dlls/uxtheme/system.c b/dlls/uxtheme/system.c index 2c041356930..83cbaea5dfa 100644 --- a/dlls/uxtheme/system.c +++ b/dlls/uxtheme/system.c @@ -641,10 +641,6 @@ static HTHEME open_theme_data(HWND hwnd, LPCWSTR pszClassList, DWORD flags, UINT if (pszUseClassList) hTheme = MSSTYLES_OpenThemeClass(pszAppName, pszUseClassList, dpi); - - /* Fall back to default class if the specified subclass is not found */ - if (!hTheme) - hTheme = MSSTYLES_OpenThemeClass(NULL, pszUseClassList, dpi); } if(IsWindow(hwnd)) SetPropW(hwnd, (LPCWSTR)MAKEINTATOM(atWindowTheme), hTheme); diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index de9419a032a..6c7e87c1040 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -583,9 +583,7 @@ static void test_OpenThemeData(void) SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, L"deadbeef::treeview;dead::beef"); - todo_wine ok(hTheme != NULL, "OpenThemeData() failed\n"); - todo_wine ok(GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError()); CloseThemeData(hTheme); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/uxtheme/msstyles.c | 10 ++++++++++ dlls/uxtheme/tests/system.c | 2 -- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index 4c67e3b15c5..7e2b98ad378 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -1076,7 +1076,12 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U parse_app_class_name(buf, szAppName, szClassName); if (szAppName[0]) + { + /* If the application class is already set then fail */ + if (pszAppName) return NULL; + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + } else cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); @@ -1088,7 +1093,12 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U if(!cls && *start) { parse_app_class_name(start, szAppName, szClassName); if (szAppName[0]) + { + /* If the application class is already set then fail */ + if (pszAppName) return NULL; + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + } else cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index 6c7e87c1040..c79a6c85a35 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -562,9 +562,7 @@ static void test_OpenThemeData(void) ok(hRes == S_OK, "Got unexpected hr %#lx.\n", hRes); SetLastError(0xdeadbeef); hTheme = OpenThemeData(hWnd, L"explorer::treeview"); - todo_wine ok(!hTheme, "OpenThemeData() should fail\n"); - todo_wine ok(GetLastError() == E_PROP_ID_UNSUPPORTED, "Got unexpected %#lx.\n", GetLastError()); SetWindowTheme(hWnd, NULL, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139636 Your paranoid android. === debian11b (64 bit WoW report) === dinput: device8.c:2238: Test failed: 0x700: got key_state[0] 0
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/msstyles.c:
+ + parse_app_class_name(buf, szAppName, szClassName); + if (szAppName[0]) + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); if(cls) break; } if(!cls && *start) { - lstrcpynW(szClassName, start, ARRAY_SIZE(szClassName)); - cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); + parse_app_class_name(start, szAppName, szClassName); + if (szAppName[0]) + cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName); Let's put these parse and find parts into a find_app_class() helper to avoid code duplication. So that you don't need to make the same changes twice in the patch set.
Let's use "szAppName[0] ? szAppName : pszAppName" instead. It will look cleaner. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_51465
+ cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName);
Let's put these parse and find parts into a find_app_class() helper to avoid code duplication. So that you don't need to make the same changes twice in the patch set.
Let's use "szAppName[0] ? szAppName : pszAppName" instead. It will look cleaner.
Can this micro optimizations be made in a folow up patches? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_51468
On Wed Nov 8 14:24:10 2023 +0000, Dmitry Timoshkov wrote:
+ cls = MSSTYLES_FindClass(tfActiveTheme, szAppName, szClassName); + else + cls = MSSTYLES_FindClass(tfActiveTheme, pszAppName, szClassName);
Let's put these parse and find parts into a find_app_class() helper to avoid code duplication. So that you don't need to make the same changes twice in the patch set.
Let's use "szAppName[0] ? szAppName : pszAppName" instead. It will look cleaner. Can this micro optimizations be made in a folow up patches? Okay, if you feel strongly about changing it now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_51507
This merge request was approved by Zhiyi Zhang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140
participants (4)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Marvin -
Zhiyi Zhang (＠zhiyi)