Refactor font style management & Ensure style removing before adding#4315
Open
Jack251970 wants to merge 7 commits intodevfrom
Open
Refactor font style management & Ensure style removing before adding#4315Jack251970 wants to merge 7 commits intodevfrom
Jack251970 wants to merge 7 commits intodevfrom
Conversation
Refactored style management to explicitly remove existing CaretBrush setters before adding new ones, preventing duplicates. Unified font property removal to consistently target Control's font properties, enhancing robustness and consistency for text control styling.
Move font and style setting logic to ApplyFontSettings for better modularity. Improve window width handling by removing existing setters before applying user settings, preventing duplicates and ensuring cleaner resource dictionaries.
Updated logic to compare dependency property objects directly instead of using string property names when identifying and manipulating CaretBrush and Foreground setters. This enhances robustness and reduces risk of errors from string comparisons.
|
🥷 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. |
Contributor
|
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:
📝 WalkthroughWalkthroughCentralizes font styling in Theme.cs via a new ApplyFontSettings(ResourceDictionary); consolidates per-control font logic (text vs non-text, caret/foreground handling); removes theme-driven Window width setters and applies _settings.WindowSize when WindowStyle exists. No public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors theme font style application to reduce duplication and adjusts style setter handling so font-related setters are removed before new ones are applied, aiming to avoid conflicts/duplicates in runtime theme resources.
Changes:
- Centralizes font application logic by routing font updates through shared helpers instead of duplicating setter additions.
- Updates
SetFontPropertiesto remove existing font setters before adding updated ones, and revises caret brush handling for text boxes. - Adjusts
GetResourceDictionaryso theme window width is overridden from settings while avoiding duplicateWidthsetters.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 276-281: The code only removes the first Width setter; update the
logic that modifies windowStyle.Setters so it removes all setters whose Property
equals FrameworkElement.WidthProperty before adding the new Setter with
_settings.WindowSize (i.e., iterate over windowStyle.Setters, filter by Setter
with Property == FrameworkElement.WidthProperty and remove each, then add the
new Setter on windowStyle). Target symbols: windowStyle, windowStyle.Setters,
Setter, FrameworkElement.WidthProperty, _settings.WindowSize.
- Around line 220-228: The code currently only removes the first caret setter
(caretBrushProperty) which leaves other TextBoxBase.CaretBrushProperty setters
behind; update the logic in the block that references style.Setters,
caretBrushProperty, TextBoxBase.CaretBrushProperty and
Control.ForegroundProperty so you first collect all setters whose Property ==
TextBoxBase.CaretBrushProperty (e.g., ToList() of
style.Setters.OfType<Setter>().Where(...)), remove each of them from
style.Setters, then (if a foregroundPropertyValue exists) add the new
Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue); ensure you no
longer rely on a single caretBrushProperty variable and remove all matches
before adding the new setter.
Previously, only the first width setter in WindowStyle was removed, which could leave conflicting width settings. Now, all width setters are removed before adding a new one based on user settings, ensuring the user's preferred window width is always applied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
220-231:⚠️ Potential issue | 🟡 MinorAlways remove existing caret setters, even when
Foregroundis missing.On Line 224, removal is conditional on
foregroundPropertyValue != null. That leaves oldCaretBrushsetters untouched in styles without a foreground setter, so stale values can persist.Proposed fix
- if (caretBrushPropertySetters.Count > 0 && foregroundPropertyValue != null) - { - foreach (var setter in caretBrushPropertySetters) - { - style.Setters.Remove(setter); - } - style.Setters.Add(new Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue)); - } + foreach (var setter in caretBrushPropertySetters) + { + style.Setters.Remove(setter); + } + + if (foregroundPropertyValue != null) + { + style.Setters.Add(new Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue)); + }🤖 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 220 - 231, The existing logic in Theme.cs only removes caretBrushPropertySetters when foregroundPropertyValue != null, leaving old TextBoxBase.CaretBrushProperty setters in styles that lack a Foreground setter; change the code to always remove any caretBrushPropertySetters (iterate and remove them unconditionally via style.Setters.Remove) and then, only if foregroundPropertyValue != null, add the new Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue); keep references to caretBrushPropertySetters, foregroundPropertyValue, and style.Setters to locate and update the code.
🤖 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 273-290: UpdateFonts() currently bypasses the width override by
loading the theme via GetThemeResourceDictionary(); change UpdateFonts() to
obtain its theme dictionary through GetResourceDictionary() (or apply the same
width-removal and user-width Setter logic there) so the _settings.WindowSize
override is always enforced after a font refresh; modify UpdateFonts() to call
GetResourceDictionary(...) instead of GetThemeResourceDictionary(...) or
duplicate the width-strip/Add Setter logic (the block that removes
FrameworkElement.WidthProperty setters and adds new Setter with
_settings.WindowSize) so the theme width never reverts after UpdateFonts runs.
---
Duplicate comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 220-231: The existing logic in Theme.cs only removes
caretBrushPropertySetters when foregroundPropertyValue != null, leaving old
TextBoxBase.CaretBrushProperty setters in styles that lack a Foreground setter;
change the code to always remove any caretBrushPropertySetters (iterate and
remove them unconditionally via style.Setters.Remove) and then, only if
foregroundPropertyValue != null, add the new
Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue); keep references
to caretBrushPropertySetters, foregroundPropertyValue, and style.Setters to
locate and update the code.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Resource/Theme.cs:255
- In the non-TextBox branch, you remove
Control.Font*setters but then addTextBlock.Font*setters. ForStyleinstances targetingTextBlock, this won’t remove existingTextBlock.Font*setters (so duplicates/conflicts can remain). For styles targeting non-TextBlock controls, addingTextBlock.Font*setters can be invalid for the target type and may throw at runtime when the style is sealed/applied. Align the remove/add properties with the style’s actual target (e.g., remove/addTextBlock.Font*forTextBlockstyles, and/or handle bothControlandTextBlockfont DPs explicitly).
// Find the setters to remove and store them in a list
var settersToRemove = style.Setters
.OfType<Setter>()
.Where(setter =>
setter.Property == Control.FontFamilyProperty ||
setter.Property == Control.FontStyleProperty ||
setter.Property == Control.FontWeightProperty ||
setter.Property == Control.FontStretchProperty)
.ToList();
// Remove each found setter one by one
foreach (var setter in settersToRemove)
{
style.Setters.Remove(setter);
}
// Add New font setter
style.Setters.Add(new Setter(TextBlock.FontFamilyProperty, fontFamily));
style.Setters.Add(new Setter(TextBlock.FontStyleProperty, fontStyle));
style.Setters.Add(new Setter(TextBlock.FontWeightProperty, fontWeight));
style.Setters.Add(new Setter(TextBlock.FontStretchProperty, fontStretch));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixed the logic for setting the CaretBrush property in styles to only add the setter if it does not already exist and a Foreground value is present. This removes unnecessary checks and setter removals, making the code more efficient and easier to maintain.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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:224">
P2: Caret brush update condition is inverted and skips replacing existing caret setters, so existing themes keep stale caret colors instead of syncing to foreground.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
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.
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Resource/Theme.cs:251
- In the
isTextBox == falsebranch, you addTextBlock.Font*setters, but this branch will also be used for styles targeting controls (e.g.,QuerySuggestionBoxStyleis aTextBoxin themes).TextBlock.FontFamilyPropertyetc. won’t apply to aTextBox, so user font settings won’t take effect. Consider basing the setter properties onstyle.TargetType(Control vs TextBlock) or splitting the flag into “use Control font DPs” vs “apply caret brush”.
// Add New font setter
style.Setters.Add(new Setter(TextBlock.FontFamilyProperty, fontFamily));
style.Setters.Add(new Setter(TextBlock.FontStyleProperty, fontStyle));
style.Setters.Add(new Setter(TextBlock.FontWeightProperty, fontWeight));
style.Setters.Add(new Setter(TextBlock.FontStretchProperty, fontStretch));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
220-227:⚠️ Potential issue | 🟡 MinorCaret brush setter is not normalized before re-apply.
At Line 224, caret brush is only added when missing. Existing
TextBoxBase.CaretBrushPropertysetters are left intact, so stale/conflicting values can persist.Proposed fix
- var caretBrushPropertyExist = style.Setters.OfType<Setter>().Any(x => x.Property == TextBoxBase.CaretBrushProperty); var foregroundPropertyValue = style.Setters.OfType<Setter>().Where(x => x.Property == Control.ForegroundProperty) .Select(x => x.Value).FirstOrDefault(); - if (!caretBrushPropertyExist && foregroundPropertyValue != null) + var caretBrushSetters = style.Setters + .OfType<Setter>() + .Where(x => x.Property == TextBoxBase.CaretBrushProperty) + .ToList(); + + foreach (var setter in caretBrushSetters) + { + style.Setters.Remove(setter); + } + + if (foregroundPropertyValue != null) { style.Setters.Add(new Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue)); }#!/bin/bash # Verify whether shipped themes already define CaretBrush in QueryBoxStyle fd -e xaml | xargs rg -n -C2 'x:Key="QueryBoxStyle"|Setter\s+Property="(Foreground|CaretBrush|TextBoxBase.CaretBrush)"'🤖 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 220 - 227, The caret brush setter handling currently only adds a CaretBrush setter when missing, leaving existing stale/conflicting TextBoxBase.CaretBrushProperty setters; update the logic in the QueryBox style processing (the variable style and its Setters) to normalize/reapply the caret brush by locating any existing Setter with Property == TextBoxBase.CaretBrushProperty and updating its Value to the computed foregroundPropertyValue (instead of leaving it), and if none exists add a new Setter as before; ensure you still only act when foregroundPropertyValue != null and preserve surrounding behavior.
🤖 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 235-238: The removal predicate currently checks for
Control.FontFamilyProperty / FontStyleProperty / FontWeightProperty /
FontStretchProperty but the insertion uses TextBlock.Font* properties, causing
leftover duplicate/conflicting setters; update the removal logic in Theme.cs to
also match TextBlock.FontFamilyProperty, TextBlock.FontStyleProperty,
TextBlock.FontWeightProperty and TextBlock.FontStretchProperty (or otherwise
match by property Name "FontFamily/Style/Weight/Stretch" regardless of
OwnerType) so that any existing TextBlock font setters are removed symmetrically
to the insertion code that uses TextBlock.Font*Property.
---
Duplicate comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 220-227: The caret brush setter handling currently only adds a
CaretBrush setter when missing, leaving existing stale/conflicting
TextBoxBase.CaretBrushProperty setters; update the logic in the QueryBox style
processing (the variable style and its Setters) to normalize/reapply the caret
brush by locating any existing Setter with Property ==
TextBoxBase.CaretBrushProperty and updating its Value to the computed
foregroundPropertyValue (instead of leaving it), and if none exists add a new
Setter as before; ensure you still only act when foregroundPropertyValue != null
and preserve surrounding behavior.
coderabbitai
bot
added
bug
Contributor
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)
149-178:⚠️ Potential issue | 🟠 MajorAvoid direct indexer lookups for theme style keys.
dict["..."]throws before theis Styleguard can fail. A custom theme that omits any one of these keys will make font refresh/theme load fail instead of just skipping that style block. Please switch these lookups toContains/TryGetValueand update each style independently.Proposed direction
- if (dict["QueryBoxStyle"] is Style queryBoxStyle && - dict["QuerySuggestionBoxStyle"] is Style querySuggestionBoxStyle) + if (dict.Contains("QueryBoxStyle") && + dict["QueryBoxStyle"] is Style queryBoxStyle) { var fontFamily = new FontFamily(_settings.QueryBoxFont); var fontStyle = FontHelper.GetFontStyleFromInvariantStringOrNormal(_settings.QueryBoxFontStyle); var fontWeight = FontHelper.GetFontWeightFromInvariantStringOrNormal(_settings.QueryBoxFontWeight); var fontStretch = FontHelper.GetFontStretchFromInvariantStringOrNormal(_settings.QueryBoxFontStretch); SetFontProperties(queryBoxStyle, fontFamily, fontStyle, fontWeight, fontStretch, true); - SetFontProperties(querySuggestionBoxStyle, fontFamily, fontStyle, fontWeight, fontStretch, false); + } + + if (dict.Contains("QuerySuggestionBoxStyle") && + dict["QuerySuggestionBoxStyle"] is Style querySuggestionBoxStyle) + { + var fontFamily = new FontFamily(_settings.QueryBoxFont); + var fontStyle = FontHelper.GetFontStyleFromInvariantStringOrNormal(_settings.QueryBoxFontStyle); + var fontWeight = FontHelper.GetFontWeightFromInvariantStringOrNormal(_settings.QueryBoxFontWeight); + var fontStretch = FontHelper.GetFontStretchFromInvariantStringOrNormal(_settings.QueryBoxFontStretch); + + SetFontProperties(querySuggestionBoxStyle, fontFamily, fontStyle, fontWeight, fontStretch, false); }🤖 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 149 - 178, The code currently uses dict["Key"] indexer inside an 'is Style' guard which throws if the key is missing; change each grouped block to independently check for presence and type using dict.TryGetValue (or dict.ContainsKey + pattern match) for each key (e.g., "QueryBoxStyle", "QuerySuggestionBoxStyle", "ItemTitleStyle", "ItemTitleSelectedStyle", "ItemHotkeyStyle", "ItemHotkeySelectedStyle", etc.) and only call SetFontProperties when that TryGetValue returns a Style; update both the query box and item/result blocks so missing theme keys are skipped instead of throwing, preserving use of _settings.QueryBoxFont/_settings.ResultFont and the existing SetFontProperties(...) calls.
♻️ Duplicate comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
127-133:⚠️ Potential issue | 🟠 MajorRoute
UpdateFonts()through the normalized dictionary path.
UpdateFonts()still loads the raw theme dictionary, so it skips the post-processing now centralized inGetResourceDictionary(). After a font refresh, that can reintroduce theme-providedWindowStyle.Widthinstead of the user setting.Proposed fix
- // Load a ResourceDictionary for the specified theme. - var theme = _settings.Theme; - var dict = GetThemeResourceDictionary(theme); - - // Apply font settings to the theme resource. - ApplyFontSettings(dict); + var dict = GetResourceDictionary(_settings.Theme); UpdateResourceDictionary(dict);🤖 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 127 - 133, UpdateFonts() is currently loading the raw theme ResourceDictionary and bypassing the normalization in GetResourceDictionary(), which causes theme defaults (e.g., WindowStyle.Width) to override user font settings; change UpdateFonts() to obtain the theme dictionary via GetResourceDictionary(theme) (instead of directly loading the raw dictionary or using GetThemeResourceDictionary), then call ApplyFontSettings(dict) and UpdateResourceDictionary(dict) so font updates go through the same post-processing path used elsewhere.
🤖 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 149-178: The code currently uses dict["Key"] indexer inside an 'is
Style' guard which throws if the key is missing; change each grouped block to
independently check for presence and type using dict.TryGetValue (or
dict.ContainsKey + pattern match) for each key (e.g., "QueryBoxStyle",
"QuerySuggestionBoxStyle", "ItemTitleStyle", "ItemTitleSelectedStyle",
"ItemHotkeyStyle", "ItemHotkeySelectedStyle", etc.) and only call
SetFontProperties when that TryGetValue returns a Style; update both the query
box and item/result blocks so missing theme keys are skipped instead of
throwing, preserving use of _settings.QueryBoxFont/_settings.ResultFont and the
existing SetFontProperties(...) calls.
---
Duplicate comments:
In `@Flow.Launcher.Core/Resource/Theme.cs`:
- Around line 127-133: UpdateFonts() is currently loading the raw theme
ResourceDictionary and bypassing the normalization in GetResourceDictionary(),
which causes theme defaults (e.g., WindowStyle.Width) to override user font
settings; change UpdateFonts() to obtain the theme dictionary via
GetResourceDictionary(theme) (instead of directly loading the raw dictionary or
using GetThemeResourceDictionary), then call ApplyFontSettings(dict) and
UpdateResourceDictionary(dict) so font updates go through the same
post-processing path used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05c6ccd9-e29c-43b0-bf99-40d8290b80d4
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Theme.cs
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.
CHANGES
Moved the logic for applying font settings to controls and result items into a new
ApplyFontSettingsmethod, reducing code duplication and making font management more maintainable. (Flow.Launcher.Core/Resource/Theme.cs, Flow.Launcher.Core/Resource/Theme.csL256-L324)Flow.Launcher.Core/Resource/Theme.cs, Flow.Launcher.Core/Resource/Theme.csL220-R248)Flow.Launcher.Core/Resource/Theme.cs, Flow.Launcher.Core/Resource/Theme.csL220-R248)Summary by cubic
Refactored font/style handling to replace setters cleanly and only add
CaretBrushwhen needed. Window width now removes all existing width setters before applying the user size to avoid conflicts.Summary of changes
Written for commit dc7eb81. Summary will update on new commits.