Refactor drop shadow handling for window border style#4318
Refactor drop shadow handling for window border style#4318Jack251970 wants to merge 1 commit intodevfrom
Conversation
Refactor drop shadow effect logic to create a new Style instance instead of modifying the existing one in-place. This approach copies all relevant resources, triggers, and setters, updating only the Margin and Effect as needed. The new Style is applied at the application level, improving robustness for theme switching and preventing issues with sealed or shared Style objects.
|
🥷 Code experts: jjw24 jjw24 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull request overview
Refactors how the WindowBorderStyle drop shadow is applied/removed by attempting to create a new Style instance rather than mutating the existing one in-place.
Changes:
- Reworked
AddDropShadowEffectToCurrentThemeto construct a newWindowBorderStyleand add Margin + DropShadowEffect setters. - Reworked
RemoveDropShadowEffectFromCurrentThemeto construct a newWindowBorderStylewithout the Effect setter and with adjusted Margin. - Switched from updating the merged theme
ResourceDictionaryto assigningApplication.Current.Resources["WindowBorderStyle"].
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get a new unsealed style based on the old one, and copy Resources and Triggers | ||
| var newWindowBorderStyle = new Style(typeof(Border)); | ||
| CopyStyle(windowBorderStyle, newWindowBorderStyle); | ||
|
|
||
| if (windowBorderStyle.Setters.FirstOrDefault(setterBase => setterBase is Setter setter && setter.Property == FrameworkElement.MarginProperty) is Setter marginSetter) | ||
| // Copy Setters, excluding the Effect setter and updating the Margin setter | ||
| foreach (var setterBase in windowBorderStyle.Setters) | ||
| { | ||
| var currentMargin = (Thickness)marginSetter.Value; | ||
| var newMargin = new Thickness( | ||
| currentMargin.Left - ShadowExtraMargin, | ||
| currentMargin.Top - ShadowExtraMargin, | ||
| currentMargin.Right - ShadowExtraMargin, | ||
| currentMargin.Bottom - ShadowExtraMargin); | ||
| marginSetter.Value = newMargin; | ||
| if (setterBase is Setter setter) | ||
| { | ||
| // Skip existing Effect (We'll remove it) | ||
| if (setter.Property == UIElement.EffectProperty) continue; | ||
|
|
||
| // Update Margin by subtracting the extra margin we added for the shadow | ||
| if (setter.Property == FrameworkElement.MarginProperty) | ||
| { | ||
| var currentMargin = (Thickness)setter.Value; | ||
| var newMargin = new Thickness( | ||
| currentMargin.Left - ShadowExtraMargin, | ||
| currentMargin.Top - ShadowExtraMargin, | ||
| currentMargin.Right - ShadowExtraMargin, | ||
| currentMargin.Bottom - ShadowExtraMargin); | ||
| newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, newMargin)); | ||
| continue; | ||
| } | ||
| } | ||
| newWindowBorderStyle.Setters.Add(setterBase); | ||
| } |
There was a problem hiding this comment.
Same issue as in Add: CopyStyle already clones setters, but this loop adds the original setterBase instances again (and also adds a new Margin setter when a Margin exists). This will create duplicate setters for the same properties and may throw when the style is applied. Prefer new Style(windowBorderStyle.TargetType) { BasedOn = windowBorderStyle } and then add overrides/removals, or otherwise ensure setters are only present once.
| SetResizeBoarderThickness(newMargin); | ||
|
|
||
| Application.Current.Resources["WindowBorderStyle"] = newWindowBorderStyle; |
There was a problem hiding this comment.
Assigning Application.Current.Resources["WindowBorderStyle"] creates a top-level resource that takes precedence over the theme merged dictionary. In RefreshFrameAsync, when BlurEnabled is true, AutoDropShadow() runs before SetBlurForWindow(), so this override can prevent SetBlurForWindow from updating WindowBorderStyle (background/corner radius changes) via UpdateResourceDictionary(dict). Consider updating the theme ResourceDictionary (and calling UpdateResourceDictionary) instead of setting a top-level key, or ensure the top-level override is removed when blur/backdrop handling needs the theme style to be authoritative.
| SetResizeBoarderThickness(null); | ||
|
|
||
| UpdateResourceDictionary(dict); | ||
| Application.Current.Resources["WindowBorderStyle"] = newWindowBorderStyle; |
There was a problem hiding this comment.
Same resource-precedence problem as in Add: setting Application.Current.Resources["WindowBorderStyle"] here can mask updates that happen later via UpdateResourceDictionary(dict) (e.g., blur/backdrop adjustments in SetBlurForWindow). If this method is intended to restore the theme-defined style, consider removing the top-level override key (or updating the merged dictionary entry) rather than always re-setting it at the application root.
| Application.Current.Resources["WindowBorderStyle"] = newWindowBorderStyle; | |
| // Update the current theme's resource dictionary instead of overriding the | |
| // application-level resource, so later theme updates keep correct precedence. | |
| dict["WindowBorderStyle"] = newWindowBorderStyle; | |
| UpdateResourceDictionary(dict); |
| // Get a new unsealed style based on the old one, and copy Resources and Triggers | ||
| var newWindowBorderStyle = new Style(typeof(Border)); | ||
| CopyStyle(windowBorderStyle, newWindowBorderStyle); | ||
|
|
||
| var effectSetter = new Setter | ||
| // Identify existing Margin to calculate new Margin, and copy other setters | ||
| Setter existingMarginSetter = null; | ||
| foreach (var setterBase in windowBorderStyle.Setters) | ||
| { | ||
| Property = UIElement.EffectProperty, | ||
| Value = new DropShadowEffect | ||
| if (setterBase is Setter setter) | ||
| { | ||
| Opacity = 0.3, | ||
| ShadowDepth = 12, | ||
| Direction = 270, | ||
| BlurRadius = 30 | ||
| // Skip existing Margin (we'll replace it) | ||
| if (setter.Property == FrameworkElement.MarginProperty) | ||
| { | ||
| existingMarginSetter = setter; | ||
| continue; | ||
| } | ||
|
|
||
| // Skip existing Effect (we'll ensure strictly one is added) | ||
| if (setter.Property == UIElement.EffectProperty) continue; | ||
| } | ||
| }; | ||
|
|
||
| if (windowBorderStyle.Setters.FirstOrDefault(setterBase => setterBase is Setter setter && setter.Property == FrameworkElement.MarginProperty) is not Setter marginSetter) | ||
| { | ||
| var margin = new Thickness(ShadowExtraMargin, 12, ShadowExtraMargin, ShadowExtraMargin); | ||
| marginSetter = new Setter() | ||
| { | ||
| Property = FrameworkElement.MarginProperty, | ||
| Value = margin, | ||
| }; | ||
| windowBorderStyle.Setters.Add(marginSetter); | ||
| // Add other setters (e.g. Background, BorderThickness) | ||
| newWindowBorderStyle.Setters.Add(setterBase); | ||
| } |
There was a problem hiding this comment.
CopyStyle(windowBorderStyle, newWindowBorderStyle) already copies all setters (including from BasedOn), but this method then iterates windowBorderStyle.Setters and adds most of them again. That will duplicate setters for the same properties (e.g. Background/BorderThickness), which can throw when the style is sealed/applied or lead to unpredictable overrides. Consider either (a) building a derived style with BasedOn = windowBorderStyle and only adding/replacing the Margin/Effect setters, or (b) changing CopyStyle to not copy setters and instead deep-copy Triggers/Resources (as the comment suggests).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Core/Resource/Theme.cs">
<violation number="1" location="Flow.Launcher.Core/Resource/Theme.cs:508">
P2: This re-adds setters that were already copied by `CopyStyle`, creating duplicate property setters in `WindowBorderStyle`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
| windowBorderStyle.Setters.Add(marginSetter); | ||
| // Add other setters (e.g. Background, BorderThickness) | ||
| newWindowBorderStyle.Setters.Add(setterBase); |
There was a problem hiding this comment.
P2: This re-adds setters that were already copied by CopyStyle, creating duplicate property setters in WindowBorderStyle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Flow.Launcher.Core/Resource/Theme.cs, line 508:
<comment>This re-adds setters that were already copied by `CopyStyle`, creating duplicate property setters in `WindowBorderStyle`.</comment>
<file context>
@@ -477,76 +477,113 @@ public bool ChangeTheme(string theme = null)
- };
- windowBorderStyle.Setters.Add(marginSetter);
+ // Add other setters (e.g. Background, BorderThickness)
+ newWindowBorderStyle.Setters.Add(setterBase);
+ }
</file context>
📝 WalkthroughWalkthroughThe pull request refactors drop shadow management in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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}
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 569-577: The code is incorrectly subtracting ShadowExtraMargin
from any theme-defined margin (in the block handling setter.Property ==
FrameworkElement.MarginProperty), which shrinks or can make margins negative
when shadows are disabled; instead, preserve the theme's original margin by
adding the existing setter.Value (currentMargin) to newWindowBorderStyle.Setters
unchanged (i.e., remove the subtraction/creation of newMargin and simply call
newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty,
setter.Value))); keep this change local to the margin-handling branch that
processes setters from GetThemeResourceDictionary(theme).
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bf9cff6-290e-49a0-a770-d5eefac7da26
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Theme.cs
| if (setter.Property == FrameworkElement.MarginProperty) | ||
| { | ||
| var currentMargin = (Thickness)setter.Value; | ||
| var newMargin = new Thickness( | ||
| currentMargin.Left - ShadowExtraMargin, | ||
| currentMargin.Top - ShadowExtraMargin, | ||
| currentMargin.Right - ShadowExtraMargin, | ||
| currentMargin.Bottom - ShadowExtraMargin); | ||
| newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, newMargin)); |
There was a problem hiding this comment.
Don’t subtract shadow margin from the baseline theme margin.
At Lines 573-576, ShadowExtraMargin is subtracted from the margin loaded from GetThemeResourceDictionary(theme) (baseline style). If a theme already defines a margin, disabling shadow shrinks it (and can go negative).
✅ Minimal fix
- var newMargin = new Thickness(
- currentMargin.Left - ShadowExtraMargin,
- currentMargin.Top - ShadowExtraMargin,
- currentMargin.Right - ShadowExtraMargin,
- currentMargin.Bottom - ShadowExtraMargin);
- newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, newMargin));
+ newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, currentMargin));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (setter.Property == FrameworkElement.MarginProperty) | |
| { | |
| var currentMargin = (Thickness)setter.Value; | |
| var newMargin = new Thickness( | |
| currentMargin.Left - ShadowExtraMargin, | |
| currentMargin.Top - ShadowExtraMargin, | |
| currentMargin.Right - ShadowExtraMargin, | |
| currentMargin.Bottom - ShadowExtraMargin); | |
| newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, newMargin)); | |
| if (setter.Property == FrameworkElement.MarginProperty) | |
| { | |
| var currentMargin = (Thickness)setter.Value; | |
| newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty, currentMargin)); |
🤖 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 569 - 577, The code is
incorrectly subtracting ShadowExtraMargin from any theme-defined margin (in the
block handling setter.Property == FrameworkElement.MarginProperty), which
shrinks or can make margins negative when shadows are disabled; instead,
preserve the theme's original margin by adding the existing setter.Value
(currentMargin) to newWindowBorderStyle.Setters unchanged (i.e., remove the
subtraction/creation of newMargin and simply call
newWindowBorderStyle.Setters.Add(new Setter(FrameworkElement.MarginProperty,
setter.Value))); keep this change local to the margin-handling branch that
processes setters from GetThemeResourceDictionary(theme).
CHANGES
Refactor drop shadow effect logic to create a new Style instance instead of modifying the existing one in-place.
Separated from #4302.
TEST
Summary by cubic
Refactored drop shadow handling to create a new WindowBorderStyle per change instead of modifying the existing style in-place. This avoids sealed/shared style issues, improves theme switching, and keeps margin and effect updates consistent.
Written for commit c3b3b5f. Summary will update on new commits.