Collections.Pooled icon indicating copy to clipboard operation
Collections.Pooled copied to clipboard

Support for ObservableCollection

Open Symbai opened this issue 6 years ago • 6 comments

Would be amazing if you could add support for ObservableCollection<T> as it's the only collection type that fully supports binding. If you add support I would recommend or suggest you also add a AddRange method as the original collection hasn't but for performance reasons it's a must-have. There are many modified versions that added such as a method but none which is optimized with ArrayPool etc.

Symbai avatar May 11 '19 08:05 Symbai

I've got some preliminary code going in a branch.

For AddRange, what would you expect to happen to the CollectionChanged events that would normally fire for each item added?

  • Should they all fire at the end after all items are added?
  • Should just one CollectionChanged event fire, but with meaningless item and index values?

jtmueller avatar Jun 23 '19 07:06 jtmueller

Thanks for getting back to this. This is my custom AddRange implementation for the ObservableCollection which I've been using for a long time now. There are a few cases where you would want it to fire per item but for me this is too slow.

public virtual void AddRange(IEnumerable<T> items)
        {
            Execute.OnUIThreadSync(() =>
            {
                OnCollectionChanging(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));

                var previousNotificationSetting = isNotifying;
                isNotifying = false;
                var index = Count;
                foreach (var item in items)
                {
                    base.InsertItem(index, item);
                    index++;
                }
                isNotifying = previousNotificationSetting;
                OnPropertyChanged(new PropertyChangedEventArgs("Count"));
                OnPropertyChanged(new PropertyChangedEventArgs("Item[]"));
                OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
            });
        }

Symbai avatar Jun 23 '19 08:06 Symbai

Unless things have changed since 2009, it seems like the main motivation for having AddRange is to avoid firing an event for each item added. However, at least at the time of writing, "ListCollectionView doesn’t implement support for non-single item CollectionChanged events."

https://blogs.msdn.microsoft.com/nathannesbit/2009/04/20/addrange-and-observablecollection/

The features/observable-collection branch currently implements the "single event with everything that was added" approach, but it may not be compatible with some commonly-used WPF controls. Hopefully those controls have been updated since 2009 and it just works. If not, maybe I could implement a workaround like this.

@Symbai any way you could test it out, see if the extra workaround is needed or not? I should mention that branch requires you to have the latest .NET Core 3 preview SDK to build it.

jtmueller avatar Jun 23 '19 17:06 jtmueller

I've been using my custom AddRange for a lot of WPF controls now without having any issues at all. A month ago it was officially added in .NET Core 3 repo, but later it got revert https://github.com/dotnet/corefx/pull/38115

Anyway, in my case the extra workaround is not required. Tested it and it works fine and is faster than the ObservableCollection for many items, nice work!

Symbai avatar Jun 23 '19 18:06 Symbai

Fyi I'm closing this, hope that's okay

Symbai avatar Jun 23 '19 18:06 Symbai

@Symbai That's great that it's working for you! However, I'm going to re-open this for now, as I haven't ported over any of the unit tests or benchmarks for Collection/ObservableCollection, and thus I'm not ready to merge the branch in question yet and include it in the NuGet package.

I'll close the issue when I merge the observable-collection branch, after I've got unit tests and benchmarks running, and I'm satisfied with the benchmark results. Thanks for checking it out.

jtmueller avatar Jun 24 '19 22:06 jtmueller