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

Collection<T> and ObservableCollection<T> do not support ranges #18087

Open
robertmclaws opened this issue Aug 13, 2016 · 372 comments · Fixed by dotnet/corefx#35772
Open

Collection<T> and ObservableCollection<T> do not support ranges #18087

robertmclaws opened this issue Aug 13, 2016 · 372 comments · Fixed by dotnet/corefx#35772
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Collections
Milestone

Comments

@robertmclaws
Copy link

Update 10/04/2018

@ianhays and I discussed this and we agree to add this 6 APIs for now:

    // Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection) => InsertItemsRange(0, collection);

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection) => InsertItemsRange(index, collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int count) => RemoveItemsRange(index, count);

    // Will allow to replace a range with fewer, equal, or more items.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Replace)
    public void ReplaceRange(int index, int count, IEnumerable<T> collection)
    {
         RemoveItemsRange(index, count);
         InsertItemsRange(index, collection);
    }

    #region virtual methods
    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
    protected virtual void RemoveItemsRange(int index, int count);
    #endregion

As those are the most commonly used across collection types and the Predicate ones can be achieved through Linq and seem like edge cases.

To answer @terrajobst questions:

Should the methods be virtual? If no, why not? If yes, how does eventing work and how do derived types work?

Yes, we would like to introduce 2 protected virtual methods to stick with the current pattern that we follow with other Insert/Remove apis to give people hability to add their custom removals (like filtering items on a certain condition).

Should some of these methods be pushed down to Collection?

Yes, and then ObservableCollection could just call the base implementation and then trigger the necessary events.

Let's keep the final speclet at the top for easier search

Speclet (Updated 9/23/2016)

Scope

Modernize Collection<T> and ObservableCollection<T> by allowing them to handle operations against multiple items simultaneously.

Rationale

The ObservableCollection is a critical collection when it comes to XAML-based development, though it can also be useful when building API client libraries as well. Because it implements INotifyPropertyChanged and INotifyCollectionChanged, nearly every XAML app in existence uses some form of this collection to bind a set of objects against UI.

However, this class has some shortcomings. Namely, it cannot currently handle adding or removing multiple objects in a single call. Because of that, it also cannot manipulate the collection in such a way that the PropertyChanged events are raised at the very end of the operation.

Consider the following situation:

  • You have a XAML app that accesses an API.
  • That API call returns 25 objects that need to be bound to the UI.
  • In order to get the data displayed into the UI, you likely have to cycle through the results, and add them one at a time to the ObservableCollection.
  • This has the side-effect of firing the CollectionChanged event 25 times. If you are also using that event to do other processing on incoming items, then those events are firing 25 times too. This can get very expensive, very quickly.
  • Additionally, that event will have ChangedItems Lists that will only ever have 0 or 1 objects in them. That is... not ideal.

This behavior is unnecessary, especially considering that NotifyCollectionChangedEventArgs already has the components necessary to handle firing the event once for multiple items, but that capability is presently not being used at all.

Implementing this properly would allow for better performance in these types of apps, and would negate the need for the plethora of replacements out there (here, here, and here, for example).

Usage

Given the above scenario as an example, usage would look like this pseudocode:

    var observable = new ObservableCollection<SomeObject>();
    var client = new HttpClient();
    var result = client.GetStringAsync("http://someapi.com/someobject");
    var results = JsonConvert.DeserializeObject<SomeObject>(result);
    observable.AddRange(results);

Implementation

This is not the complete implementation, because other *Range functionality would need to be implemented as well. You can see the start of this work in PR dotnet/corefx#10751

    // Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection)

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int count);

    // Will allow to replace a range with fewer, equal, or more items.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Replace)
    public void ReplaceRange(int index, int count, IEnumerable<T> collection);

    // Removes any item that matches the search criteria.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    // RWM: Excluded for now, will see if possible to add back in after implementation and testing.
    // public int RemoveAll(Predicate<T> match);

Obstacles

Doing this properly, and having the methods intuitively named, could potentially have the side effect of breaking existing classes that inherit from ObservableCollection to solve this problem. A good way to test this would be to make the change, compile something like Template10 against this new assembly, and see if it breaks.


So the ObservableCollection is one of the cornerstones of software development, not just in Windows, but on the web. One issue that comes up constantly is that, while the OnCollectionChanged event has a structure and constructors that support signaling the change for multiple items being added, the ObservableCollection does not have a method to support this.

If you look at the web as an example, Knockout has a way to be able to add multiple items to the collection, but not signal the change until the very end. The ObservableCollection needs the same functionality, but does not have it.

If you look at other extension methods to solve this problem, like the one in Template10, they let you add multiple items, but do not solve the signaling problem. That's because the ObservableCollection.InsertItem() method overrides Collection.InsertItem(), and all of the other methods are private. So the only way to fix this properly is in the ObservableCollection itself.

I'm proposing an "AddRange" function that accepts an existing collection as input, optionally clears the collection before adding, and then throws the OnCollectionChanging event AFTER all the objects have been added. I have already implemented this in a PR dotnet/corefx#10751 so you can see what the implementation would look like.

I look forward to your feedback. Thanks!

@robertmclaws
Copy link
Author

@joshfree @Priya91 Since I already have a PR that addresses this issue, is there any way this could be moved up to 1.1?

@LanceMcCarthy
Copy link

While you're in there adding an AddRange() method, can you throw an OnPropertyChanged() into the Count property's setter? Thanks :)

@thomaslevesque
Copy link
Member

thomaslevesque commented Sep 13, 2016

A long time ago I had implemented a RangeObservableCollection with AddRange, RemoveRange, InsertRange, ReplaceRange and RemoveAll. But it turned out that the WPF binding system didn't support CollectionChanged notifications with multiple items (I seem to remember it has been fixed since then, but I'm not sure).

@joshfree
Copy link
Member

@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with @robertmclaws ?

/cc @terrajobst

@Priya91
Copy link
Contributor

Priya91 commented Sep 13, 2016

@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with

Sure.

@Priya91
Copy link
Contributor

Priya91 commented Sep 13, 2016

@robertmclaws Can you create an api speclet on this issue, outling the api syntax, like this. Mainly interested in usage scenarios

@svick
Copy link
Contributor

svick commented Sep 14, 2016

@robertmclaws

Doing this properly, and having the methods intuitively named, could potentially have the side effect of breaking existing classes that inherit from ObservableCollection to solve this problem.

In what situation could it be a breaking change? The only issue I can think of is that it would cause a warning that tells you to use new if you meant to hide a base class member, which would be actually an error with warnings as errors enabled. Is this what you meant? Or is there another case I'm missing?

@robertmclaws
Copy link
Author

@svick Could possibly be a runtime problem. If you just upgraded the framework w/o recompiling, I'm not sure exactly how the runtime execution would react. We'd need to test it just to make sure.

@svick
Copy link
Contributor

svick commented Sep 14, 2016

@robertmclaws I think that could only be a problem if you don't recompile, but you do upgrade a library with the custom type inheriting from ObservableCollection<T>, which removed its version of AddRange() in the new version. But that would be the fault of that library.

Otherwise, adding a new member won't affect how old binaries behave.

@Priya91
Copy link
Contributor

Priya91 commented Sep 14, 2016

+1 The api sounds good to me. For manipulating multiple items , along with AddRange, does it provide value to add, InsertRange, RemoveRange, GetRange for the specified usage scenarios?

cc @terrajobst

@robertmclaws
Copy link
Author

@svick You are probably right. I personally would want to test the behavior just to be sure we're not breaking anyone... otherwise this would move to a 2.0 release item.

@Priya91 I'm not sure if a GetRange() would be necessary, but InsertRange() and RemoveRange() would be, along with ReplaceRange(), and possible a Clear() method if one is not currently available.

So if we're comfortable with the API, what's the next step? :)

@Priya91
Copy link
Contributor

Priya91 commented Sep 16, 2016

Clear is already available. We still haven't gotten the shape of apis to add, if RemoveRange and InsertRange are to be added, then we need these apis added to the speclet. And then we'll mark api-ready-for-review, to be discussed in the next api-review meeting either on tuesday or friday.

@robertmclaws
Copy link
Author

OK, I made changes to the speclet. Note that the parameters might change for the actual implementation, but those are what makes the most sense at this particular second. Please LMK if I need to do anything else. Thanks!

@Priya91
Copy link
Contributor

Priya91 commented Sep 16, 2016

RemoveRange(int index, int count) instead of RemoveRange(ICollection) ? How does RemoveRange behave when the ICollection elements are duplicated in ObservableCollection

@Priya91
Copy link
Contributor

Priya91 commented Sep 16, 2016

count instead of endIndex..

public void ReplaceRange(IEnumerable<T> collection, int startIndex, int count)

@Priya91
Copy link
Contributor

Priya91 commented Sep 16, 2016

public void AddRange(IEnumerable<T> collection, bool clearFirst = false) { }
public void InsertRange(IEnumerable<T> collection, int startIndex) { }
public void RemoveRange(int startIndex, int count) { }
public void ReplaceRange(IEnumerable<T> collection, int startIndex, int count) { }

@thomaslevesque
Copy link
Member

Basically the signatures should be the same as in List<T>.

I don't think the clearFirst parameter in AddRange is useful, and anyway optional parameters should be avoided in public APIs.

A RemoveAll method would be useful all well, for consistency with List<T>:

public int RemoveAll(Predicate<T> match)

@robertmclaws
Copy link
Author

I think RemoveRange(IEnumerable<T> collection) should remain. It would cycle through collection, call IndexOf(item) and then call RemoveAt(index). Duplicates of the same item would also be removed.

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

I'm not against a RemoveAll function.

@thomaslevesque
Copy link
Member

Also, the index parameter usually comes first in existing APIs, so InsertRange, RemoveRange and ReplaceRange should be updated accordingly.

And I don't think ReplaceRange needs a count parameter; what should the method do if the count parameter doesn't much the number of items in the replacement collection?

Here's the API as I see it:

public void AddRange(IEnumerable<T> collection) { }
public void InsertRange(int index, IEnumerable<T> collection) { }
public void RemoveRange(int index, int count) { }
public void ReplaceRange(int index, IEnumerable<T> collection) { }
public int RemoveAll(Predicate<T> match)

@thomaslevesque
Copy link
Member

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

I'm not sold on it, but hey, it's your proposal, not mine 😉. At the very least, I think it should a separate overload, rather than an optional parameter.

@thomaslevesque
Copy link
Member

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

This makes me think... there are lots of possible combination of changes you might want to do on the collection without triggering events for each one. So instead of trying to think of each case and introduce a new method for each, perhaps we should lean toward a more generic solution. Something like this:

using (collection.DeferCollectionChangedNotifications())
{
    collection.Add(...);  // no event raised
    collection.Add(...); // no event raised
    // ...
} // event raised here for all changes

@robertmclaws
Copy link
Author

robertmclaws commented Sep 16, 2016

@thomaslevesque Overload vs optional parameter makes no practical difference to the end user. It's just splitting hairs. Having overloads just adds unnecessary lines of code.

ReplaceRange with a count would remove all items in the given range, and then insert the new items at that point. The counts not matching would be irrelevant.

If the index comes first in existing APIs, then I'm fine with this:

public void AddRange(IEnumerable<T> collection, clearFirst bool = false) { }
public void InsertRange(int index, IEnumerable<T> collection) { }
public void RemoveRange(int index, int count) { }
public void ReplaceRange(int index, int count, IEnumerable<T> collection) { }
public int RemoveAll(Predicate<T> match)

@thomaslevesque
Copy link
Member

@thomaslevesque Overload vs optional parameter makes no practical difference to the end user. It's just splitting hairs.

It's not. Optional parameter can cause very real issues when used in public APIs. Read this blog post by @haacked for details.

@shmuelie
Copy link
Contributor

I'm actually liking @thomaslevesque's idea about using a batching class. It's a common pattern, well understood, and makes complex workflows easier.

@thomaslevesque
Copy link
Member

ReplaceRange with a count would remove all items in the given range, and then insert the new items at that point. The counts not matching would be irrelevant.

That would be quite inefficient. Removing items would cause all following items to be moved backwards, and inserting new ones would cause them to be moved forward again. The implementation I have in mind would replace each item in-place, without moving anything.

@robertmclaws
Copy link
Author

So instead of trying to think of each case and introduce a new method for each, perhaps we should lean toward a more generic solution.

The point of this proposal was to fill in the gaps on the existing implementation, not coming up with a new pattern for people to deal with. I'm not against that proposal, but that's an entirely new piece of functionality that I don't believe should be a part of this discussion.

@shmuelie
Copy link
Contributor

@robertmclaws but since there is no way to currently do bulk operations there isn't a "new pattern"

@robertmclaws
Copy link
Author

That would be quite inefficient. Removing items would cause all following items to be moved backwards, and inserting new ones would cause them to be moved forward again. The implementation I have in mind would replace each item in-place, without moving anything.

Why does that matter? Is it a memory allocation issue?

@thomaslevesque
Copy link
Member

that's an entirely new piece of functionality

I agree that it should probably be a separate proposal, but it does solve the initial problem you were having.

@Daniel-Svensson
Copy link
Contributor

I am pretty sure it has been mentioned before but the best change of the changes approved and still get a large performance boost would probably be to raise "Reset" event for batch changes to indicate "The contents of the collection changed dramatically." . Reset is the current best approach to get good performance with WPF and have worked fine for years

  • Normal Add/Remove should be used when the number of changes allows (< 2, but possibly some other low number in the future if it is better performance wise across several frameworks)

  • AppContext switch can be added to opt-in to have ObservableCollection raise batch version of Add/Remove events which in the future maybe can changed to opt-out (maybe depending on target framework)

It is important to remember that it is not only the core libraries needs to update but also 3rd party component vendors will need to change.

@thomaslevesque
Copy link
Member

@Daniel-Svensson good point. Yeah, a single Reset event would probably be better than many individual events.

@robertmclaws
Copy link
Author

robertmclaws commented Nov 9, 2023

@Daniel-Svensson @thomaslevesque Guys we literally already worked all that out in the API. It's already been approved. The ...Range methods let you add multiple items. The existing collection methods are unchanged.

The downstream issues with existing UI frameworks are the only remaining issues that need to be solved at this point.

There is no AppContext in .NET Core. AFAIK the compatibility switch would be implemented another way, regardless of whether it is opt-in or opt-out. I stand corrected.

The switch would determine whether the Changed event in the ___Range() methods would fire after individual entries, or after the whole operation is finished (the latter being the new desired behavior).

Either way, the switch, where it would be implemented, and how to test for it are presently what is being proposed.

@eiriktsarpalis
Copy link
Member

There is no AppContext in .NET Core. AFAIK the compatibility switch would be implemented another way, regardless of whether it is opt-in or opt-out.

Maybe I'm misunderstanding, but AppContext switches are very much a thing in modern .NET.

@robertmclaws
Copy link
Author

For reference on figuring out how to implement the switch: https://devblogs.microsoft.com/dotnet/net-5-new-networking-improvements/

image

@thomaslevesque
Copy link
Member

Guys we literally already worked all that out in the API. It's already been approved. The ...Range methods let you add multiple items. The existing collection methods are unchanged.

We're not talking about changing the already approved API (at least I'm not), just how it's implemented. If the feature switch is enabled, have the *Range methods send a single Change event. If not, send a single Reset event.

@dotMorten
Copy link

A reset event would be terrible for those controls that do support adding multiple items. The by far most common multi-item change event would be adding a set of results to a list. Like when you scroll to the bottom, you load 10 more. We don't want the entire listview to be rebuilding while also losing your current scroll offset.

@thomaslevesque
Copy link
Member

A reset event would be terrible for those controls that do support adding multiple items.

But for those, the feature switch would be enabled, so it wouldn't be a Reset event.
Unless you mean that there are frameworks where some controls support it, and others don't?

@robertmclaws
Copy link
Author

robertmclaws commented Nov 9, 2023

@thomaslevesque Hmmm. I see what you're saying, apologies for being a little thick-headed on this particular topic. So in the compatibility situation, you would still get the refresh but lose the granularity of knowing the exact changes.

@dotMorten I think the question would be for the legacy platforms that have UI controls that already throw exceptions if there is more than one item in the collection, does that actually matter? AppContext switch would potentially only be turned on for those apps.

@dotMorten
Copy link

dotMorten commented Nov 9, 2023

I really don't think having the switch is a good solution (as it'll still throw until you find this secret switch).
Again I go back to that this isn't a new issue, and WPF and WinUI is already broken if you add your own AddRange method which wouldn't know about this switch either (see below).
No apps today obviously calls AddRange unless you created your own version, so no upgrade will hit it.
Now there's the question of new apps using AddRange, but developers would quickly find out it doesn't work - maybe it'll even help put some pressure on WinUI and WPF. If anything, WPF and WinUI could add analyzers to warn about using it.

Anyone doing the below will already hit the issue, so IMHO we're not breaking anything - we're only making it slightly more obvious that something is already broken:

    public class BetterObservableCollection<T> : ObservableCollection<T>
    {
        public void AddRange(IEnumerable<T> collection)
        {
            int startIndex = this.Count;
            foreach (var item in collection)
                base.Items.Add(item); // this won't raise any events
            if(this.Count < startIndex) {
                base.OnPropertyChanged(new PropertyChangedEventArgs(nameof(Count)));
                base.OnPropertyChanged(new PropertyChangedEventArgs("Items[]"));
                base.OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add,
                changedItems: collection.ToList(), startIndex));
            }
        }
    }

@thomaslevesque
Copy link
Member

I really don't think having the switch is a good solution (as it'll still throw until you find this secret switch).

Not with the plan I had in mind (the switch is disabled by default, and is only enabled by frameworks that support it)

And yes, the issue already exists if you write your own AddRange. I think I'm either missing your point, or you're missing mine...

@Daniel-Svensson
Copy link
Contributor

A reset event would be terrible for those controls that do support adding multiple items. The by far most common multi-item change event would be adding a set of results to a list. Like when you scroll to the bottom, you load 10 more. We don't want the entire listview to be rebuilding while also losing your current scroll offset.

@dotMorten What framework have issues with the Reset event?
I've been running with Reset for WPF (even Silverlight) for over ten years and both built in controls and the 3rd party controls I've tried works as expected whit Reset, they continue to maintain the "current" scroll show the the same index in the list just as expected.

If there are multiple other frameworks which have issues with Reset that are difficult to fix then we should add the API as is and raise multiple Add /Remo replace (if anybody is listening to the event).
The proposed API will still improve perfomance and makes the collections more convenient to use

@dotMorten
Copy link

dotMorten commented Nov 11, 2023

What framework have issues with the Reset event?

None, considering it is used for Clear as well. But it causes a complete rebuild of the UI. That's not good, if all you did was add a couple of items to your list. The entire reason to add support for AddRange is for performance reasons - let's not lose sight of that.

@tranb3r
Copy link

tranb3r commented Nov 17, 2023

So disappointed to see this discussion going nowhere...

@terrajobst terrajobst modified the milestones: Future, 9.0.0 Nov 28, 2023
@terrajobst
Copy link
Member

We're re-considering this for .NET 9. I can't make promises, but I want to reflect that it's on the docket.

My goal for .NET 9 is to make a definitive decision: either we do it as proposed or we make changes to it in order to get it done or we're gonna reject it. Either way, I want to resolve this issue.

@driver1998
Copy link

driver1998 commented Nov 29, 2023

For WinRT interop, if IObservableVector can not change (because it is system API in Windows.Foudation.Collections), we can always generate wrapper code in cswinrt to go back to add events in a loop.

Also WinUI 3 has Microsoft.UI.Xaml.Interop.INotifyCollectionChanged, I can't see why that can't be
used instead.

Yeah Microsoft.UI.Xaml.Interop.INotifyCollectionChanged also support multiple insertion and deletion.

https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.interop.notifycollectionchangedaction?view=windows-app-sdk-1.4

Add | 0 | One or more items were added to the collection.

So it is more or less in the same boat as WPF I guess?

UWP and System XAML are legacy and we don't support them on .NET 9 anyway.

@airbreather
Copy link

TL;DR at the bottom.

AFAICT (I haven't read ALL the 400+ comments above this one in the thread, sorry), the issue to be solved is the same one that triggered the revert back in 2019 (dotnet/corefx#38115).

Quoted from #18087 (comment) above:

But migrating WPF apps probably won't suffer from those exceptions as they don't make use of the range features anyway, it's only new WPF apps that will have issues

If that was the case, I'd agree. Unfortunately, I believe that it's a common pattern that people added an AddRange extension method that did this functionality. I know I did, and it's the logical method name to use. With this change, that method isn't used. It'll compile just fine but then lead to a runtime exception later -- which in some ways is worse than if it just failed to compile. If that was the case, people would know and could fix.

Looking back at the analysis that was done at the time, it's summed up quite nicely in #18087 (comment) above:

But honestly this is a net new method. How can MS possibly account for every extension method ever conceived in history?

Normally we consider breaking hypothetical extension methods to be an acceptable breaking change, at least in a major version. The concern is whether this is going to affect enough people that it's not.

Narrator: it wasn't.

Taking that into account, from my outsider perspective, I see only one path that doesn't lead to "reject". One step along that path is to enhance WPF so that, where it currently throws an error in the code paths that would start getting lit up, it does something correct instead. There seems to be an outline (of sorts) for how this could look: dotnet/wpf#6097 (review).

As I mentioned in a reply to that "outline (of sorts)", I'm not super interested in having WPF do the best thing that it theoretically possibly can do when it sees this, only that it does something that's correct enough in the blocker scenarios to influence the decision on what to do with this issue.

While a WPF enhancement wouldn't change the fact that this would break "hypothetical extension methods", I do expect that it would move the needle a big distance away from that "unacceptable" zone that Dan alluded to in the comment that I linked and quoted above, based on how I expect that such extension methods are used and implemented (and my expectation is informed by my own experience, which agrees with Claire's experience that I linked and quoted above).

