Fix font select function #3377
Fix font select function #3377
Conversation
…Blur theme - Removed bold style from highlight and delegated it to individual themes
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request enhances theme and font management functionality. The core theme file updates resource dictionary retrieval for null safety and introduces methods for updating fonts and applying font settings. The settings view model modifies how font properties are updated, focusing on font changes without altering the overall theme. Additionally, redundant font weight settings have been removed from XAML style definitions, and a minor formatting cleanup was performed in another XAML file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPaneThemeVM
participant ThemeManager as Theme.cs
participant UI
User->>SettingsPaneThemeVM: Update font settings
SettingsPaneThemeVM->>ThemeManager: UpdateFonts()
ThemeManager->>ThemeManager: Retrieve ResourceDictionary (FirstOrDefault)
ThemeManager->>ThemeManager: ApplyFontSettings() to update styles
ThemeManager->>ThemeManager: SetFontProperties() for text elements
ThemeManager-->>SettingsPaneThemeVM: Return updated theme
SettingsPaneThemeVM->>UI: Refresh UI with new font settings
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
Flow.Launcher.Core/Resource/Theme.cs (3)
20-20: Possible unused import
It appearsSystem.Diagnosticsis not used in the file. Consider removing this import to keep the codebase clean.-using System.Diagnostics;
123-143: Consider user feedback on failure
UpdateFonts()silently logs an error but doesn't inform users if loading the target dictionary fails. Provide a fallback or user-friendly error message to maintain clear feedback.
194-249: Minor duplication
Removing and re-adding font setters is repeated forTextBoxvs.TextBlock. A shared helper function to reduce duplication could be beneficial, but this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Core/Resource/Theme.cs(6 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(6 hunks)Flow.Launcher/Themes/Base.xaml(0 hunks)Flow.Launcher/Themes/BlurBlack Darker.xaml(1 hunks)Flow.Launcher/Themes/BlurBlack.xaml(1 hunks)Flow.Launcher/Themes/BlurWhite.xaml(1 hunks)
💤 Files with no reviewable changes (1)
- Flow.Launcher/Themes/Base.xaml
🧰 Additional context used
🧬 Code Definitions (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
UpdateFonts(126-143)
Flow.Launcher.Core/Resource/Theme.cs (1)
Flow.Launcher.Infrastructure/Constant.cs (1)
Constant(7-57)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (13)
Flow.Launcher/Themes/BlurBlack.xaml (1)
55-57: Addition of HighlightStyle provides consistent text emphasis across themesThe new HighlightStyle adds a standardized way to apply bold formatting to inline text elements, which improves the consistency of text emphasis across the application.
Flow.Launcher/Themes/BlurWhite.xaml (1)
63-65: Addition of HighlightStyle provides consistent text emphasis across themesThe addition of this style with identical properties as in the other theme files ensures that highlighted text appears the same regardless of the selected theme.
Flow.Launcher/Themes/BlurBlack Darker.xaml (1)
57-59: Addition of HighlightStyle provides consistent text emphasis across themesThe addition of HighlightStyle to this theme maintains consistency with the other themes, ensuring highlighted text appears uniformly across all theme variants.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs (6)
345-345: Performance improvement: Using more specific font update methodReplacing
_theme.ChangeTheme()with_theme.UpdateFonts()is more efficient as it only refreshes the font resources instead of reloading the entire theme when just the font changes.
367-367: Performance improvement: Using more specific font update methodUsing
UpdateFonts()instead of a full theme change is more targeted and efficient for font-related updates.
389-389: Performance improvement: Using more specific font update methodUsing the dedicated
UpdateFonts()method provides better performance by focusing only on updating font resources.
411-411: Performance improvement: Using more specific font update methodThe focused font update approach prevents unnecessary resource reloading when only font properties have changed.
435-435: Performance improvement: Using more specific font update methodThis change improves UI responsiveness by using a targeted update method instead of triggering a full theme reload.
456-456: Performance improvement: Using more specific font update methodUsing the dedicated font update method completes the consistent approach across all font property setters.
Flow.Launcher.Core/Resource/Theme.cs (4)
105-119: Good dictionary update approach
Adding the new resource dictionary first and only then removing the old resource prevents potential flicker and broken references. Looks solid.
148-189: Controlled font updates
ApplyFontSettingsproperly checks the style keys and updates font properties. The logic is straightforward and correct.
412-430: Asynchronous UI refresh
Invoking_ = RefreshFrameAsync()after updating the dictionary helps the UI adapt smoothly without blocking. The flow is clear and well-handled.
627-627: Consistent resource retrieval
UsingGetResourceDictionary(theme)ensures that the newly applied font settings are correctly reflected in the dictionary.
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970 onesounds, 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:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
Jack251970
added
Dev branch only
|
@coderabbitai OK now? |
This comment has been minimized.
This comment has been minimized.
🧩 Analysis chain🏁 Script executed: Length of output: 43887 @onesounds Yes, the PR looks good now! Your latest commit addressing potential null references with ✅ Actions performedComments resolved. |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Theme.cs (1)
273-320: Consider refactoring to use the new ApplyFontSettings method.There's duplicated logic between this section and the new
ApplyFontSettingsmethod. Consider refactoring theGetResourceDictionarymethod to use the newApplyFontSettingsmethod instead of directly manipulating the style setters here.private ResourceDictionary GetResourceDictionary(string theme) { var dict = GetThemeResourceDictionary(theme); - if (dict["QueryBoxStyle"] is Style queryBoxStyle && - 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); - - queryBoxStyle.Setters.Add(new Setter(Control.FontFamilyProperty, fontFamily)); - queryBoxStyle.Setters.Add(new Setter(Control.FontStyleProperty, fontStyle)); - queryBoxStyle.Setters.Add(new Setter(Control.FontWeightProperty, fontWeight)); - queryBoxStyle.Setters.Add(new Setter(Control.FontStretchProperty, fontStretch)); - - var caretBrushPropertyValue = queryBoxStyle.Setters.OfType<Setter>().Any(x => x.Property.Name == "CaretBrush"); - var foregroundPropertyValue = queryBoxStyle.Setters.OfType<Setter>().Where(x => x.Property.Name == "Foreground") - .Select(x => x.Value).FirstOrDefault(); - if (!caretBrushPropertyValue && foregroundPropertyValue != null) //otherwise BaseQueryBoxStyle will handle styling - queryBoxStyle.Setters.Add(new Setter(TextBoxBase.CaretBrushProperty, foregroundPropertyValue)); - - // Query suggestion box's font style is aligned with query box - querySuggestionBoxStyle.Setters.Add(new Setter(Control.FontFamilyProperty, fontFamily)); - querySuggestionBoxStyle.Setters.Add(new Setter(Control.FontStyleProperty, fontStyle)); - querySuggestionBoxStyle.Setters.Add(new Setter(Control.FontWeightProperty, fontWeight)); - querySuggestionBoxStyle.Setters.Add(new Setter(Control.FontStretchProperty, fontStretch)); - } - - if (dict["ItemTitleStyle"] is Style resultItemStyle && - dict["ItemTitleSelectedStyle"] is Style resultItemSelectedStyle && - dict["ItemHotkeyStyle"] is Style resultHotkeyItemStyle && - dict["ItemHotkeySelectedStyle"] is Style resultHotkeyItemSelectedStyle) - { - Setter fontFamily = new Setter(TextBlock.FontFamilyProperty, new FontFamily(_settings.ResultFont)); - Setter fontStyle = new Setter(TextBlock.FontStyleProperty, FontHelper.GetFontStyleFromInvariantStringOrNormal(_settings.ResultFontStyle)); - Setter fontWeight = new Setter(TextBlock.FontWeightProperty, FontHelper.GetFontWeightFromInvariantStringOrNormal(_settings.ResultFontWeight)); - Setter fontStretch = new Setter(TextBlock.FontStretchProperty, FontHelper.GetFontStretchFromInvariantStringOrNormal(_settings.ResultFontStretch)); - - Setter[] setters = { fontFamily, fontStyle, fontWeight, fontStretch }; - Array.ForEach( - new[] { resultItemStyle, resultItemSelectedStyle, resultHotkeyItemStyle, resultHotkeyItemSelectedStyle }, o - => Array.ForEach(setters, p => o.Setters.Add(p))); - } - - if ( - dict["ItemSubTitleStyle"] is Style resultSubItemStyle && - dict["ItemSubTitleSelectedStyle"] is Style resultSubItemSelectedStyle) - { - Setter fontFamily = new Setter(TextBlock.FontFamilyProperty, new FontFamily(_settings.ResultSubFont)); - Setter fontStyle = new Setter(TextBlock.FontStyleProperty, FontHelper.GetFontStyleFromInvariantStringOrNormal(_settings.ResultSubFontStyle)); - Setter fontWeight = new Setter(TextBlock.FontWeightProperty, FontHelper.GetFontWeightFromInvariantStringOrNormal(_settings.ResultSubFontWeight)); - Setter fontStretch = new Setter(TextBlock.FontStretchProperty, FontHelper.GetFontStretchFromInvariantStringOrNormal(_settings.ResultSubFontStretch)); - - Setter[] setters = { fontFamily, fontStyle, fontWeight, fontStretch }; - Array.ForEach( - new[] { resultSubItemStyle, resultSubItemSelectedStyle }, o - => Array.ForEach(setters, p => o.Setters.Add(p))); - } + // Apply font settings to the theme resource + ApplyFontSettings(dict); /* Ignore Theme Window Width and use setting */ var windowStyle = dict["WindowStyle"] as Style; var width = _settings.WindowSize; windowStyle.Setters.Add(new Setter(FrameworkElement.WidthProperty, width)); return dict; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Resource/Theme.cs(8 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/SettingPages/ViewModels/SettingsPaneThemeViewModel.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (7)
Flow.Launcher.Core/Resource/Theme.cs (7)
59-75: Excellent defensive programming with null safety!The change from
FirsttoFirstOrDefaultwith proper null handling is a significant improvement that prevents potential crashes. This addresses the resource availability issue mentioned in the PR objectives by properly handling cases when the theme resource is not found.
104-115: Good resource management improvement.The updated resource dictionary management logic now properly checks if the new dictionary already exists before adding it and only removes the old resource if it exists and is different. This prevents potential resource conflicts and memory leaks.
120-142: Well-implemented font update method.The new
UpdateFontsmethod properly addresses the issue where theme font changes weren't being applied correctly. It provides a focused way to update just the fonts without changing the entire theme, making the UI refresh more efficient.
147-188: Comprehensive font settings implementation.This method properly applies font settings to all relevant UI elements, ensuring consistency across the application interface. Good separation of concerns by having this logic in a dedicated method.
193-248: Well-structured font property management.The
SetFontPropertiesmethod efficiently handles font application for both TextBox and TextBlock elements. The removal of existing font-related setters before adding new ones prevents duplicate style definitions.
410-430: Improved theme change logic and error handling.The updates to the
ChangeThememethod make it more robust by using a better error message format and ensuring the resource dictionary is always retrieved with font settings applied. The explicit refresh of the frame and return value are good improvements.
627-635: Good defensive programming with null checks.The addition of null checks for dictionary, window border style, and main window prevents potential null reference exceptions during the blur effect application process, making the feature more robust.
What's the PR
What was the problem
Test Cases