Skip to content
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

Add Range Manipulation APIs to Collection<T> and ObservableCollection<T> #104663

Closed
wants to merge 8 commits into from

Conversation

Daniel-Svensson
Copy link
Contributor

@Daniel-Svensson Daniel-Svensson commented Jul 10, 2024

Summary

This adds back the old solution originally merged for .NET Core 3.0 but later reverted since it did not work togheter with WPF.

This PR does the following

  1. Add back the APIs (by reverting the removal of the range APIs)
  2. Make it not break WPF (by using Reset for batch changes)
    a; Using Reset means "The contents of the collection changed dramatically." and is the current best way to do larger changes with WPF with good UI responsiveness.
    b; The Implementation and tests are prepared for adding an AppContext switch for opting into the original behaviour with "Batch notifications" where action is Add/Remove/Replace contain complete list if items being added/removed.

The tests have been run locally with both variants ("Reset" and "Range Add")

Commit overview

  • It might be good to review this commit by commit since the first 2 commits has mostly been reviewed already.
    (See commit message for first commit for manual changed needed due to merge conflicts)
  • Commit 1-3 adds back the old code
  • Commit 4-5 restructure the ObservableCollection implementation so it can do Reset events as well
  • Commit 6-7 updates the tests (and changed the RaiseBatchCollectionChangedEvents property to do Reset)

Goals

  • Add back the Range API since it is useful, it is not only more convenient when working with collection, it is also faster.
  • Make it easy to add an AppContext switch so that the "range notifications" can be opt in when using frameworks which supports them.
    • My hope is that the feature can be opt in and later, when relevant core frameworks are fixed, it might eventually become opt out in order to not break 3rd party controls.

Non goals

  • Optimal performance
    • Ideally range operation that only modify a single item should raise normal 1 item Add/Remove events instead of Reset. (It might even be better for a few items (1-2?) as well, but choosing a higher cut of would require benchmarking a few controls and frameworks)
  • Adding the actual AppContext switch
    • I hope somebody else can easily add this, the main problem is choosing a good name and updating the test
    • I looked into this quickly and found some "RemoteExecutor" code tests AppContext switchtes but it was not accessible from the ObjectModel test project
  • Modernise the code (to use collection expressions etc)
    • I tried to use "fix all" for using collection expressions, but it does not work so I decided to keep it close to the original code to make reviewing easier

Fix #18087

Daniel-Svensson and others added 7 commits July 10, 2024 11:55
…bservableCollection<T> (dotnet/corefx#35772)" (dotnet/corefx#38115)"

This reverts commit 18c2fa1.

Manual edits:
* Moved CollectionTests.netcoreapp.cs to separate folder
* Resolved confict in Tests.csproj

Co-Authored-By: Andrew Hoefling <andrew@hoeflingsoftware.com>
* Fix calls  to throwhelpers that have been renamed
* Remove // TODO-NULLABLE:
* Updates to project files
…omes easier to add a more compatible "Reset" based approach
* Rename test files (replace .netcoreapp with something more suiteable)

* CollectionTests:
  Remove redundat Assert.NotNull(collection) since collection is created with new

* ObservableCollectionTests
  * Rewrite tests for batch changes so they
       1. verify events args Action and Index
       2. use InlineData to tests both with "batch notification" and without (reset)
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 10, 2024
@Daniel-Svensson Daniel-Svensson marked this pull request as ready for review July 10, 2024 10:27
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 10, 2024

Thanks for taking the time to post this PR however,

  1. As with any change exposing new APIs, this has to go through the API review process before we can start reviewing an implementation PR. Even though Collection<T> and ObservableCollection<T> do not support ranges #18087 was approved in the past, the fact that the original design caused a regression means that we need to through the same review process with an improved design.
  2. We're currently in the final stages of .NET 9 development, which means that merging this change now would give us very little time to respond to feedback in case of breaking changes being found downstream. It also means that the team is very busy finalizing other feature work, so it would be difficult for us to evaluate this solution or react to any potential breaking changes that it might introduce.

With that said, I'm going to close this PR. If you wish we can try re-evaluating your implementation once .NET 10 development begins in earnest this fall. Thank you for your understanding.

cc @terrajobst

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection<T> and ObservableCollection<T> do not support ranges
2 participants