Problem
Continuation of Correct MVVM format. This is what I have done so far, and of course it works:
To start with, this is my MainPage.xaml:
<Grid Background="White">
<Grid.RowDefinitions>
<RowDefinition Height="100" x:Name="TitleRow"/>
<RowDefinition Height="*" x:Name="DataRow"/>
</Grid.RowDefinitions>
<Grid.ColumnDefinitions>
<ColumnDefinition Width="300" x:Name="ItemsColumn"/>
<ColumnDefinition Width="*" x:Name="DataColumn"/>
</Grid.ColumnDefinitions>
<ListBox Background="WhiteSmoke" Name="Items" Grid.Column="0" Grid.RowSpan="2" ItemsSource="{Binding ItemTitles}" SelectionChanged="OnSelectionChanged"
Margin="-2,-2,0,-2" Padding="0,10" />
<Viewbox Name="TitleView" Margin="10" VerticalAlignment="Center" HorizontalAlignment="Left" Grid.Column="1">
<TextBlock Name="TitleText" Foreground="Black" Margin="5,10,5,5"
Text="{Binding SelectedValue, ElementName=Items}" />
</Viewbox>
<Border Grid.Column="1" BorderBrush="Black" BorderThickness="0,1,0,0" VerticalAlignment="Bottom" Height="1"/>
<Frame Grid.Column="1" Grid.Row="1" Foreground="Black" FontSize="20" Margin="20,20,0,20" Name="DataFrame" VerticalAlignment="Top" />
</Grid>
This is the code-behind – MainPage.xaml.cs:
public sealed partial class MainPage : Page
{
private ViewModel Data = new ViewModel();
public MainPage()
{
this.InitializeComponent();
this.DataContext = Data;
Items.SelectedItem = "Menu 1";
}
private void OnSelectionChanged(object sender, SelectionChangedEventArgs e)
{
Data.SelectionChanged(Items.SelectedValue.ToString());
DataFrame.Navigate(Data.ItemList[Items.SelectedIndex].Page);
}
}
Here is ViewModel.cs:
public partial class ViewModel : INotifyPropertyChanged
{
public ViewModel()
{
GlobalVars.InitMenus();
ItemList.Add(GlobalVars.Menu1.Menus[0]);
ItemList.Add(GlobalVars.Menu2.Menus[0]);
ItemList.Add(GlobalVars.Menu3.Menus[0]);
ItemList.Add(GlobalVars.Menu4.Menus[0]);
foreach (MenuItem m in ItemList) ItemTitles.Add(m.Title);
}
public void SelectionChanged(string newSelection)
{
if (!newSelection.StartsWith(" "))
{
ItemList.RemoveAll(x => x.Title.StartsWith(" "));
ItemTitles.RemoveAll(x => x.StartsWith(" "));
}
switch (newSelection)
{
case "Menu 1":
foreach(MenuItem m in GlobalVars.Menu1.Menus)
{
if (!m.Title.StartsWith(" ")) continue;
ItemList.Insert(ItemTitles.IndexOf("Menu 1") + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf("Menu 1") + 1, m.Title);
}
break;
case "Menu 2":
foreach(MenuItem m in GlobalVars.Menu2.Menus)
{
if (!m.Title.StartsWith(" ")) { continue; }
ItemList.Insert(ItemTitles.IndexOf("Menu 2") + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf("Menu 2") + 1, m.Title);
}
break;
case "Menu 3":
foreach(MenuItem m in GlobalVars.Menu3.Menus)
{
if (!m.Title.StartsWith(" ")) { continue; }
ItemList.Insert(ItemTitles.IndexOf("Menu 3") + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf("Menu 3") + 1, m.Title);
}
break;
case "Menu 4":
foreach(MenuItem m in GlobalVars.Menu4.Menus)
{
if (!m.Title.StartsWith(" ")) { continue; }
ItemList.Insert(ItemTitles.IndexOf("Menu 4") + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf("Menu 4") + 1, m.Title);
}
break;
}
}
}
And VMProperties.cs (a continuation of ViewModel.cs):
public partial class ViewModel : INotifyPropertyChanged
{
private ObservableCollection<MenuItem> _itemList = new ObservableCollection<MenuItem>();
public ObservableCollection<MenuItem> ItemList
{
get { return _itemList; }
set { _itemList = value; }
}
private ObservableCollection<string> _itemTitles = new ObservableCollection<string>();
public ObservableCollection<string> ItemTitles
{
get { return _itemTitles; }
set
{
if (_itemTitles == value) return;
_itemTitles = value;
OnPropertyChanged();
}
}
public event PropertyChangedEventHandler PropertyChanged;
protected virtual void OnPropertyChanged(string propertyName = null)
{
PropertyChangedEventHandler handler = PropertyChanged;
if (handler != null)
{
handler(this, new PropertyChangedEventArgs(propertyName));
}
}
}
This is Menus.cs:
public class MenuItem
{
public MenuItem(string Title, Type Page)
{
_title = Title;
_page = Page;
}
private string _title = "";
public string Title
{
get { return _title; }
set { Title = value; }
}
private Type _page = null;
public Type Page
{
get { return _page; }
set { _page = value; }
}
}
public class Menu
{
public Menu() { }
public void Add(MenuItem m)
{
Menus.Add(m);
}
private ObservableCollection<MenuItem> _menus = new ObservableCollection<MenuItem>();
public ObservableCollection<MenuItem> Menus
{
get { return _menus; }
set { _menus = value; }
}
}
And this is GlobalVars.cs:
class GlobalVars
{
public static void InitMenus()
{
var resourceFile = new Windows.ApplicationModel.Resources.ResourceLoader();
Menu1.Add(new MenuItem(resourceFile.GetString("Menu 1"), typeof(Menus.Menu1)));
Menu1.Add(new MenuItem(resourceFile.GetString("Submenu 2"), typeof(Menus.MenuItems.Submenu2)));
Menu1.Add(new MenuItem(resourceFile.GetString("Submenu 1"), typeof(Menus.MenuItems.Submenu1)));
Menu2.Add(new MenuItem(resourceFile.GetString("Menu 2"), typeof(Menus.Menu2)));
Menu2.Add(new MenuItem(resourceFile.GetString("Submenu 4"), typeof(Menus.MenuItems.Submenu4)));
Menu2.Add(new MenuItem(resourceFile.GetString("Submenu 3"), typeof(Menus.MenuItems.Submenu3)));
Menu3.Add(new MenuItem(resourceFile.GetString("Menu 3"), typeof(Menus.Menu3)));
Menu3.Add(new MenuItem(resourceFile.GetString("Submenu 5"), typeof(Menus.MenuItems.Submenu5)));
Menu4.Add(new MenuItem(resourceFile.GetString("Menu 4"), typeof(Menus.Menu4)));
Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 10"), typeof(Menus.MenuItems.Submenu10)));
Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 9"), typeof(Menus.MenuItems.Submenu9)));
Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 8"), typeof(Menus.MenuItems.Submenu8)));
Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 7"), typeof(Menus.MenuItems.Submenu7)));
Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 6"), typeof(Menus.MenuItems.Submenu6)));
}
private static Menu _menu1 = new Menu();
public static Menu Menu1
{
get { return _menu1; }
}
private static Menu _menu2 = new Menu();
public static Menu Menu2
{
get { return _menu2; }
}
private static Menu _menu3 = new Menu();
public static Menu Menu3
{
get { return _menu3; }
}
private static Menu _menu4 = new Menu();
public static Menu Menu4
{
get { return _menu4; }
}
}
Extension.cs contains the code for ObservableCollection.RemoveAll()
, taken from SO.
These are the values in Resources.resw:
Menu 1 Menu 1
Menu 10 Menu 10
Menu 11 Menu 11
Menu 12 Menu 12
Menu 13 Menu 13
Menu 14 Menu 14
Menu 2 Menu 2
Menu 3 Menu 3
Menu 4 Menu 4
Menu 5 Menu 5
Menu 6 Menu 6
Menu 7 Menu 7
Menu 8 Menu 8
Menu 9 Menu 9
Submenu 1 Submenu 1
Submenu 10 Submenu 10
Submenu 2 Submenu 2
Submenu 3 Submenu 3
Submenu 4 Submenu 4
Submenu 5 Submenu 5
Submenu 6 Submenu 6
Submenu 7 Submenu 7
Submenu 8 Submenu 8
Submenu 9 Submenu 9
I would like my navigation, use of MVVM, and everything else reviewed. I hate the way I have to have ItemsTitle
separate from ItemList
, but my ListBox would not update in response to my clicks any other way. I am not at all sure I am doing this right, and that is why I want it reviewed. I do want to keep one ObservableCollection
or some other collection type per menu so I don’t have to iterate over hundreds of values each time I need to insert a new list. I know I can’t have menus inside menus, but I don’t want that feature now anyway.
Solution
A few things I noticed:
-
You should have a property on your
ViewModel
which represents theSelectedItem
as well as theSelectedPage
. In yourMainPage
you would bind then theSelectedItem
property of theListBox
to the view model property and theSource
of theFrame
to theSelectedPage
property. This way when someone selects a new item you can handle the change entirely inside the view model and getting rid of the code-behind in yourMainPage
. The advantage is that you can easily unit test the behaviour of menu selection. -
Your handling code for the different menus inside the
ViewModel
is repeating code at it’s best. This code block:foreach(MenuItem m in GlobalVars.Menu1.Menus) { if (!m.Title.StartsWith(" ")) continue; ItemList.Insert(ItemTitles.IndexOf("Menu 1") + 1, m); ItemTitles.Insert(ItemTitles.IndexOf("Menu 1") + 1, m.Title); }
can be generalized into a method:
private void HandleMenuSelection(string menu, IList<MenuItem> menuItems) { foreach (var m in menuItems) { if (!m.Title.StartsWith(" ")) continue; ItemList.Insert(ItemTitles.IndexOf(menu) + 1, m); ItemTitles.Insert(ItemTitles.IndexOf(menu) + 1, m.Title); } }
in which case your switch would start reading like:
switch (newSelection) { case "Menu 1": HandleMenuSelection(newSelection, GlobalVars.Menu1.Menus); break; case "Menu 2": HandleMenuSelection(newSelection, GlobalVars.Menu2.Menus); break; ... }
-
I strongly dislike the
GlobalVars
class. There is an implicit dependency from the view model to it via theMenu 1
,Menu 2
, etc. magic strings which is not easily visible and makes the code brittle. It would be better to create an interfacelike anIMenuSource
which can be queried for the different menus. This will make unit testing the code easier as well.
Update To clarify on point 3 above.
I would consider creating an interface which provides the required information about the available menus. Something along these lines:
public interface IMenuProvider
{
IEnumerable<MenuItem> GetMainMenus();
IEnumerable<MenuItem> GetSubMenus(string menuName);
}
which you can then implement accordingly and inject into your ViewModel
which would then look more like this:
public partial class ViewModel : INotifyPropertyChanged
{
private IMenuProvider _MenuProvider;
public ViewModel(IMenuProvider menuProvider)
{
ItemList.AddRange(menuProvider.GetMainMenus());
foreach (MenuItem m in ItemList) ItemTitles.Add(m.Title);
}
public void SelectionChanged(string newSelection)
{
if (!newSelection.StartsWith(" "))
{
ItemList.RemoveAll(x => x.Title.StartsWith(" "));
ItemTitles.RemoveAll(x => x.StartsWith(" "));
}
foreach (var m in menuProvider.GetSubMenus(newSelection))
{
if (!m.Title.StartsWith(" ")) continue;
ItemList.Insert(ItemTitles.IndexOf(newSelection) + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf(newSelection) + 1, m.Title);
}
}
}
It has also the advantage that the application code doesn’t have to know what menus exist so they could easily come from a database or other configuration file. When adding a new menu no code has to change.
Example implementation could be:
public class ResourceMenuProvider : IMenuProvider
{
private static Dictionary<string, Menu> _Menus = new Dictionary<string, Menu>();
static ResourceMenuProvider()
{
var resourceFile = new Windows.ApplicationModel.Resources.ResourceLoader();
var menu1 = new Menu();
menu1.Add(new MenuItem(resourceFile.GetString("Menu 1"), typeof(Menus.Menu1)));
menu1.Add(new MenuItem(resourceFile.GetString("Submenu 2"), typeof(Menus.MenuItems.Submenu2)));
menu1.Add(new MenuItem(resourceFile.GetString("Submenu 1"), typeof(Menus.MenuItems.Submenu1)));
_Menus["Menu 1"] = menu1;
var menu2 = new Menu();
menu2.Add(new MenuItem(resourceFile.GetString("Menu 2"), typeof(Menus.Menu2)));
menu2.Add(new MenuItem(resourceFile.GetString("Submenu 4"), typeof(Menus.MenuItems.Submenu4)));
menu2.Add(new MenuItem(resourceFile.GetString("Submenu 3"), typeof(Menus.MenuItems.Submenu3)));
_Menus["Menu 2"] = menu2;
var menu3 = new Menu();
menu3.Add(new MenuItem(resourceFile.GetString("Menu 3"), typeof(Menus.Menu3)));
menu3.Add(new MenuItem(resourceFile.GetString("Submenu 5"), typeof(Menus.MenuItems.Submenu5)));
_Menus["Menu 3"] = menu3;
var menu4 = new Menu();
menu4.Add(new MenuItem(resourceFile.GetString("Menu 4"), typeof(Menus.Menu4)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 10"), typeof(Menus.MenuItems.Submenu10)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 9"), typeof(Menus.MenuItems.Submenu9)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 8"), typeof(Menus.MenuItems.Submenu8)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 7"), typeof(Menus.MenuItems.Submenu7)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 6"), typeof(Menus.MenuItems.Submenu6)));
_Menus["Menu 4"] = menu4;
}
IEnumerable<MenuItem> GetMainMenus()
{
return _Menus.Select(kvp => kvp.Value.Menus.First());
}
IEnumerable<MenuItem> GetSubMenus(string menuName)
{
return _Menus[menuName].Menus.Skip(1); // skip main menu
}
}