Mfcttrf

By hashing together various online tutorials I've constructed my first MVVM application that lets users navigate between "tabs" and a "tab" (ColorList_Tab) that performs basic list functions such as adding, editing, deleting and saving colors.

A new tab can be addded by adding a new data context to the MainWindow resources and then adding the viewmodel to PageViewModels list in the MainWindowViewModel. This tab will then be represented by its Icon property from inheriting from IPageViewModel.

ColorList_Tab supports adding, editing, deleting and saving colors to a text file in a Json format. Two textboxes are used to input the color name and hex, add and edit mode can be switched between by clicking on the AddSwitchCommand and EditSwitchCommand buttons. The execute command then executes the selected mode. Edit and delete will perform the function upon the currently selected item in the Color List, with every change being saved to a text file in Json form. SampleColorCommand sets the InputHexString to the color of cursor on screen.

Besides general critique and corrections, I was hoping a few of my questions could be addressed:

1. Is MainWindowViewModel a good way of implementing navigation in MVVM?




2. Currently my App.Xaml.Cs is unused however I've seen examples where it is used to bind the viewmodel, which should I use?




3. Have I correctly placed the AddColorItem, EditColorItem and DeleteColorItem methods in the Model?




4. Should ColorClass use try and catch to create the brush or should an if else statement be used with a regex check on the hex code?

MainWindow:

<Window x:Class="MVVM_Color_Utilities.MainWindow"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
 xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
 xmlns:materialDesign="http://materialdesigninxaml.net/winfx/xaml/themes"
 xmlns:local="clr-namespace:MVVM_Color_Utilities"
 mc:Ignorable="d"

 xmlns:ViewModel ="clr-namespace:MVVM_Color_Utilities.ViewModel"
 xmlns:ColorList="clr-namespace:MVVM_Color_Utilities.ColorsList_Tab"

 Title="MainWindow" Height="600" Width="1000"
 WindowStartupLocation="CenterScreen" Background="LightGray"
 WindowStyle="None" AllowsTransparency="True">

 <Window.DataContext>
 <ViewModel:MainWindowViewModel/>
 </Window.DataContext>

 <Window.Resources>
 <DataTemplate DataType="x:Type ColorList:ColorListViewModel">
 <ColorList:ColorListView/>
 </DataTemplate>
 </Window.Resources>

 <Grid>
 <Grid.ColumnDefinitions>
 <ColumnDefinition Width="70"/>
 <ColumnDefinition Width="*"/>
 </Grid.ColumnDefinitions>
 <Grid.RowDefinitions>
 <RowDefinition Height="70"/>
 <RowDefinition Height="*"/>
 </Grid.RowDefinitions>

 <Rectangle Grid.Column="1" Fill="StaticResource BurgundyBrush" />
 <Rectangle Grid.RowSpan="2" Fill="StaticResource CyanTerracottaBrush" Margin="0,0,-0.5,0"/>

 <ItemsControl Grid.Row="1" ItemsSource="Binding PageViewModels" HorizontalAlignment="Stretch">
 <ItemsControl.ItemTemplate>
 <DataTemplate>
 <Button Height="70"
 Command="Binding DataContext.ChangePageCommand, RelativeSource=RelativeSource AncestorType=x:Type Window"
 CommandParameter="Binding ">
 <materialDesign:PackIcon Kind="Binding Icon" Height="30" Width="30" />
 </Button>
 </DataTemplate>
 </ItemsControl.ItemTemplate>
 </ItemsControl>

 <Rectangle Grid.Column="1" Fill="StaticResource BurgundyBrush" Name="DragWindow" MouseDown="DragWindow_MouseDown"/>
 <ContentControl Grid.Column="1" Grid.RowSpan="2" Content="Binding CurrentPageViewModel" />

 <!--#region Window Controls-->
 <StackPanel Grid.Column="1" Orientation="Horizontal" HorizontalAlignment="Right" VerticalAlignment="Top" Height="30" x:Name="Stack" Margin="7,0,0,0">
 <!--Minimise-->
 <Button Click="MinimizeWindowButton_Click" x:Name="MinimizeWindowButton">
 <materialDesign:PackIcon Kind="Remove" Background="x:Null" BorderBrush="x:Null"/>
 </Button>
 <!--Toggle Window State-->
 <Button Click="ChangeWindowState_Click" x:Name="ChangeWindowState" >
 <materialDesign:PackIcon Kind="CropSquare" Background="x:Null" BorderBrush="x:Null"/>
 </Button>
 <!--Close Window-->
 <Button Click="CloseWindow_Click" Height="Auto" x:Name="CloseWindow" >
 <materialDesign:PackIcon Kind="Close" Background="x:Null" BorderBrush="x:Null"/>
 </Button>
 </StackPanel>
<!--#endregion-->
 </Grid>
</Window>

MainWindowViewModel:

using System.Collections.Generic;
using System.Linq;
using System.Windows.Input;
using MVVM_Color_Utilities.ViewModel.Helper_Classes;

namespace MVVM_Color_Utilities.ViewModel

 public class MainWindowViewModel : ObservableObject
 
 #region Fields
 private ICommand _changePageCommand;
 private IPageViewModel _currentPageViewModel;
 private List<IPageViewModel> _pageViewModels;
 #endregion

 #region Constructors
 public MainWindowViewModel()
 
 PageViewModels.Add(new ColorsList_Tab.ColorListViewModel());
 //PageViewModels.Add(new ImageQuantizer_Tab.ImageQuantizerViewModel());

 CurrentPageViewModel = PageViewModels[0];
 
 #endregion
 public ICommand ChangePageCommand
 
 get
 
 if (_changePageCommand == null)
 
 _changePageCommand = new RelayCommand(
 p => ChangeViewModel((IPageViewModel)p),
 p => p is IPageViewModel);
 
 return _changePageCommand;
 
 
 public List<IPageViewModel> PageViewModels
 
 get
 
 if (_pageViewModels == null)
 _pageViewModels = new List<IPageViewModel>();
 return _pageViewModels;
 
 
 public IPageViewModel CurrentPageViewModel
 
 get
 
 return _currentPageViewModel;
 
 set
 
 if (_currentPageViewModel != value)
 
 _currentPageViewModel = value;
 OnPropertyChanged("CurrentPageViewModel");
 
 
 
 private void ChangeViewModel(IPageViewModel viewModel)
 
 if (!PageViewModels.Contains(viewModel))
 
 PageViewModels.Add(viewModel);
 

 CurrentPageViewModel = PageViewModels
 .FirstOrDefault(vm => vm == viewModel);
 
 

IPageViewModel Interface
(Sets the icon for each viewmodel for display in the main window):

using MaterialDesignThemes.Wpf;

namespace MVVM_Color_Utilities.ViewModel.Helper_Classes

 public interface IPageViewModel
 
 PackIconKind Icon get; 
 

ColorList_Tab View:

<UserControl x:Class="MVVM_Color_Utilities.ColorsList_Tab.ColorListView"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
 xmlns:d="http://schemas.microsoft.com/expression/blend/2008" 
 xmlns:local="clr-namespace:MVVM_Color_Utilities.ColorsList_Tab"
 xmlns:materialDesign="http://materialdesigninxaml.net/winfx/xaml/themes"
 mc:Ignorable="d" 
 >
 <UserControl.InputBindings>
 <KeyBinding Key="Enter" Command="Binding ExecuteCommand"/>
 <KeyBinding Modifiers="Ctrl" Key="D" Command="Binding DeleteItem"/>
 <KeyBinding Modifiers="Ctrl" Key="A" Command="Binding AddSwitchCommand"/>
 <KeyBinding Modifiers="Ctrl" Key="E" Command="Binding EditSwitchCommand"/>
 <KeyBinding Modifiers="Ctrl" Key="Q" Command="Binding SampleColorCommand"/>
 </UserControl.InputBindings>

 <Grid>
 <Grid.RowDefinitions>
 <RowDefinition Height="70"/>
 <RowDefinition Height="50"/>
 <RowDefinition Height="*"/>
 </Grid.RowDefinitions>

 <Grid Width="270" HorizontalAlignment="Left">
 <Grid.ColumnDefinitions>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="*"/>
 </Grid.ColumnDefinitions>

<!--This changes the "background" of the buttons as setting the background of buttons 
 while using material design causes an ugly shadow effect-->
 <Rectangle Grid.Column="0" >
 <Rectangle.Style>
 <Style TargetType="Rectangle">
 <Setter Property="Fill" Value="StaticResource BurgundyBrush"/>
 <Style.Triggers>
 <DataTrigger Binding="Binding AddingModeBool" Value="True">
 <Setter Property="Fill" Value="StaticResource BurgundyLightBrush"/>
 </DataTrigger>
 </Style.Triggers>
 </Style>
 </Rectangle.Style>
 </Rectangle>

 <Rectangle Grid.Column="1">
 <Rectangle.Style>
 <Style TargetType="Rectangle">
 <Setter Property="Fill" Value="StaticResource BurgundyLightBrush"/>
 <Style.Triggers>
 <DataTrigger Binding="Binding AddingModeBool" Value="True">
 <Setter Property="Fill" Value="StaticResource BurgundyBrush"/>
 </DataTrigger>
 </Style.Triggers>
 </Style>
 </Rectangle.Style>
 </Rectangle>

 <Button Grid.Column="0" Command="Binding Path=AddSwitchCommand">
 <materialDesign:PackIcon Kind="Add"/>
 </Button>
 <Button Grid.Column="1" Command="Binding Path=EditSwitchCommand" >
 <materialDesign:PackIcon Kind="Edit" />
 </Button>
 <Button Grid.Column="2" Command="Binding Path=DeleteItem">
 <materialDesign:PackIcon Kind="Delete" VerticalAlignment="Center"/>
 </Button>
 </Grid>

<!--#region ListBox Header-->
 <Grid Grid.Row="1" Background="StaticResource BurgundyLightBrush" >
 <Grid.ColumnDefinitions>
 <ColumnDefinition Width="0.8*"/>
 <ColumnDefinition Width="1.5*"/>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="0.7*"/>
 <ColumnDefinition Width="1.2*"/>
 </Grid.ColumnDefinitions>
 <Grid.Resources>
 <Style TargetType="TextBox">
 <Setter Property="Background" Value="StaticResource BurgundyFaintBrush"/>
 <Setter Property="Height" Value="20"/>
 </Style>
 </Grid.Resources>

 <Rectangle Height="18" Width="60" Fill="White"/>
 <Rectangle Height="18" Width="60" Fill="Binding IndicatorBrush" Stroke="Gray" StrokeThickness="0.4"/>

 <TextBox Grid.Column="1" Width="160" Text="Binding InputName, UpdateSourceTrigger=PropertyChanged" />
 <TextBox Grid.Column="2" Width="80" Text="Binding InputHex, UpdateSourceTrigger=PropertyChanged"/>

 <Button Grid.Column="4" Command="Binding Path=SampleColorCommand">
 <materialDesign:PackIcon Kind="Colorize" Width="40"/>
 </Button>
 <Button Grid.Column="5" Command="Binding Path=ExecuteCommand">
 <materialDesign:PackIcon Kind="done" Width="100"/>
 </Button>
 </Grid>
 <!--#endregion-->

