Refactor drop shadow handling for window border style#4318
Refactor drop shadow handling for window border style#4318Jack251970 wants to merge 5 commits 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: 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:
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.
![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.
📝 WalkthroughWalkthroughPer-theme resource dictionaries are used for WindowBorderStyle changes: styles are cloned into unsealed instances, Margin and DropShadowEffect setters are updated/removed, resize border thickness is recalculated, and ThemeHelper gained a ReplaceSetter API while CopyStyle stops recursively copying BasedOn. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
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
Replaced direct calls to CopyStyle with ThemeHelper.CopyStyle in Theme.cs. This centralizes style copying logic and improves code organization and maintainability.
Refactored ThemeHelper.CopyStyle to assign the target style's BasedOn property directly from the original style, instead of recursively copying base style properties. This reduces complexity and ensures correct style inheritance.
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 486-509: ThemeHelper.CopyStyle already copies all setters from
windowBorderStyle into newWindowBorderStyle, so the foreach loop that iterates
windowBorderStyle.Setters and re-adds setters causes duplicates; fix by keeping
the CopyStyle call and instead modify the copied setters on
newWindowBorderStyle: locate and remove any Setter with Property ==
FrameworkElement.MarginProperty and any with Property ==
UIElement.EffectProperty from newWindowBorderStyle.Setters, then add the new
computed Margin Setter (and any new Effect) once — refer to
ThemeHelper.CopyStyle, newWindowBorderStyle, windowBorderStyle, Setter,
FrameworkElement.MarginProperty, and UIElement.EffectProperty to locate and
change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdd1bf01-dcb2-4b01-af14-5b5be8106e10
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher.Core/Resource/ThemeHelper.cs
There was a problem hiding this comment.
2 issues found across 2 files
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:488">
P2: `CopyStyle` already clones the setters, so this second setter-copy pass leaves duplicate setters and does not truly replace/remove `Margin` or `Effect`.</violation>
<violation number="2" location="Flow.Launcher.Core/Resource/Theme.cs:571">
P2: Remove the shadow from the active `WindowBorderStyle`, not a freshly reloaded theme style. As written, a theme-defined border margin is restored 32px too small after a shadow toggle.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Ensure existing Margin and Effect setters are removed from newWindowBorderStyle before adding new ones. This prevents duplicate property setters and ensures only the latest values are applied.
There was a problem hiding this comment.
Actionable comments posted: 1
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/ThemeHelper.cs (1)
9-22:⚠️ Potential issue | 🟠 Major
CopyStylehelper omitsStyle.ResourcesandStyle.Triggersduring cloning.The
CopyStylemethod (lines 9–22) copies onlyBasedOnand directSettersfrom the original style. It does not preserveStyle.ResourcesorStyle.Triggers. IfWindowBorderStyle(referenced byAddDropShadowEffectToCurrentTheme()at line 486 andApplyPreviewBackground()at line 836) defines local resources or triggers, these will be lost in the cloned replacement style.Verify whether the original
WindowBorderStyledefinition in theme XAML files contains<Style.Resources>or<Style.Triggers>blocks that must be preserved.🤖 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, CopyStyle currently only copies BasedOn and Setters and therefore drops Style.Resources and Style.Triggers (which can break WindowBorderStyle used by AddDropShadowEffectToCurrentTheme and ApplyPreviewBackground). Update CopyStyle to also preserve originalStyle.Resources and originalStyle.Triggers: if originalStyle.Resources is non-null, ensure targetStyle.Resources exists and merge/clone entries from originalStyle.Resources into it (preserving keys and values or creating a copy of the ResourceDictionary), and if originalStyle.Triggers contains Trigger/BaseTrigger items, clone each trigger into targetStyle.Triggers (preserving bindings/enter/exit actions). Keep the existing BasedOn and Setter copying logic and ensure you handle null checks to avoid exceptions when resources or triggers are absent.
♻️ Duplicate comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
486-509:⚠️ Potential issue | 🟠 Major
CopyStyle+ manual re-add still duplicates every non-special setter.
ThemeHelper.CopyStyle()already clones all direct setters intonewWindowBorderStyle. The loop here then adds every setter again exceptMargin/Effect, soBackground,BorderThickness, etc. are duplicated here and again inRemoveDropShadowEffectFromCurrentTheme().#!/bin/bash rg -n -C4 'CopyStyle\(windowBorderStyle, newWindowBorderStyle\)|newWindowBorderStyle\.Setters\.Add\(setterBase\)' \ Flow.Launcher.Core/Resource/Theme.cs \ Flow.Launcher.Core/Resource/ThemeHelper.csExpected: both shadow paths show
CopyStyle(...)followed bySetters.Add(setterBase), confirming the same setters are copied and then re-added.🤖 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 486 - 509, ThemeHelper.CopyStyle(windowBorderStyle, newWindowBorderStyle) already clones the style setters, so the foreach that iterates windowBorderStyle.Setters and calls newWindowBorderStyle.Setters.Add(setterBase) causes duplicate setters; remove the code that re-adds non-special setters and instead operate on the cloned setters in newWindowBorderStyle (e.g., locate and update the Margin Setter in newWindowBorderStyle to adjust margins, and ensure only one Effect setter exists by removing/adding on newWindowBorderStyle), referencing ThemeHelper.CopyStyle, newWindowBorderStyle, and the existing loop that inspects setter.Property.
🤖 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 545: The app-level vs merged-dictionary writes for WindowBorderStyle
create precedence bugs; unify to a single update path by changing
AddDropShadowEffectToCurrentTheme and RemoveDropShadowEffectFromCurrentTheme to
modify the same merged ResourceDictionary that SetBlurForWindow updates (via
UpdateResourceDictionary) OR alternatively make SetBlurForWindow write to
Application.Current.Resources like the others and have UpdateResourceDictionary
clear any duplicate app-level entry; modify the code in
AddDropShadowEffectToCurrentTheme, RemoveDropShadowEffectFromCurrentTheme,
SetBlurForWindow and UpdateResourceDictionary so all four reference and update
the same ResourceDictionary instance (or ensure duplicates are removed) to
guarantee predictable DynamicResource resolution.
---
Outside diff comments:
In `@Flow.Launcher.Core/Resource/ThemeHelper.cs`:
- Around line 9-22: CopyStyle currently only copies BasedOn and Setters and
therefore drops Style.Resources and Style.Triggers (which can break
WindowBorderStyle used by AddDropShadowEffectToCurrentTheme and
ApplyPreviewBackground). Update CopyStyle to also preserve
originalStyle.Resources and originalStyle.Triggers: if originalStyle.Resources
is non-null, ensure targetStyle.Resources exists and merge/clone entries from
originalStyle.Resources into it (preserving keys and values or creating a copy
of the ResourceDictionary), and if originalStyle.Triggers contains
Trigger/BaseTrigger items, clone each trigger into targetStyle.Triggers
(preserving bindings/enter/exit actions). Keep the existing BasedOn and Setter
copying logic and ensure you handle null checks to avoid exceptions when
resources or triggers are absent.
---
Duplicate comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 486-509: ThemeHelper.CopyStyle(windowBorderStyle,
newWindowBorderStyle) already clones the style setters, so the foreach that
iterates windowBorderStyle.Setters and calls
newWindowBorderStyle.Setters.Add(setterBase) causes duplicate setters; remove
the code that re-adds non-special setters and instead operate on the cloned
setters in newWindowBorderStyle (e.g., locate and update the Margin Setter in
newWindowBorderStyle to adjust margins, and ensure only one Effect setter exists
by removing/adding on newWindowBorderStyle), referencing ThemeHelper.CopyStyle,
newWindowBorderStyle, and the existing loop that inspects setter.Property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f25b2f0-5b24-4ba7-87c2-47f438a69317
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.csFlow.Launcher.Core/Resource/ThemeHelper.cs
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 build and apply a new window border
Styleinstead of editing the existing one. AddedThemeHelper.ReplaceSetterto avoid duplicate Margin/Effect/Background setters and make theme switching more reliable.Style(typeof(Border))viaThemeHelper.CopyStyle(copies setters/resources/triggers;BasedOnset directly). Apply withApplication.Current.Resources["WindowBorderStyle"]. UseGetThemeResourceDictionary(_settings.Theme)and return early ifWindowBorderStyleis missing. UseThemeHelper.ReplaceSetterwhen updating Background inSetBlurForWindowfor Mica/Acrylic.ThemeHelper.ReplaceSetterto ensure only one of each. Margin math preserves existing values and adds/subtractsShadowExtraMarginon add/remove. UpdateSetResizeBoarderThicknesswith the new margin (add) or null (remove).UpdateResourceDictionarycalls. Recursive base-style copying inThemeHelper. Direct remove/add of setters in blur code (now replaced byReplaceSetter).StyleandDropShadowEffectper change; old instances are GC-eligible. No duplicate setters, no sustained growth.Written for commit 3c82661. Summary will update on new commits.