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
Comments
While you're in there adding an AddRange() method, can you throw an OnPropertyChanged() into the Count property's setter? Thanks :) |
A long time ago I had implemented a |
@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with @robertmclaws ? /cc @terrajobst |
Sure. |
@robertmclaws Can you create an api speclet on this issue, outling the api syntax, like this. Mainly interested in usage scenarios |
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 |
@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. |
@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 Otherwise, adding a new member won't affect how old binaries behave. |
+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 |
@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 So if we're comfortable with the API, what's the next step? :) |
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. |
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! |
|
count instead of endIndex..
|
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) { } |
Basically the signatures should be the same as in I don't think the A
|
I think @thomaslevesque I have the I'm not against a |
Also, the index parameter usually comes first in existing APIs, so And I don't think ReplaceRange needs a Here's the API as I see it:
|
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. |
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:
|
@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.
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) |
It's not. Optional parameter can cause very real issues when used in public APIs. Read this blog post by @haacked for details. |
I'm actually liking @thomaslevesque's idea about using a batching class. It's a common pattern, well understood, and makes complex workflows easier. |
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. |
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. |
@robertmclaws but since there is no way to currently do bulk operations there isn't a "new pattern" |
Why does that matter? Is it a memory allocation issue? |
I agree that it should probably be a separate proposal, but it does solve the initial problem you were having. |
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
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. |
@Daniel-Svensson good point. Yeah, a single Reset event would probably be better than many individual events. |
@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.
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. |
Maybe I'm misunderstanding, but AppContext switches are very much a thing in modern .NET. |
For reference on figuring out how to implement the switch: https://devblogs.microsoft.com/dotnet/net-5-new-networking-improvements/ |
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. |
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. |
But for those, the feature switch would be enabled, so it wouldn't be a Reset event. |
@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. |
I really don't think having the switch is a good solution (as it'll still throw until you find this secret switch). 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));
}
}
} |
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 |
@dotMorten What framework have issues with the Reset event? 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). |
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. |
So disappointed to see this discussion going nowhere... |
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. |
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 Yeah Microsoft.UI.Xaml.Interop.INotifyCollectionChanged also support multiple insertion and deletion.
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. |
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:
Looking back at the analysis that was done at the time, it's summed up quite nicely in #18087 (comment) above:
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 |
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. |
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. |
@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. |
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. |
Even after 7 years, a multi-trillion ($2.7 trillion) company hasn't managed to add this 🤦♂️ |
@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. |
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:
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 The easiest such approach is a new interface, that is nearly identical to 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. |
Update 10/04/2018
@ianhays and I discussed this and we agree to add this 6 APIs for now:
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:
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).
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>
andObservableCollection<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 implementsINotifyPropertyChanged
andINotifyCollectionChanged
, 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:
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.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:
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#10751Obstacles
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 theOnCollectionChanged
event has a structure and constructors that support signaling the change for multiple items being added, theObservableCollection
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 overridesCollection.InsertItem()
, and all of the other methods are private. So the only way to fix this properly is in theObservableCollection
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!
The text was updated successfully, but these errors were encountered: