From 727de3e1fcd46dabc5c21940f6379815d65b1c8b Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Thu, 20 Feb 2025 10:47:30 +0000 Subject: [PATCH] [Run] Fix dark mode detection code, plus refactor (#37324) * Fix risky int cast in dark mode detection. * Refactored Helper and Manager classes. New unit tests and changes to support Registry access mocking. * Spelling update. * Improve documentation for the registry-related classes. * Fix issue with UpdateTheme raised in review. Enhance documentation. Rewrite tests to use parameterised unit tests, and expand to cover more cases. --- .../PowerLauncher/Helper/ThemeExtensions.cs | 72 --------- .../PowerLauncher/Helper/ThemeHelper.cs | 137 ++++++++++++++++++ .../PowerLauncher/Helper/ThemeManager.cs | 64 ++++---- .../Services/IRegistryService.cs | 82 +++++++++++ .../PowerLauncher/Services/RegistryService.cs | 34 +++++ .../Services/RegistryServiceFactory.cs | 17 +++ .../launcher/Wox.Test/ThemeHelperTest.cs | 124 ++++++++++++++++ 7 files changed, 426 insertions(+), 104 deletions(-) delete mode 100644 src/modules/launcher/PowerLauncher/Helper/ThemeExtensions.cs create mode 100644 src/modules/launcher/PowerLauncher/Helper/ThemeHelper.cs create mode 100644 src/modules/launcher/PowerLauncher/Services/IRegistryService.cs create mode 100644 src/modules/launcher/PowerLauncher/Services/RegistryService.cs create mode 100644 src/modules/launcher/PowerLauncher/Services/RegistryServiceFactory.cs create mode 100644 src/modules/launcher/Wox.Test/ThemeHelperTest.cs diff --git a/src/modules/launcher/PowerLauncher/Helper/ThemeExtensions.cs b/src/modules/launcher/PowerLauncher/Helper/ThemeExtensions.cs deleted file mode 100644 index 8332f9f0dc..0000000000 --- a/src/modules/launcher/PowerLauncher/Helper/ThemeExtensions.cs +++ /dev/null @@ -1,72 +0,0 @@ -// Copyright (c) Microsoft Corporation -// The Microsoft Corporation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.Globalization; -using System.Linq; - -using ManagedCommon; -using Microsoft.Win32; - -namespace PowerLauncher.Helper -{ - public static class ThemeExtensions - { - public static Theme GetCurrentTheme() - { - // Check for high-contrast mode - Theme highContrastTheme = GetHighContrastBaseType(); - if (highContrastTheme != Theme.Light) - { - return highContrastTheme; - } - - // Check if the system is using dark or light mode - return IsSystemDarkMode() ? Theme.Dark : Theme.Light; - } - - private static bool IsSystemDarkMode() - { - const string registryKey = @"HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes\Personalize"; - const string registryValue = "AppsUseLightTheme"; - - // Retrieve the registry value, which is a DWORD (0 or 1) - object registryValueObj = Registry.GetValue(registryKey, registryValue, null); - if (registryValueObj != null) - { - // 0 = Dark mode, 1 = Light mode - bool isLightMode = Convert.ToBoolean((int)registryValueObj, CultureInfo.InvariantCulture); - return !isLightMode; // Invert because 0 = Dark - } - else - { - // Default to Light theme if the registry key is missing - return false; // Default to dark mode assumption - } - } - - public static Theme GetHighContrastBaseType() - { - const string registryKey = @"HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes"; - const string registryValue = "CurrentTheme"; - - string themePath = (string)Registry.GetValue(registryKey, registryValue, string.Empty); - if (string.IsNullOrEmpty(themePath)) - { - return Theme.Light; // Default to light theme if missing - } - - string theme = themePath.Split('\\').Last().Split('.').First().ToLowerInvariant(); - - return theme switch - { - "hc1" => Theme.HighContrastOne, - "hc2" => Theme.HighContrastTwo, - "hcwhite" => Theme.HighContrastWhite, - "hcblack" => Theme.HighContrastBlack, - _ => Theme.Light, - }; - } - } -} diff --git a/src/modules/launcher/PowerLauncher/Helper/ThemeHelper.cs b/src/modules/launcher/PowerLauncher/Helper/ThemeHelper.cs new file mode 100644 index 0000000000..12a3863f96 --- /dev/null +++ b/src/modules/launcher/PowerLauncher/Helper/ThemeHelper.cs @@ -0,0 +1,137 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.IO; +using System.Runtime.CompilerServices; +using ManagedCommon; +using PowerLauncher.Services; + +[assembly: InternalsVisibleTo("Wox.Test")] + +namespace PowerLauncher.Helper; + +/// +/// Provides functionality for determining the application's theme based on system settings, user +/// preferences, and High Contrast mode detection. +/// +public class ThemeHelper +{ + private const string ThemesKey = @"HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes"; + private const string PersonalizeKey = ThemesKey + "\\Personalize"; + + internal const int AppsUseLightThemeLight = 1; + internal const int AppsUseLightThemeDark = 0; + + /// + /// Default value for the "AppsUseLightTheme" registry setting. This value represents Light + /// mode and will be used if the registry value is invalid or cannot be read. + /// + internal const int AppsUseLightThemeDefault = AppsUseLightThemeLight; + + private readonly IRegistryService _registryService; + + private readonly Dictionary _highContrastThemeMap = + new(StringComparer.InvariantCultureIgnoreCase) + { + { "hc1", Theme.HighContrastOne }, + { "hc2", Theme.HighContrastTwo }, + { "hcwhite", Theme.HighContrastWhite }, + { "hcblack", Theme.HighContrastBlack }, + }; + + /// + /// Initializes a new instance of the class. + /// + /// The service used to query registry values. If null, a + /// default implementation is used, which queries the Windows registry. This allows for + /// dependency injection and unit testing. + public ThemeHelper(IRegistryService registryService = null) + { + _registryService = registryService ?? RegistryServiceFactory.Create(); + } + + /// + /// Determines the theme to apply, prioritizing an active High Contrast theme. + /// + /// The theme selected in application settings. + /// The resolved based on the following priority order: + /// 1. If a default High Contrast Windows theme is active, return the corresponding High + /// Contrast . + /// 2. If "Windows default" is selected in application settings, return the Windows app theme + /// ( or ). + /// 3. If the user explicitly selected "Light" or "Dark", return their chosen theme. + /// 4. If the theme cannot be determined, return . + /// + public Theme DetermineTheme(Theme settingsTheme) => + GetHighContrastTheme() ?? + (settingsTheme == Theme.System ? GetAppsTheme() : ValidateTheme(settingsTheme)); + + /// + /// Ensures the provided value is valid. + /// + /// The value to validate. + /// The provided theme if it is a defined enum value; otherwise, defaults to + /// . + private Theme ValidateTheme(Theme theme) => Enum.IsDefined(theme) ? theme : Theme.Light; + + /// + /// Determines if a High Contrast theme is currently active and returns the corresponding + /// . + /// + /// The detected High Contrast (e.g. + /// , or null if no recognized High Contrast theme + /// is active. + /// + internal Theme? GetHighContrastTheme() + { + try + { + var themePath = Convert.ToString( + _registryService.GetValue(ThemesKey, "CurrentTheme", string.Empty), + CultureInfo.InvariantCulture); + + if (!string.IsNullOrEmpty(themePath) && _highContrastThemeMap.TryGetValue( + Path.GetFileNameWithoutExtension(themePath), out var theme)) + { + return theme; + } + } + catch + { + // Fall through to return null. Ignore exception. + } + + return null; + } + + /// + /// Retrieves the Windows app theme preference from the registry. + /// + /// if the user has selected Dark mode for apps, + /// otherwise. If the registry value cannot be read or is invalid, + /// the default value () is used. + /// + internal Theme GetAppsTheme() + { + try + { + // "AppsUseLightTheme" registry value: + // - 0 = Dark mode + // - 1 (or missing/invalid) = Light mode + var regValue = _registryService.GetValue( + PersonalizeKey, + "AppsUseLightTheme", + AppsUseLightThemeDefault); + + return regValue is int intValue && intValue == 0 ? Theme.Dark : Theme.Light; + } + catch + { + return Theme.Light; + } + } +} diff --git a/src/modules/launcher/PowerLauncher/Helper/ThemeManager.cs b/src/modules/launcher/PowerLauncher/Helper/ThemeManager.cs index 66a71b5b0a..2a27494b30 100644 --- a/src/modules/launcher/PowerLauncher/Helper/ThemeManager.cs +++ b/src/modules/launcher/PowerLauncher/Helper/ThemeManager.cs @@ -3,7 +3,7 @@ // See the LICENSE file in the project root for more information. using System; -using System.IO; +using System.Collections.Generic; using System.Windows; using System.Windows.Media; using ManagedCommon; @@ -17,10 +17,11 @@ namespace PowerLauncher.Helper { private readonly PowerToysRunSettings _settings; private readonly MainWindow _mainWindow; - private ManagedCommon.Theme _currentTheme; + private readonly ThemeHelper _themeHelper = new(); + private bool _disposed; - public ManagedCommon.Theme CurrentTheme => _currentTheme; + public Theme CurrentTheme { get; private set; } public event Common.UI.ThemeChangedHandler ThemeChanged; @@ -40,23 +41,25 @@ namespace PowerLauncher.Helper } } - private void SetSystemTheme(ManagedCommon.Theme theme) + private void SetSystemTheme(Theme theme) { - _mainWindow.Background = OSVersionHelper.IsWindows11() is false ? SystemColors.WindowBrush : null; + _mainWindow.Background = !OSVersionHelper.IsWindows11() ? SystemColors.WindowBrush : null; // Need to disable WPF0001 since setting Application.Current.ThemeMode is experimental // https://learn.microsoft.com/en-us/dotnet/desktop/wpf/whats-new/net90#set-in-code #pragma warning disable WPF0001 - Application.Current.ThemeMode = theme is ManagedCommon.Theme.Light ? ThemeMode.Light : ThemeMode.Dark; - if (theme is ManagedCommon.Theme.Dark or ManagedCommon.Theme.Light) + Application.Current.ThemeMode = theme == Theme.Light ? ThemeMode.Light : ThemeMode.Dark; +#pragma warning restore WPF0001 + + if (theme is Theme.Dark or Theme.Light) { if (!OSVersionHelper.IsWindows11()) { // Apply background only on Windows 10 - // Windows theme does not work properly for dark and light mode so right now set the background color manual. + // Windows theme does not work properly for dark and light mode so right now set the background color manually. _mainWindow.Background = new SolidColorBrush { - Color = theme is ManagedCommon.Theme.Dark ? (Color)ColorConverter.ConvertFromString("#202020") : (Color)ColorConverter.ConvertFromString("#fafafa"), + Color = (Color)ColorConverter.ConvertFromString(theme == Theme.Dark ? "#202020" : "#fafafa"), }; } } @@ -64,49 +67,46 @@ namespace PowerLauncher.Helper { string styleThemeString = theme switch { - ManagedCommon.Theme.Light => "Themes/Light.xaml", - ManagedCommon.Theme.Dark => "Themes/Dark.xaml", - ManagedCommon.Theme.HighContrastOne => "Themes/HighContrast1.xaml", - ManagedCommon.Theme.HighContrastTwo => "Themes/HighContrast2.xaml", - ManagedCommon.Theme.HighContrastWhite => "Themes/HighContrastWhite.xaml", - _ => "Themes/HighContrastBlack.xaml", + Theme.HighContrastOne => "Themes/HighContrast1.xaml", + Theme.HighContrastTwo => "Themes/HighContrast2.xaml", + Theme.HighContrastWhite => "Themes/HighContrastWhite.xaml", + Theme.HighContrastBlack => "Themes/HighContrastBlack.xaml", + _ => "Themes/Light.xaml", }; + _mainWindow.Resources.MergedDictionaries.Clear(); _mainWindow.Resources.MergedDictionaries.Add(new ResourceDictionary { Source = new Uri(styleThemeString, UriKind.Relative), }); - ResourceDictionary test = new ResourceDictionary - { - Source = new Uri(styleThemeString, UriKind.Relative), - }; + if (OSVersionHelper.IsWindows11()) { // Apply background only on Windows 11 to keep the same style as WPFUI _mainWindow.Background = new SolidColorBrush { - Color = (Color)_mainWindow.FindResource("LauncherBackgroundColor"), // Use your DynamicResource key here + Color = (Color)_mainWindow.FindResource("LauncherBackgroundColor"), }; } } ImageLoader.UpdateIconPath(theme); - ThemeChanged?.Invoke(_currentTheme, theme); - _currentTheme = theme; + ThemeChanged?.Invoke(CurrentTheme, theme); + CurrentTheme = theme; } + /// + /// Updates the application's theme based on system settings and user preferences. + /// + /// + /// This considers: + /// - Whether a High Contrast theme is active in Windows. + /// - The system-wide app mode preference (Light or Dark). + /// - The user's preference override for Light or Dark mode in the application settings. + /// public void UpdateTheme() { - ManagedCommon.Theme newTheme = _settings.Theme; - ManagedCommon.Theme theme = ThemeExtensions.GetHighContrastBaseType(); - if (theme != ManagedCommon.Theme.Light) - { - newTheme = theme; - } - else if (_settings.Theme == ManagedCommon.Theme.System) - { - newTheme = ThemeExtensions.GetCurrentTheme(); - } + Theme newTheme = _themeHelper.DetermineTheme(_settings.Theme); _mainWindow.Dispatcher.Invoke(() => { diff --git a/src/modules/launcher/PowerLauncher/Services/IRegistryService.cs b/src/modules/launcher/PowerLauncher/Services/IRegistryService.cs new file mode 100644 index 0000000000..2585c7ae16 --- /dev/null +++ b/src/modules/launcher/PowerLauncher/Services/IRegistryService.cs @@ -0,0 +1,82 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using Microsoft.Win32; + +namespace PowerLauncher.Services; + +#nullable enable + +/// +/// Provides methods for interacting with the Windows Registry or an equivalent key-value data +/// store. +/// +public interface IRegistryService +{ + /// + /// Retrieves the value associated with the specified name, in the specified registry key. + /// If the name is not found in the specified key, returns the specified default value, or + /// null if the specified key does not exist. + /// + /// The full registry path of the key, beginning with a valid registry + /// root, such as "HKEY_CURRENT_USER". + /// The name of the name/value pair. + /// The value to return if does not exist. + /// + /// null if the subkey specified by does not exist; + /// otherwise, the value associated with , or + /// if is not found. + /// does not begin with a valid + /// registry root. + /// Thrown if access to the registry or + /// equivalent store is denied. + /// Implementations may throw additional exceptions depending on their internal + /// storage mechanism. + object? GetValue(string keyName, string? valueName, object? defaultValue); + + /// + /// Sets the specified name/value pair on the specified registry key. If the specified key does + /// not exist, it is created. + /// + /// The full registry path of the key, beginning with a valid registry + /// root, such as "HKEY_CURRENT_USER". + /// The name of the name/value pair. + /// The value to be stored. + /// + /// does not begin with a valid registry root. + /// + /// -or- + /// + /// is longer than the maximum length allowed (255 characters). + /// + /// Access to the key is denied; for example, + /// it is a root-level node, or the key has not been opened with write access. + void SetValue(string keyName, string? valueName, object value); + + /// + /// Sets the specified name/value pair on the specified registry key. If the specified key does + /// not exist, it is created. + /// + /// The full registry path of the key, beginning with a valid registry + /// root, such as "HKEY_CURRENT_USER". + /// The name of the name/value pair. + /// The value to be stored. + /// The registry data type to use when storing the data. + /// + /// does not begin with a valid registry root. + /// + /// -or- + /// + /// is longer than the maximum length allowed (255 characters). + /// + /// -or- + /// + /// The type of did not match the registry data type specified by + /// , therefore the data could not be converted properly. + /// + /// Access to the key is denied; for example, + /// it is a root-level node, or the key has not been opened with write access. + void SetValue(string keyName, string? valueName, object value, RegistryValueKind valueKind); +} diff --git a/src/modules/launcher/PowerLauncher/Services/RegistryService.cs b/src/modules/launcher/PowerLauncher/Services/RegistryService.cs new file mode 100644 index 0000000000..8937bd6934 --- /dev/null +++ b/src/modules/launcher/PowerLauncher/Services/RegistryService.cs @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.IO; +using System.Security; +using Microsoft.Win32; + +namespace PowerLauncher.Services; + +#nullable enable + +public class RegistryService : IRegistryService +{ + /// + /// The user does not have the permissions required to read + /// from the registry key. + /// The that contains the specified + /// value has been marked for deletion. + public object? GetValue(string keyName, string? valueName, object? defaultValue) => + Registry.GetValue(keyName, valueName, defaultValue); + + /// + /// The user does not have the permissions required to + /// create or modify registry keys." + public void SetValue(string keyName, string? valueName, object value) => + Registry.SetValue(keyName, valueName, value); + + /// + /// The user does not have the permissions required to + /// create or modify registry keys. + public void SetValue(string keyName, string? valueName, object value, RegistryValueKind valueKind) => + Registry.SetValue(keyName, valueName, value, valueKind); +} diff --git a/src/modules/launcher/PowerLauncher/Services/RegistryServiceFactory.cs b/src/modules/launcher/PowerLauncher/Services/RegistryServiceFactory.cs new file mode 100644 index 0000000000..c4be75d6b3 --- /dev/null +++ b/src/modules/launcher/PowerLauncher/Services/RegistryServiceFactory.cs @@ -0,0 +1,17 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace PowerLauncher.Services; + +/// +/// Factory for creating instances of . +/// +public static class RegistryServiceFactory +{ + /// + /// Creates the default implementation of . + /// + /// An instance of the default implementation. + public static IRegistryService Create() => new RegistryService(); +} diff --git a/src/modules/launcher/Wox.Test/ThemeHelperTest.cs b/src/modules/launcher/Wox.Test/ThemeHelperTest.cs new file mode 100644 index 0000000000..38ff891a6e --- /dev/null +++ b/src/modules/launcher/Wox.Test/ThemeHelperTest.cs @@ -0,0 +1,124 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Linq.Expressions; +using ManagedCommon; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using PowerLauncher.Helper; +using PowerLauncher.Services; + +namespace Wox.Test; + +[TestClass] +public class ThemeHelperTest +{ + // Registry key paths. + private const string ThemesKey = @"HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes"; + private const string PersonalizeKey = ThemesKey + "\\Personalize"; + + // Theme paths. + private const string HighContrastThemePath = @"C:\WINDOWS\resources\Ease of Access Themes\hcwhite.theme"; + private const string NonHighContrastThemePath = @"C:\Users\Test\AppData\Local\Microsoft\Windows\Themes\Custom.theme"; + + /// + /// The expected High Contrast theme when the is returned + /// from the registry. + /// + private const Theme HighContrastTheme = Theme.HighContrastWhite; + + /// + /// Mock , to return the value of the AppsUseLightTheme + /// key. + /// + private static readonly Expression> _mockAppsUseLightTheme = (service) => + service.GetValue(PersonalizeKey, "AppsUseLightTheme", ThemeHelper.AppsUseLightThemeDefault); + + /// + /// Mock to return the value of the CurrentTheme key. + /// + /// + /// The default value given here - string.Empty - must be the same as the default value in the + /// actual code for tests using this mock to be valid. + /// + private static readonly Expression> _mockCurrentTheme = (service) => + service.GetValue(ThemesKey, "CurrentTheme", string.Empty); + + /// + /// Test GetAppsTheme method. + /// + /// The mocked value for the AppsUseLightTheme registry key. + /// The expected output from the call to + /// . + [DataTestMethod] + [DataRow(ThemeHelper.AppsUseLightThemeLight, Theme.Light)] + [DataRow(ThemeHelper.AppsUseLightThemeDark, Theme.Dark)] + [DataRow(int.MaxValue, Theme.Light)] // Out of range values should default to Light + [DataRow(null, Theme.Light)] // Missing keys or values should default to Light + [DataRow("RandomString", Theme.Light)] // Invalid string values should default to Light + public void GetAppsTheme_ReturnsExpectedTheme(object registryValue, Theme expectedTheme) + { + var mockService = new Mock(); + mockService.Setup(_mockAppsUseLightTheme).Returns(registryValue); + + var helper = new ThemeHelper(mockService.Object); + + Assert.AreEqual(expectedTheme, helper.GetAppsTheme()); + } + + /// + /// Test . + /// + /// The mocked value for the CurrentTheme registry key. + /// The expected output from the call to + /// . + [DataTestMethod] + [DataRow(HighContrastThemePath, HighContrastTheme)] // Valid High Contrast theme + [DataRow(NonHighContrastThemePath, null)] // Non-High Contrast theme should return null + [DataRow(null, null)] // Missing keys or values should default to null + [DataRow("", null)] // Empty string values should default to null + public void GetHighContrastTheme_ReturnsExpectedTheme(string registryValue, Theme? expectedTheme) + { + var mockService = new Mock(); + mockService.Setup(_mockCurrentTheme).Returns(registryValue); + + var helper = new ThemeHelper(mockService.Object); + + Assert.AreEqual(expectedTheme, helper.GetHighContrastTheme()); + } + + /// + /// Test . + /// + /// The mocked value for the CurrentTheme registry key. + /// The value from the application's settings. + /// + /// The expected output from the call to + /// . + /// The mocked value for the AppsUseLightTheme registry key, + /// representing the system preference for Light or Dark mode. + [DataTestMethod] + [DataRow(HighContrastThemePath, Theme.System, HighContrastTheme)] // High Contrast theme active + [DataRow(HighContrastThemePath, Theme.Light, HighContrastTheme)] // High Contrast theme active - Light mode override ignored + [DataRow(HighContrastThemePath, Theme.Dark, HighContrastTheme)] // High Contrast theme active - Dark mode override ignored + [DataRow(NonHighContrastThemePath, Theme.System, Theme.Light)] // System preference with default light theme + [DataRow(NonHighContrastThemePath, Theme.System, Theme.Dark, ThemeHelper.AppsUseLightThemeDark)] // System preference with dark mode + [DataRow(NonHighContrastThemePath, Theme.Light, Theme.Light, ThemeHelper.AppsUseLightThemeDark)] // Light mode override + [DataRow(NonHighContrastThemePath, Theme.Dark, Theme.Dark, ThemeHelper.AppsUseLightThemeLight)] // Dark mode override + [DataRow(null, Theme.System, Theme.Light)] // Missing keys or values should default to Light + [DataRow("", Theme.System, Theme.Light)] // Empty current theme paths should default to Light + [DataRow("RandomString", Theme.System, Theme.Light)] // Invalid current theme paths should default to Light + [DataRow(NonHighContrastThemePath, (Theme)int.MaxValue, Theme.Light)] // Invalid theme values should default to Light + public void DetermineTheme_ReturnsExpectedTheme(string registryTheme, Theme requestedTheme, Theme expectedTheme, int? appsUseLightTheme = 1) + { + var mockService = new Mock(); + mockService.Setup(_mockCurrentTheme).Returns(registryTheme); + mockService.Setup(_mockAppsUseLightTheme).Returns(appsUseLightTheme); + + var helper = new ThemeHelper(mockService.Object); + + Assert.AreEqual(expectedTheme, helper.DetermineTheme(requestedTheme)); + } +}