Conversation
Refactored Theme.cs to improve theme blur and dark mode handling: - Replaced static IsBlurTheme() with IsThemeBlurEnabled(dict) for per-theme blur detection. - Updated all blur checks to use the new method. - Improved code style, readability, and comments. - Moved system dark mode registry check to only run when needed. - Ensured blur and dark mode are applied based on theme and user/system preferences.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors Theme.cs for readability while keeping behavior effectively the same, and updates blur detection to be evaluated per theme ResourceDictionary rather than via a global resource lookup.
Changes:
- Replace
IsBlurTheme()withIsThemeBlurEnabled(ResourceDictionary)and use it consistently across theme switching and blur application. - Minor readability/style cleanups (use of
var, pattern matchingis ... or ..., brace/formatting adjustments). - Avoid unnecessary registry reads by only querying system dark mode when
ColorSchemeis effectively “Auto”.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughTheme.cs refactor: blur capability detection moved from a parameterless helper to a ResourceDictionary-based predicate that also verifies Win32 backdrop support. Font variable declarations were standardized and BackdropType comparisons updated to pattern matching; ChangeTheme and SetBlurForWindow now consult the theme ResourceDictionary for blur capability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
295-304: Consolidate font-setting logic to a single code path.Line 295–318 keeps manual font setter construction inside
GetResourceDictionary, whileUpdateFonts()now relies onApplyFontSettings(dict). This split increases drift risk. Prefer callingApplyFontSettings(dict)fromGetResourceDictionaryand removing the duplicated per-style setter blocks.♻️ Suggested consolidation
private ResourceDictionary GetResourceDictionary(string theme) { var dict = GetThemeResourceDictionary(theme); - - if (dict["QueryBoxStyle"] is Style queryBoxStyle && - dict["QuerySuggestionBoxStyle"] is Style querySuggestionBoxStyle) - { - ... - } - - if (dict["ItemTitleStyle"] is Style resultItemStyle && - dict["ItemTitleSelectedStyle"] is Style resultItemSelectedStyle && - dict["ItemHotkeyStyle"] is Style resultHotkeyItemStyle && - dict["ItemHotkeySelectedStyle"] is Style resultHotkeyItemSelectedStyle) - { - ... - } - - if (dict["ItemSubTitleStyle"] is Style resultSubItemStyle && - dict["ItemSubTitleSelectedStyle"] is Style resultSubItemSelectedStyle) - { - ... - } + ApplyFontSettings(dict); /* Ignore Theme Window Width and use setting */ var windowStyle = dict["WindowStyle"] as Style; var width = _settings.WindowSize; windowStyle.Setters.Add(new Setter(FrameworkElement.WidthProperty, width)); return dict; }Also applies to: 310-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 295 - 304, GetResourceDictionary currently duplicates per-style font Setter construction that UpdateFonts() already centralizes in ApplyFontSettings(dict); remove the manual setter block in GetResourceDictionary and instead call ApplyFontSettings(dict) so all font logic flows through the single method. Locate the block creating fontFamily/fontStyle/fontWeight/fontStretch and the Array.ForEach that adds them to resultItemStyle, resultItemSelectedStyle, resultHotkeyItemStyle, resultHotkeyItemSelectedStyle, delete that block and invoke ApplyFontSettings(dict) (the same dict used to build styles) to ensure consistent font application across GetResourceDictionary and UpdateFonts().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Line 673: The inline comment referencing the window border style has a typo:
replace the lowercase identifier "windowborderstyle" with the correct PascalCase
symbol "WindowBorderStyle" to match the code style and existing symbol names
(reference BackdropType, Mica, MicaAlt and WindowBorderStyle in Theme.cs) so the
comment reads that if BackdropType is Mica or MicaAlt, set the WindowBorderStyle
background to transparent.
---
Nitpick comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 295-304: GetResourceDictionary currently duplicates per-style font
Setter construction that UpdateFonts() already centralizes in
ApplyFontSettings(dict); remove the manual setter block in GetResourceDictionary
and instead call ApplyFontSettings(dict) so all font logic flows through the
single method. Locate the block creating
fontFamily/fontStyle/fontWeight/fontStretch and the Array.ForEach that adds them
to resultItemStyle, resultItemSelectedStyle, resultHotkeyItemStyle,
resultHotkeyItemSelectedStyle, delete that block and invoke
ApplyFontSettings(dict) (the same dict used to build styles) to ensure
consistent font application across GetResourceDictionary and UpdateFonts().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
908-916:⚠️ Potential issue | 🟡 MinorInclude Acrylic in transparent background handling for blur effects.
The pattern match at line 908 only covers
MicaandMicaAlt, leavingAcrylicto fall through to the else clause wheremainWindow.Backgroundreceives the opaqueselectedBGcolor. However, the code comment explicitly states the intent is to "set the background to transparent if the theme supports blur," and Acrylic (which maps toDWMSBT_TRANSIENTWINDOW) also requires transparency for the blur effect to render properly.Additionally,
SetBlurForWindowalready handles Acrylic specially by setting its border background toBrushes.Transparent, making the current inconsistency a bug.🔧 Suggested fix to include Acrylic in transparent handling
- if (backdropType is BackdropTypes.Mica or BackdropTypes.MicaAlt) + if (backdropType is BackdropTypes.Mica or BackdropTypes.MicaAlt or BackdropTypes.Acrylic) { mainWindow.Background = ThemeHelper.GetFrozenSolidColorBrush(Color.FromArgb(1, 0, 0, 0)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 908 - 916, The backdrop handling mistakenly treats BackdropTypes.Acrylic like an opaque background; update the pattern match in the block that sets mainWindow.Background (which currently checks BackdropTypes.Mica or BackdropTypes.MicaAlt) to also include BackdropTypes.Acrylic so that ThemeHelper.GetFrozenSolidColorBrush(Color.FromArgb(1,0,0,0)) is used for Acrylic as well (matching the transparent handling used in SetBlurForWindow); this ensures Acrylic gets the transparent background needed for blur effects instead of receiving selectedBG.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
832-867: Consider defensive null handling for systemBG.If the theme's
SystemBGvalue exists but is not a string (e.g., a theme authoring error),as stringreturnsnull. The subsequent if-else chain won't match any condition (null != "Dark", etc.), leavinguseDarkModeasfalse.This silent fallback to light mode is reasonable, but you could make it explicit by treating
nullthe same as"Auto":💡 Optional: Make the fallback explicit
- var systemBG = dict.Contains("SystemBG") ? dict["SystemBG"] as string : "Auto"; + var systemBG = dict.Contains("SystemBG") ? dict["SystemBG"] as string ?? "Auto" : "Auto";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/Theme.cs` around lines 832 - 867, The code currently does `var systemBG = dict.Contains("SystemBG") ? dict["SystemBG"] as string : "Auto";` which yields null if the key exists but isn't a string, causing silent fallback to light mode; change the assignment of systemBG so a non-string or null value is treated as "Auto" (e.g., coalesce the `as string` result to "Auto" or use a safe conversion) before the if/else so the subsequent logic (useDarkMode, _settings.ColorScheme, Registry.GetValue) behaves as if "Auto".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 908-916: The backdrop handling mistakenly treats
BackdropTypes.Acrylic like an opaque background; update the pattern match in the
block that sets mainWindow.Background (which currently checks BackdropTypes.Mica
or BackdropTypes.MicaAlt) to also include BackdropTypes.Acrylic so that
ThemeHelper.GetFrozenSolidColorBrush(Color.FromArgb(1,0,0,0)) is used for
Acrylic as well (matching the transparent handling used in SetBlurForWindow);
this ensures Acrylic gets the transparent background needed for blur effects
instead of receiving selectedBG.
---
Nitpick comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 832-867: The code currently does `var systemBG =
dict.Contains("SystemBG") ? dict["SystemBG"] as string : "Auto";` which yields
null if the key exists but isn't a string, causing silent fallback to light
mode; change the assignment of systemBG so a non-string or null value is treated
as "Auto" (e.g., coalesce the `as string` result to "Auto" or use a safe
conversion) before the if/else so the subsequent logic (useDarkMode,
_settings.ColorScheme, Registry.GetValue) behaves as if "Auto".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1ceae95-414d-43d5-afdf-c49085a35bc9
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Theme.cs
CHANGES
Separated from #4302
Summary by cubic
Refactors theme blur and dark mode handling to be per-theme and more efficient. Behavior should be unchanged for existing themes, with fewer unnecessary system checks.
Written for commit dad8f3e. Summary will update on new commits.