ObservableDictionary not properly implementating INotifyCollectionChanged

Question

In order to bind a dictionary to a listbox I have used an observable dictionary which has been implemented by someone.

Dic[0] = "three"; is throwing System.ArgumentOutOfRangeException when I initialize ObservableDictionary before InitializeComponent().

There is something wrong with this implementation that I have tried to debug but without success. Could someone guide me or point out what wrong with this implementation?

Replacing OnCollectionChanged(NotifyCollectionChangedAction.Replace, new KeyValuePair<TKey, TValue>(key, value), new KeyValuePair<TKey, TValue>(key, item)); by OnCollectionChanged(); (in the case of initilizing ObservableDictionary beforeInitializeComponent()) seems to resolve the exception issue. Is it the real solution tho?

TestWindow.xaml

<Window x:Class="Wpf.TestWindow" x:Name="win"
    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"
    DataContext="{Binding ElementName=win}"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    mc:Ignorable="d" Title="TestWindow" Height="450" Width="800">

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

    <StackPanel Orientation="Vertical">
        <ListBox MinHeight="100" MinWidth="100"
            DataContext="{Binding ElementName=win}"
            ItemsSource="{Binding Path=Dic}"
            SelectedValuePath="Key"                         
            DisplayMemberPath="Value"/>
        <Button Content="modify" MinWidth="100" Click="Button_Click"/>
    </StackPanel>
</Grid>

TestWindow.cs

using System.Collections.ObjectModel;
using System.Windows;    
namespace Wpf
{   
    public partial class TestWindow : Window, INotifyPropertyChanged
    {
        private ObservableDictionary<int, string> dic;
        public ObservableDictionary<int, string> Dic {
            get => dic;
            set
            {
                dic = value;
                PropertyChanged(this, new PropertyChangedEventArgs(nameof(Dic)));
            }
        }

        public TestWindow()
        {
            InitializeComponent();
            Dic = new ObservableDictionary<int, string>() { {0,"Zero" }, {1, "one" }, };
            Dic.Add(2, "two");    
        }

        private void Button_Click(object sender, RoutedEventArgs e)
        {
            Dic[0] = "three";
            Dic.Remove(1);
        }
    }
}

ObservableDictionary.cs

using System.Linq;
using System.ComponentModel;
using System.Collections.Generic;
using System.Collections.Specialized;

namespace System.Collections.ObjectModel
{
    public class ObservableDictionary<TKey, TValue> : IDictionary<TKey, TValue>, INotifyCollectionChanged, INotifyPropertyChanged
    {
        private const string CountString = "Count";
        private const string IndexerName = "Item[]";
        private const string KeysName = "Keys";
        private const string ValuesName = "Values";

        private IDictionary<TKey, TValue> _Dictionary;
        protected IDictionary<TKey, TValue> Dictionary
        {
            get { return _Dictionary; }
        }

        #region Constructors
        public ObservableDictionary()
        {
            _Dictionary = new Dictionary<TKey, TValue>();
        }
        public ObservableDictionary(IDictionary<TKey, TValue> dictionary)
        {
            _Dictionary = new Dictionary<TKey, TValue>(dictionary);
        }
        public ObservableDictionary(IEqualityComparer<TKey> comparer)
        {
            _Dictionary = new Dictionary<TKey, TValue>(comparer);
        }
        public ObservableDictionary(int capacity)
        {
            _Dictionary = new Dictionary<TKey, TValue>(capacity);
        }
        public ObservableDictionary(IDictionary<TKey, TValue> dictionary, IEqualityComparer<TKey> comparer)
        {
            _Dictionary = new Dictionary<TKey, TValue>(dictionary, comparer);
        }
        public ObservableDictionary(int capacity, IEqualityComparer<TKey> comparer)
        {
            _Dictionary = new Dictionary<TKey, TValue>(capacity, comparer);
        }
        #endregion

        #region IDictionary<TKey,TValue> Members

        public void Add(TKey key, TValue value)
        {
            Insert(key, value, true);
        }

        public bool ContainsKey(TKey key)
        {
            return Dictionary.ContainsKey(key);
        }

        public ICollection<TKey> Keys
        {
            get { return Dictionary.Keys; }
        }

        public bool Remove(TKey key)
        {
            if (key == null) throw new ArgumentNullException("key");

            TValue value;
            Dictionary.TryGetValue(key, out value);
            var removed = Dictionary.Remove(key);
            if (removed)
                //OnCollectionChanged(NotifyCollectionChangedAction.Remove, new KeyValuePair<TKey, TValue>(key, value));
                OnCollectionChanged();    
            return removed;
        }    

