Better Dark Mode & Image Viewer Toggle Fixes#1965
Open
EaZyStudiio wants to merge 2 commits into
Open
Conversation
Overview This pull request introduces improved dark mode support by adding a tri-state theme cycle option to the tray menu, and fixes an issue with the Image Viewer background toggle affecting the global application theme. Changes: 1. Global Theme Tray Cycle: Added AppTheme option in SettingHelper and SetTheme method in App.xaml.cs; added tray menu item in TrayIconManager.cs to cycle Auto/Light/Dark with dynamic label. 2. Image Viewer Background Toggle Fix: Decoupled Image Viewer canvas background from global Theme; added CanvasTheme property and saved LastCanvasTheme; updated bindings in ImagePanel.xaml. Files modified: QuickLook/App.xaml.cs; QuickLook/TrayIconManager.cs; QuickLook.Plugin.ImageViewer/ImagePanel.xaml.cs; QuickLook.Plugin.ImageViewer/ImagePanel.xaml How to test: Run QuickLook, use tray menu to cycle theme, open image and toggle background; canvas background should change without affecting global UI.
Reviewer's GuideAdds a tri-state (Auto/Light/Dark) app theme setting wired through startup and a new tray menu item, and decouples the Image Viewer canvas background from the global theme by introducing a separate CanvasTheme with its own persisted setting. Sequence diagram for tray theme tri_state cyclingsequenceDiagram
actor User
participant TrayIconManager
participant SettingHelper
participant App
participant ThemeManager
participant OSThemeHelper
User->>TrayIconManager: click _itemThemeCycle RelayCommand
TrayIconManager->>SettingHelper: Get(AppTheme, 0, QuickLook)
TrayIconManager-->>TrayIconManager: next = (current + 1) % 3
TrayIconManager->>SettingHelper: Set(AppTheme, next, QuickLook)
TrayIconManager->>App: SetTheme(next)
App->>ThemeManager: Apply(ApplicationTheme.Light) [theme == 1]
App->>ThemeManager: Apply(ApplicationTheme.Dark) [theme == 2]
App->>OSThemeHelper: AppsUseDarkTheme() [theme == 0]
OSThemeHelper-->>App: appsUseDark
App->>ThemeManager: Apply(ApplicationTheme.Dark) [appsUseDark]
App->>ThemeManager: Apply(ApplicationTheme.Light) [!appsUseDark]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider replacing the magic integer values for
AppTheme(0/1/2) with a strongly-typed enum or constants to make the theme switching logic self-documenting and less error-prone. - The tray theme cycle menu item currently uses hard-coded English headers (e.g.,
"Theme: Auto"); wiring these throughTranslationHelperlike the other menu items will keep the UI consistent and localizable. - The setting keys like
"AppTheme"and"LastCanvasTheme"are repeated as raw strings; centralizing them as shared constants would reduce the risk of typos and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the magic integer values for `AppTheme` (0/1/2) with a strongly-typed enum or constants to make the theme switching logic self-documenting and less error-prone.
- The tray theme cycle menu item currently uses hard-coded English headers (e.g., `"Theme: Auto"`); wiring these through `TranslationHelper` like the other menu items will keep the UI consistent and localizable.
- The setting keys like `"AppTheme"` and `"LastCanvasTheme"` are repeated as raw strings; centralizing them as shared constants would reduce the risk of typos and make future changes easier.
## Individual Comments
### Comment 1
<location path="QuickLook/App.xaml.cs" line_range="231-235" />
<code_context>
- // Set initial theme based on system settings
- ThemeManager.Apply(OSThemeHelper.AppsUseDarkTheme() ? ApplicationTheme.Dark : ApplicationTheme.Light);
+ // Set initial theme based on system or user settings
+ var appTheme = SettingHelper.Get("AppTheme", 0, "QuickLook");
+ SetTheme(appTheme);
</code_context>
<issue_to_address>
**suggestion:** Consider using a strongly-typed enum (or wrapper) instead of raw integers for `AppTheme`.
Passing `AppTheme` as a raw `int` makes it easy to supply invalid values and hides the intent of each option. Defining a strongly-typed enum (e.g. `AppThemeSetting { Auto = 0, Light = 1, Dark = 2 }`) or a small wrapper would clarify the meaning of each value and prevent misuse, especially now that this setting is shared between App and `TrayIconManager`.
Suggested implementation:
```csharp
// Set initial theme based on system or user settings
var appThemeSetting = (AppThemeSetting)SettingHelper.Get("AppTheme", (int)AppThemeSetting.Auto, "QuickLook");
SetTheme(appThemeSetting);
```
To fully implement the strongly-typed `AppThemeSetting` suggestion, you will also need to:
1. Define the enum (in a shared location accessible by both `App` and `TrayIconManager`, e.g. a common settings or utilities namespace):
```csharp
public enum AppThemeSetting
{
Auto = 0,
Light = 1,
Dark = 2
}
```
2. Update the `SetTheme` method signature in `App.xaml.cs` (and its implementation) from something like:
```csharp
private void SetTheme(int appTheme)
```
to:
```csharp
private void SetTheme(AppThemeSetting appThemeSetting)
```
and internally map `AppThemeSetting` to `ApplicationTheme` / OS theme:
```csharp
private void SetTheme(AppThemeSetting appThemeSetting)
{
ApplicationTheme theme;
switch (appThemeSetting)
{
case AppThemeSetting.Light:
theme = ApplicationTheme.Light;
break;
case AppThemeSetting.Dark:
theme = ApplicationTheme.Dark;
break;
case AppThemeSetting.Auto:
default:
theme = OSThemeHelper.AppsUseDarkTheme()
? ApplicationTheme.Dark
: ApplicationTheme.Light;
break;
}
ThemeManager.Apply(theme);
}
```
3. Ensure all other callers (e.g. in `TrayIconManager` or any settings UI) use `AppThemeSetting` instead of raw integers when setting or reading the theme preference. When persisting, continue storing the underlying `int` value:
```csharp
SettingHelper.Set("AppTheme", (int)appThemeSetting, "QuickLook");
```
4. If `SettingHelper.Get` supports generics, consider adding a typed overload:
```csharp
public static TEnum GetEnum<TEnum>(string key, TEnum defaultValue, string section)
where TEnum : struct, Enum
{
var intValue = Get(key, Convert.ToInt32(defaultValue), section);
return Enum.IsDefined(typeof(TEnum), intValue)
? (TEnum)Enum.ToObject(typeof(TEnum), intValue)
: defaultValue;
}
```
and then replace the cast with:
```csharp
var appThemeSetting = SettingHelper.GetEnum("AppTheme", AppThemeSetting.Auto, "QuickLook");
```
</issue_to_address>
### Comment 2
<location path="QuickLook/TrayIconManager.cs" line_range="74-82" />
<code_context>
Header = TranslationHelper.Get("Icon_OpenDataFolder"),
Command = new RelayCommand(() => Process.Start("explorer.exe", SettingHelper.LocalDataPath)),
},
+ _itemThemeCycle = new TrayMenuItem()
+ {
+ Header = "Theme: Auto",
+ Command = new RelayCommand(() =>
+ {
+ var current = SettingHelper.Get("AppTheme", 0, "QuickLook");
+ var next = (current + 1) % 3;
+ SettingHelper.Set("AppTheme", next, "QuickLook");
+ App.SetTheme(next);
+ }),
+ },
</code_context>
<issue_to_address>
**suggestion:** Theme menu labels are hardcoded and not localized; consider centralizing label generation with translations.
The tray menu currently hardcodes English labels ("Theme: Auto/Light/Dark") in both the constructor and `TrayOnOpening`, bypassing `TranslationHelper` and duplicating label logic. Please centralize header generation in a helper (e.g., `UpdateThemeMenuHeader(int theme)`) that pulls from translations/resources, and call it from both sites to keep labels consistent and localized.
Suggested implementation:
```csharp
private readonly TrayMenuItem _itemAutorun = null!;
private readonly TrayMenuItem _itemCloseOnLostFocus = null!;
private readonly TrayMenuItem _itemThemeCycle = null!;
private void UpdateThemeMenuHeader(int theme)
{
string key;
switch (theme)
{
case 1:
key = "Tray_Theme_Light";
break;
case 2:
key = "Tray_Theme_Dark";
break;
default:
key = "Tray_Theme_Auto";
break;
}
_itemThemeCycle.Header = TranslationHelper.Get(key);
}
private TrayIconManager()
```
```csharp
var currentTheme = SettingHelper.Get("AppTheme", 0, "QuickLook");
_itemThemeCycle = new TrayMenuItem()
{
Header = string.Empty,
Command = new RelayCommand(() =>
{
var current = SettingHelper.Get("AppTheme", 0, "QuickLook");
var next = (current + 1) % 3;
SettingHelper.Set("AppTheme", next, "QuickLook");
App.SetTheme(next);
UpdateThemeMenuHeader(next);
}),
};
UpdateThemeMenuHeader(currentTheme);
```
1. Ensure you add the corresponding localized resources for the keys used in `UpdateThemeMenuHeader`:
- `Tray_Theme_Auto` (e.g., "Theme: Auto")
- `Tray_Theme_Light` (e.g., "Theme: Light")
- `Tray_Theme_Dark` (e.g., "Theme: Dark")
2. In `TrayOnOpening` (or any other place where the theme tray item header is updated), replace any hard-coded `"Theme: Auto"`, `"Theme: Light"`, or `"Theme: Dark"` strings with calls to `UpdateThemeMenuHeader`, e.g.:
```csharp
var currentTheme = SettingHelper.Get("AppTheme", 0, "QuickLook");
UpdateThemeMenuHeader(currentTheme);
```
This will centralize header generation and ensure both the constructor and `TrayOnOpening` use the same localized logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Added strongly-typed AppThemeMode enum (Auto, Light, Dark) to replace magic integers - Created centralized setting keys (KeyAppTheme, KeyLastCanvasTheme) in SettingHelper for type-safe access - Wired tray menu text through TranslationHelper for full localization support with English fallbacks - Updated all theme and canvas theme references to use enum and constants throughout App.xaml.cs, TrayIconManager.cs, and ImagePanel.xaml.cs - Improved code clarity and maintainability by eliminating raw string literals and magic numbers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This pull request introduces improved dark mode support by adding a tri-state theme cycle option to the tray menu, and fixes an issue with the Image Viewer background toggle affecting the global application theme.
Changes:
Files modified: QuickLook/App.xaml.cs; QuickLook/TrayIconManager.cs; QuickLook.Plugin.ImageViewer/ImagePanel.xaml.cs; QuickLook.Plugin.ImageViewer/ImagePanel.xaml
How to test: Run QuickLook, use tray menu to cycle theme, open image and toggle background; canvas background should change without affecting global UI.
PR Checklist
explorer_Qui64mXdgo.mp4
To put it simply no one should have to go through this (the constant white flashes) at 3am in the morning 😭
Related Issue (if any)
Please provide related issue numbers:
Additional Notes
Add any extra notes here:
Summary by Sourcery
Introduce a tray theme cycle and decouple the Image Viewer canvas background from the global application theme.
New Features:
Bug Fixes:
Enhancements: