ItemsControl do not update itself when ObservableCollection fires CollectionChanged

Question

I've read lot of answers of this question, but they often contains something like "you missed INotifyPropertyChanged". I use MVVM light for implementation of ViewModelBase, ObservableObject etc.

View:

<Window x:Class="BaseFlyingFigure.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:local="clr-namespace:BaseFlyingFigure"
    xmlns:helpers="clr-namespace:BaseFlyingFigure.Helpers"
    xmlns:i="http://schemas.microsoft.com/expression/2010/interactivity"
    xmlns:cmd="clr-namespace:GalaSoft.MvvmLight.Command;assembly=GalaSoft.MvvmLight.Platform"
    xmlns:system="clr-namespace:System;assembly=mscorlib"
    mc:Ignorable="d"
    Title="MainWindow" Height="450" Width="525"
    DataContext="{Binding MainViewModel, Source={StaticResource Locator}}">
<i:Interaction.Triggers>
    <i:EventTrigger EventName="Loaded">
        <cmd:EventToCommand Command="{Binding LoadedCommand}" />
    </i:EventTrigger>
    <i:EventTrigger EventName="PreviewKeyDown">
        <cmd:EventToCommand Command="{Binding PreviewKeyDownCommand}"
                            PassEventArgsToCommand="True" />
    </i:EventTrigger>
</i:Interaction.Triggers>
<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="Auto" />
        <RowDefinition Height="*" />
    </Grid.RowDefinitions>
    <Menu Grid.Row="0">
        <MenuItem Header="File">
            <MenuItem Header="Exit" Command="{Binding AppExitCommand}" />
        </MenuItem>
    </Menu>
    <ItemsControl Grid.Row="1" helpers:SizeObserver.Observe="True"
                  helpers:SizeObserver.ObservedWidth="{Binding CanvasWidth, Mode=OneWayToSource}"
                  helpers:SizeObserver.ObservedHeight="{Binding CanvasHeight, Mode=OneWayToSource}"
                  ItemsSource="{Binding Elements, Converter={helpers:ElementToShapeConverter}, 
        Mode=OneWay, UpdateSourceTrigger=PropertyChanged}"
                  >
        <ItemsControl.ItemsPanel>
            <ItemsPanelTemplate>
                <Canvas ClipToBounds="True" />
            </ItemsPanelTemplate>
        </ItemsControl.ItemsPanel>
    </ItemsControl>
</Grid>

ViewModel:

using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Windows;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Shapes;
using BaseFlyingFigure.Services.Interfaces;
using GalaSoft.MvvmLight;
using GalaSoft.MvvmLight.Command;

namespace BaseFlyingFigure.ViewModels
{
    public class MainViewModel : ViewModelBase
    {
        private readonly IFigureRepository _repository;
        private ObservableCollection<Element> _elements = new ObservableCollection<Element>();


        public MainViewModel(IFigureRepository repository)
        {
            _repository = repository;

            AppExitCommand = new RelayCommand(Exit);
            LoadedCommand = new RelayCommand(WindowLoaded);

            PreviewKeyDownCommand = new RelayCommand<KeyEventArgs>(PreviewKeyDown);

            Elements.Add(new Element(new Ellipse {
                    Fill = Brushes.HotPink,
                    Width = 100,
                    Height = 100
                })
                {Left = 250, Top = 250});
        }

        public RelayCommand AppExitCommand { get; private set; }
        public RelayCommand LoadedCommand { get; private set; }
        public RelayCommand<KeyEventArgs> PreviewKeyDownCommand { get; private set; }

        public double CanvasWidth { get; set; }
        public double CanvasHeight { get; set; }

        public ObservableCollection<Element> Elements
        {
            get { return _elements; }
            set 
            {
                if (value != _elements)
                    Set(ref _elements, value);
            }
        }

        private void PreviewKeyDown(KeyEventArgs e)
        {
            switch (e.Key) {
                case Key.OemPlus:
                    Elements.Add(new Element(new Ellipse
                    {
                            Fill = Brushes.HotPink,
                            Width = 100,
                            Height = 100
                        })
                        {Left = 250, Top = 250});
                    Debug.WriteLine("+");
                    break;
            }
        }

        private void WindowLoaded()
        {
            Elements.CollectionChanged += (sender, args) => Debug.WriteLine("changed");
        }

        private void Exit() => Application.Current.Shutdown();
    }
}

Converter:

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Windows.Data;
using System.Windows.Markup;
using BaseFlyingFigure.ViewModels;

namespace BaseFlyingFigure.Helpers
{
    public class ElementToShapeConverter : MarkupExtension, IValueConverter
    {
        private static ElementToShapeConverter _converter;

        public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
        {
            var list = (value as ICollection<Element>)?.Select(el => el.Shape).ToList();
            return list;
        }

        public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
        {
            return null;
        }

        public override object ProvideValue(IServiceProvider serviceProvider)
        {
            return _converter ?? (_converter = new ElementToShapeConverter());
        }
    }
}

Element:

using System.Windows.Controls;
using System.Windows.Shapes;
using GalaSoft.MvvmLight;

namespace BaseFlyingFigure.ViewModels
{
    public class Element : ObservableObject
    {
        private double _left;
        private Shape _shape;
        private double _top;

        public Element(Shape shape)
        {
            Shape = shape;
        }

        public double Left
        {
            get { return _left; }
            set
            {
                Set(ref _left, value);
                Canvas.SetLeft(Shape, value);
            }
        }

        public double Top
        {
            get { return _top; }
            set
            {
                Set(ref _top, value);
                Canvas.SetTop(Shape, value);
            }
        }

        public Shape Shape
        {
            get { return _shape; }
            set { Set(ref _shape, value); }
        }
    }
}

CollectionChanged fires when we press +. But canvas display shapes that made and added in constructor. ViewModelLocator:

public class ViewModelLocator
{
    public ViewModelLocator()
    {
        ServiceLocator.SetLocatorProvider(() => SimpleIoc.Default);
        SimpleIoc.Default.Register<MainViewModel>();
        SimpleIoc.Default.Register<IFigureRepository, FigureRepository>();
    }

    public MainViewModel MainViewModel => ServiceLocator.Current.GetInstance<MainViewModel>();
}

What's wrong with that? Any mistakes in XAML or something else?

Answer 1

The basic problem of your approach is that you use Shape objects in your view model.

Besides that it will be impossible to have more than one view that visualizes this view model (because UIElements can only have a single parent), it also forces you to use an unconventional and defective approach to convert an ObservableCollection<Element> to a List<Shape> in a Converter of the ItemsSource Binding. As already stated in the comments and the other answer, the List<Shape> returned from your converter does not notify the view about changes in the ObservableCollection<Element>.

A proper MVVM approach would use a representation of a shape without UI elements, e.g. like this:

public class Element
{
    public Geometry Shape { get; set; }
    public Brush Fill { get; set; }
    public Brush Stroke { get; set; }
    public double StrokeThickness { get; set; }
}

You could now declare a regular DataTemplate for the visualization of a shape in your ItemsControl:

<ItemsControl ItemsSource="{Binding Elements}">
    <ItemsControl.ItemsPanel>
        <ItemsPanelTemplate>
            <Canvas/>
        </ItemsPanelTemplate>
    </ItemsControl.ItemsPanel>
    <ItemsControl.ItemTemplate>
        <DataTemplate>
            <Path Data="{Binding Shape}"
                  Fill="{Binding Fill}"
                  Stroke="{Binding Stroke}"
                  StrokeThickness="{Binding StrokeThickness}"/>
        </DataTemplate>
    </ItemsControl.ItemTemplate>
</ItemsControl>

Adding the sample ellipse to your view model would now look like this:

Elements.Add(new Element
{
    Shape = new EllipseGeometry(new Point(250, 250), 50, 50),
    Fill = Brushes.HotPink
});

---

If for any reason you also need an additional x/y position offset for each element, you may add two properties to the Element class

public class Element
{
    ...
    public double X { get; set; }
    public double Y { get; set; }
}

and add an ItemsContainerStyle to the ItemsControl that uses these properties:

<ItemsControl ItemsSource="{Binding Elements}">
    ...
    <ItemsControl.ItemContainerStyle>
        <Style TargetType="ContentPresenter">
            <Setter Property="Canvas.Left" Value="{Binding X}"/>
            <Setter Property="Canvas.Top" Value="{Binding Y}"/>
        </Style>
    </ItemsControl.ItemContainerStyle>
</ItemsControl>

Answer 2

I think it's because ElementToShapeConverter.Convert creates a new List<> and returns that. That will maybe not break the binding to the collection in the view model, but the collection in the view model will no longer be invalidated by changes.

I think your ViewModel should have a separate ObservableCollection Property called Shapes that filters the Shapes out of the Elements collection when ever it is called instead of the Converter. You can invalidate it in the CollectionChanged event of the Elements collection.