Benjaming/mvvm#21
Conversation
…ow list item selection is tracked in the view model
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive migration to MVVM architecture for options panels in the ProfileExplorer application. The changes replace the existing code-behind event handling with proper MVVM ViewModels using the CommunityToolkit.Mvvm framework.
Key Changes
- Added CommunityToolkit.Mvvm NuGet package for MVVM infrastructure
- Created ViewModels for all options panels with proper two-way data binding
- Introduced new MVVM-compatible interface (
IMvvmOptionsPanel) alongside existing architecture - Updated XAML files to use ViewModel properties and commands instead of event handlers
Reviewed Changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ProfileExplorerUI.csproj | Added CommunityToolkit.Mvvm package reference |
| OptionsPanelHostPopup.xaml.cs | Added MVVM support with CreateMvvm method and wrapper infrastructure |
| OptionsPanelBase.cs | Introduced IMvvmOptionsPanel interface for gradual MVVM migration |
| ViewModels/*.cs | Created comprehensive ViewModels for all options panels with proper data binding |
| Services/*.cs | Added service layer for UI operations (dialogs, file operations, etc.) |
| *.xaml.cs files | Converted code-behind to use ViewModels instead of direct event handling |
| *.xaml files | Updated data bindings to use ViewModel properties with underscore suffix |
| FunctionMarkingSettings.cs | Made Markings record public and added methods for MVVM support |
| ProfileLoadWindow.xaml.cs | Replaced direct method call with comment indicating data binding handles reload |
| Various panel files | Updated to use CreateMvvm method for MVVM-compatible panels |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| while (SelectedSymbolPathIndex_ != -1) { | ||
| SymbolPaths_.RemoveAt(SelectedSymbolPathIndex_); |
There was a problem hiding this comment.
This creates an infinite loop. The condition != -1 should be >= 0 and the loop should decrement or use a different approach. The current logic will loop indefinitely if SelectedSymbolPathIndex_ is not -1.
| while (SelectedSymbolPathIndex_ != -1) { | |
| SymbolPaths_.RemoveAt(SelectedSymbolPathIndex_); | |
| if (SelectedSymbolPathIndex_ >= 0 && SelectedSymbolPathIndex_ < SymbolPaths_.Count) { | |
| SymbolPaths_.RemoveAt(SelectedSymbolPathIndex_); | |
| // Update selection: select previous item, or -1 if list is empty | |
| if (SymbolPaths_.Count == 0) { | |
| SelectedSymbolPathIndex_ = -1; | |
| } else if (SelectedSymbolPathIndex_ >= SymbolPaths_.Count) { | |
| SelectedSymbolPathIndex_ = SymbolPaths_.Count - 1; | |
| } |
| private void SetLongCallStackPopupDuration() { | ||
|
|
||
| } |
There was a problem hiding this comment.
The method body is empty. This command method should set the popup duration to a long value, similar to other duration setter methods in the codebase.
| ItemsSource="{Binding StyleButtonContextMenuItems_, Mode=TwoWay}"> | ||
| <MenuItem Header="Default" /> | ||
| <Style TargetType="{x:Type MenuItem}"> | ||
| <Setter Property="Command" Value="{Binding RelativeSource={RelativeSource AncestorType={x:Type ContextMenu}}, Path=PlacemantTarget.DataContext.ItemChanged }"/> |
There was a problem hiding this comment.
There's a spelling error: 'PlacemantTarget' should be 'PlacementTarget'.
| <Setter Property="Command" Value="{Binding RelativeSource={RelativeSource AncestorType={x:Type ContextMenu}}, Path=PlacemantTarget.DataContext.ItemChanged }"/> | |
| <Setter Property="Command" Value="{Binding RelativeSource={RelativeSource AncestorType={x:Type ContextMenu}}, Path=PlacementTarget.DataContext.ItemChanged }"/> |
|
|
||
| [RelayCommand] | ||
| private void RemoveMappedPath() { | ||
| var toRemove = FilePathMappings_.Where(p => p.IsSelected_).ToList(); |
There was a problem hiding this comment.
Inconsistent indentation. The line should be aligned with the method declaration above it.
| var toRemove = FilePathMappings_.Where(p => p.IsSelected_).ToList(); | |
| var toRemove = FilePathMappings_.Where(p => p.IsSelected_).ToList(); |
| public void PanelResetting() { } | ||
| public void PanelAfterReset() { } | ||
|
|
||
| public static OptionsPanelHostPopup Create<T, S>(SettingsBase settings, FrameworkElement relativeControl, |
There was a problem hiding this comment.
Might be good to add something like [Obsolete("This method now exists primarily for legacy panels not migrated to MVVM. Use New panels should use CreateMvvm() when possible.")] over this function, just to make a note that future development probably should be using the MVVM variant instead?
There was a problem hiding this comment.
True, I think I've transitioned all the options panels though if I remember correctly. This PR could also perform any final refactoring to remove this obsolete method
There was a problem hiding this comment.
Finishing the refactor might make the most sense imo. From a quick search, there's only one(?) instance of the Create() function still being used, in the SourceFilePanel
| } | ||
|
|
||
| private record Markings(FunctionMarkingSet Current, List<FunctionMarkingSet> Saved); | ||
| public record Markings(FunctionMarkingSet Current, List<FunctionMarkingSet> Saved); |
There was a problem hiding this comment.
Should this still be private?
No description provided.