runtime: Collection and ObservableCollection do not support ranges
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<T>?
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
CollectionChangedevent 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
ChangedItemsLists 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!
About this issue
- Original URL
- State: open
- Created 8 years ago
- Reactions: 198
- Comments: 370 (216 by maintainers)
API Review 2nd round
In order of appearance in code
See implementation and explanations in PR dotnet/corefx#26482.
public void AddRange(IEnumerable<T> collection);public void InsertRange(int index, IEnumerable<T> collection);public void RemoveRange(int index, int count);public void RemoveRange(IEnumerable<T> collection);public int RemoveAll(Predicate<T> match);public int RemoveAll(int index, int count, Predicate<T> match);public void ReplaceRange(IEnumerable<T> collection);public void ReplaceRange(IEnumerable<T> collection, IEqualityComparer<T> comparer);public void ReplaceRange(int index, int count, IEnumerable<T> collection);public void ReplaceRange(int index, int count, IEnumerable<T> collection, IEqualityComparer<T> comparer);Legend:
Notes:
List<T>), down toCollection<T>(The base ofObservableCollection<T>).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.
Well, this is disappointing… but I’m not going to suggest that Microsoft put on its’ big-kid panties if I’m not willing to do it myself, so we move forward. 😃
I spoke with Karel and Andrew, and we have the beginnings of a plan to tackle this properly in .NET 5… I will open a new issue soon and link back to it here when we’re ready for a bigger discussion.
Regarding any API changes, I am going to HIGHLY suggest that we keep pressing forward with completing the solution we have today, and not reopen the API Design Review process. I think the right thing to do @weitzhandler would be to let this process finish out and be merged into the codebase before you propose any new features. Here’s why:
We need to be respectful of the people that worked so hard to get this as far as they have. Andrew + the reviewers + the risk management team did a crapload of work, and Andrew is the only one (out of 3 people that tried!) to actually get this over the finish line. @weitzhandler, you and I both pushed PRs that weren’t accepted… so we should be working to be tailwind for Andrew, not headwind.
The original spec had some of those additional method signatures, and Microsoft rejected them in favor of a more simplified approach. We all discussed that (ad-nauseum, I might add) and I’m not sure the situation has changed.
Speaking only for myself (and trying to be diplomatic here), but your original PR would have been accepted if you hadn’t over-complicated it with stuff that wasn’t agreed on. When I first started on this, I had an implementation done and a PR submitted, based on Xamarin’s
ObservableRangeCollection. That PR was rejected for not having completed an API Design Review first. So I went through the process and opened this Issue like I was supposed to. You kind of jumped in halfway through and took over that process, tried to circumvent the approval by piggybacking on my work, and your PR was ultimately rejected for being off-spec. If you are thinking about being involved moving forward, the right thing to do here is assist in unblocking the current solution, not block it further by trying to again advance a twice-rejected agenda. Just my $0.02.At any rate, Andrew and I will be working together to come up with a detailed mitigation plan, and we’re start prepping to hit this hard when the .NET 5 branch is available. Thanks to @karelz @onovotny @danmosemsft @terrajobst and everyone else for your help over the last few weeks, and looking forward to getting this out the door properly. 😃
You know, I was thinking about this a lot again this afternoon, and reading another thread on Twitter about API breakage (between @davidfowl and @terrajobst). I really think we’re thinking about this the wrong way, and here’s why:
The
*Rangechanges are going to be a HUGE breaking change across the entire UI ecosystem. This stems from the fact thatObservableCollectionhas always been incorrectly designed.NotifyCollectionChangedEventArgshas always had the capability of containing multiple items in the notification, but from a practical standpointObservableCollectionnever actually had the capability of throwing an event with more than one item in the EventArgs, because you could only ever change one item at a time.There is going to be a metric crap-ton of customer code out there depending on that behavior. I cannot stress this fact enough, and I pointed it out in my very first post under “Obstacles”.
This is why there are so many different variations of subclassing out there that inherited from
ObservableCollectionand rolled their own Range implementation… because the design was bad in the first place. Xamarin had to ship their ownObservableRangeCollection.csin every VS project template they shipped, for as long as I can remember.We can fix this across the core frameworks all we want. But customer code is STILL going to have to change, because they will no longer be able to depend on the old behavior. Waiting until .NET 5 isn’t going to make that any better… in fact it’s actually going to make the problem a LOT worse, because you’ll have WPF code actually shipping on .NET Core, and right now you don’t.
Besides, we seem to be forgetting that WPF apps are not going to “just run” on .NET Core without recompiling and redeploying. Recompiling the same code without changes on a whole new framework is a nice idea, but should not be prioritized over making bad designs live forever on a brand new platform… otherwise why the hell did Microsoft put the industry through building a new framework in the first place?
The WHOLE point of .NET Core was to be able to start fresh and fix all the bad design decisions of the past, because you can have multiple versions of the framework installed on the same system.
I also feel it important to point out that if I had implemented this API three years ago when I proposed it (instead of spending all my available free time for it on the spec discussion and then getting burnt out), you’d still have this problem now anyways, only you wouldn’t be able to back out.
Thinking about the big picture, and what .NET will look like 5 years from now, I really feel like the best way forward is:
Just because it’s going to suck a little doesn’t mean it’s not worth doing. Otherwise we’re screwing the future for years just to save people 10 minutes of fixes on something they’re going to have to recompile anyways.
@layomia sorry, but I don’t understand why. This has been reverted in
3.0-Preview7to minimize the impact of WPF apps migration to .NET Core 3. This was understandable to allow a simple migration without side effects, it was a priority.Now .NET 5 is still in preview, not release candidate and breaking changes should still be allowed. Based on Announcing .NET 5.0 Preview 6 blog post, the release will be feature-complete with
Preview 8.The PR is already reviewed and accepted. In my opinion, this should be an acceptable change for
5.0-Preview7.Unfortunately, the cost of this feature is too high to fit in 5.0 at this stage in the release. The initial implementation in 3.0 had to be reverted due to the change breaking WPF (https://github.com/dotnet/runtime/issues/29721), and it’s likely we’ll be in a similar situation if we proceed in this release.
@Grauenwolf that is 100% inaccurate. Avalonia, Uno, Blazor… all can benefit from this change without giving a rat’s ass what WPF does. In fact, if this proposal was implemented in 2016 when it was first discussed, WPF would not have been an issue because WPF was not a part of .NET Core.
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.
@layomia could be 6.0.0 candidate? This would allow to be supported from the first version of .NET MAUI 😀
@robertmclaws
This a really well written and reflecting comment on what it means to work together to achieve more. This is truly remarkable and part of the reason why I love our OSS community so much. You all rock. You’ve all been incredibly patient with us. This API has been sitting there for a long time before we even looked at it.
@weitzhandler
Don’t feel too bad. It’s quite clear based on your effort and PRs that you care about this API and want the right thing to happen. As I’ve said before: I prefer working with people who care, because more the key differentiator between a good dev and a great dev isn’t necessarily skill, but level of care.
As the OP who has dedicated hundreds of hours to his life on this problem, everything I say here is with peace & love to everyone watching this thread with great interest, and none of it is meant to be offensive… it’s meant to be a focusing function.
The simple problem that has delayed this for 5 years now is that WPF highly leverages
ObservableCollections, and certain controls in WPF have specific behaviors that THROW EXCEPTIONS if more than one item is present in the CollectionChanged event. That wasn’t the most brilliant engineering decision ever made, but we’ve all made those kinds of mistakes in our coding careers.The problem now is that it’s a major breaking change that has significant downstream impacts, as many of those controls are subclassed by thousands of other controls in the ecosystem, and those controls may depend on those exceptions at runtime.
We know for a fact it has downstream impacts because the fix made it into .NET Core 3.0, and had to be immediately rolled back because a ton of stuff broke, and the WPF team was trying to get their .NET Core port finished in time… so they didn’t have the bandwidth to deal with it. It was a very emotional situation for those of us that worked so hard to get this over the finish line.
Creating a new collection type will not fix the problem. Extension methods will not fix the problem. This will fix the problem:
ObservableRangeCollection.csfile in every project, which inherited fromObservableCollectionand added the Range behavior we’re overriding. Because it was a raw file and not in an assembly, there could be thousands of permutations of that file floating around.That’s it. If we can stay focused on that, we can get this done. After nearly 300 comments over 5 years, new outside commentary is not exactly helpful, as GitHub doesn’t make it easy to highlight useful discussion and hide noise.
The only thing missing at this point is attention from the WPF team to give it the engineering love it needs, as we already have PRs to solve the problem.
If you cannot help with that, then please consider adding emoji reactions to the first post on dotnet/wpf#1887 to show the WPF team how important it is to you.
If you got this far, again with peace and love, thank you for your support on this fix. @ahoefling, @karelz, @terrajobst, @michael-hawker, @predavid, @ryandemopoulos, and I appreciate you, and we look forward to FINALLY getting this into your hands as soon as humanly possible.
Here is how I feel about this:
This is an acceptable breaking change. It’s clearly valuable and completely sensible to support this in UI. It’s likely that we can’t always support it in the most efficient way, but at the very minimum the receiver can always treat it as individual events or as a reset.
The product needs to self-consistent. Even for acceptable breaking changes, we can’t ship them if part of the product can’t deal with them. Of course, we don’t always know this upfront, but we do now know about WPF being broken in 3.0.
It’s very late in .NET Core 3.0. We only have a few months left and the WPF team still has a ton of work to do. Judging by the number of references from WPF in .NET Framework, this is likely going to require some non-trivial amount of work in WPF. We don’t believe we have enough runway in 3.0 to walk this entire list and ensure WPF works consistently.
My position is that we postpone this API to the next major version of .NET (because .NET Core 3.1 is mostly minor fixes).
I’ve assigned the issue to myself in order to take a deep look into it and decide whether it fits in the framework or not for our 3.0 plans.
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.
Given the positive feedback, let’s see if we can re-review the updated proposal:
Which adds the global flag to the API already approved in https://github.com/dotnet/runtime/issues/18087#issuecomment-428312248
Looks good, but we should add a
ReplaceItemsRange:A similar proposal has been rejected in CommunityToolkit.Maui, because they’re deferring all MVVM stuff to the MVVM toolkit. Another proposal to include it in CommunityToolkit.Mvvm has also been rejected, because this should be part of dotnet. But it’s been discussed for more than 7 years… this is ridiculous. Meanwhile, we have no other solution than to include old nugets like MvvmHelpers or copy the code. So, what’s the status of this proposal as of today? I understand this won’t be part of net8, but is there any hope to have a solution soon?
🚩🚩🚩So, I just noticed is that there is a nasty side effect of
ObservableCollection.UseRangedNotifications, in that a third-party dependency could decide to silently disable functionality that the rest of an app needs in order to function properly.Picture this scenario: You’re building an app and decide to use a UI framework like HAVIT.Blazor to implement a new dropdown. That framework can’t be bothered with fixing their implementation (not that particular team because they’re awesome), so they decide to bake
ObservableCollection.UseRangedNotifications = false;into their library.Now, an app that was working fine 5 minutes ago is not behaving the same way, with no indication as to why there is a problem. Performance in the rest of my app could go to 💩💩💩 because someone else got to decide for me whether my code works the way I want it to or not. That’s not cool, to put it nicely.
It seems to me that good people working on this issue keep trying to engineer solutions to problems that 👉SemVer👈 is supposed to fix. A major version update indicates that there are things that might be broken, and update at your own risk.
Libraries that don’t implement the change properly can update NuGet package definitions to filter out dependent versions that don’t work until they are fixed. Those same libraries can be coded to behave differently on different .NET versions thanks to version-dependent conditional compilation constants. Breaking changes & mitigations can be cleanly documented in release notes, and expected behavior can be maintained with minimal effort.
I believe the proposed API flag is a flawed design that swaps one problem for an equal and opposite problem, will make things worse not better, and should not be implemented.
Hey there, we’ve talked with the MAUI team (tracking issue here: https://github.com/dotnet/maui/issues/6471), and they’ve added this feature to their .NET 7 milestone and said they’d test this as soon as it’s out to ensure it works great there. @ahoefling is currently working on an implementation (#65101), and also said he could contribute support for this for WPF, which I believe was the last blocking issue on the DevDiv side (considering MAUI will support this already). Given this proposal has also already gone through .API review and been approved, could we get this added to the .NET 7 milestone too? 🙂
cc. @terrajobst, @eiriktsarpalis, could you help with this? I reckon the “blocked” tag should also be removed now, since the reason this was blocked last time around (no WPF support) should no longer apply at this point anymore, as mentioned.
Thank you!
@robertmclaws thanks for you thoughts, unfortunately I have to respectfully disagree with some of your points – the landscape is a bit more complicated than what you highlighted above. Here are my points:
Yes, that was the original plan in .NET Core 1.0. And then the reality hit us and had to put back TONS of old APIs in .NET Core 2.0, reverted our Reflection “start-fresh” work, etc. All that mainly because that was the key blocking problem for migration to .NET Core from .NET Framework – too many different things. Without backing out on our “start-fresh full-blown” idea, .NET Core platform would not have the success it has today. We are still able to break things and right some wrongs, but we are extremely picky and cautious about which things to do – the benefit has to be clear, the impact understood, the migration path has to be paved.
While that is true, history shows that people don’t like surprises. They love when they recompile and things just work. Whenever they hit issues, especially those that are not super-clear from the error how to address, they lose faith in the platform. It creates anxiety. And it hurts platform adoption. I am not saying we have to be 100% compatibility, but we have to be super-smart and picky about which things we want to break. Moreover, WPF is targeting mainly enterprise customers. Enterprise customers are known to like surprises / differences even less than the rest of the developer community.
The fact is people expect things to just mostly work. The fact is that we cannot get rid of all the bad designs - we got rid of some, which are super-painful and held the platform innovation back (think AppDomains, remoting). And even those are painful and slow down migrations. The key value of .NET Core is side-by-side and ability to break a bit over time. In corner-cases. Or in places where the impact is understood and we have clear data it won’t prevent people from migrating from .NET Framework or older .NET Core versions, and it won’t negatively affect libraries ecosystem.
The problem exists in the platform for ages. Since its inception. I don’t see how waiting one more release is going to make a significant difference. It is not ideal, but far from end of the world IMO. Yes, we will have more WPF code on .NET Core 3.0, but the key value of waiting is: We will FULLY understand the impact of this change on ecosystem and what has to happen. Right now we are 5 minutes from 12 and that is REALLY BAD time to make drastic changes which may slow down adoption.
First, I am sorry you got burned out on this. On the other hand, I am not surprised, because this is a COMPLEX problem. There are many opinions and the impact is HUGE. Finding the right balance between “right solution” and “compatible enough” is EXTREMELY important here and therefore it takes time and lots of effort. It is a significant challenge. Thank you for driving the effort! Please don’t take the current state as invalidation of your effort, contributions and ideas. They are great. The platform and ecosystem needs just a bit more bake time on the plan to make it happen in least-disruptive way. The key DIFFERENCE between having the changes in now (3.0 RC time frame) vs. in last release is: We would have had the whole .NET Core 3.0 release to discover the problem and address it. You are right we would not be able to back out the changes if they shipped in 2.1, but we would have more runway to address WPF scenarios, think of mitigations, migration path, documentation, Roslyn analyzers, more testing and validation by customers, etc. … we do NOT have such runway now. We will have it again in .NET 5 though, if we make the change early on.
I agree. These are good arguments to do it in .NET Core. I just disagree that these are arguments for pushing changes into .NET Core 3.0 vs. .NET 5.
Your quote of “10 minutes of fixes” hints that you understand all the implications of this change on WPF code base and other scenarios more than we do (incl. our WPF experts). That is what I highlighted as next step above: https://github.com/dotnet/corefx/issues/10752#issuecomment-497870471 (see point [1]).
Most of my comments above are about the fact that currently we DO NOT UNDERSTAND all impact on WPF scenarios and given where we are in the release, risk-management kicks in, which leads to ideas like pushing the change out to .NET 5.
If you or anyone can help us with full analysis and understanding of the changes necessary for WPF scenarios (and other UI scenarios), then this may have a chance to get into .NET Core 3.0 still (no promises as the decision is not just up to me). Sadly, we won’t have time to do it ourselves though, as we are focusing on other higher-priority changes and fixes in the code base. From our point of view, this one is nice-to-have, but not critical for the release (just trying to be transparent here). Once we have full understanding, we can start the discussion, if it is acceptable, if there are better alternatives, better tooling that needs to accompany it, etc. If we get to consensus in time (and we do not know the exact deadline at the moment sadly), then I can see it happening in .NET Core 3.0.
Does it make sense? Does it help to explain our point of view?
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:
Why not put it behind a legacy app switch instead?
AppContext.SetSwitch("Switch.something.something.observablecollection.usesinglenotifications", true);Raising multiple events is what the system already does. The point of these changes is to NOT have the UI repaint on every item insert. This means we need to take the time to do it right, and WPF/Xamarin/UWP will benefit far more from implementing the API properly through the stack than from taking shortcuts to ram it through any given release window.
It’s not. Optional parameter can cause very real issues when used in public APIs. Read this blog post by @haacked for details.
Dear Jesus, no. We were at the finish line. Let’s just fix the problem and move on, not overcomplicate it worse.
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.
I agree that a flag would be a good way to solve the problem, but I don’t think it should be a public static settable property, because it would allow any code, including 3rd party libraries, to change this setting at runtime. Why not use the usual feature flag mechanism in
runtimeconfig.jsoninstead?There’s an interesting discussion happening currently in #65101. I’m singling out an idea proposed by @legistek
Effectively, it would be providing a global flag for impacted users to opt out from the breaking change.
@sharpninja To your point, an analyzer would certainly help notify developers if there are breakage issues… that’s not functionality that existed 5 years ago. Would love to see you or someone else build that when we’re ready, I think it would be a godsend.
@legistek I’m not sure why you’re coming in so hot and so matter-of-fact, but you are mistaken on virtually all your points.
It has nothing to do with second-guessing me, this is not about me. (My code is not even in the implementation anymore, it’s @ahoefling’s). It’s about getting the feature shipped on this release cycle. Shipping this feature costs Microsoft money, so minimizing the budget to get this done is VERY important to getting the WPF team to take it up… that means no unnecessary cycles.
If you were going to send the events individually, you would use existing codepaths to loop through the collection and call
Add(), just like everyone has done for almost 20 years.AddRangeis specifically to send the notifications in bulk, not to simplify adding individual items to the collection. So abulkNotifyparameter is redundant and adds unnecessary complexity.You continue to think the problem is with method naming. The problem is how other systems process the events that are raised. I’m not sure how much clearer I can be about that. Please stop suggesting method name changes.
I’m not sure why you decided that WPF wasn’t going to be fixed, but statements like that hurt our conversations with the team. We’re in a new development cycle and people are actively working on getting in front of the team to fix it. The .NET team presently busy shipping .NET 6 this week, and then taking the week off… so cut them a little slack.
The work is not likely to be re-scoped for .NET 7. Because we shipped a solution, and it had to be un-shipped. We’re going to re-ship it again and fix the downstream issues, not endure another API design review and implementation cycle.
To use a US football analogy: we’re at 1st and Goal from the 5 yard line. We’re not punting right now, we’re going to play the 4 downs we were given. Please let the team that’s on the field do their job.
Well, that really sums it up doesn’t it? @robertmclaws I understand you’re very close to this issue and can see how the perception that outside people are coming in years later and second guessing you could be frustrating. I’m also quite certain it was frustrating to have done so much work only to see it backed out in the seemingly rushed way it was.
I’ll thus not take personally your suggestion that none of us can add anything of value to this discussion. True, I did not read all 300+ comments - until now. I see that indeed, many of the points I raised have been previously litigated, including by yourself. I understand that the “breakage” in question was due to peoples’ extension methods being ignored in favor of the new ones - which can happen ANY TIME content is added to an existing class - and frankly I agree with your and others’ prior comments that this breaking change was acceptable. I see TPTB did not agree with you. They articulated their reasons well, I think, and it was their decision for better or worse. C’est la vie.
That said, I cannot agree with you or let go unanswered your insistence that breaking WPF vs. fixing WPF are the only choices. Coming to this issue with a fresh perspective - the value of which you can surely appreciate - there are clearly other options, including some that don’t appear to have really been fleshed out if even raised. Off the top of my head, here are a few:
AddRange(IEnumerable<T> collection, bool bulkNotify = false), orpublic static bool BulkNotify {get; set;} = false;`. Or (as @sharpninja suggested) change the behavior based on whether the WPF SDK is in use.ObservableCollectionand reimplement it asSystem.Collections.Generic.ObservableList<T>with full INCC support, make it clear in the documentation that the class should not be used with WPF, and move on with life.Is fixing WPF the ideal solution? Yes of course it is. No one disagrees with that. But that’s not going to happen.
It is a shame that all your work didn’t get used, and I’m sure it will be a pain to re-scope the proposal. That doesn’t mean it cannot or should not be done. Your solution may be a good one, but it’s not the only one.
Again, it’s time to discharge this technical debt in WPF and implement something that will add value to the other platforms while minimizing (if not eliminating) the risk that it will cause side effects in existing WPF code. That this can be done is clear. That it SHOULD be done is my opinion, but I think a widely shared one.
Gosh, I love seeing everyone come together to get this thing off the ground again. Fast forward nearly 5 years, and I’m running into these issues again building Blazor apps, so would love to see this happen in .NET 6.
Would be happy to help contribute fixes anywhere necessary. The WPF changes don’t seem like that big of a deal to get done. We might want to also loop the MAUI team in and see if there are scenarios that this would impact and/or fix for them.
I remain cautiously excited. 😃
It would be really nice to get these changes to observable collection!
A long time ago I had implemented a
RangeObservableCollectionwithAddRange,RemoveRange,InsertRange,ReplaceRangeandRemoveAll. But it turned out that the WPF binding system didn’t supportCollectionChangednotifications with multiple items (I seem to remember it has been fixed since then, but I’m not sure).While you’re in there adding an AddRange() method, can you throw an OnPropertyChanged() into the Count property’s setter? Thanks 😃
Well, you could… but you would have to use reflection to do it properly and get the right CollectionChanged event, but it would require reflection to manipulate collection internals, which wouldn’t be guaranteed to work between versions and negates some of the benefits of the event being used correctly.
It crashes because the original implementer was short-sighted enough to throw an exception in an event handler inside WPF internals, which never should have passed a code review in the first place.
One or two people on the WPF team are holding up implementation. They have been unwilling to prioritize approving the changes required for their controls to properly implement the events, because WPF apps had to engineer workarounds for this incredibly stupid behavior, and now they depend on it.
Even though the whole point of .NET Core is that there is no reasonable assumption of compatibility between major versions. They would rather people continue to use implementations that should NEVER have cleared code review in the first place then finally fix their bad decisions.
So, like any major division of science when people that have been in a job too long hold up innovation, we have to wait until they either get fired or die before we get this change.
You’ll have to excuse my saltiness, but I’ve put in major efforts to get this implemented 3 different times and have been stopped at some point close to the finish every time. I’ve personally spent hundreds of hours on this problem, and as a contractor I charge several hundred dollars an hour… but the time of all the contributors to this solution are apparently not valuable enough to push a solution through.
Which is why https://github.com/dotnet/runtime/issues/18087#issuecomment-1162900958 proposes this be configurable on the application level. Final shape is TBD pending API review.
BCL components don’t adhere to SemVer and honestly I don’t believe SemVer could ever work for .NET. Each major version update ships with the sdk, CLR, shared libraries, aspnet components, etc. so introducing breaking changes willy-nilly in one BCL component effectively blocks the upgrade path for the entire runtime. It would be a very different story if, say, System.Collections.ObjectModel were released and versioned independently of .NET.
Funny, I was thinking the same thing. I’m sorry but essentially saying that we whose commercial livelihoods depend on these products should just shut up and sit down because we supposedly don’t know what we’re talking about really doesn’t communicate “peace & love”. Perhaps I misunderstood you, but that’s what I heard.
It’s A problem, It’s not THE problem. I have to assume no one expected (or certainly no one should have) when this feature was being scoped that these ranged methods would be usable with WPF. The reason the contribution had to be reverted was that these WPF-unsafe methods were getting called unintentionally by existing code expecting its own WPF-safe extensions to be called instead. This raised the very events which caused WPF to throw those exceptions, which should have been a surprise to literally no one. Yes, fixing WPF solves the problem. Preventing the new methods from getting called/raised unintentionally ALSO solves the problem (but arguably in a much faster, cheaper, and more direct way). And frankly it’s starting to feel like gaslighting at this point to keep being told otherwise.
I get that you want WPF fixed and view the alternatives as inferior for one reason or another. That doesn’t mean they’re not alternatives, or that there are not other alternatives that haven’t even been considered yet. And I find it unfortunate that rather than engage in a substantive discussion about possible alternatives you want to just shut the conversation down.
Well, it was a prediction based on the fact that there hasn’t been a substantive commit to the main branch in a long time, and the fact that, two major .NET versions after the PR was reverted, this issue is still open. And I think the resources could be better spent on a clean solution to this that can be exploited in the new frameworks while doing everything possible, within reason, to keep that solution from interfering with legacy code. In fact, as the publisher of a commercial WPF app that is still actively maintained, I’d be far more concerned about regressions caused by trying to make WPF work with multi-item collection changes than I would be by adding ranged methods to OC or any other collection, which I might accidentally use in WPF. At least I’m in control of the latter case.
Regardless, it has nothing to do with cutting the team slack or not. I love WPF. I love the people who made it. But I think I’m a decent judge of when technical debt is worth repaying or discharging, and IMHO this is the latter. Obviously others will disagree, but Microsoft gave me the ability to express that opinion presumably because they value it; if they come to a different conclusion, that’s perfectly fine.
Again, I’m pretty sure if the team had a problem with hearing from the community they wouldn’t have moved their entire operation to a public github repo. Regardless, I have every confidence in Microsoft and the community contributors here, including yourself, to find an optimal solution together. I appreciate the opportunity to air my views and hope my suggestions have been helpful, and am sorry you didn’t find them to be.
This is the last I hope to say on this. Thanks again, to all of you.
Please don’t.
Once you start bolting on extra features you go down the rabbit hole that caused Collection<T> to have its problems. Start with List<T>, add a virtual method for intercepting add/remove, and a subclass with collect changed events.
That’s it. Nothing fancy. Nothing tuned for one specific scenario above all others. Just a high performance collection class that fixes the design mistakes in the original.
@terrajobst I really appreciate you engaging us on this, and I don’t mean to be arguing, but I do want to share a few thoughts in reply to what you’re saying, as someone who’s used and loved WPF almost since it was first created, and studied the reference source extensively even before it was open source.
WPF
ItemsControls don’t care aboutObservableCollectionor what methods are added to that class in the future; ; it/they care aboutINotifyCollectionChanged. Certain types of INCC change notifications - that are perfectly legal under that interface contract and its documentation - cause WPF to throw exceptions if the instance is bound to anItemsSourceand an event is raised with aNotifyCollectionChangedActionvalue that WPF doesn’t like. This has always been the case (AFAIK), and will remain the case until someone on that team fixes it. (And given that there hasn’t been a substantive update to that project in ages, that doesn’t seem likely to happen.).So, anyone could, right now, write a
Collectionclass fully implementing INCC, following that interface spec to the letter by enabling its full potential range of functionality. They could bind that collection to anItemsControl.ItemsSource, and if they used that extended functionality, WPF would throw exceptions. INCC is what breaks WPF. It breaks it now and it will continue to break it. Holding backAddRangeand related methods fromCollectionor OC won’t change that.I think the main issue really is not backwards compatibility or breaking changes. It’s the perception that
ObservableCollectionis 100% safe to use with every WPFItemsControl(even though INCC itself is not), and thus a concern that if you extended OC to fully exploit INCC, future coders might use the extended functionality in WPF without realizing it’s not compatible with WPF. This is not a good reason to hold back a feature IMO.Notwithstanding whatever prior attempts were made that broke WPF, assuming a conservative implementation of the APIs proposed in the OP, 100% of WPF apps that worked the day before the proposed changes should work the day after them. The only apps that should break would be apps that subsequently started using that new functionality with collections bound to WPF controls. However, this can be solved through documentation or other means - including catching the exception you know WPF is going to throw and falling back to a compatible
NotifyCollectionChangedActionwhen it does so, along with a big debug console warning.And even if everything I’ve said above is wrong for some reason - though I don’t see how it could be - you could implement these APIs using extension methods rather than actually modifying
Collectionor OC. Just change some of theprotected’s toprotected internal’s and let the extension methods handle the insertions/removals and the notifications. Then there is exactly 0 chance of breaking any existing WPF app and, again, you deter future WPF coders from using them through warnings, or you catch the exception, etc.So to sum up, you’ve got a wonderful powerful interface in INCC that lacks a full featured implementation in the BCL out of concern that future code written against an obsolete platform might fall trap to a 15+ year old bug. Meanwhile the non-obsolete platforms - WinUI, MAUI, Blazor, etc. - that don’t have that same bug nonetheless suffer because there is no standard cross-platform way of fully and efficiently implementing INCC that everyone can agree on.
I have to say, you guys have made breaking changes in .NET under far less compelling circumstances - and this doesn’t even qualify as a breaking change.
Thanks for listening. 😃
PS - Amazing job with .NET6! I’m sure you don’t hear that enough.
These are the two open issues on the WPF repo relating to this issue: https://github.com/dotnet/wpf/issues/52 and https://github.com/dotnet/wpf/issues/1887
@predavid can we get buy-in/support from the WPF team to either implement a solution or prioritize PRs from the community for this so that this can be fixed for .NET 6?
@ryandemopoulos WinUI 3 would also need to be updated (couldn’t find if there’s a specific issue already open for this). I assume UWP wouldn’t be effected as it’s still on .NET Native with .NET Standard 2.0 and thus wouldn’t be consuming the new implementation?
To be more specific about this, WPF has UI controls that throw exceptions if you have more than one object in the
INotifyCollectionChangedevent. There are some controls that actually depend on this behavior, so at the moment, we’re stuck.Just based on what I know, this is not likely to get into .NET 5.0 (unfortunately, I haven’t had the time to spearhead this from the outside, and i believe that .NET 5.0 is feature complete for a September-ish launch), but I think we’ll be able to get it into .NET 6…0, which will follow right behind.
To be clear, that is not Microsoft’s official stance, just my opinion after trying to shepherd this long overdue feature for the last 4 years.
If they implemented the INotifyCollectionChanged protocol correctly, no, they wouldn’t need one. Its a generalized interface that already supported bulk operations, just the stock implementation of ObservableCollection didn’t make use of it.
WPF has only been broken because they don’t implement the protocol of INotifyCollectionChanged correctly, WPF has to fix it regardless of whether this issue is implemented or not, since users can pass their own collection classes along.
I’d imagine if WinUI made similar mistakes that someone already has been complaining about it (in fact there were issues that INotifyCollectionChanged stopped working after adding Desktop support).
So why does nobody fix WPF as part of this? 😦
Its handling of collection change events is broken, it just should do a reset for anything not supported, not throw an exception.
FYI: Moved to milestone 5.0 as promised. WPF team will be busy until we ship 3.0, but if we want to take changes into CoreFX that won’t break WPF badly in master, we can do it now.
Just to clarify: We have decided between architects, managers and PM on CoreFX team, that we WILL NOT take this issue for 3.0. We are fine to take it on the first day of .NET 5 in master (which should be in July … likely mid-July if I read the tea-leaves right … the dates are subject to change though). I discussed it with @robertmclaws over Skype and we came to same consensus, given all restrictions (not much runway, important WPF scenario, plenty of remaining WPF work left).
Moving to Future (will move it to .NET 5, once we have the milestone - at the moment we branch off 3.0 out of master)
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.
Next steps:
Note: Preview 7 is RC (go-live) and we won’t be able to add new APIs after that. Even if this change is too close to shipping Preview 7, we may have to postpone it to next major version (.NET 5) … while it would make me sad, we can’t simply break WPF developers and make their transition to .NET Core harder because of this. Let’s try to figure out how to deliver on both - great WPF experience and this new API in.
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.
Basically the signatures should be the same as in
List<T>.I don’t think the
clearFirstparameter inAddRangeis useful, and anyway optional parameters should be avoided in public APIs.A
RemoveAllmethod would be useful all well, for consistency withList<T>:Another example of how ObservableCollection had an obvious original design that multiple Microsoft engineers chose to ignore, the end result being a showstopper for the evolution of the product.
At this point it will probably be easier to create a brand new class in the BCL that people can move to if they choose, than to convince 2-5 different Microsoft teams to dedicate the budget to fix this.
That WPF PR does not unblock this issue, it just removes the explicitly thrown exceptions, as soon as this goes in you’d have the same regressions, just happening in a different way.
@legistek, I agree that new folks on the thread are speaking past the people working on the problem… because it doesn’t appear you folks have taken the time to understand what is going on. You quoted a bunch of my post… but clearly missed the point.
WPF on .NET Core heavily uses the codebase we want to change. FULL STOP.
My dad used to say something that has served me well: The truth doesn’t care about your feelings. So the fact that WPF is affected doesn’t care whether you like it or not: it exists and must be addressed. Nothing you can say will change that.
I get it, you want it fixed. Nobody wants it fixed more than I do… I did the original writeup, engineered the original fix, and fought a lot of these battles with the team already (as is documented in the thread).
The problem is deeper than you demonstrably understand, and commentary to convince the truth that it isn’t true is a waste of time for the people who can actually fix it.
@robertmclaws again, thank you for engaging and for your hard work on this. We appreciate you as well. But I think we (the folks pushing for this vs. the .NET team) might be speaking past each other.
I think the issue is how you define “the problem.” If the “the problem” is WPF can’t handle more than one item in the NCC event argument, then of course you’re right. Nothing will fix that other than fixing WPF.
If “the problem” is that the BCL contains no standardized cross-platform implementation of INCC that supports multi-item notifications, then the solutions proposed of course DO solve that problem, and they wouldn’t “break” WPF. They just would not be USABLE by WPF. Those are very different things.
I confess I haven’t studied your original contribution in detail to figure out why it “broke” WPF (as you say even that team didn’t have the time to figure it out). But surely you aren’t suggesting that it’s fundamentally impossible to add ranged methods to
CollectionorObservableCollection- regardless if they’re actually used in someone’s WPF app - without breaking that WPF app. If you are, then that seems like a problem with WPF that goes well beyond this one issue.I’m sorry if you don’t find this helpful, but what I and I think others are really trying to communicate is that we feel it’s time to move on from the WPF issue, because, let’s be honest, the likelihood of it ever being fixed is low. Don’t let the perfect be the enemy of the good; if you can extend or subclass OC to support multi-item notifications - even if the USE of those new methods would be impossible in a WPF app, I for one think it’s worth doing.
I thought the whole point of moving to independent .NET versions that are not necessarily backwards compatible was to not let legacy code hold back the evolution of the framework. It seems to me that’s exactly what’s happening here. And I’m simply saying that I feel the inability to use the proposed functionality in WPF apps is not a good reason not to move forward with it. That’s just my opinion and probably that of many others.
Thanks again.
@airbreather I should have added: If the WPF is interested. If there is no clear statement from the WPF that they are willing to accept PRs in that direction i can’t invest more time in PRs that are then being ignored by the WPF team. 8 out of currently open 91 PRs in the WPF repo are mine and the first one is from July 2019…
@weitzhandler I definitely think that you should put a proposal together for your additional APIs. I just think it should wait. If you start on your proposal now in your own repo, then you can copy it to this repo after we get this API over the finish line. Just check out the Speclet format in the first post, should give you a great start. Happy to review it in your repo while you’re working on it, if you like. 😃
@terrajobst Thanks man. I worked really hard on that post… I had hoped it wouldn’t offend anyone. 😃
@weitzhandler No need to be sorry man, I know you were helping, and I had a lot of personal stuff going on back then, so my head wasn’t in the right place at the time. We’ve all learned valuable lessons in this process. At this point I just want to see this cross the finish line so that all of our collective passionate investments will have been worth it in the hundreds of apps that will run faster because of this work. 🍻
@robertmclaws ok. that makes a lot of sense. thanks for being kind, and sorry.
I’m sorry that you see it this way, I’ve based my work on a
RangeObservableCollectionI was using internally in our projects for a long time, and I’m sorry for kicking in, I wouldn’t have done it if I hadn’t had the feeling you backed off. A thousand apologies. Apart from the work I invested in the filtering, I also think it’s very important to UI performance, hence the nudge, sorry everyone about that and let’s just move on.Just had my call with @karelz. I want to talk briefly with Andrew before I report the results of that conversation on the rest of the thread.
@benaadams The replacement didn’t fail. What failed was that a WPF control coded in throwing exceptions when a broken API gained the functionality it originally intended. The Observable’s EventArgs were a bad design, and the WPF control was a bad design. We fixed the first, now we have to a) fix the second, and b) find out what else might be broken, c) build a tool that can help developers porting their stuff to find potential problems.
I’m going to try to jump on a call with Karel today so that the people working on a fix + the people running the show can all get on the same page.
Right, and reasonable. So what is the threshold for this? Also, is there another avenue to a solution without reverting the code? I feel as though I’m in the same boat whereby we’ve got a custom
ObservableRangeCollection<T>in our source, the idea here is to actually be able to delete it in favor of the code @ahoefling contributed… ie: we want to stop supporting our own custom implementation.WPF developer from a large exchange-listed company here.
It’s incredible how much of a difference a proper .AddRange() function will make to the speed of ObservableCollection. Adding items one-by-one with a CollectionChanged event after every add is incredibly slow. Adding lots of items with one CollectionChanged at the end is orders of magnitude faster.
tl;dr AddRange() makes the difference between a huge grid with 10,000 items being fast, fluent and responsive vs. slow, clunky and unresponsive (to the point of being unusable).
So this PR gets my +1 vote. Of all the improvements to grid speed, this is by far the most important.
@safern is this on track on being done soonish? If so, we could still add it to .NET Standard.
The issue is marked api-ready-for-review. It is on our list. We have just been prioritizing review of APIs critical for 2.1. I hope we will be able to get to the Future backlog in 2-3 weeks.
Even after 7 years, a multi-trillion ($2.7 trillion) company hasn’t managed to add this 🤦♂️
So disappointed to see this discussion going nowhere…
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.
@Sergio0694 actually I was not aware you were helping push fixes internally, so thanks for that. Know that my ire is not directed at you at all.
I believe that there is a combination of IntelliSense instruction, documenation, and Roslyn analyzers that can solve this problem for new developers on .NET X that want to use the feature. You could EASILY detect when ObservableCollection.AddRange() is called in a front-end app that also uses one of these Microsoft UI libraries, and throw compile-time exceptions.
Then it’s Library A’s responsibility to fix the problem.
I get that in some cases Library A is another Microsoft team… but realistically these design choices were stupid and short-sighted in the first place, given the CRYSTAL CLEAR intention of ObservableCollection’s
CollectionChangedevent.Less than 5 people at Microsoft made a stupid architectural decision, and 5M Microsoft ecosystem developers are forced to deal with it FOREVER now. That seems unreasonable.
Sometimes intentionally breaking things is how changes get forced downstream. That’s the ENTIRE reason why the .NET Framework became .NET Core in the first place… to allow innovation to happen free of the BS of the past.
I have to deal with breaking changes in the libraries I use all the time, that’s why I have no sympathy or empathy for any complaints about it. That’s the life of a development shop. 🤷🏻♂️
It seems a long time from 2016 to 2023 to bring this feature in. Is this one dead and will it ever be implemented?
@robertmclaws
My comment is about the implentation of the following method in your speclet:
If you’re going to remove and add these items, the entire collection following the change index will be reset and rerenderred lagging up the UI. The proper way to do it is to strive to replace each of the items individually in their designated index if count equals, after the whole replacement, notify UI about the group replacement.
Not really a regression. It didn’t work before and now it might work or might not.
This, except make it thread-safe and dispatch notifications only on the thread it was created on.
@AdmirableTable You can’t do Step 4 & later until Steps 1-3 are done. And we can’t move forward re-integrating the work from Steps 1-3 until the WPF Team signs off on making the investment for .NET 7.0.
So the only thing that can be done right now is voting up the issue.
Once we get approval and we map out the work, we can see what “up for grabs” work the community can do.
If the user declared an extension method over a type they don’t own then stuff like this can and will happen. I’m not OK with holding the entire stack back because of a corner case that might break some people. That’s the other extreme that’s not actionable for us either.Of course, we should still document this as a known issue.Edit See above. It’s not just an extension method issue; it’s that WPF can’t handle batch events.
@weltkante You are nitpicking about the difference between controls and the underlying infrastructure that is technically a part of the control when it is executing. And you’re also nitpicking on controls “depending” on that behavior. Because it’s not possible to have the event contain more than one entry (because if it does, an exception is thrown) the controls are not designed with the possibility of multiple entries in mind. Meaning they depend on only one event coming through the system. And just throwing reset events may not be the right approach either. Making sure the ecosystem doesn’t get broken in the process will require more testing.
When we get around to producing a build that has both fixes to be able to identify downstream issues, we will add them to the ticket as appropriate.
Wait another year and a half.
Unfortunately it was reverted https://github.com/dotnet/corefx/pull/38115
What is the status of this? The PR was seemingly merged but the issue is still open?
But honestly this is a net new method. How can MS possibly account for every extension method ever conceived in history? Sure it’s in products that are used, but their extension methods are within their code base that they have control over.
Not necessarily, since
IList<>does not inheritIList.My plan for this weekend:
My over-arching concern is that we implement this in a way that does not break subclasses that already try to implement this functionality.
Once we establish that this implementation meets the API surface already approved, and doesn’t break compatibility at this level, we can decide if the functions can be moved further down the stack without breaking anything else. But I don’t think we should confuse the two right now, this whole process has been hard enough to follow 😉.
Thoughts?
I think
RemoveRange(IEnumerable<T> collection)should remain. It would cycle throughcollection, callIndexOf(item)and then callRemoveAt(index). Duplicates of the same item would also be removed.@thomaslevesque I have the
clearFirstparameter 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 callClear()first, it will fire anotherCollectionChangedevent, which is not always desirable.I’m not against a
RemoveAllfunction.@robertmclaws
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
newif 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?@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.
Maybe I’m misunderstanding, but AppContext switches are very much a thing in modern .NET.
How about using a feature flag? Implement the feature, but only enable it if a given feature flag is enabled. If the feature is disabled, the new
*Rangemethods will emit multiple events. Let frameworks that support range events enable the feature flag. Frameworks that don’t will still get the old behavior.@Sergio0694 If you folks are going to API review it live, I would love to be a part of the discussion, and I promise to be respectful and not salty.
Since Microsoft has written case studies on my solutions for Blazor, which could heavily leverage this change, I can easily demonstrate the benefit to my business. And the person that did the pull request for the BCL and WPF work still has the published performance improvements for apps that make a clear case for the change.
Regarding the Roslyn analyzers and what not making things more complicated… that is the only work that is left to do (outside of Microsoft’s UI library fixes). My friend and I already did the work to implement the change in ObservableCollection and in WPF. Those changes just need to be merged into the BCL and WPF so that we can get real-world data on the downstream impact and start to build the analyzers to warn developers.
If it’s actually going to get done, I’m happy to once again contribute to the effort for any new things that need to be built to support it. But if we’re just going to get Charlie Browned with the football again (the BCL change was integrated into .NET already and then reversed at the last minute), a lot of people are not going to be happy.
If they are extension methods, you lose the performance benefits from doing all of the inserts at one time. Specifically, raising one collection change event rather than one per item.
Yikes. Ok then yeah, I have to agree that if none of the official frameworks would support this there’s little point until they commit to doing so, and that will require a lot of left hands and right hands to be coordinating. Definitely above my paygrade. 😃
The one thing I guess I would stand really firm on though is assuming MAU and WinUI were extended to support this, even if WPF isn’t, it should still be done in some form. Just please don’t wait for WPF.
I would suggest to take a step back here and consider the long term vision for this feature. This new API proposal does not actually solve the issue, can still lead to crashes, and is unnecessarily clunky:
ObservableCollectionclass today. The idea of adding that for the sole purpose of exposing this static property just to act as a workaround for a possible niche issue on a couple of frameworks seems like a complete non-starter to me.@eiriktsarpalis I think we should just coordinate across the various teams involved and come up with a proper plan to do this right. We have waited so long that trying to rush this with a clunky API shape doesn’t really seem ideal to me. We should just get the right folks from the WPF team, WinUI, likely someone from the OS team as well due to WinRT APIs and projections being needed to enable this correctly on WinUI 3 as well, and someone from MAUI, and figure out what we need to do to hopefully get this in with .NET 8, or some future release. Then we can comment here when we have a concrete roadmap to enable this 🙂
Attempting to set
ObservableCollection.UseRangedNotifications = truecould always throw ArgumentException, likeAppDomain.MonitoringIsEnabled = falsedoes. So if a library has setObservableCollection.UseRangedNotifications = falsebecause it does not support ranged notifications, there would be no way to enable them again, except by private reflection.🦙 @eiriktsarpalis see Issue: https://github.com/dotnet/wpf/issues/1887 and PR: https://github.com/dotnet/wpf/pull/6097 for WPF.
@sharpninja because even if you do that, it still changes the behavior of the
CollectionChangedevent, which does not solve the core problem for anyone trying to use that method in affected UI frameworks.Also, you’re in the same codebase, so why on earth would you add an extension method vs just adding a method to the class? Doesn’t make any sense.
If you look back at the original posts in this feed, you’ll see I posted an extension method you can put in your own projects that does exactly this. The POINT of this issue in the first place was to bring that functionality to everyone in an architecturally-correct way.
I cannot stress this enough, if you’re new to this discussion and want to contribute to the solution, please read the whole thread. There is literally nothing you can say that hasn’t already been said. Some of the smartest .NET developers on the planet have been working on this… it’s not an issue you’re going to be able to do a “drive-by” on that will magically move the needle.
Why can’t an internal scoped property be added to allow extension methods in the same assembly?
This seems like the perfect usage of extension methods to add the AddRange functionality to the existing WPF-safe implementation coupled with an Analyzer that will register an error if the OC is monitored by an object that derives from a WPF control.
So definitely not trying to hijack this thread, but for anyone who’s in immediate need of this for production code, there is an alternative in DynamicData’s SourceCache<T> (preferred) and SourceList<T>.
Wouldn’t any newly implemented class implement the same interfaces and run into the same compatibility problems with WPF? I guess you could spec that the new class isn’t compatible with WPF, but I don’t really like fragmenting the implementations rather than fixing the bugs.
I don’t recall the details. If that’s indeed the only issue, then making this change during the .NET 6 preview cycle seems acceptable.Edit See above. It’s not just an extension method issue; it’s that WPF can’t handle batch events.
@sharpninja
That’s not how we roll. We ship .NET as an overall platform. We don’t just regress some users of certain workloads, pat them on the back and say good luck. We’re OK with making deliberate breaks if that’s in the interest of the greater good, but we still don’t do massive breaking changes. That would be irresonsible.
OK, now that it’s been confirmed (sadly), can we consider adding filtered replace to enable reusing items (good for UI performance, so those items don’t have to be rendered again). Imagine a drop-down list or plenty of other scenarios I’ve used and encountered, that this behavior of reusing items made it slick.
Here’s my previous comment summarizing the approved and suggested API, please have a look before you downvote. You can also have a look at PR dotnet/corefx#26482 to see the behavior.
Those filters are not there as a syntax sugar. They’re to save UI performance.
@terrajobst Thank you SO MUCH for that link to the references in WPF. I’m going to put together a triage list and see what might be affected.
@ChaseFlorell existing apps / assets = existing customers = existing products.
@karelz Do you have like 15 minutes at some point in the next couple days to chat briefly on Teams? I think we could get on the same page real quick with a face-to-face discussion.
@SamuelEnglard but unless I’m missing something, we’re not breaking any existing products, we’re potentially breaking other developers workarounds to existing deficiencies.
Shouldn’t the priority here to be to finally make it right? I find it kind of odd that this conversation is directed at supporting folks who had to wire up their own extensions in order to make the API work correctly in the first place. Now that we have a working structure, those folks extensions are inevitably being deprecated.
This seems like an instance of the Needs of the Many Outweigh the Needs of the Few⁉︎
Just to reiterate: It is the test bed that did NOT discover the problem in the first place. While we can boost it up, the coverage of the scenario will be only what we will add, i.e. skewed by our understanding and minimal. The key will be to come up with good test plan to discover potential downsides of the solution we are going to propose. Ideally some exploratory/end-to-end testing.
@robertmclaws
I don’t fully understand your statement/point here. Can you please rephrase it / clarify for me?
My key viewpoint is: App migration to .NET Core matters A LOT. To the point that if we have to break apps in a way that is not super-easy to detect upfront, I would not support taking the change in ANY future release of .NET Core (at least given current knowledge from this thread which indicates that many apps will fall into this trap).
I hope so too!
If we have to break apps (e.g. in .NET 5), we have to understand it more - having it analyzed from left and right, to understand the IMPACT on apps (how many and how badly are impacted), if we truly cannot MITIGATE the impact more (Roslyn analyzers, magic and unicorns considered) and what is the VALUE customers will get (how much perf benefit, not just “it is better and the right design”). Such analyzes will take weeks and likely months. Not something that can be done for .NET Core 3.0. So, from my point of view, if we cannot greatly mitigate the breaking change impact by e.g. modifying the PR, some WPF code, or adding smart Roslyn analyzers or something like that, it is automatically out of .NET Core 3.0 for me at this moment (with potential consideration for in .NET 5 after a lengthy discussion and analysis mentioned above).
To be explicitly clear: I am HESITANT to view documentation, release notes, and/or guidance as SUFFICIENT MITIGATION of the breaking change impact which manifests as weird runtime exception. I am curious what others on CoreFX team think abut it (@danmosemsft).
I personally do not believe that any amount of XML docs would truly help. First, you would need time-machine and go back 20 years and backfill everything (not realistic) and second, I have near-zero trust in developers, that they would update the docs as we would progress in future and the code and paradigm would change. Just being very practical here. Adding XML docs is good idea, but it is not silver bullet that will save us or prevent all/most mishaps. At best it will help a bit. IMO the cost/benefit ratio is very poor. Having better test coverage is IMO much better cost/benefit investment for product, customers, behavior documentation, etc.
Personally, I would not support any change after 6/14 given my current knowledge of the release schedule (which may change). Anything after 6/14 is just too close to snap for July release and therefore it creates uncomfortable amount of risk for me. As an engineer and someone who wants to deliver high quality products, I can’t support anything later than that.
That validation was presumably there for a reason. Do we know why? Just removing the exception doesn’t necessarily mean things will work as expected. Do the mechanisms in WPF that call that method work correctly with range methods, have they been tested?
@onovotny I appreciate your concern as anything we do now will have long term consequences (good or bad). I am going to try and dig through this to better understand the problem to see if we can come up with something that is acceptable.
Not trying to over-complicate, just manage risk.
Awesome. Yeah PRs are our in CoreCLR and CoreFX. Will try to get them in ASAP
cc @danmosemsft @safern @ianhays to see if it would fit our 3.0 plans (with UI stacks coming to .NET Core) - the API had a share of back-and-forth and it could use some love from our team …
@weitzhandler I am not sure what you’re asking for. If there are concerns about API shape or behavior, then they need to be addressed prior to finalizing & approving the API. If we overlooked some concerns earlier, it does not mean there is a free pass to ignore them now. It could (and did) happen that approved API gets rejected in PR, because new API concerns arise which were missed during previous API approval review. We are not slaves of process. The process is here to help us come up with solid APIs which developers will use for decades. We want to deliver high-quality APIs and ideally learn from our past experience to make APIs even better now. And yes, API design is HARD and sometimes TRICKY.
I don’t think it is a good idea to rush this API. Moreover, technically speaking we are past API freeze. Given that the change and the new API shape didn’t get enough scrutiny from community / experts tells me it is not as simple as it looks. If I remember correctly, even the original approval was a bit open-ended with “we need more implementation validation, before we are fully ok with the API”. Once we ship the API, we will live with it forever. Therefore, we should be fairly sure it is right. I don’t have the confidence from the discussion above that we are there yet.
Collection<T>as virtual methods with an implementation. That way calls likeObservableCollection<T>.AddRange()could just callbase.AddRange(), and then raise the right events afterwards.Collection<T>as well.We’re fine with this APIs shape:
The add with the boolean seems quite weird and like a deviation from the rest of the BCL. If this is needed, you should use
collection.ReplaceRange(0, collection.Count, items).There are still some open issues the design needs to answer as well:
ObservableCollection<T>or on the base,Collection<T>?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.
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.
Not to have to rehash the same issue AGAIN, but the WPF team will not implement the change because existing controls DO leverage
ObservableCollection, and if developers DO callAddRange(), WPF will throw an exception because more than one change will be in theEventArgs.Making the behavior Opt-In would impact when CollectionChanged is raised in ObservableCollection and allow existing controls to work as expected.
I would LOVE to have WPF fix the problem. As I stated before, when this process first started, WPF was not involved at all.
Providing a way forward that maintains the existing behavior under certain situations may be the ONLY way this gets done, seeing as how if this PR was a child it would be starting 3rd grade in the US.
That is for the .NET Framework not .NET Core. We’re suggesting the behavior be opt-in so that existing applications aren’t broken. Eventually it would be made the default.
That assumes that end developers care whether you folks make the change in your libraries or not. Blazor has a real opportunity to take advantage of this change to the BCL, but the apps I build can’t because the Microsoft UI Framework teams can’t get their crap together.
I think I can speak for many people outside Microsoft when I say I don’t think anyone out here cares whether you folks fix the issue on your end or not. WPF should simply stop throwing exceptions so that NEW apps are unblocked. If WPF/UWP/WinRT/WinUI3 chose to continue the same stupid behavior of throwing one even per item, that is your choice.
Microsoft can ship analyzers that warn users not to use those methods if those UI frameworks are involved, and report telemetry when that happens. That way you could collect real data on how prevalent the downstream problems are, which would provide impetus and budget to fix it.
But the performance benefits at this point are too great to ignore.
I guess it would be problematic since it is a WinRT parametric type, but can we lift the interface out? Like Microsoft.UI.Xxx.IObservableVector<T>?
@Sergio0694
WPF is still officially supported last I checked. Is that changing?
Yes that was in my proposal but it sounds like @thomaslevesque’s idea is even better.
Requires 1 line of code vs. changing likely 1000s to make WPF fully support ranged notifications which the PM said would be months of work. And if it’s done through the SDK and runtimeconfig.json it sounds like the WPF devs wouldn’t even need to do anything.
See https://github.com/dotnet/runtime/pull/65101#issuecomment-1160732696
And the migration guide puts the burden on the app developers to be aware of the issue and address it proactively, vs a transparent solution that will work for 99.99…% of cases.
Good point, I forgot that it’s generic so a single static member won’t work. How about putting it in
NotifyCollectionChangedEventArgsinstead? That’s even better since then it’s clear the issue is not just OC but any INCC implementation now or in the future.What is the proper plan in your view? A complete overhaul of everything in WPF that depends on INCC? Have you ever been into the guts of the WPF source code? I have (ported a huge chunk of it to WASM as an experiment). This would be a massive effort for a 20+ year old framework that is in maintenance mode. But there are modern, actively developed frameworks that can benefit from this now without breaking legacy code.
This notion that it’s either “fix” WPF or “break” WPF is a false choice. Yes, it’s legacy code, technical debt, etc. So do what’s almost always done in these cases - mitigate the possibility of breaking the old while moving on with the new.
It could be set automatically in MSBuild (using
<RuntimeHostConfigurationOption>) by the desktop SDK.I have been following this for years and rolled my eyes at WPF as much as anyone here. I’ve extended ObservableCollection more times than I care to count. The extension method idea was to prevent the AddRange method even showing up in a WPF project, thus it cannot break WPF. Hiding the extension method from WPF wouldn’t be feasible, thus the reason for an analyzer that prevents being able to use the AddRange extension method without explicitly disabling the analysis which squarely puts responsibility on the developer.
The problem is: we’d be adding this functionality directly to the collection type, which can be used by WPF (well, is). Given that one of the key usages of this collection type is WPF, it would be silly to ship the BCL change when we know that such events would cause exceptions in WPF.
No it wouldn’t since WPF doesn’t check the type, there are interfaces on collections for a reason. The only thing you can do is give up on WPF and break it (by documenting the new collection will be incompatibel to WPF), not sure if Microsoft is prepared to do that yet. Personally I want WPF to be fixed rather than given up on.
AFAIK that’s what we did last time and we ended backing out the change because it did break WPF. I don’t recall the details.
Edit This is the thread where we concluded that this breaks WPF. I don’t believe we can make this change.
To be fair, it doesn’t really ‘break’ WPF. You just can’t use the new method with WPF, which has always been a problem for any observable collection.
WPF needs a kick in the pants if this situation is ever going to change. The only question is how that kick should be delivered.
Hi @eiriktsarpalis, the PR is already done and accepted, but reverted on 3.0 Preview 6: https://github.com/dotnet/runtime/issues/18087#issuecomment-497869494
The accepted implementation is still valid for .NET 6? A new PR must be done from the original? Unless @ahoefling wants to handle it, I can redo his original implementation on a new PR.
This would be great to have. As you have already noticed in WPF, controls consuming this would likely need updates though to support/take advantage of this. Same would hold for WinUI controls like ListView/GridView/ItemsRepeater etc.
Let’s ask @vatsan-madhavan about that
@ChaseFlorell I should have been clearer:
Do not beak existing products, applications, or code; whether the product, application, or code belongs to Microsoft or not.
Those workarounds are in products, products that are used by people now. And it’s not potentially, as the reason this was reopened in the first place was an example of it not working for a developer.
@robertmclaws sure, ping on me gitter, Twitter, Skype or email - see my GH profile for links to the info.
@ChaseFlorell One must remember an extremely important policy of .NET (and many other Microsoft products): Do not break existing products, even with major release. This has been in place for years and will continue to be.
To help everyone out here is the snippet in question that @robertmclaws referenced:
After reviewing this code it makes sense to me that the root cause has to deal with this function throwing
SRID.RangeActionNotSupportedExceptions. If we remove that code it should resolve the problem.As a way to be proactive about other platforms in .NET Core that may be affected by this change maybe we could get a list of repositories we can search through for similar code?
We just snapped Preview 6 for release in a week or so. Preview 7 is in a month, but that will be the first preview widely supported : we cannot make (or at least can’t plan to make) breaking changes after that. If we put this back in Preview 7 then discovered it caused to much pain to keep, we would have to make such a breaking change.
I wonder if we could build a Roslyn Analyzer to catch extension methods on ObservableCollection so you get a design-time error instead of a runtime one.
No, because it changes the method binding. Instead of binding to the extension method, it’ll silently use the version in
Collection<T>.It’s pretty simple: any place there’s an
AddRangeon an ObservableCollection in the codebase, there’s a potential issue due to binding. Here’s the first one I noticed, but there are many others in use in the various editors. https://github.com/NuGetPackageExplorer/NuGetPackageExplorer/blob/4868073d5a58a390f2c3c4363c2d7af29ffa7261/PackageViewModel/PackageChooser/PackageChooserViewModel.cs#L370In preview 5, notice that it’s binding to an extension method that calls
Add()in a loop. Then, in Preview 6, it doesn’t.@weitzhandler I remember seeing your attempt and I remember looking at it when I was working on my PR. I need to review my code and your code to see the differences and refresh my memory on the implementations.
BTW, @ahoefling, thanks for your PR! Would you mind taking a look at my implementation and see if it makes sense? It avoids replacing same item, so that the UI doesn’t refresh for them. I did that because I had an
ObservableCollectionwith 15k items that made the UI laggy.@karelz thanks for the detailed explanation, it is much appreciated! Is there a deadline we could agree to as a hard-stop when we would need to get a Pull Request submitted to fix this problem with WPF? If we as a community are going to research this and try to hunt down the problem a hard-stop date will be helpful so we can prioritize our time.
@onovotny do you think you have time to show us on a call/screenshare exactly what is going on with the NuGet Package Explorer? If we can get some time together to outline the problem I can create some detailed documentation for us to further investigate.
I believe this would give anyone interested in digging into this (@robertmclaws, myself and anyone else) details to go on and hopefully enough to solve the problem for Preview 7
If that was the case, I’d agree. Unfortunately, I believe that it’s a common pattern that people added an
AddRangeextension 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.@chaseflorell this repo is for.NET Core as you mention. The dotnet/standard repo is the place where the standard is defined and requests should go in In issues there
I think following this will be enough: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits
Yes, you can open a simultaneous PR in corefx to expose the APIs in the reference assemblies, add tests and also add the implementation for ObservableCollection. However the CI builds will fail until the coreCLR PR is merged and corefx consumes a new version of the coreclr package.
After adding the APIs in Collection<T> implementation in order to test your changes in CoreFX, once you added the APIs to the reference assembly for Collection<T> you will want to follow this steps to build CoreFX wit a custom version of coreclr: https://github.com/dotnet/corefx/blob/d9193a1bd70eee4320f8dcdd4e237ee9acf689e9/Documentation/building/advanced-inner-loop-testing.md#compile-corefx-with-self-compiled-coreclr-binaries
Please let me know here or in gitter if you need help from my side 😄
Well I was developing against the existing tests. I shall try adding tests that verify that. I don’t know if we will be able to add tests that make use of
CollectionViewSourcebut I’ll try tracing down the error.The individual events in my implementation will only occur in non-consecutive replacements - not additions, which is good because the UI will only refresh the required indices.
I think I’m just taking that further by sending either zero or one collection change event; that’s even fewer. The intentional tradeoff being, reset events are more likely. Sending a property change events for
Countis not something I’ve ever found useful.Great, that’s useful. Let’s first wait on first wave of feedback and when we are in general ok, we can run these new APIs by API review. Thanks for your PR!
My initial work is here. Feel free to comment before I open a PR, Or you can wait till after.
Sorry, I’m working on it. Will try to have a PR over the holiday break.
While I’ll understand the reasoning, I don’t think
Resetis the proper event in this case. The documentation forResetsays:Which IMO should only happen when
Clearis called.RemoveAllcould raise multipleRemoveevents instead (for each range of consecutive removed items).@SamuelEnglard I’ve had to update it about 7 times now… if I did a new post every time, I think it would make the thread worse. I added a Title header with the update date to make things clearer, and make it stand out more.
@robertmclaws What’s the purpose of ReplaceRange(int, int, ICollection), we don’t have a ReplaceRange on any of the other collection types. Is there a use case for this? If not, then we can defer on this.
Alright, I’ve updated the speclet with the other functions, implementing
clearFirstandReplaceRangeoptions as overloads. As I just mentioned, I think buffering events would need to be a different proposal, because it would affect how every existing method fires events, which is not a bad idea, but is outside the scope of this proposal.Ok, I think I came up with a way to meet everyone’s requests. Please give me a few minutes to amend my speclet.
UPDATE: I was thinking about a boolean flag that let you buffer events, but that would be a pretty significant change to existing functionality, and I don’t know if that is a good idea in this proposal. I think we should get these core functions added first, and THEN see if it makes sense to create a way to let the collection buffer events until you ask it specifically to flush them.