Conversation
…validation on Windows theme change Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
|
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}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWraps resource dictionary updates in defensive checks, adds a private helper to derive CaretBrush values from Foreground safely, freezes newly created brushes for immutability, makes CopyStyle static, hardens dispatcher usage in theme refresh/blur async flows, and limits frame refresh on theme change to the System color scheme. (≈33 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as MainWindow
participant Theme as Theme
participant Disp as Dispatcher
participant Win as Window
UI->>Theme: ActualApplicationThemeChanged(event)
alt ColorScheme == System
Theme->>Disp: RefreshFrameAsync()
Disp-->>Theme: Ensure on UI thread (reinvoke if needed)
Theme->>Theme: Resolve Foreground (including DynamicResource)
Theme->>Theme: GetNewCaretValue(foreground)
Theme->>Theme: Create new SolidColorBrush & Freeze()
Theme->>Win: Apply brush/setters (CopyStyle called statically)
Theme->>Disp: SetBlurForWindowAsync() (dispatcher guard)
else
UI-->>Theme: Skip RefreshFrameAsync
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This pull request suppresses a spurious error dialog that appears when users change Windows themes or accent colors. The error is caused by a WPF framework bug where SystemResources.InvalidateTreeResources incorrectly attempts to clone Color structs as if they were Freezable or Expression objects, resulting in an InvalidCastException that the application cannot prevent.
Changes:
- Added
IsRecoverableSystemResourceExceptionmethod to detect the specific WPF system resource invalidation exception - Integrated the new exception check into
ErrorReporting.Reportto suppress the error dialog while still logging the exception - Added comprehensive documentation explaining the WPF framework issue
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/Helper/ExceptionHelper.cs | Added new method to detect InvalidCastException from WPF's SystemResources.InvalidateTreeResources |
| Flow.Launcher/Helper/ErrorReporting.cs | Integrated the new exception check to prevent error dialog from being shown |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ource invalidation on Windows theme change" This reverts commit 372f142.
Refactored caret and background brush assignment to avoid sharing mutable instances and ensure proper resource referencing. Added GetNewCaretValue helper for safe caret brush creation. Brushes for backgrounds are now frozen for performance. Improved foreground value retrieval and dynamic resource key extraction. Made some methods static for clarity and consistency. Enhances resource management and reliability in theme handling.
coderabbitai
bot
removed
bug
Jack251970
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
814-819:⚠️ Potential issue | 🟡 MinorRemove ToString() when looking up DynamicResource keys in ResourceDictionary.
At line 814, converting
dynamicResource.ResourceKeyto string viaToString()violates WPF resource lookup semantics.ResourceDictionaryrequires matching the actual key object (e.g.,ComponentResourceKey), not its string representation. This inconsistency is evident at line 260, where the code correctly preservesResourceKeyobject identity.Proposed fix
- var resourceKey = dynamicResource.ResourceKey.ToString(); + var resourceKey = dynamicResource.ResourceKey; - if (Resources.Contains(resourceKey)) + if (resourceKey != null && Resources.Contains(resourceKey)) { var colorResource = Resources[resourceKey];🤖 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 814 - 819, The code is converting dynamicResource.ResourceKey to a string before lookup which breaks WPF resource key identity; in the Theme.cs block around the dynamicResource handling, stop calling ToString() and use the ResourceKey object directly when checking and retrieving from the ResourceDictionary (i.e., replace the use of var resourceKey = dynamicResource.ResourceKey.ToString() and subsequent Resources.Contains(resourceKey)/Resources[resourceKey] calls with checks that use dynamicResource.ResourceKey itself so ComponentResourceKey and other non-string keys resolve correctly).
🤖 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 814-819: The code is converting dynamicResource.ResourceKey to a
string before lookup which breaks WPF resource key identity; in the Theme.cs
block around the dynamicResource handling, stop calling ToString() and use the
ResourceKey object directly when checking and retrieving from the
ResourceDictionary (i.e., replace the use of var resourceKey =
dynamicResource.ResourceKey.ToString() and subsequent
Resources.Contains(resourceKey)/Resources[resourceKey] calls with checks that
use dynamicResource.ResourceKey itself so ComponentResourceKey and other
non-string keys resolve correctly).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously, the theme was refreshed on every theme change event. Now, `_theme.RefreshFrameAsync()` is only called if the color scheme setting is set to "System", preventing unnecessary refreshes in other scenarios.
|
🥷 Code experts: jjw24 jjw24, Jack251970 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Refactored RefreshFrameAsync and SetBlurForWindowAsync to check Dispatcher access before invoking UI updates, ensuring thread safety and reducing unnecessary Dispatcher calls. Also removed redundant code and unused using directive.
Pull request was converted to draft
Improve IsBlurTheme to check blur support directly in the current theme's resource dictionary, ensuring precise and context-specific detection. Updated related logic and comments for clarity.
Renamed the variable themeName to theme in the UpdateFonts method to improve naming consistency. No functional changes were made.
Reworked how drop shadow effects are added and removed by creating new unsealed Style instances instead of modifying sealed ones. Resources, triggers, and setters are copied, with margin adjustments and effect setter management to ensure reliability. Introduced GetNewWindowBorderStyle helper for style copying.
Explicitly set DispatcherPriority.Render when invoking RefreshFrameAsync and SetBlurForWindowAsync on the application's dispatcher. This ensures theme-related UI changes are processed with rendering priority, improving responsiveness and visual consistency. Also added System.Windows.Threading import for access to DispatcherPriority.
Refactored theme resource dictionary lookups to safely check for "WindowBorderStyle" before accessing, preventing potential exceptions. Updated three methods to handle missing styles gracefully and standardized resource key checks. GetWindowBorderStyleBackground now returns transparent if style is missing.
Renamed IsBlurTheme to IsThemeBlurEnabled and moved system backdrop support checks out of the helper function. Blur availability is now determined by combining theme settings and system capability in the calling code. Updated all usages to reflect the new logic for improved clarity.
Refactored dark mode logic to first use user's ColorScheme preference when "SystemBG" is set to "Auto". Only falls back to system dark mode setting if ColorScheme is not explicitly set to "Dark" or "Light". This ensures user preferences take precedence over system defaults.
Freeze SolidColorBrush instances before assigning them to mainWindow.Background to improve performance and immutability. Applied to both transparent and selected background scenarios.
Replaces GetNewWindowBorderStyle with direct instantiation of a new Style for Border and uses CopyStyle to transfer properties. Removes the old helper method and moves style copying logic inline for improved clarity and maintainability.
Changed assignment of WindowBorderStyle to use Application.Current.Resources for global effect, replacing previous use of _oldResource. This ensures theme changes are applied consistently across the application.
Jack251970
changed the title
Jack251970
removed
bug
This pull request makes several improvements and bug fixes to the theme and window styling logic in
Theme.cs. The main focus is on making theme resource management more robust, improving drop shadow effect handling, ensuring safe brush usage, and enhancing dispatcher/thread safety for UI updates.Theme resource management and robustness:
InvalidCastException, preventing crashes when resource dictionaries contain unexpected types. (UpdateResourceDictionary, Flow.Launcher.Core/Resource/Theme.csL103-R124)RefreshFrameAsync,SetBlurForWindowAsync) to ensure thread safety and avoid exceptions when called from non-UI threads. (RefreshFrameAsync, [1];SetBlurForWindowAsync, [2]Drop shadow and style application:
AddDropShadowEffectToCurrentTheme,RemoveDropShadowEffectFromCurrentTheme, Flow.Launcher.Core/Resource/Theme.csL480-R632)AutoDropShadow, [1] [2]Brush and resource safety:
GetNewCaretValue, Flow.Launcher.Core/Resource/Theme.csR262-R292)SetBlurForWindow, Flow.Launcher.Core/Resource/Theme.csL670-R780)Resource dictionary and style retrieval improvements:
GetWindowBorderStyleBackground, Flow.Launcher.Core/Resource/Theme.csL749-R850)GetWindowBorderStyleBackground, Flow.Launcher.Core/Resource/Theme.csL768-R873)Minor refactors and cleanups:
Most important changes:
Theme resource management and thread safety:
InvalidCastException, preventing crashes when adding new theme resources.RefreshFrameAsyncandSetBlurForWindowAsync. [1] [2]Drop shadow and style handling:
Brush and resource safety:
Resource dictionary and style retrieval: