-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adds Support for ObservableCollection<T> and Collection<T> Range APIs from dotnet/runtime #6097
base: main
Are you sure you want to change the base?
Conversation
…ge() and RemoveRange() apis for ObservableCollection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this does is remove the immediate NotSupportedException when ListCollectionView gets a range event. It doesn't add support for those events. To do that you'd need to
- add logic to
ProcessCollectionChanged
to do the right thing for range events (currently it assumes one item) - add logic to methods it calls that also assume one item
- make similar changes to all handlers of CollectionChanged (there are dozens)
- make changes to secondary events and virtuals, e.g.
ItemsControl.OnItemsChanged
,ItemContainerGenerator.ItemsChanged
- track the range event through transformations like shaping (sorting/filtering/grouping), live-shaping, cross-thread notifications, etc.
Without that work, range events will get handled by logic that isn't prepared for them, resulting in unexpected behavior - crashes, data loss or corruption, incorrect/undesired layout, etc. Better to throw NotSupportedException up front.
(Doing all that work is a huge project - probably months of work.)
From my perspective as an outsider, I'm only really interested (near-term) in seeing the dotnet/runtime PR get unblocked. I'd really like to start using efficient versions of the "Range" APIs in other situations ( But is a "full, proper" implementation really the only way to remove the blocker? Our .NET Framework 4.8 application has been using a subclass of So this may be a bit of a naïve question, but would it not be sufficient to resolve this issue by doing something similar on the receiving end in The main* downside that I can see to doing it that way is that If that's the main downside to that idea, then I imagine that the "full, proper" support could include If there's something more to it, then what other reasons might there be not to take that lighter approach? *I can also concede that it would cause the overall system to show suboptimal performance in many important situations, such as when an Still, though, as annoying as it will be for someone who will find themselves in this situation, it would only affect new code, where some people (at worst) have to use a wrapper collection that modifies various calls to work around |
Yes, Microsoft, please make this happen! I have been trying to get this fixed since August 13, 2016. Throwing exceptions in an event handler is a really bad decision that should have never been allowed to happen. That single decision has been blocking people from making the right architectural decisions for over 15 years. And it has kept WPF from being the amazing platform it was supposed to be. People know that major .NET version releases break things... that's what SemVer is for. We will fix our own applications... and we'll help fix the runtime too. @SamBent We don't know each other, but I have been in the .NET world since the .NET 1.0 preview. I am asking you, one .NET expert to another, please stop allowing perfect to be the enemy of good, and unblock us. Once this fix is in, we have several people that will work through the codebase and make the required improvements. It will not take months, it will take weeks. We can get it into the .NET 7 release if you just let us. 🙏🙏🙏 |
Also, please see this comment: dotnet/runtime#18087 (comment) MAUI is already working to get it integrated in the .NET 7 timeframe. |
This PR does not unblock the dotnet runtime changes. Without WPF handling ranges properly, as soon as the dotnet runtime changes go in, WPF will still be regressed and it'll (probably) have to be rolled back again. The most you can hope to achieve with this PR is to hope that it "slips through the cracks" and nobody cares about fixing WPF anymore. |
But if the code were to be modified to address the issues that were identified in the review feedback further up the page, then it would, wouldn't it? If so, then something similar could be said about roughly any PR that doesn't pass an initial review. |
No offense intended to anyone, but this is a terrible idea to implement this as is. As was pointed out both here and at dotnet/runtime#18087 (comment), the issue that forced the original PR adding range support to The problem was that the methods (AddRange, etc.) being added to The same thing will happen again if you go down this route, the only difference being the exception will not throw. However, peoples' applications will still break because - at best - the notifications won't get handled properly. @SamBent (who obviously knows WPF better than anyone else here) is 100% right that this absolutely will not work without a complete overhaul of how WPF handles INCC events. This is not about the perfect being the enemy of the good. This is about not breaking 20 years of legacy code. Please do not attempt this. Adding the full range of INCC event handling to a built-in class is a worthy goal but neither |
Well, yes, if you turn it into a PR that does something entirely else, namely implement the missing support, then yes that will solve the problem. My comment was just highlighting, that the PR does not attempt that, thus is not unblocking this issue. As far as this discussion is concerned the PR could as well be empty - all work is yet to be done. |
Can you please explain the flaw in my comment above, #6097 (comment)? As you say, a common workaround is to have the sender raise the INCC event with |
@legistek Creating an N+1 problem is not the solution here. Fixing WPF to do the right thing in a compatible way is the solution. You CAN fix WPF to maintain compatibility in the final .NET 7 release... but to do that you need to remove this exception so the people working to fix it can be unblocked. |
Hey @airbreather I thought your idea was really interesting on its own. All the potential downsides I can think of are things you allude to. Perhaps the biggest issue that would concern me is that depending on the size of the existing collection vs. the size of what's being added or removed, either iteration or reset could be horribly inefficient. Each developer who's faced this and made their own But the real problem lies with #18087. If On the other hand, if we instead had a totally opt-in |
In my opinion, removing Take this line of code for exemple: It expects a single added item, which will silently break if you remove the validation without changing this code. |
@SkyeHoefling please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
Fixes #1887
Description
Removes the
CollectionChanged
exceptions fromListCollectionVIew
when the record count is not equal to 1. This PR is required to support dotnet/runtime#65101.The change to dotnet/runtime as originally attempted in 2019 and it was discovered during preview builds that it broke WPF. I have spoken with both the .NET Team and the WPF Team and both teams are ready to accept this change. See dotnet/runtime#65101 for a complete detail of history and API changes.
Customer Impact
If this change is accepted it will allow developers to use
AddRange()
andRemoveRange()
APIs on theirObservableCollection<T>
. This will reduce the call stack from event invocations by N number of items that are being added to the collection. If an application is adding 5000 items to anObservableCollection<T>
currently it will invoke 5000 events, with this change it will only invoke 1 event.If this change is not accepted, then dotnet/runtime#65101 will not be accepted as it will break just about every WPF application that uses
ListCollectionView
.Regression
N/A
Testing
I had problems running the wpf project locally and wasn't able to test as I would have liked to. I wanted to submit this PR and discuss with the community if someone could give me some help testing or instructions. I think the testing is giving me issues because I need a custom build of .NET 7 and the SDK project, then I need a custom build of WPF and finally will be able to test.
Risk
As far as I understand this PR mitigates the risk of breaking changes from dotnet/runtime#65101. I think it is important once we get a build we can test with to use some large WPF projects as a test solution like the NuGet Package Explorer. That was the project that originally caught the issue from the first attempt in 2019