        public bool TryGetValue(TKey key, out TValue value)
        {
            return Dictionary.TryGetValue(key, out value);
        }


        public ICollection<TValue> Values
        {
            get { return Dictionary.Values; }
        }


        public TValue this[TKey key]
        {
            get
            {
                return Dictionary[key];
            }
            set
            {
                Insert(key, value, false);
            }
        }


        #endregion


        #region ICollection<KeyValuePair<TKey,TValue>> Members

        public void Add(KeyValuePair<TKey, TValue> item)
        {
            Insert(item.Key, item.Value, true);
        }

        public void Clear()
        {
            if (Dictionary.Count > 0)
            {
                Dictionary.Clear();
                OnCollectionChanged();
            }
        }

        public bool Contains(KeyValuePair<TKey, TValue> item)
        {
            return Dictionary.Contains(item);
        }


        public void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex)
        {
            Dictionary.CopyTo(array, arrayIndex);
        }


        public int Count
        {
            get { return Dictionary.Count; }
        }


        public bool IsReadOnly
        {
            get { return Dictionary.IsReadOnly; }
        }


        public bool Remove(KeyValuePair<TKey, TValue> item)
        {
            return Remove(item.Key);
        }


        #endregion


        #region IEnumerable<KeyValuePair<TKey,TValue>> Members


        public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator()
        {
            return Dictionary.GetEnumerator();
        }


        #endregion


        #region IEnumerable Members


        IEnumerator IEnumerable.GetEnumerator()
        {
            return ((IEnumerable)Dictionary).GetEnumerator();
        }


        #endregion


        #region INotifyCollectionChanged Members


        public event NotifyCollectionChangedEventHandler CollectionChanged;


        #endregion


        #region INotifyPropertyChanged Members


        public event PropertyChangedEventHandler PropertyChanged;


        #endregion


        public void AddRange(IDictionary<TKey, TValue> items)
        {
            if (items == null) throw new ArgumentNullException("items");


            if (items.Count > 0)
            {
                if (Dictionary.Count > 0)
                {
                    if (items.Keys.Any((k) => Dictionary.ContainsKey(k)))
                        throw new ArgumentException("An item with the same key has already been added.");
                    else
                        foreach (var item in items) Dictionary.Add(item);
                }
                else
                    _Dictionary = new Dictionary<TKey, TValue>(items);


                OnCollectionChanged(NotifyCollectionChangedAction.Add, items.ToArray());
            }
        }


        private void Insert(TKey key, TValue value, bool add)
        {
            if (key == null) throw new ArgumentNullException("key");


            TValue item;
            if (Dictionary.TryGetValue(key, out item))
            {
                if (add) throw new ArgumentException("An item with the same key has already been added.");
                if (Equals(item, value)) return;
                Dictionary[key] = value;

                //OnCollectionChanged();
                OnCollectionChanged(NotifyCollectionChangedAction.Replace, new KeyValuePair<TKey, TValue>(key, value), new KeyValuePair<TKey, TValue>(key, item));
            }
            else
            {
                Dictionary[key] = value;

                OnCollectionChanged(NotifyCollectionChangedAction.Add, new KeyValuePair<TKey, TValue>(key, value));
            }
        }


        private void OnPropertyChanged()
        {
            OnPropertyChanged(CountString);
            OnPropertyChanged(IndexerName);
            OnPropertyChanged(KeysName);
            OnPropertyChanged(ValuesName);
        }


        protected virtual void OnPropertyChanged(string propertyName)
        {
            if (PropertyChanged != null) PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
        }


        private void OnCollectionChanged()
        {
            OnPropertyChanged();
            if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
        }


        private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair<TKey, TValue> changedItem)
        {
            OnPropertyChanged();
            if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, changedItem));
        }


        private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair<TKey, TValue> newItem, KeyValuePair<TKey, TValue> oldItem)
        {
            OnPropertyChanged();
            if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, newItem, oldItem));
        }


        private void OnCollectionChanged(NotifyCollectionChangedAction action, IList newItems)
        {
            OnPropertyChanged();
            if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, newItems));
        }
    }
}

I don't have the privledge yet myself, but the duplicate flag on this question should be removed. This question is not simply asking about what an `ArgumentOutOfRangeException` is and how to generally deal with one. This question uncovers a specific issue where an implementation of a collection-type class- which *seems* like it should work- strangly causes an exception in *other* code around it. Anyone seeing this with the privledge should vote to remove the tag. — Keith Stein, May 10 '20 at 17:55
I'm not sure that this is a correct use of the moderator flag- but it might be. The normal action would be to vote on it, but neither of us have the privledge yet. You could try editing the question slightly to show the diffference between the two questions and see if that get's people to vote. If not, and you still care enough, flagging for a mod might be the only other option- but keep in mind a given mod might not be knowedgable enough in this specific area to judge this. — Keith Stein, May 10 '20 at 18:10
@Keith: _"This question is not simply asking about what an ArgumentOutOfRangeException is and how to generally deal with one"_ -- the way that the question is currently presented, it very much is exactly that. If the `ObservableDictionary` class shown is producing incorrect results, and the author of the question is seeking help for how to avoid those results, then they should ask about that, not about the `ArgumentOutOfRangeException`. If they don't seek to fix the dictionary, then they very much are asking how to avoid the exception, which is what the marked duplicate is about. — Peter Duniho, May 10 '20 at 21:35
All that said, the whole concept of using `INotifyCollectionChanged` with `IDictionary` is fundamentally flawed. The basic dictionary interface in .NET is _unordered_, while the notification interface assumes an _ordered_ collection. Hence the use of indexes in the event args to describe the event. If an implementor is going to deviate from this norm, it is up to them to fully document their intended use, and for the client code to follow that documentation. But given that many clients of `INotifyCollectionChanged` may not have that option, IMHO it's a bad idea in the first place. — Peter Duniho, May 10 '20 at 21:39

Keith Stein · Accepted Answer · 2020-05-10T18:01:59.340

After reproducing this issue and doing my own tests, I've found the problem.

Looking at the call stack, the exception is actually coming from CollectionView, which is what ItemsControl uses internally to monitor its ItemsSource. This is why the exception only occures when the ObservableDictionary is bound to.

I compared the implementation of ObservableDictionary above to the .NET source code for ObservableCollection. This link takes you to the method in the source that gets called when an item is replaced. You'll notice that they use an overload of the NotifyCollectionChangedEventArgs constructor which includes index. But the ObservableDictionary implementation uses a different constructor which does not.

Switching the constructor to the one being used by ObservableCollection prevents this exception.

So, insead of:

private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair<TKey, TValue> newItem, KeyValuePair<TKey, TValue> oldItem)
{
    OnPropertyChanged();
    if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, newItem, oldItem));
}

Use this:

private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair<TKey, TValue> newItem, KeyValuePair<TKey, TValue> oldItem)
{
    OnPropertyChanged();
    if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, newItem, oldItem, Dictionary.ToList().IndexOf(newItem)));
}

Remove() was throwing Collection Remove event must specify item position. exception because the called overload of CollectionChanged was also missing an index parameter.

public bool Remove(TKey key)
        {
            if (key == null) throw new ArgumentNullException("key");

            TValue value;
            if (!Dictionary.TryGetValue(key, out value)) { return false; }
            var removeditem = new KeyValuePair<TKey, TValue>(key, value);
            var removedindex = Dictionary.ToList().IndexOf(removeditem);
            var removed = Dictionary.Remove(key);

            if (removed)
                OnCollectionChanged(NotifyCollectionChangedAction.Remove, removeditem, removedindex);

            return removed;
        }

And the new overload called in this case:

private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair<TKey, TValue> changedItem, int removedindex)
        {
            OnPropertyChanged();
            if (CollectionChanged != null) CollectionChanged(this, new NotifyCollectionChangedEventArgs(action, changedItem, removedindex));
        }

I've left a note on the GitHub page for the original code (the one you linked to) informing them of this error so they can fix it- or at least so that future users are aware of it.

Thanks for the detailed explanation, there still a bug in remove `Remove()` collection event it should send index while it is not possible to add `Dictionary.ToList().IndexOf(changedItem)` because changedItem is already deleted. — Cfun, May 10 '20 at 13:00
I think an overload like `private void OnCollectionChanged(NotifyCollectionChangedAction action, KeyValuePair changedItem, int removedindex)` is missing. `removedindex` will be handeled at `public bool Remove(TKey key)` methode. — Cfun, May 10 '20 at 13:02
@johan472 I agree. You should use an overload like that, check the index before you remove the item, then pass that index along. — Keith Stein, May 10 '20 at 17:41
@johan472 I approved your edit with one important change: you needed to check if the key exists before calling `IndexOf` to avoid an unwanted exception. — Keith Stein, May 10 '20 at 18:03

John Colvin · Answer 2 · 2020-05-10T05:16:29.420

You may have stumbled onto a bug in ObservableDictionary. When the value of an existing key is replaced, it appears that the index is left unspecified in the subsequent CollectionChanged event's NotifyCollectionChangedEventArgs. If unspecified, the New/OldItemIndex properties default to -1. The collection changed handler used by the ListBox probably either doesn't expect this, or deliberately doesn't support it.

ObservableDictionary not properly implementating INotifyCollectionChanged

2 Answers2