Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for ObservableCollection #32

Open
Symbai opened this issue May 11, 2019 · 6 comments
Open

Support for ObservableCollection #32

Symbai opened this issue May 11, 2019 · 6 comments
Assignees

Comments

@Symbai
Copy link

Symbai commented May 11, 2019

Would be amazing if you could add support for ObservableCollection 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.

@jtmueller jtmueller self-assigned this Jun 22, 2019
@jtmueller
Copy link
Owner

jtmueller commented Jun 23, 2019

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?

@Symbai
Copy link
Author

Symbai commented Jun 23, 2019

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));
            });
        }

@jtmueller
Copy link
Owner

jtmueller commented Jun 23, 2019

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.

@Symbai
Copy link
Author

Symbai commented Jun 23, 2019

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 dotnet/corefx#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
Copy link
Author

Symbai commented Jun 23, 2019

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

@Symbai Symbai closed this as completed Jun 23, 2019
@jtmueller
Copy link
Owner

@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 jtmueller reopened this Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants