🧱 Mason: [structural improvement] Refactor CharacterReceived out of code-behind#385
🧱 Mason: [structural improvement] Refactor CharacterReceived out of code-behind#385google-labs-jules[bot] wants to merge 5 commits into
Conversation
… to ViewModel - What: Moved `Grid_CharacterReceived` logic out of `MainWindow.xaml.cs` into `MainViewModel.cs` using a new `CharacterReceivedCommand` attached property. - Why: Removes business logic (updating filter state) from the View's code-behind, addressing technical debt and keeping the View tightly focused on layout. - Architecture: Employs an attached property for command binding and utilizes a ViewModel event (`SearchFocusRequested`) that the View subscribes to for handling purely UI-specific operations (setting focus). This maintains a strict MVVM separation where the ViewModel does not reference any WinUI `UIElement`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
… to ViewModel - What: Moved `Grid_CharacterReceived` logic out of `MainWindow.xaml.cs` into `MainViewModel.cs` using a new `CharacterReceivedCommand` attached property. - Why: Removes business logic (updating filter state) from the View's code-behind, addressing technical debt and keeping the View tightly focused on layout. - Architecture: Employs an attached property for command binding and utilizes a ViewModel event (`SearchFocusRequested`) that the View subscribes to for handling purely UI-specific operations (setting focus). This maintains a strict MVVM separation where the ViewModel does not reference any WinUI `UIElement`. Fixes whitespace format errors.
…_CharacterReceived #341 added SearchBox_KeyDown for Enter/Down-arrow navigation. This PR replaces Grid_CharacterReceived with a CharacterReceivedCommand attached property. Both changes are kept; only the code-behind handler that was superseded by the attached property is dropped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| public static readonly DependencyProperty CharacterReceivedCommandProperty = |
There was a problem hiding this comment.
Formatting violation — will break CI.
DependencyProperty.RegisterAttached( is at the same indentation level as the field declaration instead of being indented 4 more spaces. dotnet format --verify-no-changes will fail on this.
| public static readonly DependencyProperty CharacterReceivedCommandProperty = | |
| public static readonly DependencyProperty CharacterReceivedCommandProperty = | |
| DependencyProperty.RegisterAttached( | |
| "CharacterReceivedCommand", | |
| typeof(ICommand), | |
| typeof(ListViewBaseExtensions), | |
| new PropertyMetadata(null, OnCharacterReceivedCommandPropertyChanged)); |
| @@ -85,6 +92,44 @@ private static void OnItemClick(object sender, ItemClickEventArgs e) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Formatting — double blank line. There is one blank line here already (end of OnItemClick); the extra blank added by this PR creates a double blank line that dotnet format will reject. Remove the extra blank.
| **Learning:** In WinUI 3, `ListViewBase` controls (like `GridView`) do not natively fire the `ItemClick` event on an `Enter` key press, even when `IsItemClickEnabled` is true. To implement MVVM-friendly keyboard navigation without code-behind, an attached property can subscribe to the `KeyDown` event, explicitly check for `VirtualKey.Enter`, resolve the focused item via `FocusManager.GetFocusedElement(xamlRoot)`, and execute an `ICommand` using the focused element's `DataContext`. | ||
| **Action:** When implementing keyboard navigation for item selection in WinUI 3 `ListViewBase` controls, avoid code-behind event handlers. Instead, extend or create attached properties (like `ListViewBaseExtensions.CommandProperty`) that handle both `ItemClick` and `KeyDown` events to provide a unified MVVM integration point. | ||
|
|
||
| ## 2023-10-27 - UI Focus in MVVM via Events |
There was a problem hiding this comment.
Wrong date. 2023-10-27 is nearly 3 years in the past and inconsistent with the adjacent 2025-03-01 entry. CLAUDE.md explicitly requires running date +%Y-%m-%d before writing dates to files — the current date is 2026-05-11.
| ## 2023-10-27 - UI Focus in MVVM via Events | |
| ## 2026-05-11 - UI Focus in MVVM via Events |
| [RelayCommand] | ||
| private void CharacterReceived(string? character) | ||
| { | ||
| if (string.IsNullOrEmpty(character)) return; | ||
| FilterText += character; | ||
| SearchFocusRequested?.Invoke(this, EventArgs.Empty); | ||
| } |
There was a problem hiding this comment.
Missing tests. The new CharacterReceivedCommand handler is testable (it's pure ViewModel logic — string append + event raise) and MainViewModel.cs is already file-linked into the test project. No tests were added, which risks a regression against the 80% line-coverage threshold in CI.
At minimum, please add tests covering:
- A printable character appends to
FilterTextand firesSearchFocusRequested - A null/empty argument is a no-op (guard clause path)
- A
char.IsControlcharacter is filtered out by the attached-property layer (can be tested by calling the command directly with a control-character string and verifying no state change)
Code Review — PR #385: Refactor CharacterReceived out of code-behindWhat this PR doesMoves the Overall assessmentThe architectural direction is sound and the MVVM split is handled correctly. The There are two blocking issues that will fail CI checks before this can merge: Blocking issues1. Indentation on 2. Double blank line in Should-fix issues3. Wrong date in 4. Missing tests for No issues found
@jules please address the issues noted above |
I have addressed all the issues raised:
Everything should be good to go now! |
💡 What: Moved the
Grid_CharacterReceivedevent handler out ofMainWindow.xaml.csand replaced it with aCharacterReceivedCommandinMainViewModel. The View now uses a new attached property inListViewBaseExtensionsto bind the event to the command.🎯 Why: To eliminate code-behind business logic. The
Grid_CharacterReceivedhandler was directly mutating theViewModel.FilterTextproperty, which violates strict MVVM separation.🏗️ Architecture: The solution introduces an attached property to bridge the XAML event to the ViewModel cleanly. For the purely UI-specific task of focusing the
SearchBoxafter receiving input, the ViewModel raises a standard .NETEventHandler(SearchFocusRequested). The View (MainWindow.xaml.cs) subscribes to this event to executeSearchBox.Focus(), maintaining zero knowledge of WinUI classes within the ViewModel.PR created automatically by Jules for task 8777854732081602518 started by @mikekthx