Freeze SolidColorBrush to improve performance#4319
Merged
Jack251970 merged 4 commits intodevfrom Mar 9, 2026
Merged
Conversation
Refactor background brush creation to assign, freeze, and reuse SolidColorBrush instances for better WPF performance. Replace string-based property checks with type-safe Control.BackgroundProperty comparisons for improved robustness.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve WPF performance and safety by freezing SolidColorBrush instances that are created for theme/window background rendering, reducing mutability and improving rendering efficiency.
Changes:
- Freeze newly created
SolidColorBrushinstances before assigning them toStylesetters in blur scenarios. - Freeze
SolidColorBrushused for the preview window background style. - Freeze
SolidColorBrushassigned toMainWindow.Backgroundwhen blur is available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
📝 WalkthroughWalkthroughReplaces property-name background checks with direct Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
676-686: Cache constant frozen brushes instead of recreating them on each refresh.Line 677, Line 684, and Line 926 still allocate new
SolidColorBrushinstances for fixed colors. Reusing class-level frozen singletons would better match the PR’s reuse/perf goal and remove repeated allocations.♻️ Suggested refactor
+ private static readonly SolidColorBrush MicaTransparentBrush = CreateFrozenBrush(Color.FromArgb(1, 0, 0, 0)); + private static readonly SolidColorBrush TransparentBrush = CreateFrozenBrush(Colors.Transparent); + + private static SolidColorBrush CreateFrozenBrush(Color color) + { + var brush = new SolidColorBrush(color); + brush.Freeze(); + return brush; + } @@ - var brush = new SolidColorBrush(Color.FromArgb(1, 0, 0, 0)); - brush.Freeze(); - windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, brush)); + windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, MicaTransparentBrush)); @@ - var brush = new SolidColorBrush(Colors.Transparent); - brush.Freeze(); - windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, brush)); + windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, TransparentBrush)); @@ - var backgroundBrush = new SolidColorBrush(Color.FromArgb(1, 0, 0, 0)); - backgroundBrush.Freeze(); - mainWindow.Background = backgroundBrush; + mainWindow.Background = MicaTransparentBrush;Also applies to: 926-934
🤖 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 676 - 686, Create and reuse class-level frozen SolidColorBrush singletons for the fixed colors instead of allocating new instances each refresh: add readonly static fields (e.g., FrozenAlmostTransparent = new SolidColorBrush(Color.FromArgb(1,0,0,0)).Freeze() and FrozenTransparent = new SolidColorBrush(Colors.Transparent).Freeze()) and replace the local allocations where windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty, brush)) is used (the blocks around the current SolidColorBrush creations and the similar code in the region referenced at 926-934) to reference these frozen fields; ensure the fields are frozen once at initialization and used everywhere the same constant brushes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 676-686: Create and reuse class-level frozen SolidColorBrush
singletons for the fixed colors instead of allocating new instances each
refresh: add readonly static fields (e.g., FrozenAlmostTransparent = new
SolidColorBrush(Color.FromArgb(1,0,0,0)).Freeze() and FrozenTransparent = new
SolidColorBrush(Colors.Transparent).Freeze()) and replace the local allocations
where windowBorderStyle.Setters.Add(new Setter(Border.BackgroundProperty,
brush)) is used (the blocks around the current SolidColorBrush creations and the
similar code in the region referenced at 926-934) to reference these frozen
fields; ensure the fields are frozen once at initialization and used everywhere
the same constant brushes are needed.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea83e89b-ddb2-4ede-a1ae-202bbb0172b9
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Theme.cs
jjw24
reviewed
Mar 8, 2026
Flow.Launcher.Core/Resource/Theme.cs
Outdated
| { | ||
| mainWindow.Background = new SolidColorBrush(Color.FromArgb(1, 0, 0, 0)); | ||
| var backgroundBrush = new SolidColorBrush(Color.FromArgb(1, 0, 0, 0)); | ||
| backgroundBrush.Freeze(); |
Member
There was a problem hiding this comment.
There a bit of repeated code, is it possible to create a new method for creating SolidColorBrush?
Introduce ThemeHelper.GetFreezeSolidColorBrush to centralize and reuse SolidColorBrush creation and freezing logic. Refactor Theme.cs to use this helper, improving code clarity and maintainability. Also, use Brushes.Transparent directly where appropriate.
Moved the CopyStyle method from Theme to a new static ThemeHelper class for better code organization and reusability. Updated all references in Theme to use ThemeHelper.CopyStyle. Added ThemeHelper.cs and necessary using directives.
Renamed the method GetFreezeSolidColorBrush to GetFrozenSolidColorBrush in ThemeHelper.cs for improved naming consistency. Updated all references in Theme.cs accordingly. No changes to functionality.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 676-682: The removal predicate for the existing background Setter
is using Control.BackgroundProperty but the style is for a Border, so update the
Remove calls in the windowBorderStyle manipulation (the lines handling
windowBorderStyle.Setters.Remove(...)) to check for Border.BackgroundProperty
instead of Control.BackgroundProperty so the old Border background Setter is
actually found and removed before adding the new Setter (affects the branches
using ThemeHelper.GetFreezeSolidColorBrush(...) and Brushes.Transparent when
backdropType is Acrylic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3244241-8734-487f-b9ae-aa19153d8337
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher.Core/Resource/ThemeHelper.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Flow.Launcher.Core/Resource/ThemeHelper.cs (2)
24-29: This still allocates a new brush on every call.
Freeze()removes change tracking, but it does not satisfy the “reuse” part of the optimization.SetBlurForWindow,ApplyPreviewBackground, andColorizeWindownow funnel through this helper, so repeated theme refreshes will still allocate identical brushes. Cache frozen instances byColorif the goal is to reduce allocations as well.Possible caching approach
+using System.Collections.Concurrent; using System.Linq; using System.Windows; using System.Windows.Media; @@ public static class ThemeHelper { + private static readonly ConcurrentDictionary<Color, SolidColorBrush> FrozenBrushCache = new(); + @@ public static SolidColorBrush GetFrozenSolidColorBrush(Color color) { - var brush = new SolidColorBrush(color); - brush.Freeze(); - return brush; + return FrozenBrushCache.GetOrAdd(color, static c => + { + var brush = new SolidColorBrush(c); + brush.Freeze(); + return brush; + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/ThemeHelper.cs` around lines 24 - 29, GetFrozenSolidColorBrush currently creates a new SolidColorBrush on every call which Freeze() alone doesn't prevent; change it to cache and reuse frozen brushes keyed by Color (so SetBlurForWindow, ApplyPreviewBackground, and ColorizeWindow receive the same frozen instance for identical colors). Implement a static thread-safe cache (e.g., ConcurrentDictionary<Color, SolidColorBrush> or a dictionary with a lock) inside the ThemeHelper and have GetFrozenSolidColorBrush return an existing entry if present, otherwise create, Freeze(), add to the cache, and return it. Ensure Color is used as the cache key and that cache population is atomic to avoid creating duplicates under concurrent calls.
9-22: Clarify thatCopyStyleis only copying setters.As a new shared helper,
CopyStylereads like a fullStyleclone, but it only flattensSetters from theBasedOnchain and skips things likeTriggers,Resources, andEventSetters. Either narrow the name/API or document that contract explicitly before other call sites start using it generically.Possible tightening
- public static void CopyStyle(Style originalStyle, Style targetStyle) + /// Copies only Setter entries from `originalStyle` and its `BasedOn` chain. + /// Triggers, resources, and event setters are intentionally not copied. + public static void CopyStyle(Style originalStyle, Style targetStyle)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Resource/ThemeHelper.cs` around lines 9 - 22, The method CopyStyle only flattens and copies Setter entries from originalStyle and its BasedOn chain and does not copy Triggers, Resources, EventSetter instances or other Style metadata; update the API to make this explicit by either renaming CopyStyle to CopySettersOnly (or adding an overload CopyStyleCloneFull for a full clone) and/or add a clear XML doc comment on the CopyStyle method stating it only copies Setter objects (uses originalStyle.Setters.OfType<Setter>() and targetStyle.Setters.Add) and that Triggers, Resources and EventSetter are intentionally skipped so callers know the contract; ensure any new name or doc mentions BasedOn flattening and reference the Setter, BasedOn, Setters, Triggers, Resources, and EventSetter symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Flow.Launcher.Core/Resource/ThemeHelper.cs`:
- Around line 24-29: GetFrozenSolidColorBrush currently creates a new
SolidColorBrush on every call which Freeze() alone doesn't prevent; change it to
cache and reuse frozen brushes keyed by Color (so SetBlurForWindow,
ApplyPreviewBackground, and ColorizeWindow receive the same frozen instance for
identical colors). Implement a static thread-safe cache (e.g.,
ConcurrentDictionary<Color, SolidColorBrush> or a dictionary with a lock) inside
the ThemeHelper and have GetFrozenSolidColorBrush return an existing entry if
present, otherwise create, Freeze(), add to the cache, and return it. Ensure
Color is used as the cache key and that cache population is atomic to avoid
creating duplicates under concurrent calls.
- Around line 9-22: The method CopyStyle only flattens and copies Setter entries
from originalStyle and its BasedOn chain and does not copy Triggers, Resources,
EventSetter instances or other Style metadata; update the API to make this
explicit by either renaming CopyStyle to CopySettersOnly (or adding an overload
CopyStyleCloneFull for a full clone) and/or add a clear XML doc comment on the
CopyStyle method stating it only copies Setter objects (uses
originalStyle.Setters.OfType<Setter>() and targetStyle.Setters.Add) and that
Triggers, Resources and EventSetter are intentionally skipped so callers know
the contract; ensure any new name or doc mentions BasedOn flattening and
reference the Setter, BasedOn, Setters, Triggers, Resources, and EventSetter
symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 764a04f9-8679-4f56-a7a5-2e2d67e6cf01
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher.Core/Resource/ThemeHelper.cs
jjw24
approved these changes
Mar 9, 2026
This was referenced Mar 9, 2026
Merged
Closed
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
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.
Refactor background brush creation to assign, freeze, and reuse SolidColorBrush instances for better WPF performance.
Separated from #4302
Test: All types of themes work as expected.
Summary by cubic
Freezes SolidColorBrush instances, switches to type‑safe Background property checks, and moves style‑copy logic to ThemeHelper to cut allocations and improve WPF rendering. Uses Brushes.Transparent where clearer.
Written for commit 7766d70. Summary will update on new commits.