[PATCH v5 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... -- v5: 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 | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index d119b68d4fd..3c1cad2dddb 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -558,6 +558,19 @@ 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, 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 3c1cad2dddb..71fdcc6d296 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 71fdcc6d296..4e01a076ddd 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 | 8 ++++++++ dlls/uxtheme/system.c | 4 ---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index a9b46f8ef22..e0f4e538ec6 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -1078,7 +1078,11 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U if (szAppName[0]) 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) { @@ -1086,7 +1090,11 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U if (szAppName[0]) 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); -- 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 e0f4e538ec6..81757898005 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 4e01a076ddd..e29bfbd68a9 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
+ hTheme = OpenThemeData(hWnd, L"dead:beef;explorer::treeview");
Sorry if I gave you the wrong suggestion. But we should use :: between dead and beef.
Using "dead::beef" instead of "dead:beef" makes OpenThemeData() call to fail under Windows. Under Wine (after adding support for parsing "app::class") this call succeeds. It looks like OpenThemeData() should fail when first element contains invalid data, however I believe that fixing this case doesn't belong to this MR, it already starts to include not related things like fixing SetWindowTheme/OpenThemeData sequence. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_50619
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/tests/system.c:
/* 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());
Let's add a succeeding test for OpenThemeData(hWnd, L"deadbeef::treeview;dead::beef;") for 5/6. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_50958
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/tests/system.c:
"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() ); -
I am not sure of the exact intent of the original commit d9c5cef339c7e169e6b9dbd96a3158f91fa9056c. But it seems to me here IsThemePartDefined() is deliberately tested after the theme handle is closed. Similar to the GetWindowTheme(). So I would prefer not to move this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_50959
- ok( bTPDefined == FALSE, "Expected FALSE\n" ); - ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); -
I am not sure of the exact intent of the original commit d9c5cef339c7e169e6b9dbd96a3158f91fa9056c. But it seems to me here IsThemePartDefined() is deliberately tested after the theme handle is closed. Similar to the GetWindowTheme(). So I would prefer not to move this.
There are several hTheme leaks, so I'd guess that it's pure luck that the IsThemePartDefined() test succeded. It looks like the author of the commit didn't notice that the result of the test depends on a side effect. I'd expect to see a comment with an explanation why the test is placed after the CloseThemeData() call. If you insist I'll leave this test alone, and not fix hTheme leaks at all, however it's not guaranteed that somebody else (Coverity?) will notice the leaks, and will try to fix them later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_50961
On Sun Nov 5 11:26:47 2023 +0000, Dmitry Timoshkov wrote:
- ok( bTPDefined == FALSE, "Expected FALSE\n" ); - ok( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 0x%08lx\n", GetLastError() ); -
I am not sure of the exact intent of the original commit d9c5cef339c7e169e6b9dbd96a3158f91fa9056c. But it seems to me here IsThemePartDefined() is deliberately tested after the theme handle is closed. Similar to the GetWindowTheme(). So I would prefer not to move this. There are several hTheme leaks, so I'd guess that it's pure luck that the IsThemePartDefined() test succeded. It looks like the author of the commit didn't notice that the result of the test depends on a side effect. I'd expect to see a comment with an explanation why the test is placed after the CloseThemeData() call. If you insist I'll leave this test alone, and not fix hTheme leaks at all, however it's not guaranteed that somebody else (Coverity?) will notice the leaks, and will try to fix them later. Okay, let's keep your patch as it is. Thanks for working on this by the way.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4140#note_50962
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Zhiyi Zhang (＠zhiyi)