-
-
Notifications
You must be signed in to change notification settings - Fork 397
Fix property binding in WebSearch plugin #3889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🥷 Code experts: Jack251970, onesounds 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:
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. |
📝 WalkthroughWalkthroughRefactors the WebSearch plugin settings: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsControl (View)
participant Settings (ViewModel/Model)
User ->> SettingsControl: Toggle suggestion checkbox
SettingsControl ->> Settings: set EnableSuggestion(value)
Settings ->> Settings: update backing field
Settings ->> SettingsControl: OnPropertyChanged("EnableSuggestion")
SettingsControl ->> SettingsControl: UI updates (bindings re-evaluate)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs (1)
97-99
: Guard against empty sort keysConsider rejecting empty/whitespace values too to avoid creating invalid SortDescriptions when headers are blank.
- if (sortBy != null) + if (!string.IsNullOrWhiteSpace(sortBy)) { Sort(sortBy, direction);Plugins/Flow.Launcher.Plugin.WebSearch/setting.json (1)
144-146
: No migration logic detected for renamed settings keysI ran searches across the WebSearch plugin for any handling of the old keys (
EnableWebSearchSuggestion
/WebSearchSuggestionSource
) or JSON‐extension‐data migrations and found nothing. This suggests users who upgrade will lose their existing preference values.Could you please confirm whether any migration or alias‐mapping exists in your settings‐deserialization code? If not, I can draft a minimal fallback snippet to map the old keys to the new ones at load time.
Plugins/Flow.Launcher.Plugin.WebSearch/Settings.cs (2)
10-11
: Private field naming consistencyPrefer the underscore-prefixed pattern for private backing fields to match common C# conventions.
- private bool enableSuggestion; + private bool _enableSuggestion;And update usages accordingly (see next comment).
196-204
: Avoid redundant PropertyChanged notifications and align with field renameAdd a value equality guard and (optionally) use the underscore-prefixed field name for clarity.
- public bool EnableSuggestion - { - get => enableSuggestion; - set - { - enableSuggestion = value; - OnPropertyChanged(nameof(EnableSuggestion)); - } - } + public bool EnableSuggestion + { + get => _enableSuggestion; + set + { + if (value == _enableSuggestion) return; + _enableSuggestion = value; + OnPropertyChanged(nameof(EnableSuggestion)); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Plugins/Flow.Launcher.Plugin.WebSearch/Settings.cs
(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs
(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/setting.json
(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
Plugins/Flow.Launcher.Plugin.WebSearch/Settings.cs
📚 Learning: 2025-04-23T15:14:49.986Z
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.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
📚 Learning: 2025-03-28T21:12:13.386Z
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
📚 Learning: 2025-03-28T21:20:54.978Z
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:20:54.978Z
Learning: In WPF applications like Flow.Launcher, Border elements cannot directly display text content and require a child element like TextBlock to handle text rendering. This separation of concerns (Border for visual container styling, TextBlock for text display) follows WPF best practices and provides greater styling flexibility.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.WebSearch/Settings.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml (1)
137-139
: Binding directly to Settings.EnableSuggestion is the right fixThis removes the ElementName workaround and lets OnPropertyChanged propagate correctly to enable/disable the ComboBox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes property binding issues in the WebSearch plugin by implementing proper OnPropertyChanged
notifications and standardizing property names. The changes remove the need for workarounds that were previously required due to broken data binding.
- Implement proper
OnPropertyChanged
notification for theEnableSuggestion
property - Standardize property names in configuration and remove unused properties
- Fix XAML binding to use proper Settings properties instead of element names
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
setting.json | Updates property names from EnableWebSearchSuggestion /WebSearchSuggestionSource to EnableSuggestion /Suggestion |
SettingsControl.xaml.cs | Minor code formatting fix for brace placement |
SettingsControl.xaml | Removes unused style, updates binding from element name to Settings property, removes workaround comment |
Settings.cs | Implements proper property with backing field and OnPropertyChanged notification, removes unused browser-related properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Thanks for your contribution!
The
OnPropertyChanged
was not triggered, that is why there was a workaround for binding Settings.EnableSuggestion in the WebSearch plugin.