Improve Setting Window Font & Format xaml styles & Fix log message & Improve code quality #3487
Conversation
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis update refactors the handling of application-wide font settings and improves encapsulation and dependency management throughout the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant App
participant ResourceDictionary
participant Window
User->>Settings: Change SettingWindowFont
Settings->>ResourceDictionary: Update "SettingWindowFont" and "ContentControlThemeFontFamily"
ResourceDictionary-->>Window: DynamicResource triggers font update
Window-->>User: UI updates with new font
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)Flow.Launcher/Helper/AutoStartup.cs (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 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: 0
🧹 Nitpick comments (3)
Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml (1)
61-63: Remove redundant Button FontSize
Since the nested<ui:FontIcon>explicitly setsFontSize, theFontSize="14"on the<Button>is redundant and can be removed to simplify the markup.Flow.Launcher/SettingPages/Views/SettingsPaneAbout.xaml (1)
106-107: Improve accessibility of icon-only Button
The<Button>contains only a<ui:FontIcon>and no textual content, which may not be accessible. Consider adding anAutomationProperties.Nameor tooltip to convey its purpose to screen readers.Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
117-117: Consider adding error handling for invalid font names.The current implementation creates a new FontFamily instance directly without validating if the font exists or handling potential exceptions when an invalid font name is provided.
- Application.Current.Resources["SettingWindowFont"] = new FontFamily(value); + try { + Application.Current.Resources["SettingWindowFont"] = new FontFamily(value); + } catch (Exception ex) { + Log.Error($"Failed to set font '{value}': {ex.Message}", ex); + // Consider falling back to a default font + // Application.Current.Resources["SettingWindowFont"] = new FontFamily(Win32Helper.GetSystemDefaultFont(false)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(2 hunks)Flow.Launcher/CustomQueryHotkeySetting.xaml(0 hunks)Flow.Launcher/CustomShortcutSetting.xaml(0 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/Resources/CustomControlTemplate.xaml(1 hunks)Flow.Launcher/Resources/SettingWindowStyle.xaml(1 hunks)Flow.Launcher/SelectBrowserWindow.xaml(0 hunks)Flow.Launcher/SelectFileManagerWindow.xaml(0 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneAbout.xaml(2 hunks)Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml(1 hunks)Flow.Launcher/SettingWindow.xaml(0 hunks)Flow.Launcher/WelcomeWindow.xaml(1 hunks)
💤 Files with no reviewable changes (5)
- Flow.Launcher/CustomShortcutSetting.xaml
- Flow.Launcher/SelectFileManagerWindow.xaml
- Flow.Launcher/SettingWindow.xaml
- Flow.Launcher/SelectBrowserWindow.xaml
- Flow.Launcher/CustomQueryHotkeySetting.xaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Flow.Launcher/Resources/SettingWindowStyle.xaml (1)
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Flow.Launcher/Resources/CustomControlTemplate.xaml (1)
undefined
<retrieved_learning>
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher/Resources/SettingWindowStyle.xaml (1)
10-10: Verify resource dictionary merging
Ensure thatSettingWindowStyle.xamlis included in the application’s merged dictionaries (e.g., in App.xaml), so that theSettingWindowFontresource is available at runtime.Flow.Launcher/Languages/en.xaml (1)
358-358: Verify localization key replacement
Confirm that the old"settingWindowFont"key was removed to avoid duplicate resource entries, and that all references now use"settingWindowFontTitle".Flow.Launcher/SettingPages/Views/SettingsPaneAbout.xaml (1)
149-149: Resource key rename approved
The binding has been updated to{DynamicResource settingWindowFontTitle}, matching the new key inen.xaml.Flow.Launcher/WelcomeWindow.xaml (1)
144-146: EnsureFontIconstyle is applied globally
These buttons now use nested<ui:FontIcon>elements. Verify that a global style forui:FontIcon(e.g., settingFontFamily="Segoe Fluent Icons") is defined and merged so that icons render correctly throughout the app.Also applies to: 153-155
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
110-118: Good implementation of the centralized font management approach!This change centralizes font application through the resource dictionary, which aligns with WPF best practices for styling. By setting the FontFamily resource when the property changes, all UI elements using this resource will automatically update.
I see this supports the learnings about using implicit styles in WPF applications rather than setting FontFamily properties directly on controls.
Flow.Launcher/Resources/CustomControlTemplate.xaml (1)
7-19: Well-implemented global font styling approach!This change correctly implements a centralized approach to font styling by defining implicit styles that set the font family for various UI element types. Rather than setting FontFamily directly on individual controls throughout the application, these styles apply the font consistently via dynamic resources.
The approach follows WPF best practices by:
- Setting icon-specific fonts for
ui:FontIconelements- Using a dynamic resource
SettingWindowFontfor text elements- Providing consistent styling across all control types
- Supporting automatic font application while still allowing explicit overrides where needed
This implementation perfectly aligns with the recommendation to use implicit styles for font management in WPF applications.
|
@coderabbitai Review it. |
|
Tip For best results, initiate chat on the files or code changes. |
Jack251970
added
kind/ui
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:
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:
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 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This pull request improves the font management in the settings window by adding dynamic updates and separating icon styling.
- Dynamically updates the application resource "SettingWindowFont" to reflect user changes.
- Removes direct FontFamily settings from controls and centralizes the update logic.
- Replaces MDL2 with Fluent UI for context menu icons.
Reviewed Changes
Copilot reviewed 2 out of 12 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/App.xaml.cs | Sets the "SettingWindowFont" resource during main window initialization. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Updates the "SettingWindowFont" resource in the property setter for dynamic font updates. |
Files not reviewed (10)
- Flow.Launcher/CustomQueryHotkeySetting.xaml: Language not supported
- Flow.Launcher/CustomShortcutSetting.xaml: Language not supported
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/Resources/CustomControlTemplate.xaml: Language not supported
- Flow.Launcher/Resources/SettingWindowStyle.xaml: Language not supported
- Flow.Launcher/SelectBrowserWindow.xaml: Language not supported
- Flow.Launcher/SelectFileManagerWindow.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneAbout.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPanePlugins.xaml: Language not supported
- Flow.Launcher/WelcomeWindow.xaml: Language not supported
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I put all changes in #3486. And all things look good to me. Tested. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors several components to improve the dynamic update of font settings, refines view model initialization, updates error logging, and cleans up dependency injection setup.
- Dynamically update and synchronize font resources for settings and content controls.
- Refactor view model initialization for welcome pages and update notify icon/context menu handling.
- Improve dependency injection, error logging, and code comments for clarity.
Reviewed Changes
Copilot reviewed 43 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/Resources/Pages/WelcomePage2.xaml.cs | Refactored view model initialization and updated Settings access modifiers. |
| Flow.Launcher/Resources/Pages/WelcomePage1.xaml.cs | Similar refactoring as WelcomePage2 with improved dependency injection. |
| Flow.Launcher/Resources/Controls/HotkeyDisplay.xaml.cs | Enhanced null-check handling using pattern matching. |
| Flow.Launcher/MainWindow.xaml.cs | Reorganized notify icon and context menu initialization; improved logging. |
| Flow.Launcher/CustomShortcutSetting.xaml.cs | Removed redundant dependency injection for Settings. |
| Flow.Launcher/CustomQueryHotkeySetting.xaml.cs | Updated settings injection pattern. |
| Flow.Launcher/App.xaml.cs | Refined dependency injection and dynamic resource updates; improved logging. |
| Flow.Launcher.Infrastructure/UserSettings/Settings.cs | Updated font setter to update dynamic resources on change. |
| Flow.Launcher/Core/Plugin/JsonRPCPluginSettings.cs | Minor change in control creation logic with null-forgiving operator. |
Files not reviewed (11)
- Flow.Launcher/ActionKeywords.xaml: Language not supported
- Flow.Launcher/CustomQueryHotkeySetting.xaml: Language not supported
- Flow.Launcher/CustomShortcutSetting.xaml: Language not supported
- Flow.Launcher/HotkeyControl.xaml: Language not supported
- Flow.Launcher/HotkeyControlDialog.xaml: Language not supported
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/Resources/Controls/HotkeyDisplay.xaml: Language not supported
- Flow.Launcher/Resources/CustomControlTemplate.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage1.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage2.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage3.xaml: Language not supported
| @@ -270,6 +271,9 @@ private async void OnLoaded(object sender, RoutedEventArgs _) | |||
| case nameof(Settings.KeepMaxResults): | |||
| SetupResizeMode(); | |||
| break; | |||
| case nameof(Settings.SettingWindowFont): | |||
| InitializeContextMenu(); | |||
There was a problem hiding this comment.
Reinitializing the entire context menu when the font setting changes may lead to unintended side effects. Consider updating only the context menu text or related dynamic styles to better isolate the font change behavior.
| InitializeContextMenu(); | |
| foreach (var item in _contextMenu.Items) | |
| { | |
| if (item is MenuItem menuItem) | |
| { | |
| menuItem.FontFamily = new FontFamily(_settings.SettingWindowFont); | |
| menuItem.FontSize = _settings.FontSize; // Assuming FontSize is part of settings | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
It cannot work here. We must recreate menu items here.
| // Update dynamic resources base on settings | ||
| Current.Resources["SettingWindowFont"] = new FontFamily(_settings.SettingWindowFont); | ||
| Current.Resources["ContentControlThemeFontFamily"] = new FontFamily(_settings.SettingWindowFont); | ||
|
|
There was a problem hiding this comment.
Dynamic resource updates for font families are performed both here and in the Settings property setter. Consider consolidating this logic to a single location to avoid redundant or conflicting updates.
| // Update dynamic resources base on settings | |
| Current.Resources["SettingWindowFont"] = new FontFamily(_settings.SettingWindowFont); | |
| Current.Resources["ContentControlThemeFontFamily"] = new FontFamily(_settings.SettingWindowFont); | |
| // Update dynamic resources based on settings | |
| UpdateFontFamilyResources(); |
Copilot uses AI. Check for mistakes.
Flow.Launcher/App.xaml.cs
Outdated
| .AddSingleton<IAlphabet, PinyinAlphabet>() | ||
| .AddSingleton<StringMatcher>() | ||
| .AddSingleton<Internationalization>() | ||
| .AddSingleton<IPublicAPI, PublicAPIInstance>() | ||
| .AddSingleton<MainViewModel>() | ||
| .AddSingleton<Theme>() | ||
| // Welcome view model & setting window view model is very simple so we just use one instance |
There was a problem hiding this comment.
I am not convinced by this logic. What do you mean by simple and complex?
There was a problem hiding this comment.
Previous version may confuse you and I have improved the code comments.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the settings window font behavior, refines XAML style formatting, adjusts log messages, and enhances overall code quality and dependency injection across the application. Key changes include dynamic font updates without explicit FontFamily settings for controls, refactored viewmodel and settings initialization, and revised context menu/notify icon handling for better responsiveness.
Reviewed Changes
Copilot reviewed 44 out of 55 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/Resources/Pages/WelcomePage2.xaml.cs | Refactored settings and viewmodel initialization to use private setters and fields. |
| Flow.Launcher/Resources/Pages/WelcomePage1.xaml.cs | Similar refactoring with duplicate settings property removal and improved dependency injection. |
| Flow.Launcher/Resources/Controls/HotkeyDisplay.xaml.cs | Updated dependency property change handling using pattern matching for cleaner code. |
| Flow.Launcher/MainWindow.xaml.cs | Revised context menu initialization and notify icon event handling, with enhanced logging and comments. |
| Flow.Launcher/CustomShortcutSetting.xaml.cs | Removed direct injection of Settings to favor a more explicit dependency approach. |
| Flow.Launcher/CustomQueryHotkeySetting.xaml.cs | Improved consistency by replacing a public Settings property with a private field. |
| Flow.Launcher/App.xaml.cs | Adjusted dependency injection setup, dynamic resource updates, and log message formatting. |
| Flow.Launcher/Infrastructure/UserSettings/Settings.cs | Added dynamic updating of application resources when the SettingWindowFont property changes. |
| Flow.Launcher/Core/Plugin/JsonRPCPluginSettings.cs | Introduced an explicit check for creating a setting panel, with a null-forgiving operator usage. |
Files not reviewed (11)
- Flow.Launcher/ActionKeywords.xaml: Language not supported
- Flow.Launcher/CustomQueryHotkeySetting.xaml: Language not supported
- Flow.Launcher/CustomShortcutSetting.xaml: Language not supported
- Flow.Launcher/HotkeyControl.xaml: Language not supported
- Flow.Launcher/HotkeyControlDialog.xaml: Language not supported
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/Resources/Controls/HotkeyDisplay.xaml: Language not supported
- Flow.Launcher/Resources/CustomControlTemplate.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage1.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage2.xaml: Language not supported
- Flow.Launcher/Resources/Pages/WelcomePage3.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher/Core/Plugin/JsonRPCPluginSettings.cs:157
- Using the null-forgiving operator to return null may hide potential null reference issues at runtime. Consider explicitly returning null or handling the case so that callers can safely process a missing setting panel.
if (!NeedCreateSettingPanel()) return null!;
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
jjw24
removed
bug
Summary
FontFamilyno longer needs to be explicitly set for controls to adopt the user-defined font.FontFamilysetting from the main window.ui:FontIconusage of MDL2 with Fluent UI, now used in context menu icons.Notes
Tests