feat: support preference for tray icon color schema #287
Conversation
Reviewer's GuideThis PR adds a new user preference for tray icon color mode—auto, light, or dark—by defining a settings key, updating TrayManager to load and honor that preference (with a GNOME special case), and introducing two new settings UI components to allow users to configure and immediately apply their choice. Sequence diagram for updating tray icon after user changes color modesequenceDiagram
actor User
participant SettingsUI
participant SettingsManager
participant TrayManager
participant SystemTray
User->>SettingsUI: Selects tray icon color mode
SettingsUI->>SettingsManager: Save new color mode
SettingsUI->>TrayManager: updateTrayIcon()
TrayManager->>SettingsManager: get tray icon color mode
TrayManager->>SystemTray: setImage(iconPath)
Entity relationship diagram for tray icon color mode preference keyerDiagram
SETTINGS_MANAGER {
STRING kTrayIconColorModeKey
}
USER ||--o{ SETTINGS_MANAGER : sets
SETTINGS_MANAGER ||--o{ TRAY_MANAGER : reads
Class diagram for new and updated tray icon color mode settings UI componentsclassDiagram
class TrayIconColorModeSettings {
+String? trayIconColorMode
+SettingsManager _settingsManager
+_loadSettings()
+build()
}
class TrayIconColorModeSettingsState {
+String? trayIconColorMode
+SettingsManager _settingsManager
+_loadSettings()
+build()
}
TrayIconColorModeSettings --|> TrayIconColorModeSettingsState
class TrayIconColorModeSetting {
+String trayIconColorMode
+_loadTrayIconColorMode()
+_updateTrayIconColorMode(String)
+build()
}
class TrayIconColorModeSettingState {
+String trayIconColorMode
+_loadTrayIconColorMode()
+_updateTrayIconColorMode(String)
+build()
}
TrayIconColorModeSetting --|> TrayIconColorModeSettingState
Class diagram for updated TrayManager logicclassDiagram
class TrayManager {
+Future<TrayIcon> getTrayIcon()
+Future<void> updateTrayIcon()
}
class TrayIcon {
+String path
+bool isInstalled
}
TrayManager --> TrayIcon
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 there - I've reviewed your changes - here's some feedback:
- Consider using an enum or static constants for the tray icon modes ("auto", "light", "dark") instead of raw strings to avoid typos and centralize mode definitions.
- Manual color selection is only applied to the Windows .ico assets—macOS still always returns the same SVG; if you want macOS users to override the icon color, add or switch to light/dark SVG variants in getTrayIcon.
- getTrayIcon currently only handles macOS and Windows; add a fallback return or explicit handling for other platforms (e.g. Linux) to ensure it never returns null.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using an enum or static constants for the tray icon modes ("auto", "light", "dark") instead of raw strings to avoid typos and centralize mode definitions.
- Manual color selection is only applied to the Windows .ico assets—macOS still always returns the same SVG; if you want macOS users to override the icon color, add or switch to light/dark SVG variants in getTrayIcon.
- getTrayIcon currently only handles macOS and Windows; add a fallback return or explicit handling for other platforms (e.g. Linux) to ensure it never returns null.
## Individual Comments
### Comment 1
<location> `lib/utils/tray_manager.dart:38-40` </location>
<code_context>
- ? Brightness.dark.name
- : Brightness.light.name;
+ // Get user preference for tray icon color mode
+ final trayIconColorMode =
+ await $settingsManager.getValue<String>(kTrayIconColorModeKey) ??
+ "auto";
+
+ String brightness;
</code_context>
<issue_to_address>
**suggestion:** Defaulting to "auto" is reasonable, but consider validating the value.
Validate trayIconColorMode against the allowed values before using it to prevent issues from invalid or corrupted settings.
</issue_to_address>
### Comment 2
<location> `lib/screens/settings_theme/widgets/tray_icon_color_mode_setting.dart:26-32` </location>
<code_context>
+ Future<void> _loadTrayIconColorMode() async {
+ final storedMode =
+ await $settingsManager.getValue<String>(kTrayIconColorModeKey);
+ setState(() {
+ trayIconColorMode = storedMode ?? "auto";
+ });
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling asynchronous setState more robustly.
Check if the widget is mounted before calling setState in _loadTrayIconColorMode to prevent errors if the widget is disposed during the async operation.
```suggestion
Future<void> _loadTrayIconColorMode() async {
final storedMode =
await $settingsManager.getValue<String>(kTrayIconColorModeKey);
if (mounted) {
setState(() {
trayIconColorMode = storedMode ?? "auto";
});
}
}
```
</issue_to_address>
### Comment 3
<location> `lib/screens/settings_theme/widgets/tray_icon_color_mode_setting.dart:40-41` </location>
<code_context>
+ await $settingsManager.setValue(kTrayIconColorModeKey, newMode);
+
+ // Update the tray icon immediately
+ $tray.updateTrayIcon();
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Consider awaiting updateTrayIcon for consistency.
Awaiting updateTrayIcon will ensure the tray icon is updated before proceeding and allow proper error handling.
```suggestion
// Update the tray icon immediately
await $tray.updateTrayIcon();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| final trayIconColorMode = | ||
| await $settingsManager.getValue<String>(kTrayIconColorModeKey) ?? | ||
| "auto"; |
There was a problem hiding this comment.
suggestion: Defaulting to "auto" is reasonable, but consider validating the value.
Validate trayIconColorMode against the allowed values before using it to prevent issues from invalid or corrupted settings.
| Future<void> _loadTrayIconColorMode() async { | ||
| final storedMode = | ||
| await $settingsManager.getValue<String>(kTrayIconColorModeKey); | ||
| setState(() { | ||
| trayIconColorMode = storedMode ?? "auto"; | ||
| }); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling asynchronous setState more robustly.
Check if the widget is mounted before calling setState in _loadTrayIconColorMode to prevent errors if the widget is disposed during the async operation.
| Future<void> _loadTrayIconColorMode() async { | |
| final storedMode = | |
| await $settingsManager.getValue<String>(kTrayIconColorModeKey); | |
| setState(() { | |
| trayIconColorMode = storedMode ?? "auto"; | |
| }); | |
| } | |
| Future<void> _loadTrayIconColorMode() async { | |
| final storedMode = | |
| await $settingsManager.getValue<String>(kTrayIconColorModeKey); | |
| if (mounted) { | |
| setState(() { | |
| trayIconColorMode = storedMode ?? "auto"; | |
| }); | |
| } | |
| } |
| // Update the tray icon immediately | ||
| $tray.updateTrayIcon(); |
There was a problem hiding this comment.
suggestion: Consider awaiting updateTrayIcon for consistency.
Awaiting updateTrayIcon will ensure the tray icon is updated before proceeding and allow proper error handling.
| // Update the tray icon immediately | |
| $tray.updateTrayIcon(); | |
| // Update the tray icon immediately | |
| await $tray.updateTrayIcon(); |
dosubot
bot
added
C - l10n
|
Sorry, I didn't quite understand the context of this PR. When would there be a situation where the system color information is incorrect? |
|
Thanks for asking and reading my PR! The current logic is if system theme is light, use dark tray icon. And, use light tray icon if system theme is dark. However there is a situation that system theme is light, but user still needs a light tray icon. For example, GNOME always has a dark topbar, no matter user theme is light or dark. If one user uses light system theme, then Rune will render a dark tray icon, displaying on the dark topbar, which will be hard to see. So, I think having a option will be good in these circumstances. |
Thank you for the reply, I see the whole picture now. My main concern is that I don't want the system to be cluttered with these trivial settings, turning it into a monster like Foobar that has no simplicity whatsoever. But the situation you mentioned is indeed a problem. I think perhaps we could add it to a hidden menu (for power users only) instead of in the settings interface? We already have a hidden setting menu in the system |
Thank you again, and I've understand your concern. It's true that simplicity is important. I can move the setting into hidden setting menu later. However, I think as a normal user, it is maybe hard to realize that there is a hidden setting menu? Btw, I have another thought now. Maybe we can add a detection that when GNOME environment is detected, by default to offer a light tray icon. (AS GNOME always has a dark topbar.) This keeps simplicity and provides out-of-the-box fresh ready-to-go experience for GNOME users. What do you think about this idea? |
That's a good idea! |
|
Ok, I will move the color preference into hidden setting menu, and attempt to implement the GNOME detection feature! |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider replacing the raw string values ("auto", "light", "dark") with a typed enum to avoid magic strings and make the code more maintainable.
- The tray icon color mode settings UI is duplicated in two places—extract a shared widget or component to reduce code duplication.
- You might move the system‐theme and GNOME detection logic into its own helper method or service to simplify TrayManager.getTrayIcon and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the raw string values ("auto", "light", "dark") with a typed enum to avoid magic strings and make the code more maintainable.
- The tray icon color mode settings UI is duplicated in two places—extract a shared widget or component to reduce code duplication.
- You might move the system‐theme and GNOME detection logic into its own helper method or service to simplify TrayManager.getTrayIcon and improve readability.
## Individual Comments
### Comment 1
<location> `lib/utils/tray_manager.dart:38-40` </location>
<code_context>
- ? Brightness.dark.name
- : Brightness.light.name;
+ // Get user preference for tray icon color mode
+ final trayIconColorMode =
+ await $settingsManager.getValue<String>(kTrayIconColorModeKey) ??
+ "auto";
+
+ String brightness;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider validating trayIconColorMode value before using it.
Without validation, an invalid trayIconColorMode value could cause the tray icon to display incorrectly. Implementing a check or default fallback will help prevent user-facing issues.
</issue_to_address>
### Comment 2
<location> `lib/utils/tray_manager.dart:61-62` </location>
<code_context>
+ brightness = trayIconColorMode;
+ }
if (Platform.isWindows) {
return TrayIcon('assets/tray_icon_$brightness.ico', false);
</code_context>
<issue_to_address>
**suggestion:** Consider normalizing brightness value for Windows tray icon filename.
Unexpected brightness values could result in missing icon files and tray icon load failures. Validating brightness before using it in the filename would improve robustness.
```suggestion
if (Platform.isWindows) {
// Normalize brightness value to avoid missing icon files
final allowedBrightness = ['light', 'dark'];
final normalizedBrightness = allowedBrightness.contains(brightness) ? brightness : 'light';
return TrayIcon('assets/tray_icon_$normalizedBrightness.ico', false);
```
</issue_to_address>
### Comment 3
<location> `lib/screens/settings_laboratory/widgets/settings/tray_icon_color_mode_settings.dart:62-69` </location>
<code_context>
+ onChanged: (value) async {
+ if (value == null) return;
+ setState(() => trayIconColorMode = value);
+ await _settingsManager.setValue<String>(kTrayIconColorModeKey, value);
+
+ // Update the tray icon immediately
+ $tray.updateTrayIcon();
+ },
+ ),
</code_context>
<issue_to_address>
**suggestion:** Consider awaiting $tray.updateTrayIcon for consistency.
Not awaiting this async method may cause race conditions or UI issues if the tray icon update isn't finished before subsequent actions. Awaiting ensures the update completes first.
```suggestion
onChanged: (value) async {
if (value == null) return;
setState(() => trayIconColorMode = value);
await _settingsManager.setValue<String>(kTrayIconColorModeKey, value);
// Update the tray icon immediately
await $tray.updateTrayIcon();
},
```
</issue_to_address>
### Comment 4
<location> `lib/screens/settings_theme/widgets/tray_icon_color_mode_setting.dart:41` </location>
<code_context>
+ await _settingsManager.setValue<String>(kTrayIconColorModeKey, value);
+
+ // Update the tray icon immediately
+ $tray.updateTrayIcon();
+ },
+ ),
</code_context>
<issue_to_address>
**suggestion:** Consider awaiting $tray.updateTrayIcon for consistency.
Not awaiting updateTrayIcon may cause race conditions or inconsistent UI updates. Awaiting ensures the tray icon reflects the latest state before proceeding.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Now moved the color schema settings into the hidden setting menu, and default to light tray icon if current DE is GNOME. |
|
LGTM, Thanks! |
2 participants
This PR introduces a feature allowing users to manually select a light or dark version for the Rune tray icon. This option is useful when the automatic detection is incorrect.
Summary by Sourcery
Allow users to override automatic tray icon theming by selecting a fixed light or dark icon mode or keeping automatic system-based behavior
New Features:
Enhancements: