Refactor ShellSettings with Binding & Fix IsEnabled logic #4023
Conversation
prlabeler
bot
added
bug
There was a problem hiding this comment.
Pull Request Overview
Refactors the ShellSettings UI component to use modern WPF data binding patterns instead of event handlers, and fixes the IsEnabled logic for shell options that don't apply to RunCommand.
- Replaces event-driven UI updates with MVVM pattern using data binding
- Implements proper property change notifications in Settings class
- Adds converter to handle conditional enabling of shell options based on command type
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ShellSetting.xaml.cs | Replaces old event-based code-behind with clean ViewModel instantiation |
| ShellSetting.xaml | Updates XAML to use data binding instead of event handlers and adds converter for conditional enabling |
| ShellSettingViewModel.cs | New ViewModel implementing proper property binding and mutual exclusion logic |
| Settings.cs | Converts auto-properties to full properties with change notifications and adds localization attributes |
| Main.cs | Adds namespace import for Views |
| LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs | New converter for determining when shell options should be enabled |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
🥷 Code experts: jjw24 Jack251970, jjw24 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors the Shell plugin UI to MVVM: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant V as CMDSetting (View)
participant VM as ShellSettingViewModel
participant S as Settings
participant C as LeaveShellOpenOrCloseShellAfterPressEnabledConverter
U->>V: Toggle "CloseShellAfterPress" checkbox
V->>VM: Bound property updated (CloseShellAfterPress)
VM->>S: Update Settings.CloseShellAfterPress
VM->>VM: Enforce mutual exclusion -> set LeaveShellOpen = false
VM-->>V: PropertyChanged notifications
V->>C: MultiBinding inputs (CloseShellAfterPress, SelectedShell)
C-->>V: Return IsEnabled for dependent controls
sequenceDiagram
autonumber
participant U as User
participant V as CMDSetting (View)
participant VM as ShellSettingViewModel
participant S as Settings
U->>V: Select Shell from ComboBox
V->>VM: SelectedShell bound value updates
VM->>S: Settings.Shell = selected
VM-->>V: PropertyChanged -> view updates (converter re-evaluates)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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). (8)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (1)
226-231: Stray “/c” after pause in cmd path
pausedoesn’t take/c; the trailing/clikely prints “Invalid switch - /c”. Remove it.- $"{(_settings.CloseShellAfterPress ? - $" && echo {notifyStr} && pause > nul /c" : + $"{(_settings.CloseShellAfterPress ? + $" && echo {notifyStr} && pause > nul" : "")}");
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.Shell/Converters/LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs (1)
9-19: Converter logic matches requirementsEnables only when the other option is false and shell != RunCommand. Meets “disable when RunCommand” objective.
For safer defaults during binding initialization, consider returning
falseinstead ofBinding.DoNothingso controls start disabled until values are ready.Plugins/Flow.Launcher.Plugin.Shell/Main.cs (2)
132-136: Guard against zero/invalid history limitIf ShowOnlyMostUsedCMDsNumber is 0, users get no history. Clamp to a minimum of 1 when limiting is enabled.
Apply this diff in both places:
- if (_settings.ShowOnlyMostUsedCMDs) - return [.. history.Take(_settings.ShowOnlyMostUsedCMDsNumber)]; + if (_settings.ShowOnlyMostUsedCMDs) + { + var n = Math.Max(1, _settings.ShowOnlyMostUsedCMDsNumber); + return [.. history.Take(n)]; + }Also applies to: 185-189
66-68: Duplicate predicate in autocomplete filterThe
results.Any(p => o.Equals(p.Title, ...))check is repeated twice. Drop one for clarity.- .Where(o => o.StartsWith(cmd, StringComparison.OrdinalIgnoreCase) && - !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase)) && - !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase))).ToList(); + .Where(o => o.StartsWith(cmd, StringComparison.OrdinalIgnoreCase) && + !results.Any(p => o.Equals(p.Title, StringComparison.OrdinalIgnoreCase))).ToList();Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs (1)
10-14: Remove redundant DataContext assignment
DataContext = viewModel;is set twice. Keep one (before InitializeComponent is fine if XAML bindings depend on it).- DataContext = viewModel; - InitializeComponent(); - DataContext = viewModel; + DataContext = viewModel; + InitializeComponent();Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (1)
74-77: Fail fast on nullsettings
Please guard the constructor argument so we fail fast with a descriptiveArgumentNullExceptioninstead of running into harder-to-debug null dereferences later.- public ShellSettingViewModel(Settings settings) - { - Settings = settings; - } + public ShellSettingViewModel(Settings settings) + { + Settings = settings ?? throw new ArgumentNullException(nameof(settings)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Plugins/Flow.Launcher.Plugin.Shell/Converters/LeaveShellOpenOrCloseShellAfterPressEnabledConverter.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Shell/Main.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Shell/Settings.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Shell/ShellSetting.xaml.cs(0 hunks)Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml(2 hunks)Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Plugins/Flow.Launcher.Plugin.Shell/ShellSetting.xaml.cs
🧰 Additional context used
🧬 Code graph analysis (3)
Plugins/Flow.Launcher.Plugin.Shell/Settings.cs (1)
Plugins/Flow.Launcher.Plugin.Sys/Settings.cs (2)
Settings(6-127)Settings(8-14)
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml.cs (1)
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (2)
ShellSettingViewModel(5-78)ShellSettingViewModel(74-77)
Plugins/Flow.Launcher.Plugin.Shell/ViewModels/ShellSettingViewModel.cs (1)
Plugins/Flow.Launcher.Plugin.Shell/Main.cs (4)
List(29-97)List(99-136)List(163-189)List(440-480)
⏰ 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). (6)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Shell/Settings.cs (3)
6-20: INotifyPropertyChanged migration looks goodAdopting BaseModel with change notifications across settings is consistent and enables clean bindings.
64-76: RunAsAdministrator default is true — confirm intentDefaulting to elevation changes behavior and UX. If previous default was false, this is a notable change. Please confirm this is intentional for existing users/settings migrations.
131-145: Enum localization annotations — LGTMAttributes on Shell enum align with the new localization pipeline.
Plugins/Flow.Launcher.Plugin.Shell/Views/ShellSetting.xaml (2)
41-47: IsEnabled MultiBinding + converter — LGTMCorrectly disables both options when RunCommand is selected and enforces mutual exclusivity at the UI level.
Also applies to: 55-61
81-84: Bindings for Shell and “Most Used” number — solidData-driven shells with SelectedValuePath=Value is clean. The number ComboBox wiring is correct; once Settings defaults to a valid value (e.g., 5), selection will populate as expected.
If you keep 0 as a persisted value from older configs, consider normalizing it on load to 5 to avoid a blank selection.
Also applies to: 95-97
Co-authored-by: Copilot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This PR contains a TODO statement. Please check to see if they should be removed. |
CHANGES
Bindinginstead of event handler to handle UI logic.Flow.Launcher.Localizationto improve code quality.IsEnabledlogic: When command type isRunCommand, these two options cannot take effect -Close Command Prompt after pressing any key&Do not close Command Prompt after command execution- so we should disabled these two options.ShowOnlyMostUsedCMDsNumberdefault value issue: Since the old Settings class set default value ofShowOnlyMostUsedCMDsNumberto 0 which is a wrong value, we need to fix it here to make sure the default value is 5TEST
Setting panel can work well