<!--#region ListBox-->
 <ListBox Grid.Row="2" ItemsSource="Binding ColorListSource" SelectedIndex="Binding SelectedItemIndex">
 <ListBox.ItemContainerStyle>
 <Style TargetType="ListBoxItem" BasedOn="StaticResource x:Type ListBoxItem">
 <Setter Property="HorizontalContentAlignment" Value="Stretch"/>
 <Style.Triggers>
 <MultiTrigger>
 <MultiTrigger.Conditions>
 <Condition Property="IsSelected" Value="true"/>
 <Condition Property="Selector.IsSelectionActive" Value="true"/>
 </MultiTrigger.Conditions>
 <Setter Property="Background" Value="Gray"/>
 </MultiTrigger>
 </Style.Triggers>
 </Style>
 </ListBox.ItemContainerStyle>
 <ListBox.ItemTemplate>
 <DataTemplate>
 <Grid
 >
 <Grid.ColumnDefinitions>
 <ColumnDefinition Width="0.8*"/>
 <ColumnDefinition Width="1.5*"/>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="*"/>
 <ColumnDefinition Width="0.7*"/>
 <ColumnDefinition Width="1.2*"/>
 </Grid.ColumnDefinitions>
 <Rectangle Height="18" Width="60" Fill="Binding SampleBrush" Stroke="Gray" StrokeThickness="0.4"/>
 <TextBlock Text="Binding Name" Width="160" Grid.Column="1"/>
 <TextBlock Text="Binding Hex" Width="70" Grid.Column="2"/>
 </Grid>
 </DataTemplate>
 </ListBox.ItemTemplate>
 </ListBox>
<!--#endregion-->
 </Grid>
</UserControl>

ColorList_Tab ViewModel:

using System.Collections.ObjectModel; 
using MVVM_Color_Utilities.ViewModel.Helper_Classes;
using MaterialDesignThemes.Wpf;
using System.Windows.Media;
using System.Windows.Input;
using MVVM_Color_Utilities.Helpers;
using System.Text.RegularExpressions;

namespace MVVM_Color_Utilities.ColorsList_Tab

 class ColorListViewModel : ObservableObject, IPageViewModel
 
 private readonly ColorListModel model = new ColorListModel();
 private Regex _hexReg = new Regex("^#([0-9a-fA-F]0,8)?$"); //"^#(?:(?:[0-9a-fA-F]3)1,2

Observable Objects (not my code)

using System.ComponentModel;

namespace MVVM_Color_Utilities.ViewModel.Helper_Classes

 public abstract class ObservableObject : INotifyPropertyChanged
 
 public event PropertyChangedEventHandler PropertyChanged;

 protected void OnPropertyChanged(string propertyName)
 
 var handler = PropertyChanged;
 if (PropertyChanged != null)
 
 handler(this, new PropertyChangedEventArgs(propertyName));
 
 
 

RelayCommand (not my code)
:

using System;
using System.Diagnostics;
using System.Windows.Input;

namespace MVVM_Color_Utilities.ViewModel.Helper_Classes

 public class RelayCommand : ICommand
 
 #region Fields
 readonly Action<object> _execute;
 readonly Predicate<object> _canExecute;
 #endregion // Fields

 #region Constructors
 public RelayCommand(Action<object> execute)
 : this(execute, null)
 
 
 public RelayCommand(Action<object> execute, Predicate<object> canExecute)
 
 if (execute == null)
 
 throw new ArgumentNullException("execute");
 
 _execute = execute;
 _canExecute = canExecute;
 

 #endregion // Constructors

 #region ICommand Members

 [DebuggerStepThrough]
 public bool CanExecute(object parameters)
 
 return _canExecute == null ? true : _canExecute(parameters);
 

 public event EventHandler CanExecuteChanged
 
 add CommandManager.RequerySuggested += value; 
 remove CommandManager.RequerySuggested -= value; 
 

 public void Execute(object parameters)
 
 _execute(parameters);
 

 #endregion // ICommand Members
 

ColorList_Tab Model:

using System.IO;
using System.Collections.ObjectModel;
using System.Windows.Media;
using Newtonsoft.Json;

namespace MVVM_Color_Utilities.ColorsList_Tab

 public class ColorListModel
 
 #region Fields
 private readonly static string projectPath = Directory.GetParent(Directory.GetCurrentDirectory()).Parent.FullName; //Get Path of ColorItems file
 private readonly static string colorsFilePath = projectPath + "/Resources/ColorItemsList.txt";
 #endregion

 #region Properties
 public ObservableCollection<ColorClass> ColorClassList get; 
 =JsonConvert.DeserializeObject<ObservableCollection<ColorClass>>(File.ReadAllText(colorsFilePath));

 public int NextID
 
 get
 
 return ColorClassList.Count > 0 ? ColorClassList[0].ID + 1 : 0;
 
 
 #endregion

 #region Methods
 private void SaveColorsList()
 
 try
 
 File.WriteAllText(colorsFilePath, JsonConvert.SerializeObject(ColorClassList));
 
 catch 
 
 public void AddColorItem(int index,string hexString, string nameString)
 
 if(ColorClassList.Count > index 
 public void EditColorItem(int index,string hexString, string nameString)
 
 if (ColorClassList.Count > index && ColorClassList.Count > 0)
 
 ColorClassList[index] = new ColorClass(NextID, hexString, nameString);
 SaveColorsList();
 
 
 public void DeleteColorItem(int index)
 
 if (ColorClassList.Count > index &&ColorClassList.Count>0)
 
 ColorClassList.RemoveAt(index);
 SaveColorsList();
 
 
 #endregion
 

ColorClass:

 public class ColorClass
 
 public ColorClass(int id, string hex, string name)
 
 ID = id;
 Name = name;
 Hex = hex;
 

 public int ID get; set; 
 public string Hex get; set; 
 public string Name get; set; 

 public SolidColorBrush SampleBrush
 
 get
 
 Color color;
 try
 
 color = (Color)ColorConverter.ConvertFromString(Hex);
 
 catch//Invalid hex defaults to white.
 
 color = (Color)ColorConverter.ConvertFromString("#FFFF");
 Hex = "#FFFF";
 
 return new SolidColorBrush(color);
 
 
 

Preface

There is a lot to review. This review is focused on the parts that are not related to the View or ViewModel.

Note: You have also edited the question concurrently with me making this review to state ObservableObject and RelayCommand are third-party classes. That's fine by me, since I still wanted to point out to you you shouldn't just copy these classes from internet and use them without any changes or proper consideration.

ColorListModel

You cannot reuse this class in scenarios where the paths are different than below. Also, when changing paths, you'd have to update this code with it. Consider reading these paths from a settings or configuration file.

private readonly static string projectPath 
 = Directory.GetParent(Directory.GetCurrentDirectory())...
private readonly static string colorsFilePath 
 = projectPath + "/Resources/ColorItemsList.txt";

Consider using the lazy pattern to load properties on first demand to avoid unnecessary resources when not required: property ColorClassList.

SaveColorsList swallows all exceptions. At least log something or return a bool in sandbox mode if you don't like this method to throw exceptions.

AddColorItem, EditColorItem and DeleteColorItem only execute when the index is in bounds. The caller does not get feedback about out-of-bounds. Throw an exception or return a bool to let caller handle edge cases. Furthermore, AddColorItem does not use index as it stores on index 0 instead. Is this as designed?

(third-party code)

ObservableObject

This class is a design decision that I would challenge. It provides insufficient context to be a good candidate for a base class for other classes. I put it in the list of classes as ComparableBase, EquatableBase, DisposableBase. Think about whether a common base clase is really helpful here.

In addition, this class provides a public event PropertyChanged but never disposes it. Even if the WPF framework is able to subscribe and unsubscribe correctly from it, your own code-behind and other application logic is also allowed to subscribe. Classes that provide events should in my opinion always implement IDisposable.

RelayCommand

This is a famous allround command. I think it originates from Telerik, but several other variants are out there as well (DelegateCommand for instance). I would mention the use of third-party code in a question so we know how to review it.

This pattern with Action<object> execute is contravariant, but since object is the deepest base class, it's not that useful. For instance, you cannot exchange execute with an Action<string>. For this reason, consider creating also a RelayCommand<T> that accepts Action<T> execute. This class is more usable for code-behind and other application logic.

The predicate parameter should be made optional. You might also like to create an AsyncRelayCommand<T> with a composite CancelCommand (Example).