-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Collection<T> and ObservableCollection<T> do not support ranges #18087
Comments
While you're in there adding an AddRange() method, can you throw an OnPropertyChanged() into the Count property's setter? Thanks :) |
A long time ago I had implemented a |
@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with @robertmclaws ? /cc @terrajobst |
Sure. |
@robertmclaws Can you create an api speclet on this issue, outling the api syntax, like this. Mainly interested in usage scenarios |
In what situation could it be a breaking change? The only issue I can think of is that it would cause a warning that tells you to use |
@svick Could possibly be a runtime problem. If you just upgraded the framework w/o recompiling, I'm not sure exactly how the runtime execution would react. We'd need to test it just to make sure. |
@robertmclaws I think that could only be a problem if you don't recompile, but you do upgrade a library with the custom type inheriting from Otherwise, adding a new member won't affect how old binaries behave. |
+1 The api sounds good to me. For manipulating multiple items , along with AddRange, does it provide value to add, InsertRange, RemoveRange, GetRange for the specified usage scenarios? cc @terrajobst |
@svick You are probably right. I personally would want to test the behavior just to be sure we're not breaking anyone... otherwise this would move to a 2.0 release item. @Priya91 I'm not sure if a So if we're comfortable with the API, what's the next step? :) |
Clear is already available. We still haven't gotten the shape of apis to add, if RemoveRange and InsertRange are to be added, then we need these apis added to the speclet. And then we'll mark api-ready-for-review, to be discussed in the next api-review meeting either on tuesday or friday. |
OK, I made changes to the speclet. Note that the parameters might change for the actual implementation, but those are what makes the most sense at this particular second. Please LMK if I need to do anything else. Thanks! |
|
count instead of endIndex..
|
public void AddRange(IEnumerable<T> collection, bool clearFirst = false) { }
public void InsertRange(IEnumerable<T> collection, int startIndex) { }
public void RemoveRange(int startIndex, int count) { }
public void ReplaceRange(IEnumerable<T> collection, int startIndex, int count) { } |
Basically the signatures should be the same as in I don't think the A
|
I think @thomaslevesque I have the I'm not against a |
Also, the index parameter usually comes first in existing APIs, so And I don't think ReplaceRange needs a Here's the API as I see it:
|
I'm not sold on it, but hey, it's your proposal, not mine 😉. At the very least, I think it should a separate overload, rather than an optional parameter. |
This makes me think... there are lots of possible combination of changes you might want to do on the collection without triggering events for each one. So instead of trying to think of each case and introduce a new method for each, perhaps we should lean toward a more generic solution. Something like this:
|
@thomaslevesque Overload vs optional parameter makes no practical difference to the end user. It's just splitting hairs. Having overloads just adds unnecessary lines of code.
If the index comes first in existing APIs, then I'm fine with this: public void AddRange(IEnumerable<T> collection, clearFirst bool = false) { }
public void InsertRange(int index, IEnumerable<T> collection) { }
public void RemoveRange(int index, int count) { }
public void ReplaceRange(int index, int count, IEnumerable<T> collection) { }
public int RemoveAll(Predicate<T> match) |
It's not. Optional parameter can cause very real issues when used in public APIs. Read this blog post by @haacked for details. |
I'm actually liking @thomaslevesque's idea about using a batching class. It's a common pattern, well understood, and makes complex workflows easier. |
That would be quite inefficient. Removing items would cause all following items to be moved backwards, and inserting new ones would cause them to be moved forward again. The implementation I have in mind would replace each item in-place, without moving anything. |
The point of this proposal was to fill in the gaps on the existing implementation, not coming up with a new pattern for people to deal with. I'm not against that proposal, but that's an entirely new piece of functionality that I don't believe should be a part of this discussion. |
@robertmclaws but since there is no way to currently do bulk operations there isn't a "new pattern" |
Why does that matter? Is it a memory allocation issue? |
I agree that it should probably be a separate proposal, but it does solve the initial problem you were having. |
This was attempted in the PR that triggered the comment I linked in my previous reply on this thread, dotnet/wpf#6097.
The first comment on that PR, dotnet/wpf#6097 (review), included a bulleted list of five items that need to change in order for it to be OK to remove the validation. Furthermore, as the discussion continued on that thread, someone else chimed in, dotnet/wpf#6097 (comment), pointing to a specific line of code. The link is stale now... here's a permalink: https://github.com/dotnet/wpf/blob/ae1790531c3b993b56eba8b1f0dd395a3ed7de75/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Data/ListCollectionView.cs#L1748 Personally, I'm also a little perplexed as to why this is still considered to be a big enough blocker when the
|
This is not true and completely misrepresents how the .NET team(s) work (and the type of challenges they are facing). I don't think anybody ever expressed the opinion that this shouldn't be fixed, in fact consensus is very strongly in favor of this being addressed eventually. The simple reality is that resources are limited and priorities can be conflicting so as with any effort requiring substantial coordination the windows of opportunity are tiny and easy to miss. |
WPF is open source. |
Another solution to problem of Current problems:
I propose to create Downside is that UI frameworks will need to adopt new type. |
Since the error only strikes on recompile, why not have WPF ship an analyzer with a build error if any AddRange that now binds to the instance method could also find an AddRange extension method in scope that it would have previously been calling? If deleting or dereferencing the extension method is not an option, you can silence the error once for the project/solution. Or, taking a page from the C# language's breaking changes strategy, start providing a warning on the extension methods today and leave it in place for at least one .NET cycle. The warning will tell you to future-proof your app to be ready for .NET 10/11, maybe with a light bulb fix having you rename the extension method so that you aren't broken on upgrade. |
I wish it was that simple... But in the WPF repo approving even the smallest fixes with no known risk may take forever. In contrast, WinForms guys usually react super fast, or even encourage me to send a PR so it will go through quicker. |
This is what I was trying to get at. As someone who's just been keeping an eye on this issue loosely, it seems to me like what keeps happening is that Microsoft says, "It's going to take time to coordinate", but nobody has actually even begun the process. |
Again, not accurate. The teams do make legitimate attempts to prioritize this issue every year (many of which are reflected in this discussion) however please consider that every engineer is accountable for literally hundreds of issues like this one and the fact that there sometimes exist conflicting business priorities. |
@eiriktsarpalis I understand that, what I'm advocating for is clearer messaging. When you say, "The change has to be made in coordination with downstream frameworks, which takes a long time" IMHO that doesn't tell most of us much about what the actual state of the issue is. Is it in progress? Is communication and coordination happening? Or are you saying that nobody has been able to commit to it yet, and it hasn't moved at all? It can lead to a feeling that nothing is happening on the issue. |
Whelp, Implicit Extension Types just got moved to .NET 10 (scroll to the bottom), so I guess I'm not solving this problem that way this year either. |
Yes and no. I can absolutely imagine that every engineer is accountable for hundreds of issues. But are those issues really "like this one"? That is, are they equally old, advanced, and perceived as important? Has each and every one of those issues invoked dozens of libraries and thousands of apps implementing workarounds, caused hundreds of questions leading to upvotes on this issue, got dozens of duplicates because they all want the same? I find it hard to believe that for 8 years, every single time the threshold of "important enough" lies just above the importance of this one. I think the real issue that it is always easier to add new stuff even if only 5,000 people need that, than to fix old stuff 10,000,000 people need when it breaks something for 10,000 of them. |
Yes, unequivocally. Issue age is not the only deciding factor. |
Having slept about it, I wonder: if the core reason this whole API is so hard to roll out to the public is "just" the batching of events breaking downstream libraries, would it be possible to add the new methods plus a switch This way there would be some movement in the right direction, that at least adds the new methods so people can start to use them. And if their program is capable of handling the batched notifications, they can change the switch to And later on, when sufficient progress was made in downstream libraries, the default of that switch can be set to And even if in the very distant future the switch would eventually get removed altogether, it still doesn't break anything, because it's just a compiler switch and defining them without any place in the code using them doesn't break anything either. I think it would be a good idea to get the API out there to be used, even if it doesn't provide performance benefits yet by default. But at least we would be able to start to do the right thing (use Start to do the right things now, benefit later. |
I support the implementation of a feature toggle but strongly oppose defaulting it to 'off'. This feature addresses a longstanding need within the community, bringing valuable new functionality. The real bottleneck lies with non-MS libraries and applications using custom extension methods, which impede progress. As this change does not affect legacy applications unless they opt for a newer binary, any update should handle either resolving breaking changes or disabling the feature via the feature flag. Since this is entirely new additive code and not a true breaking change, it should be included by default in standard updates. |
Defaulting to off, while debatable, would still be better than not shipping it at all in |
Apologies that this got punted again from .NET 9. To a large extent this was my fault for not doing a better job driving an investigation with design options. Since it's clear that this request isn't simple, I've started a design document that we can use at the beginning of .NET 10 to do a better job socializing the changes across the .NET teams and getting the necessary buy-in to complete it (or decide that we don't want to and close it). Since there was some conversation around prioritization and how/why this hasn't happened, I thought it might be helpful to put into context how the .NET team works. Generally speaking we work both "top down" and "bottom up", the direction indicating who in the org chart is in the driver seat.
Why do we do both?
Now, in practice these two aren't binary but locations on a spectrum because the org chart has several levels. The resources you get to fund work increases as you go up in the org chart, at the expense of reduced throughput due to higher scrutiny and organizational overhead. This brings us back to this item. It started as "let's just add some methods on a type to allow bulk operations". We do changes like this all the time and it usually fits into our bottom-up workflow. Then we ran into issues. Given that this team is very close to the bottom of the stack, we can't just make changes we identified as disruptive and tell the upstream teams to just deal with it. That's irresponsible because it adds risk to the release. Where we failed our customers here is that we didn't move this issue to a more top-down item in order to secure corresponding work from the WPF team. Another way to think about this is that "Microsoft" isn't a centralized being. Rather, it's a decentralized set of people. If we fail to deliver something that requires multiple parties to coordinate, it's almost always a failure of communication and/or a problem of getting priorities aligned. That's why it's not as simple as saying "this work costs Microsoft X, but it costs the ecosystem 10 * X, therefore it should be worth it" because there often is no single person that has both the power and the context to make that decision. It's an exercise of building consensus and at any given point in time there are only so many of those that people like me are able to pick up. |
I've marked the API as "needs work" because we don't have an API at this point. We need to figure out the interactions of the feature and how we handle the fact that WPF is broken. There are several options and the design document is the artifact driving those. |
@terrajobst Nice write-up Immo. |
@terrajobst Thanks so much for taking the time to put everything together, including this response. You're a fantastic leader of .NET and I for one really appreciate you. 🤜🏻 |
@dotMorten I've been looking for an opportunity to participate in .NET development, but I'm new to this, so I don't know yet if I have what it takes to make changes to core components. But I think we could try to compile a list of the controls that need fixing, which makes it easier for me and others to judge if there's some controls that are "on our level" of complexity. I have to admit I haven't looked through all 500 posts or linked/related items, so maybe this list already exists somewhere. |
@terrajobst Thanks for the fantastic explanation. For me, just having an idea what's going on like that really does help! |
To me, it seems that the main problem is the wrong handling of the Here's my suggestion for an analyzer:
This analyzer should produce a build warning. Fixing this warning should ensure that range operations work as expected. |
Update 10/04/2018
@ianhays and I discussed this and we agree to add this 6 APIs for now:
As those are the most commonly used across collection types and the
Predicate
ones can be achieved through Linq and seem like edge cases.To answer @terrajobst questions:
Yes, we would like to introduce 2 protected virtual methods to stick with the current pattern that we follow with other Insert/Remove apis to give people hability to add their custom removals (like filtering items on a certain condition).
Yes, and then
ObservableCollection
could just call the base implementation and then trigger the necessary events.Let's keep the final speclet at the top for easier search
Speclet (Updated 9/23/2016)
Scope
Modernize
Collection<T>
andObservableCollection<T>
by allowing them to handle operations against multiple items simultaneously.Rationale
The
ObservableCollection
is a critical collection when it comes to XAML-based development, though it can also be useful when building API client libraries as well. Because it implementsINotifyPropertyChanged
andINotifyCollectionChanged
, nearly every XAML app in existence uses some form of this collection to bind a set of objects against UI.However, this class has some shortcomings. Namely, it cannot currently handle adding or removing multiple objects in a single call. Because of that, it also cannot manipulate the collection in such a way that the
PropertyChanged
events are raised at the very end of the operation.Consider the following situation:
CollectionChanged
event 25 times. If you are also using that event to do other processing on incoming items, then those events are firing 25 times too. This can get very expensive, very quickly.ChangedItems
Lists that will only ever have 0 or 1 objects in them. That is... not ideal.This behavior is unnecessary, especially considering that
NotifyCollectionChangedEventArgs
already has the components necessary to handle firing the event once for multiple items, but that capability is presently not being used at all.Implementing this properly would allow for better performance in these types of apps, and would negate the need for the plethora of replacements out there (here, here, and here, for example).
Usage
Given the above scenario as an example, usage would look like this pseudocode:
Implementation
This is not the complete implementation, because other
*Range
functionality would need to be implemented as well. You can see the start of this work in PR dotnet/corefx#10751Obstacles
Doing this properly, and having the methods intuitively named, could potentially have the side effect of breaking existing classes that inherit from
ObservableCollection
to solve this problem. A good way to test this would be to make the change, compile something like Template10 against this new assembly, and see if it breaks.So the
ObservableCollection
is one of the cornerstones of software development, not just in Windows, but on the web. One issue that comes up constantly is that, while theOnCollectionChanged
event has a structure and constructors that support signaling the change for multiple items being added, theObservableCollection
does not have a method to support this.If you look at the web as an example, Knockout has a way to be able to add multiple items to the collection, but not signal the change until the very end. The
ObservableCollection
needs the same functionality, but does not have it.If you look at other extension methods to solve this problem, like the one in Template10, they let you add multiple items, but do not solve the signaling problem. That's because the
ObservableCollection.InsertItem()
method overridesCollection.InsertItem()
, and all of the other methods are private. So the only way to fix this properly is in theObservableCollection
itself.I'm proposing an "AddRange" function that accepts an existing collection as input, optionally clears the collection before adding, and then throws the
OnCollectionChanging
event AFTER all the objects have been added. I have already implemented this in a PR dotnet/corefx#10751 so you can see what the implementation would look like.I look forward to your feedback. Thanks!
The text was updated successfully, but these errors were encountered: