-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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. |
It has been discussed extensively in this thread, but TL;DR we can't just break WPF users and get away with it by declaring that "WPF is bugged". If you take a look at the history of the issue this was implemented, merged and then promptly reverted because it broke too many applications. The change has to be made in coordination with downstream frameworks, which takes a long time. |
@eiriktsarpalis That's understandable. Is that coordination with downstream frameworks happening, though? |
At this point it's not on track for .NET 9 inclusion. I've updated the milestone to reflect that. |
Remember #18087 (comment)
|
OP + OPR here. Being totally unsurprised that this happened, I'd like to share my thoughts in a mostly-positive-yet-honest way... an attempt to think critically without being overly or unreasonably critical. AppreciationI want to start off by saying that I appreciate everyone internally that has tried to spearhead getting this done yet again. Your efforts have not gone unnoticed, and while we are defeated yet again, I sincerely appreciate you so much. And for everyone that has also been waiting for this & has put in effort to move it along, thank you so much. ObservationsI opened this issue well before WPF was ever on the new .NET, which was SUPPOSED to be a platform for .NET to move fast and break things. Ever since we got it over the finish line just to be rejected a few hours later, subsequent release cycles have just been met with the same excuse on an infinite loop that is now nearing an
It actually won't take that long at all. BUT as we've clearly seen, it takes a lot more time if nobody actually does it. 🤷🏻♂️ There are ways to fix the problem that will minimize impact. But after this many release cycles it is crystal clear there is at least someone at Microsoft that just don't want to. That's not how Open Source is supposed to work. Sometimes you have to force change by breaking things. That's the whole point of a SemVer 2.0 major release integers. It just requires the fortitude to actually do it early enough to let people take the time to fix it while giving them clear guidance how. Impact AnalysisWhy we're allowing every other UI library to suffer because of one terrible decision made 15+ years ago is beyond me. There are significant, documented and obvious performance improvements from this fix that will significantly improve every app that uses these UI libraries. How Microsoft Should Solve This From HereThis October when .NET 9 is forked for release, this should be the VERY FIRST pull request accepted to the .NET 10 branch. Rip off the band-aid and just do it. Everyone else will have a full year to fix it. The UI libraries will fix it, trust in them. If we need to build analyzers to help between now and then, awesome and happy to help. How I Am Solving This Issue From HereHonestly, after this issue has been open for more than 2 US Presidential cycles (it's a month away from its' 8th birthday) we now have C#13 Implicit Extension Types around the corner, along with the
Maybe Microsoft will see that this is a valid approach and make it an official solution for .NET 9... tho I will certainly not be holding my breath. ConclusionIt will be real nice to see Blazor work the way it is supposed to, unencumbered by architectural decisions that never should have survived a code review during the Longhorn Reset. If you read this far, again I really appreciate you. I will continue to advocate for this solution for as long as I am able. |
I don't get the argument that this will break WPF. As has been pointed out the interface already support multi item change and a custom collection could do it's own AddRange according to spec and it would break WPF today. |
@dotMorten, @SkyeHoefling pointed out the core problem here: #18087 (comment) In short, someone on the WPF team almost 20 years ago decided that the So now there is code dependent on that behavior, including downstream controls that are specifically designed NOT to have more than one change in the set. The fix is simple: delete the validation code and anything referencing it, then see what breaks and fix it. It's really NOT that complicated. My personal opinion is that someone involved with this boneheaded decision is still on the WPF team and have dug their heels in because they don't want their "record" blemished with an expensive fix. Once this change broke their code, they threw up their hands and have been a never-ending roadblock. (Anyone on the WPF team is free to e-mail me and explain otherwise). I have never really understood why they keep getting in the way, because unblocking this would be a MASSIVE perf improvement for WPF. Like by orders of magnitude. This single flaw is a huge reason why people perceive WPF as such a dog when it comes to performance. At the end of the day, I wish someone would just tell us who specifically is blocking this, so I can have LinkedIn notify me when they change jobs. I feel like that's the only way this is ever going to get an official fix. @dotMorten regarding your solution about adding a compatibility switch, I feel like I know some of that may sound mean or rude, and that's not my intention. But after 8 years I just don't know how else we're all supposed to look at it. There is no justifiable reason this remains unfixed. I'd be happy to go in and create PRs for every single problem that comes up across the ecosystem. But I've been burned so many times on this I wouldn't do it without assurances they'd be accepted. That's my $0.02. HTH! |
IIRC (this thread is way too long to search, so I'll just go from memory), the idea was that many existing WPF applications have an extension method that looks like: static void AddRange<T>(this ICollection<T> @this, IEnumerable<T> items)
{
foreach (T item in items)
{
@this.Add(item);
}
} Calling that extension method to """bulk""" insert a sequence of items into an Generally, the .NET team will accept changes like this, but I think the conclusion was that this pattern is just WAY too common to justify the incremental benefit of having the range methods. I made a comment over here dotnet/wpf#6097 (comment) proposing a stop-gap that ought to be reasonably cheap to implement in the |
This was attempted in the PR that triggered the comment I linked in my previous reply on this thread, dotnet/wpf#6097.
The first comment on that PR, dotnet/wpf#6097 (review), included a bulleted list of five items that need to change in order for it to be OK to remove the validation. Furthermore, as the discussion continued on that thread, someone else chimed in, dotnet/wpf#6097 (comment), pointing to a specific line of code. The link is stale now... here's a permalink: https://github.com/dotnet/wpf/blob/ae1790531c3b993b56eba8b1f0dd395a3ed7de75/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Data/ListCollectionView.cs#L1748 Personally, I'm also a little perplexed as to why this is still considered to be a big enough blocker when the
|
This is not true and completely misrepresents how the .NET team(s) work (and the type of challenges they are facing). I don't think anybody ever expressed the opinion that this shouldn't be fixed, in fact consensus is very strongly in favor of this being addressed eventually. The simple reality is that resources are limited and priorities can be conflicting so as with any effort requiring substantial coordination the windows of opportunity are tiny and easy to miss. |
WPF is open source. |
Another solution to problem of Current problems:
I propose to create Downside is that UI frameworks will need to adopt new type. |
Since the error only strikes on recompile, why not have WPF ship an analyzer with a build error if any AddRange that now binds to the instance method could also find an AddRange extension method in scope that it would have previously been calling? If deleting or dereferencing the extension method is not an option, you can silence the error once for the project/solution. Or, taking a page from the C# language's breaking changes strategy, start providing a warning on the extension methods today and leave it in place for at least one .NET cycle. The warning will tell you to future-proof your app to be ready for .NET 10/11, maybe with a light bulb fix having you rename the extension method so that you aren't broken on upgrade. |
I wish it was that simple... But in the WPF repo approving even the smallest fixes with no known risk may take forever. In contrast, WinForms guys usually react super fast, or even encourage me to send a PR so it will go through quicker. |
This is what I was trying to get at. As someone who's just been keeping an eye on this issue loosely, it seems to me like what keeps happening is that Microsoft says, "It's going to take time to coordinate", but nobody has actually even begun the process. |
Again, not accurate. The teams do make legitimate attempts to prioritize this issue every year (many of which are reflected in this discussion) however please consider that every engineer is accountable for literally hundreds of issues like this one and the fact that there sometimes exist conflicting business priorities. |
@eiriktsarpalis I understand that, what I'm advocating for is clearer messaging. When you say, "The change has to be made in coordination with downstream frameworks, which takes a long time" IMHO that doesn't tell most of us much about what the actual state of the issue is. Is it in progress? Is communication and coordination happening? Or are you saying that nobody has been able to commit to it yet, and it hasn't moved at all? It can lead to a feeling that nothing is happening on the issue. |
Whelp, Implicit Extension Types just got moved to .NET 10 (scroll to the bottom), so I guess I'm not solving this problem that way this year either. |
Yes and no. I can absolutely imagine that every engineer is accountable for hundreds of issues. But are those issues really "like this one"? That is, are they equally old, advanced, and perceived as important? Has each and every one of those issues invoked dozens of libraries and thousands of apps implementing workarounds, caused hundreds of questions leading to upvotes on this issue, got dozens of duplicates because they all want the same? I find it hard to believe that for 8 years, every single time the threshold of "important enough" lies just above the importance of this one. I think the real issue that it is always easier to add new stuff even if only 5,000 people need that, than to fix old stuff 10,000,000 people need when it breaks something for 10,000 of them. |
Yes, unequivocally. Issue age is not the only deciding factor. |
Having slept about it, I wonder: if the core reason this whole API is so hard to roll out to the public is "just" the batching of events breaking downstream libraries, would it be possible to add the new methods plus a switch This way there would be some movement in the right direction, that at least adds the new methods so people can start to use them. And if their program is capable of handling the batched notifications, they can change the switch to And later on, when sufficient progress was made in downstream libraries, the default of that switch can be set to And even if in the very distant future the switch would eventually get removed altogether, it still doesn't break anything, because it's just a compiler switch and defining them without any place in the code using them doesn't break anything either. I think it would be a good idea to get the API out there to be used, even if it doesn't provide performance benefits yet by default. But at least we would be able to start to do the right thing (use Start to do the right things now, benefit later. |
I support the implementation of a feature toggle but strongly oppose defaulting it to 'off'. This feature addresses a longstanding need within the community, bringing valuable new functionality. The real bottleneck lies with non-MS libraries and applications using custom extension methods, which impede progress. As this change does not affect legacy applications unless they opt for a newer binary, any update should handle either resolving breaking changes or disabling the feature via the feature flag. Since this is entirely new additive code and not a true breaking change, it should be included by default in standard updates. |
Defaulting to off, while debatable, would still be better than not shipping it at all in |
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: