Minimal support for ObservableCollection<T> range actions
#10845
Conversation
dotnet-policy-service
bot
requested review from
dipeshmsft,
singhashish-wpf and
Kuldeep-MS
dotnet-policy-service
bot
added
PR
|
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 😅 |
Sergio0694
force-pushed
the
user/sergiopedri/observable-range-support
branch
from
461531a to
9314b53
Compare
|
@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. |
| private bool IsCollectionChangedRangeAction(NotifyCollectionChangedEventArgs e) | ||
| { | ||
| 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.
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.
All it needs is private protected on CollectionView 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 in https://github.com/dotnet/wpf-test but afaik that's not everything. |
|
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. |
Fixes #52
Supersedes #9568, #6097
Description
This PR is needed to unblock dotnet/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