ObservableCollection : calling OnCollectionChanged with multiple new items

Question

please note that I am trying to use NotifyCollectionChangedAction.Add action instead of .Reset. the latter does work, but it is not very efficient with large collections.

so i subclassed ObservableCollection:

public class SuspendableObservableCollection<T> : ObservableCollection<T>

for some reason, this code:

private List<T> _cachedItems;
...

    public void FlushCache() {
        if (_cachedItems.Count > 0) {

        foreach (var item in _cachedItems)
            Items.Add(item);

        OnCollectionChanged(new NotifyCollectionChangedEventArgs(
            NotifyCollectionChangedAction.Add, (IList<T>)_cachedItems));
        }
    }

is throwing A collection Add event refers to item that does not belong to collection

this appears to be a bug in BCL ?

I can step through and see prior to calling OnCollectionChanged that new items are added to this.Items

WOW

just made a staggering discovery. None of these approaches worked for me (flush, addrange), because the error appears to be triggered ONLY if this collection is bound to my Listview!!

TestObservableCollection<Trade> testCollection = new TestObservableCollection<Trade>();
List<Trade> testTrades = new List<Trade>();

for (int i = 0; i < 200000; i++) 
    testTrades.Add(t);

testCollection.AddRange(testTrades); // no problems here.. 
_trades.AddRange(testTrades); // this one is bound to ListView .. BOOOM!!!

In conclusion, ObservableCollection does support adding incremental lists, but a ListView doesn't. Andyp figured out a workaround to make it work with CollectionView below, but since .Refresh() is called, that is no different than just calling OnCollectionChanged( .Reset )..

Accompany

Why RemoveRange, AddRange fires Reset? Maybe somebody could not understand difference between remove, add and reset meanings? – Epistaxis

Answer 1

you can implement AddRange() for the ObservableCollection like this as shown here:

public class RangeObservableCollection<T> : ObservableCollection<T>
{
    private bool _SuppressNotification;

    public override event NotifyCollectionChangedEventHandler CollectionChanged;

    protected virtual void OnCollectionChangedMultiItem(
        NotifyCollectionChangedEventArgs e)
    {
        NotifyCollectionChangedEventHandler handlers = this.CollectionChanged;
        if (handlers != null)
        {
            foreach (NotifyCollectionChangedEventHandler handler in 
                handlers.GetInvocationList())
            {
                if (handler.Target is CollectionView)
                    ((CollectionView)handler.Target).Refresh();
                else
                    handler(this, e);
            }
        }
    }

    protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
    {
        if (!_SuppressNotification)
        {
            base.OnCollectionChanged(e);
            if (CollectionChanged != null)
                CollectionChanged.Invoke(this, e);
        }
    }

    public void AddRange(IEnumerable<T> list)
    {
        if (list == null)
            throw new ArgumentNullException("list");

        _SuppressNotification = true;

        foreach (T item in list)
        {
            Add(item);
        }
        _SuppressNotification = false;

        OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, list));
    }
}

UPDATE: After binding to ListBox I was seeing an InvalidOperationException too (same message you were seeing). According to this article that's because CollectionView doesn't support range actions. Luckily the article also supplies a solution (although it feels a little "hack-ish").

UPDATE 2: Added a fix that raises the overridden CollectionChanged event in the overridden implementation of OnCollectionChanged().

Wherefore

thanks, but i am trying to change away from .Reset action. the whole point here is that i want to add only new items. if my collection reaches large size, .reset is very slow as i am filtering it as well – Accompany

Ah, I missed that - updated my code to use NotifyCollectionChangedAction.Add instead of Reset. – Wherefore

Added a link and code that resolves (avoids) the CollectionView's problem with range operations. – Wherefore

hmm... i plugged your collection in as is and AddRange works w/out throwing!!! :) .. but for some mysterious reason items are not being added from regular .Add(foo).. most likely the problem is on my side.. investigating.. – Accompany

Nope, sorry, it wasn't you. My implementation didn't raise the overridden CollectionChange event in the overridden OnCollectionChange method. Fixed that, now Add() works too. – Wherefore

thanks! any way you could elaborate on why GetInvocationList has to be called in this scenario and why CollectionView needs to be refreshed.. does that make it equivalent with just called Reset event? – Accompany

yah.. looks like there is no performance benefit of doing it this way.. (same as just calling .Reset) – Accompany

@SonicSoul - CollectionView for the ListBox (in this case) is a ListCollectionView, which doesn't support bulk adding (or removing) of items. Therefore, if you fire these events, the ListCollectionView doesn't know how to process them. The CollectionView.Refresh() is to compensate for this. If you want a collection that's compatible with all CollectionViews, you need to fire individual events for each item added/removed. – Koller

In CollectionChanged handler NewItems collection there is only one item, which is of type list. Is it possible to use a version of NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, list) that leads to more than one item in NewItems collection? – Anglofrench

What is the purpose of base.OnCollectionChanged(e)? Is anyone could subscribe on base.CollectionChanged? – Sapodilla

Answer 2

Thanks for the inspiration AndyP. I had a few problems with your implementation, such as the use of CollectionView instead of ICollectionView in the test, as well as manually calling "Reset" on the elements. Elements that inherit from CollectionView might actually deal with these args in more ways than calling "this.Reset()", so it's preferable to still fire their handlers, just with the Action=Reset args that they require instead of the improved event args that include the list of items changed. Below is my (very similar) implementation.

public class BaseObservableCollection<T> : ObservableCollection<T>
{
    //Flag used to prevent OnCollectionChanged from firing during a bulk operation like Add(IEnumerable<T>) and Clear()
    private bool _SuppressCollectionChanged = false;

    /// Overridden so that we may manually call registered handlers and differentiate between those that do and don't require Action.Reset args.
    public override event NotifyCollectionChangedEventHandler CollectionChanged;

    public BaseObservableCollection() : base(){}
    public BaseObservableCollection(IEnumerable<T> data) : base(data){}

    #region Event Handlers
    protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
    {
        if( !_SuppressCollectionChanged )
        {
            base.OnCollectionChanged(e);
            if( CollectionChanged != null )
                CollectionChanged.Invoke(this, e);
        }
    }

    //CollectionViews raise an error when they are passed a NotifyCollectionChangedEventArgs that indicates more than
    //one element has been added or removed. They prefer to receive a "Action=Reset" notification, but this is not suitable
    //for applications in code, so we actually check the type we're notifying on and pass a customized event args.
    protected virtual void OnCollectionChangedMultiItem(NotifyCollectionChangedEventArgs e)
    {
        NotifyCollectionChangedEventHandler handlers = this.CollectionChanged;
        if( handlers != null )
            foreach( NotifyCollectionChangedEventHandler handler in handlers.GetInvocationList() )
                handler(this, !(handler.Target is ICollectionView) ? e : new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
    }
    #endregion

    #region Extended Collection Methods
    protected override void ClearItems()
    {
        if( this.Count == 0 ) return;

        List<T> removed = new List<T>(this);
        _SuppressCollectionChanged = true;
        base.ClearItems();
        _SuppressCollectionChanged = false;
        OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removed));
    }

    public void Add(IEnumerable<T> toAdd)
    {
        if( this == toAdd )
            throw new Exception("Invalid operation. This would result in iterating over a collection as it is being modified.");

        _SuppressCollectionChanged = true;
        foreach( T item in toAdd )
            Add(item);
        _SuppressCollectionChanged = false;
        OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, new List<T>(toAdd)));
    }

    public void Remove(IEnumerable<T> toRemove)
    {
        if( this == toRemove )
            throw new Exception("Invalid operation. This would result in iterating over a collection as it is being modified.");

        _SuppressCollectionChanged = true;
        foreach( T item in toRemove )
            Remove(item);
        _SuppressCollectionChanged = false;
        OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, new List<T>(toRemove)));
    }
    #endregion
}

Big

How would you accomplish this if BaseObservableCollection is defined in a portable project? I believe ICollectionView is specific to windows and so it's not available to use. – Antilles

@Antilles , put the check for ICollectionView (and possibly the action for that (if using the ICollectionView for anything, like calling Refresh() above)) into a separate virtual method. In your portable base project, the virtual method would not reference ICollectionView and just revert to the default behaviour. Then in a windows library, you override the class and the check method to do the actual check for ICollectionView. – Intricate