TL;DR: It looks like we don't have this today because too many people use the trivial obvious AddRange extension methods, and so if this happened today, then it would break too many people's perfectly fine and correct applications. So if this is going to happen, then part of the solution probably needs to be that those frameworks do at least enough so that this change will break only a few people's perfectly fine and correct applications.

@jnm2
Copy link
Contributor

jnm2 commented Nov 29, 2023

The C# language is considering a means of providing warnings for planned breaking changes. I wonder if the SDK could do something similar: flag AddRange extension methods even while you still target .NET 8, warning that they may break the app upon upgrading to .NET 9/whatever.

@terrajobst
Copy link
Member

My approach would be:

Get the change in early for .NET 9 and see what feedback we're getting. Ignoring UI frameworks themselves (WPF, UWP, WinUI, Avalonia, and Uno are the ones I'm tracking) I don't think there is a high risk for breaking changes to be honest. I'm sure it will break someone somewhere, but that's true for virtually all behavior changes. The question is whether there is a particular pattern that breaks sufficiently many people. So far, I don't see sufficient evidence for that; if there is, I think a pre-Build conference preview will help us identify that.

@dotMorten
Copy link

@terrajobst It might be good to put an issue into Maui, WinUI and WPF repos, to give them a heads up that this change is coming. Since anyone can just subclass ObservableCollection today and add the add-range method, those UI frameworks can already today start testing what would happen and prepare accordingly. It might just be they do a quick fix to convert to multiple single-item changes in a first pass, or warn people not to use the new methods if actively binding to the collection.

@terrajobst
Copy link
Member

Agreed. I'm actually being a bit more practive here: I'm reaching out to all the owners the UI frameworks (WPF, UWP, WinUI, Avalonia, and Uno) and try to discuss it with them.

@tim-gromeyer
Copy link

Even after 7 years, a multi-trillion ($2.7 trillion) company hasn't managed to add this 🤦‍♂️

@symbiogenesis
Copy link

@terrajobst any updates? This is one of the top-requested features in .NET going back many years. I remember seeing a list of all feature requests sorted by likes, and this one was like number 5 overall. Absolutely huge community interest in this.

@KevinCathcart
Copy link
Contributor

I hesitate to even suggest this, because it should be a last resort, but there is one final alternative for adding these that would allow for not breaking existing UI frameworks while still allowing other frameworks and other parts of applications to consume multi-item changed events that I did not see explicitly discussed in this issue.

That alternative being to:

  1. accept that widespread throwing implementations of event handlers for INotifyCollectionChanged has effectively poisoned the previously designed multiple item version of the INotifyCollectionChanged protocol. The simple fact is that even if WPF and WinUI get addressed, it is not immediately obvious how many custom controls and other handlers that make similar assumptions exist, and it might turn out that that number is too large to accept. (This would really only come to light after testing with a WPF and/or WinUI patched to fully handle such events).
  2. Instead add a new protocol for multi-item changes that only gets consumed only by listeners that definitely can handle them, with collections (like ObservableCollection<T>) that want maximum compatibility with GUI frameworks and other older code being encouraged to emit single item events on the old protocol for best compatibility.

I'm not really in love with this, as it would mean existing handlers that are already multi-item event capable would need changes to opt in to seeing multiple items from ObservableCollection<T>. But it might be better than just refusing to implement these methods at all due to breakage risk. This basic alternative can be done with a few approaches, all of which need to add additional public API surface, obviously.

The easiest such approach is a new interface, that is nearly identical to INotifyCollectionChanged, but with a different event name, and only consumers that support multiple items will look for and subscribe to the new interface's event. Subscribers on the old interface for ObservableCollection<T> would pessimistically be assumed to be unable to handle multiple items, since that is the most widely compatible option.

A less enticing alternative approach (since I already typed it) An alternative approach without needing a new interface would be to always have `ObservableCollection` emit multiple single item events, but provide a way to pass additional information with each event that receivers which opt-into handling multi-item changes would look for. In this scheme the first event in a generated single item event sequence would also pass along the multi-item operation details, and the later events including a flag indicating "I'm a single item event that was sent as part of multi event sequence, so if you know how to find and process the multi-item details passed with the first event, you should skip processing me". This would let subscribers perform the bulk action on receiving the first request, and then just do nothing on the rest of the single item events from that sequence. The idea of documenting this approach seems horrifying, and having to send events that often get ignored is not ideal perf wise, but it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Collections
Projects
None yet