Adds Range Manipulation APIs for Collection<T> and ObservableCollection<T> #65101
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsRe-Adds range manipulation APIs for Resolves: #18087 Added the following new API that have been approved in #18087: // 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);
}
// virtual methods
protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
protected virtual void RemoveItemsRange(int index, int count);The code contained in this PR has already gone through a full PR review that was originally submitted in 2019 and was pulled due to downstream breaking changes in WPF. I have been talking with the .NET Team and the WPF Team and they both have approved the necessary changes to WPF to get this to work. Original PRs
Related WPF Issue Pending ChangesI am still working on the changes to dotnet/wpf, once I submit the PR to dotnet/wpf I will link it here.
|
|
Change blocked on dotnet/wpf#6097. |
|
First thing, this is awesome, so glad to see this being picked up again! 🎉 @ahoefling have you also talked with the WinUI 3 team to confirm whether they're planning to add support for this? UWP is not a concern given it would never have access to these new APIs, but WinUI 3 very much is, just like WPF is/was (hence the delay). |
|
Thank you @Sergio0694 I am glad we are getting everyone involved early enough to get this in for .NET 7. I have not spoken with the WinUI 3 team yet, do you know who we should loop into this PR so we can make sure there are no issues. Are there any other downstream projects that we should be worried about for breaking changes? Last time I looked at the MAUI/Xamarin.Forms code they do something different with |
|
Thanks @ahoefling. I'm converting this PR to a draft until dotnet/wpf#6097 has been resolved. |
|
Do any of the build artifacts provide a ready to use sdk I can use to compile a wpf sample application? I am having some trouble getting my changes in this PR to be used in my WPF application as it is a new API. If not is there a doc link on the dotnet/sdk project? The pages I have read thus far don't mention how to use your own dotnet/runtime |
Pinging @ryandemopoulos as we had talked about this internally a while back, he'd know who to relay this to I'm sure 😊
As for actual frameworks, can't think of other Microsoft-maintained ones other than WPF, WinUI 3 and MAUI (MAUI is owned by the .NET team anyway so someone from there will be able to comment on whether that needs any changes or not). As for other projects, we'll likely need to support this as well in the Windows Community Toolkit (cc. @michael-hawker) and in the MAUI Community Toolkit (cc. @brminnick), as well as likely obsoleting additional APIs from there once these are built-in. This is not meant to be an exhaustive list so please do chime in if I missed anything here 😄 |
|
I believe this was discussed in the MAUI Community Toolkit when it was the Xamarin Community Toolkit. They have their own implementation of AddRange for ObservableCollection as it wasn't included in runtime. Once we get this merged that code can be removed from the toolkit. @brminnick do you know if we had an issue documented for that? |
I spent some time today working with @snickler today and we were able to get the sdk to compile WPF with the code in this PR. This allows me to create a console app as well, so I am going to create benchmarks and attach them to the main description to provide a more in-depth analysis of the performance impact this PR has. |
Yup, this was my understanding as well. It's also why so far I didn't want to port them over or implement equivalent APIs in the MVVM Toolkit either, as I wanted to instead do this right and just wait for proper BCL support, so that users would've had a single (built-in) API to use anywhere, instead of needing external helpers just to work around specific framework limitations. That's also why I'm really happy to see this being picked up again, and really hope it'll make it this time around 😄 |
|
I tried creating a benchmark project and wasn't able to get it working. Looks like csproj <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net7.0</TargetFramework>
<RuntimeIdentifier>win10-x86</RuntimeIdentifier>
<Nullable>enable</Nullable>
</PropertyGroup>
<ItemGroup>
<FrameworkReference Update="Microsoft.NETCore.App" RuntimeFrameworkVersion="7.0.0-dev" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="BenchmarkDotNet" Version="0.13.1" />
<PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" Version="0.13.1" />
</ItemGroup>
</Project>Error message when running the benchmark application Some Things I've TriedStandard publish dotnet build/run Removing |
|
@ahoefling you should try reusing the infrastructure in dotnet/performance, see https://github.com/dotnet/performance/blob/main/src/benchmarks/micro/README.md#private-runtime-builds for instructions. |
|
Thanks @eiriktsarpalis for pointing me in the right direction. I was able to run benchmarks on all the new APIs and compared them to the old APIs. The performance and allocation improvements are significant. Below are 2 sets of benchmarks to demonstrate performance improvement and allocation improvements on a small dataset of 512 records and a large dataset of 262144 records. It appears that all the new range APIs are providing significant performance improvements and allocation improvements. Size: 512Size: 262144Benchmarking Methodology and CodeI created the benchmarks using the https://github.com/dotnet/performance project and ran it against my locally compiled x64 version of .NET 7 runtime. Most of the APIs had an equivelant old API with the exception of Replace/ReplaceRange. There is no `Replace` API so I had to add additional code to simulate this behavior. This may skew the results for that particular benchmark [Benchmark]
public ObservableCollection<T> ObservableCollection_Add()
{
var collection = new ObservableCollection<T>();
foreach (T value in _uniqueValues)
{
collection.Add(value);
}
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_AddRange()
{
var collection = new ObservableCollection<T>();
collection.AddRange(_uniqueValues);
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_Insert()
{
var collection = new ObservableCollection<T>();
for (int i = 0; i < _uniqueValues.Length; i++)
{
collection.Insert(i, _uniqueValues[i]);
}
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_InsertRange()
{
var collection = new ObservableCollection<T>();
collection.InsertRange(0, _uniqueValues);
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_Remove()
{
var collection = new ObservableCollection<T>(_uniqueValues);
for (int i = 0; i < collection.Count; i++)
{
collection.Remove(collection[i]);
}
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_RemoveRange()
{
var collection = new ObservableCollection<T>(_uniqueValues);
collection.RemoveRange(0, collection.Count);
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_Replace()
{
var collection = new ObservableCollection<T>(_uniqueValues);
for (int i = 0; i < collection.Count; i++)
{
collection.RemoveAt(i);
collection.Insert(i, _uniqueValues[i]);
}
return collection;
}
[Benchmark]
public ObservableCollection<T> ObservableCollection_ReplaceRange()
{
var collection = new ObservableCollection<T>(_uniqueValues);
collection.ReplaceRange(0, collection.Count, _uniqueValues);
return collection;
} |
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
|
Is there a way we can keep this open as a draft as things are going to take a little bit of time while I balance work commitments and finishing up the WPF work? I hope the performance numbers I posted help convince everyone else how important this PR is to overall runtime performance for |
|
@eiriktsarpalis could you help reopen this? 🙂 @ahoefling is there any help you need from the teams with the other PRs you're doing for WPF etc. or is it just a matter of finding the time to finish them? I'll also see if I can ask WinUI 3 folks internally about this (like WPF, they'll need to support the new events in the built-in controls or things will fall apart). We would really like to see this change to make it to .NET 7 to support it in the MVVM Toolkit (which is also the reference MVVM implementation for MAUI at this point), as we're explicitly not wanting to add polyfills or workarounds for this and just do it properly once this is merged. Will also need to ask about MAUI support for this but I assume that shouldn't be a problem. There's still plenty of time (not too much, but a fair amount), so I'm cautiously optimistic. This would be a great feature to finally have 😄 |
|
Thanks for the feedback, we'll see if we can update the automation so fair warning is provided before drafts get auto-closed. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
|
Starting to lose hope here as we're cutting it close at this point — @ahoefling what's the status on your PR, is there any remaining work on your end and do you need help? @PureWeen the MAUI team has added support for this to the .NET 7 milestone, is that correct? And is that still on track? And does anyone have an update on the WPF side, @ahoefling were you also going to contribute support for this there? Were there any blockers on that front? Asking here and not internally given all of those projects are open source, so wanted Andrew and other folks to be able to see follow ups here if possible 🙂 |
If you would have taken just a quick look at the WPF repo, you would immediately see that there is no chance for anything else than bug fixes. Not even complete community PR's are getting reviewed. dotnet/wpf#6542 dotnet/wpf#6556 (comment) dotnet/wpf#6171 (comment) WPF community is more than frustrated about WPF team reject anything else than bug fixes since it has been open sourced. If this HERE is waiting for WPF then it has no chance at all as long as the WPF team remains the same. |
|
When I reached out to the WPF team they confirmed they will not have the bandwidth to work in this in .NET 7. Effectively, the situation remains the same since this PR was last reverted. FWIW, I tend to agree with @legistek's analysis in this and other threads: fundamentally, this is changing the contract for observable collections and has the potential to break any observer depending on the current behavior, not just WPF. |
|
I would argue this does not break WPF in of itself. The collection changed event args already support ranged operations, so WPF is already broken with any custom observable collection raising such an event (it's just people never bothered to implement them since WPF doesn't support them). Even with this API in, it's not like WPF apps upgrading to .NET 7 would break, they would only break if new code targeting .NET 7 then called the new ranged APIs. But I don't see that as being conceptually that different than, as I mentioned, someone just implementing a custom collection and raising a the collection changed event with multiple items (which has been possible since .NET 1, basically). That said, I can understand the WPF team not having bandwidth. I will say though it's disappointing that we're still stuck with this after 6 years (since the issue, otherwise after way more). |
I would guess it's yet another manifestation of Hyrum's Law. Wouldn't be surprised if other components in the ecosystem exhibited the same problem.
I'm not particularly knowledgeable about WPF, but my recollection is that it broke in a substantial way as soon as runtime merged this change originally.
Would doing that be a less disruptive way for newer frameworks to get range support? |
I agree this is very likely not the only one, it's just one that keeps popping up quite often. It's been a pain point for years (first on WPF, then on UWP, now on both MAUI and WinUI 3, etc.), and something that has received countless feature requests for. The solution many have come up with is to just roll their own collection types with support for "range operations", with the big drawback being though that since no UI framework among these actually supports the API, they're just forced to either iterate over items one by one (which is just terrible for performance), or just reset the collection and reinitialize it (which is also bad for performance, maybe just a little bit less, and also completely breaks all animations). It's just a very unfortunate situation where customers get frustrated, and they often have to both do extra work to build these helpers, and then still end up with a suboptimal experience. I mean, we have the same exact custom collection type in the Microsoft Store too, and it has the same exact issues (and I hate it). It's just a really common issue, and I'm saying I'd love for us to find the time to coordinate with the right folks and maybe try to finally get this resolved at least for .NET 8 🙂 I've also received multiple requests to add such a custom collection type in the .NET Community Toolkit, and I've had to reject them for the reasons mentioned above (it still results in a very bad experience, and I'd much rather just fix this properly in the BCL). The net result though is that at least for now, the API is not there and plenty of folks are frustrated.
I find that surprising and I'd love to learn more about that. Even with the new APIs, as long as nobody actually calls them, there should be no functional differences in the way the currently existing APIs work at all. So I'm not sure I understand what broke. Are you sure it wasn't just folks trying out the new APIs, and then reporting WPF crashing/breaking in that scenario? 🤔
I personally don't think that'd be the correct solution for this issue, for a number of reasons:
I would really love for us to just manage to coordinate across the various teams and officially add support for this in a future release, not sooner than .NET 8 at this point. We could both update all underlying frameworks to support this (which I realize is non trivial work, especially since eg. WinUI 3 would likely need some new WinRT APIs and projections for this), and then provide the necessary migration docs, if needed. I believe this would be the best long term solution for this issue 🙂 |
I initially assumed this as well. But after digging very deep into the issue I learned the issue was extension methods. Many people (including myself) took matters into their own hands over the years and defined their own extension methods such as: These were WPF friendly because they didn't use the INCC modes that break WPF. However, when the new So you see, there truly is no way to make these changes to But another solution exists that would not break any existing code 100% guaranteed. Just make a new class entirely - You could still do this in .NET7 I reckon. |
|
I see, so the issue is just when recompiling from source. What I meant is that existing binaries just running on .NET 7 would not be affected, which would still be the case (eg. if you were referencing some NuGet package, that would be fine). If the issue is this specific case, that's something that could be called out in a migration guide for this. Or, we could very well include an analyzer (that is, issue a warning if the new API is called from a callsite where an extension would've been in scope as well), if we decided this was worth it. I absolutely disagree with the conclusion that this is a valid reason to not implement this.
There's absolutely no way to get a new API like that proposed, reviewed, approved and implemented in .NET 7. |
|
Correct nothing would break at the binary level.
You may be right, but TPTB back when this all went down were faced with the same facts and decided to revert it. Part of it may have been being pressed for time as it was discovered late in the .NET Core 3 preview cycle IIRC. You're right, a migration guide or analyzer could help. People retargeting their apps just need to be very keenly aware of this issue. Renaming their extension methods before migration should do the trick, but that's still putting the burden on them to find them all. (You have to figure most WPF apps have been around for quite awhile). This would be a lot more effort than your typical breaking change IMO, with a lot of opportunities to miss offending code. The most important thing from my perspective is making sure folks like yourself understand that: (1) nothing has changed since this was originally done and reverted; (2) WPF is not going to be "fixed" to truly support ranges; the PM already essentially called this a nonstarter; and (3) the folks saying they have fixed WPF already to support this are incorrect; their proposals are just bandaids. So whatever you all do just please do so with accurate information and make sure we all are VERY AWARE of what you did so we can plan for it. Thanks! |
If that is what people have to do, to make this not a breaking change. Well then we could use that argument for literally everything that break existing code and just say "Devs have to change their code then before they can use it". You know that many people are using automatic builds? Adding it to the analyzer just generating a warning will not be seen. It has to generate an error instead forcing builds to break. But then again, it's a breaking change. If anyone want this feature so badly, I'd recommend to go ahead and create a PR on WPF repo. Chances are low but that is the only chance you have (unless breaking change will be accepted). |
|
On the topic of fixing the frameworks vs. changing the BCL, I think these need to be uncoupled because they're really not dependent on each other at all. On changing the frameworks, everyone needs to accept that WPF isn't going to be changed. BUT the other frameworks could be and there's no reason they need to wait on anything in the BCL to add support for ranged INCC notifications, if for no other reason than to support individuals' own implementations. On the BCL end, whether the frameworks are changed still has nothing to do with whether you're risking people accidentally invoking new methods when they didn't intend do. See my example above with the A new class does the job with zero risk. No breaking code, no analyzers, or migration guides. As someone who runs a company that sells software to real people for actual money, my biggest concern in any update is not introducing new bugs. It tends to make customers upset. So while I care about things like abstraction paradigms and how tedious it might be to opt-into new functionality, I'd much rather sacrifice elegance and spend a little extra effort in favor of a guarantee that I won't be introducing unintended and difficult to detect side effects. Practical concerns that impact real commercial applications need to come first; otherwise, what's the point of any of this? |
|
Ok one more comment, sorry, and also about to contradict myself. I could see scenarios where you would want to share viewmodels between WPF and WinUI or MAUI. If you do have a new class, you won't be able to use it with WPF, so sharing would then not be possible. The same problem arises if you change OC though. So here's a thought - What if you kept this PR mostly as is but added Then in the WPF repo - and any other framework that doesn't (yet) support this - maybe in a static constructor for Then - I think, possibly, maybe - you could make this change and not break WPF, even if extension methods got overridden. |
|
Sounds like an interesting workaround. Could you share it #18087 just so that it gains more traction and feedback? If I'm honest, it's unlikely we'd get to this for .NET 7, we're fairly late in development and there needs to be another round of API review and documenting the breaking change/workaround. |
|
Good deal @eiriktsarpalis. I would love to post the idea on #18087 but unfortunately the individual who opened that issue (who doesn't appear to be affiliated with Microsoft) seems to have disliked my prior comments on that issue and blocked me personally which prevents me from commenting further on that or any issues he started. |
|
Well, that sucks. We'll see if there's a way to have your voice represented in that issue (worst case scenario we'll create a new one). |
|
lol, well, there are far worse things in life. :) But thanks for throwing it out there! |
|
@legistek Apologies, I don't remember blocking you, but sorry if I did. I think your workaround has a lot of merit. |
|
Cheers. Just happy to contribute what I can toward a solution to what has been a complex and vexing problem for a long time. |
|
@Sergio0694 answering your question earlier about an update as you mentioned you spoke with @PureWeen from .NET MAUI. I've had very poor mental health this year and have had some life events that prevented me from getting to the WPF code. I had all intentions of doing it but at this point I really don't see myself finding the time or energy to bring this over the finish line for .NET 7. I do hope someone can volunteer their time to finish the WPF code. I'm sorry 😔 |
|
I like the observable list here. Time to stop doing bad things in view models. |
|
I would ask for your apologies for the rant you’re about to read, but after 6 years and all the patience I can muster, I believe I’ve earned it. The original issue and PR for this bug is about to celebrate it’s SEVENTH BIRTHDAY. Over that time Skye and I have expended so much energy trying to get this over the finish line. It is incredibly disappointing that there are so many at Microsoft continuing to get in the way of getting this done. .NET Core was sold to us developers as a way to break from the past, and major releases were going to be places where breaking changes could be made. Yet for 6 years I’ve continued to hear excuse after excuse. To quote Yoda, “always with you it cannot be done.” Had this been fixed with .NET Core 2 when I submitted the first PR, MAUI and the other platforms would have dealt with it when they were being built, and WPF would have been fixed when it was being migrated. But it was punted to .NET Core 3. Then, after the PR was already accepted and integrated, I was told WPF was already late and had too many bugs to be able to get the work done in time for the .NET Core 3 release. Our work was held up because the WPF team over-promised and couldn’t get its act together. Victory was ripped away from us at the last possible second. In the time since, instead of guiding people building new frameworks like MAUI towards making decisions that would allow this work to get done more easily, Microsoft finds time to invest in things like “Minimal APIs” that only marginally impact developers. So you’ll have to forgive me if, after missing it in .NET Core 2, 3, 5, 6, AND 7, that maybe I don’t believe we’ll see it in .NET 8. It’s cool though. Maybe we’ll see this get fixed by the time .NET 12 rolls around. In the meantime, I’ll just continue to wonder how anyone EVER let WPF get past a code review with exceptions being thrown in event handlers, and why it seems like the same people are being allowed to hold back amazing frameworks like Blazor from taking advantage of these features. @SkyeHoefling thank you so much for your efforts. I can only imagine the emotional toll that having your status as “.NET contributor” ripped away year after year after effing year has had on you. It sucks having so many people stand in the way of something so obviously broken. |
|
I don't get the worry here that multiple frameworks doesn't support multi item changes. This PR has nothing to do with this. Sure this PR makes it easier to raise multi item changes on a commonly used collection, but it doesn't in any way expose an issue that wasn’t already there. It merely implements the existing interface to its fullest. Holding up this PR because WPF has a problem that it always had and we’ve complained about for years, when this change can create huge performance gains for other UI frameworks is frankly incredibly disappointing. I guess we’ll just have to continue with using our own observable collection implementations. |
|
Absolutely agree with @dotMorten here. WPF already has a bug, that shouldn't hold back improvements to the BCL. And ObservableCollection badly needs this API. The correct thing to do here is to fix WPF. And given that WPF is also a Microsoft project, this shouldn't be an insurmountable problem. WPF is already broken as things are. I thought the whole point of the new .NET was to be able to more easily do these kinds of potentially breaking changes. If this breaks another part of .NET, just go fix that part as well. |
|
What part of WinUI 3 doesn't support this? Works for me? Judging from animation it correctly animated two adds at a time. Update: insert does have issues and only detects the first element in WinUI3 |
|
I just rebased this branch on the latest changes in the main branch. Code is now up to date, there were no conflicts. |
|
Thanks @SkyeHoefling. Let's revisit an implementation once the updated proposal has been reviewed. We're fairly late in the development of .NET 7, so even if it does get approved, we'd likely push this to .NET 8 in order to give libraries adequate reaction time. |
|
Just an FYI to everyone on this thread, I believe I have discovered an issue with the updated proposal and have left comments on that thread. The short version is that we should be using SemVer + conditional NuGet package references + breaking changes documentation to solve the problem, not allowing 3rd party libraries to set a flag that silently overrides how the entire .NET runtime works simply by installing an offending NuGet package. If I go through all the effort to fix my libraries, some lazy developer should not be able to silently override my work or change the behavior or my customer's apps. THEIR code should break, not mine. The proposed fix just creates an equal-and-opposite problem. To address this comment:
The new range-supporting methods are not being "inadvertently" called, this is EXACTLY how .NET should behave. Notification here can be easily achieved with built-in Roslyn analyzers and/or Trace.Warn messages, in addition to "breaking changes" documentation for the .NET 8.0 release. If we're going to punt this AGAIN, there is plenty of time to build the needed Roslyn analyzer. We should get THAT spec'd out and planned so that some non-Microsoft developer can contribute it while .NET 7 is being RTM'd. |
ghost
locked as resolved and limited conversation to collaborators
Re-Adds range manipulation APIs for
Collection<T>andObservableCollection<T>Resolves: #18087
Added the following new API that have been approved in #18087:
The code contained in this PR has already gone through a full PR review that was originally submitted in 2019 and was pulled due to downstream breaking changes in WPF. I have been talking with the .NET Team and the WPF Team and they both have approved the necessary changes to WPF to get this to work.
Original PRs
Related WPF Issues/PRs
Benchmarks - 2/11/2022