Answer 3

After many iterations, we ended up with this version of ObservableRangeCollection and ReadOnlyObservableRangeCollection which is based on the code from the accepted answer, and which we didn't have to modify in the last 6 months:

public class ObservableRangeCollection<T> : ObservableCollection<T>
{
    private bool suppressNotification;

    public ObservableRangeCollection() { }

    public ObservableRangeCollection(IEnumerable<T> items)
        : base(items)
    {
    }

    public override event NotifyCollectionChangedEventHandler CollectionChanged;

    protected virtual void OnCollectionChangedMultiItem(
        NotifyCollectionChangedEventArgs e)
    {
        var handlers = CollectionChanged;
        if (handlers == null) return;

        foreach (NotifyCollectionChangedEventHandler handler in handlers.GetInvocationList())
        {
            if (handler.Target is ReadOnlyObservableCollection<T>
                && !(handler.Target is ReadOnlyObservableRangeCollection<T>))
            {
                throw new NotSupportedException(
                    "ObservableRangeCollection is wrapped in ReadOnlyObservableCollection which might be bound to ItemsControl " +
                    "which is internally using ListCollectionView which does not support range actions.\n" +
                    "Instead of ReadOnlyObservableCollection, use ReadOnlyObservableRangeCollection");
            }
            var collectionView = handler.Target as ICollectionView;
            if (collectionView != null)
            {
                collectionView.Refresh();
            }
            else
            {
                handler(this, e);
            }
        }
    }

    protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
    {
        if (suppressNotification) return;

        base.OnCollectionChanged(e);
        if (CollectionChanged != null)
        {
            CollectionChanged.Invoke(this, e);
        }
    }

    public void AddRange(IEnumerable<T> items)
    {
        if (items == null) return;

        suppressNotification = true;

        var itemList = items.ToList();

        foreach (var item in itemList)
        {
            Add(item);
        }
        suppressNotification = false;

        if (itemList.Any())
        {
            OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, itemList));
        }
    }

    public void AddRange(params T[] items)
    {
        AddRange((IEnumerable<T>)items);
    }

    public void ReplaceWithRange(IEnumerable<T> items)
    {
        Items.Clear();
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
        AddRange(items);
    }

    public void RemoveRange(IEnumerable<T> items)
    {
        suppressNotification = true;

        var removableItems = items.Where(x => Items.Contains(x)).ToList();

        foreach (var item in removableItems)
        {
            Remove(item);
        }

        suppressNotification = false;

        if (removableItems.Any())
        {
            OnCollectionChangedMultiItem(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removableItems));
        }
    }
}

public class ReadOnlyObservableRangeCollection<T> : ReadOnlyObservableCollection<T>
{
    public ReadOnlyObservableRangeCollection(ObservableCollection<T> list)
        : base(list)
    {            
    }

    protected override event NotifyCollectionChangedEventHandler CollectionChanged;

    protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
    {
        var handlers = CollectionChanged;
        if (handlers == null) return;

        foreach (NotifyCollectionChangedEventHandler handler in handlers.GetInvocationList())
        {
            var collectionView = handler.Target as ICollectionView;
            if (collectionView != null)
            {
                collectionView.Refresh();
            }
            else
            {
                handler(this, e);
            }
        }
    }
}

We basically replaced all usages of ObservableCollection in our app by ObservableRangeCollection, and it works like a charm.

Silva

Why notificating in ReplaceWithRange is after Add? – Sapodilla

you want to notify only once after all items have been added. – Silva

ICollectionView no longer has Refresh method? – Pacha

Answer 4

I believe you need to cast it to an IList:

base.OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, (IList)_cachedItems));

Softhearted

thanks, now i am back to "A collection Add event refers to item that does not belong to collection" – Accompany

hmmm, how about instead of Items.Add(item), base.Add(item)? – Softhearted

George, tried Add(item) and base.Add(item), still complains it is not part of the collection... – Accompany

I actually meant this.Add(item), sorry about that... I did get this error before when implementing an ObservableCollection<T>, but I don't have access to the code at the moment I'll into it tomorrow. – Softhearted

isnt this.Add same as Add ? anyways, i tried it just to be sure, and still same error.. i think this may be a bug :( – Accompany