Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/tests/system.c:
> + /* Call ShouldSystemUseDarkMode() and check for errors */
> + SetLastError(ERROR_SUCCESS); /* Clear error buffer so that we can detect if an error occurred. */
> + function_result = pShouldSystemUseDarkMode();
> + last_error = GetLastError();
> + ok(last_error == ERROR_SUCCESS, "ShouldSystemUseDarkMode failed (?) with error %ld.\n",
> + last_error);
> +
> + /* Expect same value as key */
> + ok(function_result == !system_uses_light_theme, "Expected value %d, got %d.\n",
> + !system_uses_light_theme, function_result);
> +
> + ls = RegDeleteValueA(hk, "AppsUseLightTheme");
> + ok(ls == 0, "RegDeleteValueA failed: %ld.\n", ls);
> +
> + /* Call ShouldSystemUseDarkMode() and check for errors with deleted value */
> + SetLastError(ERROR_SUCCESS); /* Clear error buffer so that we can detect if an error occurred. */
For the last error code. You can SetLastError(0xdeadbeef) before the call and check that the last error is set to ERROR_SUCCESS after the call. And then you set the last error code in the ShouldSystemUseDarkMode() implementation.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47274
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/system.c:
> UnregisterUserApiHook();
> return TRUE;
> }
> +
> +/**********************************************************************
> + * ShouldSystemUseDarkMode (UXTHEME.138)
> + *
> + * RETURNS
> + * Whether or not the system/app should use dark mode.
Shouldn't `system/app` be `system`? because there is a ShouldAppsUseDarkMode() as you said previously.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47273
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/system.c:
> UnregisterUserApiHook();
> return TRUE;
> }
> +
> +/**********************************************************************
> + * ShouldSystemUseDarkMode (UXTHEME.138)
> + *
> + * RETURNS
> + * Whether or not the system/app should use dark mode.
> + */
> +BOOL WINAPI ShouldSystemUseDarkMode(void)
> +{
> + DWORD system_uses_light_theme_size = sizeof(DWORD);
You can remove the system_uses_ prefix. These codes are in ShouldSystemUseDarkMode() so it's already known that it's for querying system settings.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47272
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/system.c:
> +/**********************************************************************
> + * ShouldSystemUseDarkMode (UXTHEME.138)
> + *
> + * RETURNS
> + * Whether or not the system/app should use dark mode.
> + */
> +BOOL WINAPI ShouldSystemUseDarkMode(void)
> +{
> + DWORD system_uses_light_theme_size = sizeof(DWORD);
> + /* Persists between calls, in windows it might look up some internal table. */
> + static DWORD system_uses_light_theme = TRUE;
> +
> + /* We don't necessarily care that this might fail because it doesn't affect
> + * system_uses_light_theme if it does.
> + */
> + RegGetValueA(HKEY_CURRENT_USER,
Please use RegGetValueW() instead so that we don't have to do ASCII/Unicode conversion every time this gets called.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47271
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/tests/system.c:
> DestroyWindow(hwnd);
> }
>
> +static void test_ShouldSystemUseDarkMode(void)
I think these tests might be a bit too much. I like the initial version that uses RegGetValueA() and confirms the registry value is the opposite of the ShouldSystemUseDarkMode. With some checks for the last error code and I think that's enough. These newly added tests confirm caching for ShouldSystemUseDarkMode() and that's good. However, ShouldSystemUseDarkMode() still need to query the registry key so it's not that useful.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47270
Zhiyi Zhang (@zhiyi) commented about dlls/uxtheme/system.c:
> +
> +/**********************************************************************
> + * ShouldSystemUseDarkMode (UXTHEME.138)
> + *
> + * RETURNS
> + * Whether or not the system/app should use dark mode.
> + */
> +BOOL WINAPI ShouldSystemUseDarkMode(void)
> +{
> + DWORD system_uses_light_theme_size = sizeof(DWORD);
> + /* Persists between calls, in windows it might look up some internal table. */
> + static DWORD system_uses_light_theme = TRUE;
> +
> + /* We don't necessarily care that this might fail because it doesn't affect
> + * system_uses_light_theme if it does.
> + */
While comments are nice to have, these are pretty obvious so I think you can ignore it.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/3959#note_47269
On FreeBSD, using `environ` in a shared library linked with `-Wl,-z,defs` causes an undefined reference error:
```
gcc -m64 -o dlls/msv1_0/msv1_0.so -shared -Wl,-Bsymbolic -Wl,-soname,msv1_0.so -Wl,-z,defs dlls/msv1_0/unixlib.o dlls/ntdll/ntdll.so
/usr/local/bin/ld: dlls/msv1_0/unixlib.o: in function `ntlm_fork':
/usr/home/pip/wine/build64/../dlls/msv1_0/unixlib.c:206: undefined reference to `environ'
collect2: error: ld returned 1 exit status
*** Error code 1
```
This is unfortunately a common issue on FreeBSD, see:
- https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263265
- https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=265008
- https://reviews.freebsd.org/D30842
Reported by Gerald Pfeifer.
--
v3: configure: Don't use -Wl,-z,defs if causes link errors with 'environ'.
https://gitlab.winehq.org/wine/wine/-/merge_requests/3984