- Notifications
You must be signed in to change notification settings - Fork1.2k
Minimal support forObservableCollection<T> range actions#10845
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Minimal support forObservableCollection<T> range actions#10845
Conversation
Looks like a reasonable compromise to me. Did you test it gets you unblocked? |
@miloush I'm not really familiar with the WPF test infrastructure, I was hoping for some guidance/help on that part 😅 |
461531a to9314b53Comparemichael-hawker commentedMay 12, 2025
@Sergio0694 were you able to run the existing tests as called out in the contribution guide here?https://github.com/dotnet/wpf/blob/main/Documentation/developer-guide.md It'd be great if someone from the WPF team (or familiar with the codebase) could update the contribution guide to explain how to add new tests, since that's been a requirement for new fixes/features. |
| privateboolIsCollectionChangedRangeAction(NotifyCollectionChangedEventArgse) | ||
| { | ||
| return | ||
| (e.Action==NotifyCollectionChangedAction.Add&&e.NewItems.Count>1)|| | ||
| (e.Action==NotifyCollectionChangedAction.Remove&&e.OldItems.Count>1)|| | ||
| (e.Action==NotifyCollectionChangedAction.Replace&&(e.OldItems.Count>1||e.NewItems.Count>1))|| | ||
| (e.Action==NotifyCollectionChangedAction.Move&&(e.OldItems.Count>1||e.NewItems.Count>1)); | ||
| } | ||
| /// <summary> |
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.
You are creating 4x the same method. Cant this be made static and moved into a helper class or something like that instead of duplicating code all over?
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 it needs isprivate protected onCollectionView as its a base class for all of them,Selector can live with dupl imho.
@Sergio0694 I think its best if you get in touch with the team internally, a lot of the tests aren't in this repo, especially the functional ones. There are also CV tests inhttps://github.com/dotnet/wpf-test but afaik that's not everything. |
michael-hawker commentedMay 14, 2025
Thanks@h3xds1nz for the heads up, we've reached out to them directly too! Just bootstrapping the process here with the PR to lead the conversation in the open. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#52
Supersedes#9568,#6097
Description
This PR is needed to unblockdotnet/runtime#18087.
To keep work/risk to a minimum,this PR only unblocks WPF by avoiding crashes.
That is, it treats all range operations as collection resets, and nothing else.
This can be further improved later, but it's sufficient to at least unblock upstream .NET work.
Customer Impact
WPF apps with code using any new range APIs (dotnet/runtime#18087) will not crash.
Regression
No.
Testing
I'll need help with this.
Risk
Minimal. Changes are minimal and scoped, and only affect code paths that were previously throwing already.
Microsoft Reviewers:Open in CodeFlow