Runtime: Collection<T> and ObservableCollection<T> do not support ranges

110

Update 10/04/2018

@ianhays and I discussed this and we agree to add this 6 APIs for now:

    // 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);
    }

    #region virtual methods
    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
    protected virtual void RemoveItemsRange(int index, int count);
    #endregion

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:

Should the methods be virtual? If no, why not? If yes, how does eventing work and how do derived types work?

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).

Should some of these methods be pushed down to Collection?

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> and ObservableCollection<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 implements INotifyPropertyChanged and INotifyCollectionChanged, 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:

  • You have a XAML app that accesses an API.
  • That API call returns 25 objects that need to be bound to the UI.
  • In order to get the data displayed into the UI, you likely have to cycle through the results, and add them one at a time to the ObservableCollection.
  • This has the side-effect of firing the 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.
  • Additionally, that event will have 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:

    var observable = new ObservableCollection<SomeObject>();
    var client = new HttpClient();
    var result = client.GetStringAsync("http://someapi.com/someobject");
    var results = JsonConvert.DeserializeObject<SomeObject>(result);
    observable.AddRange(results);

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#10751


    // Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection)

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int 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);

    // Removes any item that matches the search criteria.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    // RWM: Excluded for now, will see if possible to add back in after implementation and testing.
    // public int RemoveAll(Predicate<T> match);

Obstacles

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 the OnCollectionChanged event has a structure and constructors that support signaling the change for multiple items being added, the ObservableCollection 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 overrides Collection.InsertItem(), and all of the other methods are private. So the only way to fix this properly is in the ObservableCollection 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!

robertmclaws picture robertmclaws  ·  13 Aug 2016

Most helpful comment

30

API Review 2nd round

In order of appearance in code

See implementation and explanations in PR dotnet/corefx#26482.

  • [x] Inserts a range at the end.
    public void AddRange(IEnumerable<T> collection);
  • [x] Inserts a range
    public void InsertRange(int index, IEnumerable<T> collection);
  • [x] Removes a range.
    public void RemoveRange(int index, int count);
  • [ ] Removes the first occurence of each item in the specified collection
    public void RemoveRange(IEnumerable<T> collection);
  • [x] Removes only matching items.
    public int RemoveAll(Predicate<T> match);
  • [ ] Removes only matching items, constraining the search to a specific range in the collection.
    public int RemoveAll(int index, int count, Predicate<T> match);
  • [ ] Clears the current collection and replaces it with the specified collection using default eq. comparer.
    public void ReplaceRange(IEnumerable<T> collection);
  • [ ] Clears the current collection and replaces it with the specified collection using specified comparer.
    public void ReplaceRange(IEnumerable<T> collection, IEqualityComparer<T> comparer);
  • [x] Repalces a range with fewer, equal, or more items, reusing equal items in similar indices.
    public void ReplaceRange(int index, int count, IEnumerable<T> collection);
  • [ ] Repalces a range with fewer, equal, or more items, using the specified comparer to skip equal items in equal indices.
    public void ReplaceRange(int index, int count, IEnumerable<T> collection, IEqualityComparer<T> comparer);

Legend:

  • [x] API approved
  • [ ] API proposed

Notes:

  • All methods above are implemented and tested, some not as extensively as they should.
    It's up to you guys to decide what remains here (will add more tests upon API verdict).
  • In addition to the proposed API, some optimizations were added to the existing methods, to reduce chance of raising unnecessary events, for example, adding a range with an empty collection will do nothing. Similarly clearing an empty collection will do nothing, and some more situations.
  • We would also want to decide whether to make some of the methods virtual, or even bring some of them (specifically does existing in List<T>), down to Collection<T> (The base of ObservableCollection<T>).
  • We should decide which of them tunnel down to base and which stand for themselves as virtual/sealed (see this comment).
  • Most of the overloaded methods tunnel down to one of the lower level methods, so code is reused rather then just pasted out. After we decide on what API sticks with us, we maybe want to clean down the code even further.
weitzhandler picture weitzhandler  ·  20 Jan 2018

All comments

0

@joshfree @Priya91 Since I already have a PR that addresses this issue, is there any way this could be moved up to 1.1?

robertmclaws picture robertmclaws  ·  13 Sep 2016
5

While you're in there adding an AddRange() method, can you throw an OnPropertyChanged() into the Count property's setter? Thanks :)

LanceMcCarthy picture LanceMcCarthy  ·  13 Sep 2016
4

A long time ago I had implemented a RangeObservableCollection with AddRange, RemoveRange, InsertRange, ReplaceRange and RemoveAll. But it turned out that the WPF binding system didn't support CollectionChanged notifications with multiple items (I seem to remember it has been fixed since then, but I'm not sure).

thomaslevesque picture thomaslevesque  ·  13 Sep 2016
0

@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with @robertmclaws ?

/cc @terrajobst

joshfree picture joshfree  ·  13 Sep 2016
0

@Priya91 can you help shepherd this through the API review process http://aka.ms/apireview with

Sure.

Priya91 picture Priya91  ·  14 Sep 2016
0

@robertmclaws Can you create an api speclet on this issue, outling the api syntax, like this. Mainly interested in usage scenarios

Priya91 picture Priya91  ·  14 Sep 2016
2

@robertmclaws

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.

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 new if you meant to hide a base class member, which would be actually an error with warnings as errors enabled. Is this what you meant? Or is there another case I'm missing?

svick picture svick  ·  14 Sep 2016
0

@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 picture robertmclaws  ·  14 Sep 2016
0

@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 ObservableCollection<T>, which _removed_ its version of AddRange() in the new version. But that would be the fault of that library.

Otherwise, adding a new member won't affect how old binaries behave.

svick picture svick  ·  14 Sep 2016
0

+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

Priya91 picture Priya91  ·  14 Sep 2016
0

@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 GetRange() would be necessary, but InsertRange() and RemoveRange() would be, along with ReplaceRange(), and possible a Clear() method if one is not currently available.

So if we're comfortable with the API, what's the next step? :)

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

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.

Priya91 picture Priya91  ·  16 Sep 2016
0

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!

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

RemoveRange(int index, int count) instead of RemoveRange(ICollection) ? How does RemoveRange behave when the ICollection elements are duplicated in ObservableCollection

Priya91 picture Priya91  ·  16 Sep 2016
0

count instead of endIndex..

public void ReplaceRange(IEnumerable<T> collection, int startIndex, int count)
Priya91 picture Priya91  ·  16 Sep 2016
0

c# 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) { }

Priya91 picture Priya91  ·  16 Sep 2016
4

Basically the signatures should be the same as in List<T>.

I don't think the clearFirst parameter in AddRange is useful, and anyway optional parameters should be avoided in public APIs.

A RemoveAll method would be useful all well, for consistency with List<T>:

public int RemoveAll(Predicate<T> match)
thomaslevesque picture thomaslevesque  ·  16 Sep 2016
2

I think RemoveRange(IEnumerable<T> collection) should remain. It would cycle through collection, call IndexOf(item) and then call RemoveAt(index). Duplicates of the same item would also be removed.

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

I'm not against a RemoveAll function.

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

Also, the index parameter usually comes first in existing APIs, so InsertRange, RemoveRange and ReplaceRange should be updated accordingly.

And I don't think ReplaceRange needs a count parameter; what should the method do if the count parameter doesn't much the number of items in the replacement collection?

Here's the API as I see it:

public void AddRange(IEnumerable<T> collection) { }
public void InsertRange(int index, IEnumerable<T> collection) { }
public void RemoveRange(int index, int count) { }
public void ReplaceRange(int index, IEnumerable<T> collection) { }
public int RemoveAll(Predicate<T> match)
thomaslevesque picture thomaslevesque  ·  16 Sep 2016
4

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

I'm not sold on it, but hey, it's your proposal, not mine :wink:. At the very least, I think it should a separate overload, rather than an optional parameter.

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
6

@thomaslevesque I have the clearFirst parameter in there specifically because it IS useful, as in I'm using it in production code right now. Consider in UWP apps when you are resetting a UI... if you call Clear() first, it will fire another CollectionChanged event, which is not always desirable.

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:

using (collection.DeferCollectionChangedNotifications())
{
    collection.Add(...);  // no event raised
    collection.Add(...); // no event raised
    // ...
} // event raised here for all changes
thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

@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.

ReplaceRange with a count would remove all items in the given range, and then insert the new items at that point. The counts not matching would be irrelevant.

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)
robertmclaws picture robertmclaws  ·  16 Sep 2016
7

@thomaslevesque Overload vs optional parameter makes no practical difference to the end user. It's just splitting hairs.

It's not. Optional parameter can cause very real issues when used in public APIs. Read this blog post by @haacked for details.

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

I'm actually liking @thomaslevesque's idea about using a batching class. It's a common pattern, well understood, and makes complex workflows easier.

SamuelEnglard picture SamuelEnglard  ·  16 Sep 2016
0

ReplaceRange with a count would remove all items in the given range, and then insert the new items at that point. The counts not matching would be irrelevant.

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.

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

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.

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 picture robertmclaws  ·  16 Sep 2016
0

@robertmclaws but since there is no way to currently do bulk operations there isn't a "new pattern"

SamuelEnglard picture SamuelEnglard  ·  16 Sep 2016
0

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.

Why does that matter? Is it a memory allocation issue?

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

that's an entirely new piece of functionality

I agree that it should probably be a separate proposal, but it does solve the initial problem you were having.

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

@robertmclaws but since there is no way to currently do bulk operations there isn't a "new pattern"

There _are_ existing patterns, in about a dozen custom collections across different NuGet packages and what-not that inherit from ObservableCollection to fill in these gaps. The point of this proposal was simply to bring that functionality back into the native class.

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

Why does that matter? Is it a memory allocation issue?

That's one of the issues, yes. I'd have to check to be sure, but I think the underlying array is trimmed down when you remove items, and reallocated with a larger size (if necessary) when you add items.

Even if that's not the case, items after the range you're replacing would have to be moved twice, which has no impact on allocations, but makes the CPU do more work that necessary.

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

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 not against it being an overload. But in reality a) this class hasn't changed in years, b) this method would not likely change once implemented, and c) having overload means you would have to have an AddRangeInternal that pushes the reentrancy check to the parent functions, so that you're not checking for it twice or duplicating functionality. I personally would rather have one function that is not likely to change, vs playing XXXRangeInternal gymnastics the code stays DRY... but that is just me.

Regarding ReplaceRange(start, end, collection), I would expect that could also be an overload that would allow people to replace a range of a different size if they so desire. If that is the behavior the developer wants, then the memory/CPU allocations can't really be avoided.

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

There are existing patterns, in about a dozen custom collections across different NuGet packages and what-not that inherit from ObservableCollection to fill in these gaps. The point of this proposal was simply to bring that functionality back into the native class.

Fair point. Thinking on it a bit I think what we should do is implement the bulk grouping like @thomaslevesque suggested and then implement the methods you suggested on top of that (possibly as extension methods)

This actually has me thinking about a different proposal I might make, hmm

SamuelEnglard picture SamuelEnglard  ·  16 Sep 2016
0

That's one of the issues, yes. I'd have to check to be sure, but I think the underlying array is trimmed down when you remove items, and reallocated with a larger size (if necessary) when you add items.

I just checked, and I was mistaken. The underlying collection is always a List<T>, which doesn't automatically trim down capacity when items are removed. So, your approach wouldn't cause additional allocations (unless you insert more items than you remove, of course).

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

@thomaslevesque OK, cool. The more I think about it, you can't really replace a range unless you have a source range to replace. That's either going to come with indexes, or a group of existing items. Otherwise, it's just an InsertRange call.

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

Regarding ReplaceRange(start, end, collection), I would expect that could also be an overload that would allow people to replace a range of a different size if they so desire.

Yes, our approaches to ReplaceRange are not functionally equivalent.

  • yours would allow replacing a range with a collection of different size, but would be slower
  • mine would only allow replacing a range with a collection of the same size, but would be faster

If that is the behavior the developer wants, then the memory/CPU allocations can't really be avoided.

Indeed. Not sure which approach should be preferred (maybe both could be done, but it might make the API more confusing).

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
1

Ok, I think I came up with a way to meet everyone's requests. Please give me a few minutes to amend my speclet.

UPDATE: I was thinking about a boolean flag that let you buffer events, but that would be a pretty significant change to existing functionality, and I don't know if that is a good idea in this proposal. I think we should get these core functions added first, and THEN see if it makes sense to create a way to let the collection buffer events until you ask it specifically to flush them.

robertmclaws picture robertmclaws  ·  16 Sep 2016
1

Alright, I've updated the speclet with the other functions, implementing clearFirst and ReplaceRange options as overloads. As I just mentioned, I think buffering events would need to be a different proposal, because it would affect how every existing method fires events, which is not a bad idea, but is outside the scope of this proposal.

robertmclaws picture robertmclaws  ·  16 Sep 2016
0
public void ReplaceRange(IEnumerable<T> source, IEnumerable<T> replacement) { } //Might not be plausible, but should be attempted.

I'm not sure about this one. It would have to do a linear search for each item in source, which would result in O(m*n) complexity.

The more I think about it, the less I like the idea of ReplaceRange. We already realized that we had very different ideas of how it should work, and anyway, I don't think replacing items by batch like this is a common scenario (at least I can't think of a single time where I needed that).

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
0

OK, I removed that signature from the speclet. Everyone happy with what's left?

robertmclaws picture robertmclaws  ·  16 Sep 2016
0

OK, I removed that signature from the speclet. Everyone happy with what's left?

LGTM :+1:

thomaslevesque picture thomaslevesque  ·  16 Sep 2016
1

@robertmclaws What's the purpose of ReplaceRange(int, int, ICollection), we don't have a ReplaceRange on any of the other collection types. Is there a use case for this? If not, then we can defer on this.

Priya91 picture Priya91  ·  16 Sep 2016
0

I only added it because @thomaslevesque said he had one in his implementation when he first commented, so I wanted to be inclusive. I can't think of an active use case, so we can leave it out. I updated the speclet.

robertmclaws picture robertmclaws  ·  16 Sep 2016
2

We're fine with this APIs shape:

``` C#
// Inserts a range at the end.
public void AddRange(IEnumerable collection);

// Inserts a range
public void InsertRange(int index, IEnumerable collection);

// Removes a range.
public void RemoveRange(int index, int count);

// Will allow to repalce a range with fewer, equal, or more items.
public void ReplaceRange(int index, int count, IEnumerable collection);

// Raises event with Reset action
public int RemoveAll(Predicate match);
```

The add with the boolean seems quite weird and like a deviation from the rest of the BCL. If this is needed, you should use collection.ReplaceRange(0, collection.Count, items).

There are still some open issues the design needs to answer as well:

  • Should the APIs live on ObservableCollection<T> or on the base, Collection<T>?
  • If they live on the base, should they be virtual or should we follow the existing pattern where we have non-virtual public methods and protected virtual members.
  • It seems some of the APIs in the system assume the ranges contain a single item. That's clearly a design mistake, but it may indicate that we might have trouble forwarding the events as true ranges across all the handlers.
  • We should also explicitly list what the event will look like for each of the methods.
terrajobst picture terrajobst  ·  20 Sep 2016
2
  • RE: Add + Boolean - Good point. I'm fine with that.
  • RE: Where they live - I think the cleanest implementation would be to have the actual items manipulation code live in Collection<T> as virtual methods with an implementation. That way calls like ObservableCollection<T>.AddRange() could just call base.AddRange(), and then raise the right events afterwards.
  • RE: Design mistake - It seems like there were a number of design decisions when implementing this system that were a mistake/oversight. I can easily imagine a situation where the events initially raised a single item, then someone thought they should be a collection, but then patched the internal implementations to maintain behavior. I think it may be the best bet to leave those implementations alone for now, and then open work items for the 2.0 milestone when we can introduce breaking changes.
  • RE: Events raised - I can update the speclet with the results from the API review, and include the events raised from each. I can also change the title and make it clear that the changes will also be on Collection<T> as well.
robertmclaws picture robertmclaws  ·  20 Sep 2016
0

@robertmclaws could you make the updated speclet a new post? Makes following the chain easier and less confusing if you've updated it or not yet

SamuelEnglard picture SamuelEnglard  ·  20 Sep 2016
1

@SamuelEnglard I've had to update it about 7 times now... if I did a new post every time, I think it would make the thread worse. I added a Title header with the update date to make things clearer, and make it stand out more.

robertmclaws picture robertmclaws  ·  20 Sep 2016
0

@robertmclaws that works, thanks!

SamuelEnglard picture SamuelEnglard  ·  20 Sep 2016
0

@robertmclaws The updated speclet looks good, 2 suggestions:

  1. Please remove implementation of AddRange, have the API listed like other APIs.
  2. The event fired for RemoveAll should be Reset event, and not Remove event - RemoveAll removes items based on a predicate, meaning, the items removed may not be consecutive. The EventArgs for Remove sets the OldItems and OldStartingIndex for tracking the consecutive items. This doesn't apply to RemoveAll.
Priya91 picture Priya91  ·  23 Sep 2016
0

1) Will do tomorrow AM.
2) So, I'm not sure what the event is doing there is the correct behavior, and I don't believe that Reset is the expected event to be raised. I think "Reset" for most developers would mean the collection is back to the same state it was when it was first instantiated, which would almost always be zero. In this case, the full NotifyCollectionChangedEventArgs constructor can set the OldStartingIndex to -1.

If I saw the Removed event fired, and a starting index set, I personally wouldn't think that meant that the removed items were consecutive. But that's just me. Seems like something that could be solved through documentation.

robertmclaws picture robertmclaws  ·  23 Sep 2016
1

The event fired for RemoveAll should be Reset event, and not Remove event - RemoveAll removes items based on a predicate, meaning, the items removed may not be consecutive. The EventArgs for Remove sets the OldItems and OldStartingIndex for tracking the consecutive items. This doesn't apply to RemoveAll.

While I'll understand the reasoning, I don't think Reset is the proper event in this case. The documentation for Reset says:

The content of the collection was cleared.

Which IMO should only happen when Clear is called. RemoveAll could raise multiple Remove events instead (for each range of consecutive removed items).

thomaslevesque picture thomaslevesque  ·  23 Sep 2016
0

Which IMO should only happen when Clear is called. RemoveAll could raise multiple Remove events instead (for each range of consecutive removed items).

The only thing I don't like about this is that it could conceivably raise as many events as removing them individually. The point of these new functions is to minimize the number of events raised to maximize efficiency.

I'm having a really hard time thinking of a use case where knowing the index of the removed items is necessary. If it truly is necessary, then it might be better to return a list containing a new type of object that contains the item removed, and the index that particular item was at.

robertmclaws picture robertmclaws  ·  23 Sep 2016
0

Which IMO should only happen when Clear is called. RemoveAll could raise multiple Remove events instead (for each range of consecutive removed items).

If this is the intended behavior for firing remove event, then the outcome is no different than calling Remove single item on a loop based on a predicate match from user code. I don't think this should be the behavior of RemoveAll, I still propose Reset event, to differentiate this removal of non-consecutive items Or @robertmclaws idea also sounds good on returning a negative index to differentiate the non-consecutive remove, can discuss with the review board.

Priya91 picture Priya91  ·  23 Sep 2016
0

The only thing I don't like about this is that it could conceivably raise as many events as removing them individually.

True, but there's no way around it... The INotifyCollectionChanged API only supports changes to consecutive items. It might be more efficient in some cases to just raise a Reset event, depending on the number of items removed. In light of this dilemma, maybe including RemoveAll in the API isn't such a good idea after all.

I'm having a really hard time thinking of a use case where knowing the index of the removed items is necessary.

It makes removing the corresponding UI items (in a ListBox, ListView etc) more efficient, since there's no need to scan the whole list to find the right ones.

thomaslevesque picture thomaslevesque  ·  23 Sep 2016
0

It makes removing the corresponding UI items (in a ListBox, ListView etc) more efficient, since there's no need to scan the whole list to find the right ones.

That's why Reset event, to say, the collection was dramatically changed, re-parse the collection in the handler.

Priya91 picture Priya91  ·  23 Sep 2016
0

It makes removing the corresponding UI items (in a ListBox, ListView etc) more efficient, since there's no need to scan the whole list to find the right ones.

If you're binding those items in XAML directly to an ObservableCollection (which really is the point to this change) then that should not be necessary. If this benefits some legacy app framework, so be it. But the change is really to support UWP and modern app development.

The INotifyCollectionChanged API only supports changes to consecutive items.

How so, by intent or by practice? Because of the way NotifyCollectionChangeEventArgs works today? If so, the reality of that is that it's also only designed to work with individual items, so the really isn't anything stopping us from making this new feature work in the simplest and most consistently logical way possible. Even if that means adding new properties/return types on NotifyCollectionChangeEventArgs.

But I honestly don't see how we could intelligently have that discussion until we actually have an implementation that we can look at and see from there what makes the most sense.

robertmclaws picture robertmclaws  ·  23 Sep 2016
0

If you're binding those items in XAML directly to an ObservableCollection (which really is the point to this change) then that should not be necessary.

Sorry, I didn't express myself clearly. I didn't mean that you'd have to do it yourself, but that the UI control which is bound to the collection would have to do it.

How so, by intent or by practice? Because of the way NotifyCollectionChangeEventArgs works today? If so, the reality of that is that it's also only designed to work with individual items

No, it can already work with ranges; that's why the NewItems and OldItems properties are collections

thomaslevesque picture thomaslevesque  ·  23 Sep 2016
0

Sorry, I didn't express myself clearly. I didn't mean that you'd have to do it yourself, but that the UI control which is bound to the collection would have to do it.

We might need to dig into some of the UWP binding code and see how it handles these things. If that is the case, then "Reset" may indeed be the event that needs to be thrown. I can buit that into my custom implementation that I'm already using and see what happens.

No, it can already work with ranges; that's why the NewItems and OldItems properties are collections

Yes, but based on comments from earlier, I don't believe that is from work that was started but never finished. It certainly wasn't thought out properly. So if we need to change behavior / add functionality to NotifyCollectionChangedEventArgs to make it work in ways that will easily meet developer expectations, then we should be open to discovering what that means.

robertmclaws picture robertmclaws  ·  23 Sep 2016
0

Hope everyone's had a great week so far. So where are we at with this?

robertmclaws picture robertmclaws  ·  6 Oct 2016
0

@robertmclaws

But I honestly don't see how we could intelligently have that discussion until we actually have an implementation that we can look at and see from there what makes the most sense.

Agreed. How about this:

  • Let's excludeRemoveAll for now as this API has the most complicated eventing issues.
  • We can then approve this suggestion and you're unblocked with adding a PR.
  • We take this learning to decide what we do with RemoveAll
  • My concern is still that we can't plumb the batching fully through

How does this sound?

terrajobst picture terrajobst  ·  11 Oct 2016
0

@terrajobst Works for me, let's do it! :)

robertmclaws picture robertmclaws  ·  11 Oct 2016
0

PS, I updated the speclet at the top, and deleted the speclet in the middle. I'm ready to re-open and update my PR.

robertmclaws picture robertmclaws  ·  13 Oct 2016
0

@robertmclaws

Sounds good. It would be good in the PR to call out how the following elements will be addressed/can be addressed:

  • Will/can the APIs live on ObservableCollection<T> or on the base, Collection<T>?

    • If they live on the base, should they be virtual or should we follow the existing pattern where we have non-virtual public methods and protected virtual members.

  • It seems some of the APIs in the system assume the ranges contain a single item. That's clearly a design mistake, but it may indicate that we might have trouble forwarding the events as true ranges across all the handlers. Hopefully, test coverage will show that, but I doubt that as many of the consumers live in parts of the stack that isn't part of .NET Core (e.g. WPF)
terrajobst picture terrajobst  ·  1 Nov 2016
0

@robertmclaws just curious if you're still working on it, or we should make it available for grabs ...

karelz picture karelz  ·  17 Nov 2016
0

@robertmclaws ping?

karelz picture karelz  ·  23 Nov 2016
1

Sorry, I'm working on it. Will try to have a PR over the holiday break.

robertmclaws picture robertmclaws  ·  23 Nov 2016
0

Sure, no pressure, just want to make sure we are somewhat up to date on who is working on what.

karelz picture karelz  ·  23 Nov 2016
0

Actually one more thing to think about--> what if we let's say want to add and remove stuff before firing the event?
Lets say as mentioned before an --> i want to remove some Items & add some Items, or even insert on different indexes, move things around all at once.
My guess is that for this specific it might be better to implement another collection with INotifyCollectionChanged & INotifyPropertyChanged and add some kind of disposable transaction.

i.e.:
```c#
var collection = new OmnipotentCollection({"1", "2", "3","4", "5","6","7","8","9","10"});
using(var transaction = collection.MultiUpdate()) {
transaction.Insert(3, "A");
transaction.Insert(5, "B");
transaction.Remove("3");
transaction.AddRange({"C", "D"});
}

or something like this 
```c#
collction.Update( x=>
{
    x.Insert(3, "A");
    x.Insert(5, "B");
    x.Remove("3");
    x.AddRange({"C", "D"});
}

I guess something like this would be overkill ^^ - Still maybe someone has a simple idea to do something like that (without over-complicating things like I do)

groege picture groege  ·  28 Nov 2016
0

I use this pattern with a custom-implemented list type:

```c#
using (list.SuppressListChangedEvents())
{
// mutate list
} // On dispose, fires Reset if events are incompatible


In fact I've found this to be so useful that lists that do it implement a common interface. This allows extensions methods like the following:

```c#
public static void Reset<T>(this ICollection<T> collection, IEnumerable<T> replacementItems)
{
    using ((collection as IControlListChangedEvents)?.SuppressListChangedEvents())
    {
        var enumeratedBeforeClear = replacementItems.ToArray();
        collection.Clear();
        collection.AddRange(enumeratedBeforeClear);
    }
}
jnm2 picture jnm2  ·  28 Nov 2016
0

@jnm2 I wouldn't mind seeing how you've implemented that pattern. It might be an interesting addition to the spec.

robertmclaws picture robertmclaws  ·  28 Nov 2016
0

We should probably discuss this pattern in a separate issue. If it is common enough and the need is high, we could consider it.

karelz picture karelz  ·  28 Nov 2016
0

@karelz, I don't know that the pattern @jnm2 is suggesting is actually a separate issue. It's fairly relevant to the implementation here, as that could be the method of bypassing the events internally as well. Since I'm working on the PR right now, I'd like to see his implementation now, to see if it would make sense in the broader context of the work I'm doing.

robertmclaws picture robertmclaws  ·  28 Nov 2016
0

@robertmclaws wouldn't it change the API shape? -- That would require a new API approval.

karelz picture karelz  ·  28 Nov 2016
0

If it were made public, yes. However, I could implement the pattern as an internal, and then we can talk about making it external. IF it's worth it. Won't know that until I see it.

robertmclaws picture robertmclaws  ·  28 Nov 2016
0

Got it, that makes sense.

karelz picture karelz  ·  28 Nov 2016
0

Would it make sense to adapt the changed event to represent all changes without the reset event?
So we can add/move/remove items without having to look at the whole collection - and if yes how could this be done in a proper and easy to understand manner.

groege picture groege  ·  29 Nov 2016
0

@robertmclaws Mine is IBindingList-oriented but this concept could be even more powerful with INotifyCollectionChanged.

Here is how I would design it: https://gist.github.com/jnm2/d950053fd3825818371b87730f208839#file-eventbufferingcollection-cs

jnm2 picture jnm2  ·  30 Nov 2016
0

Working on my implementation now. As an FYI to @terrajobst @karelz @Priya91 etc, since Collection lives in CoreClr and not CoreFx, this is going to have to be a two-part PR (as mentioned in the referenced issue above this comment).

robertmclaws picture robertmclaws  ·  3 Dec 2016
1

My initial work is here. Feel free to comment before I open a PR, Or you can wait till after.

robertmclaws picture robertmclaws  ·  4 Dec 2016
0

Here's my implementation, hopefully it gives some ideas. It's optimized for re-usage of items, and raising lesser Reset events.

weitzhandler picture weitzhandler  ·  29 Aug 2017
0

Unassigning @robertmclaws and marking it up-for-grabs as it looks like no one is working on it.
Next step: Submit PR.

karelz picture karelz  ·  29 Aug 2017
0

@karelz, thanks.
Ok. I'm willing to implement this, and I'd like to include a few changes that I found important and performance impacting on the UI thread.
The real time consumer is resetting the collection or not re-using existing items is, which is why:

  • Avoid Reset events whenever possible
  • Safe-check before raising events or notify properties, and avoid doing so if unnecessary, for example calling Clear when collection already empty
  • We should also consider offering the OC act like a HashSet<T> reusing existing items of the provided comparer (maybe we should provide additional boolean property to determine re-usability of items?)
  • Because of the above, we might need to change some test cases

Please have a look at my code here, I'm going to reuse some of the code from there.

weitzhandler picture weitzhandler  ·  29 Aug 2017
0

@karelz, I had an implementation ready to go... I was waiting on comments.

robertmclaws picture robertmclaws  ·  29 Aug 2017
0

@robertmclaws it wasn't clear if it was just a prototype, or the real thing.
If it is the real thing, I would suggest to create a PR out of it. Assigning back to you ...

karelz picture karelz  ·  29 Aug 2017
0

@karelz Gotcha. If you look at the conversation here: https://github.com/robertmclaws/coreclr/commit/841672090c9245ab961bf25cb682833ef599c358 there was a pretty in-depth discussion about breakage, which was part of the reason this was not implemented yet, and why we didn't go down the route @weitzhandler suggested. We were iterating on how to handle subclasses in a non-breaking way, and then I think the holidays kicked in and it was forgotten about. Happy to look at it with fresh eyes and get it pushed through.

robertmclaws picture robertmclaws  ·  29 Aug 2017
0

Sounds good! I bet @weitzhandler will be happy to code review it as well.
PRs in general get more attention in code reviews, so it will be a good thing. If there are still open design/breaking-changes questions, I would suggest to raise them and address them before creating the PR.

karelz picture karelz  ·  29 Aug 2017
0

@karelz
I think in the ObservableCollection<T> range support will anyway require overriding the range methods if we choose to add them to Collection<T>, what I mean to say is that we can implement the ObservableRangeCollection<T> range methods separately. Though I agree we'd better off starting with overriding appropriate range methods rather than adding them later to Collection<T>. Shame I joined this party late. Sorry if interrupting.

@robertmclaws

and why we didn't go down the route @weitzhandler suggested

What do you mean?

@robertmclaws @karelz
Are you working on it? Otherwise I'd like to be assigned.


Anyway, here's an initial commit:
https://github.com/dotnet/corefx/compare/master...weitzhandler:observable-range-collection
I'm gonna create tests for the methods I've added over the next week.

weitzhandler picture weitzhandler  ·  31 Aug 2017
0

@weitzhandler if you think the API as approved above is insufficient, please make sure you provide enough clear evidence to others. If that's the case (and @robertmclaws agrees with your conclusions), we would have to take it first to a new round of API review.

karelz picture karelz  ·  31 Aug 2017
3

My plan for this weekend:

  • [ ] Review the following implementations:

    • [ ] Xamarin's ObservableRangeCollection

    • [ ] My existing PR to my personal .NET fork

    • [ ] @weitzhandler's implementation

  • [ ] Pull the best from all 3 in a way that meets the pre-approved API surface and performs well w/o unnecessary allocations.
  • [ ] Submit a PR that includes testing for compatibility scenarios.

My over-arching concern is that we implement this in a way that does not break subclasses that already try to implement this functionality.

Once we establish that this implementation meets the API surface already approved, and doesn't break compatibility at _this_ level, we can decide if the functions can be moved further down the stack without breaking anything else. But I don't think we should confuse the two right now, this whole process has been hard enough to follow ;).

Thoughts?

robertmclaws picture robertmclaws  ·  31 Aug 2017
0

In my implementation I added the following features:

  • Collection avoids raising notifications and events when unnecessary and avoids redundant actions, here's a couple of examples

    • Clearing collection when already empty

    • Added range is an empty collection

  • The following methods:

    • public void AddRange(IEnumerable<T> collection)

    • public void InsertRange(int index, IEnumerable<T> collection)

    • public void RemoveRange(IEnumerable<T> collection)

    • public void RemoveRange(int index, int count)

    • public void ReplaceRange(IEnumerable<T> collection) //clears and adds

  • Stack events and notifications to the minimum possible

Additionally I'd want to add a new property, something like ReuseItems, which when set to true, adding or inserting an item already in the collection (using a comparer) will result in no action.
And you must think 'hey, searching the collection has a performance cost', and you're right, but since adding and resetting slots in ObservableCollection is the major performance issue, because any insert or replace potentially causes template creation and UI change, so the optimization goal in ObservableCollection should be avoiding unnecessary collection changes, even if it comes with an slight cost from the background thread (e.g. ViewModel), especially since it was requested on demand setting that property. But that I'll leave for later.

@robertmclaws Since your last message I backed out to give you the honor of implementing it, also because you're the one who opened the issue and already worked on a PR before.
Are you still working on this? Otherwise I'd be happy to be assigned. @karelz

weitzhandler picture weitzhandler  ·  20 Sep 2017
0

Looks like @robertmclaws didn't have time to work on it. If you have implementation of the approved API surface, feel free to submit a PR @weitzhandler.

karelz picture karelz  ·  27 Nov 2017
0

@robertmclaws @weitzhandler @karelz Any progress on this?

OsirisTerje picture OsirisTerje  ·  10 Jan 2018
30

API Review 2nd round

In order of appearance in code

See implementation and explanations in PR dotnet/corefx#26482.

  • [x] Inserts a range at the end.
    public void AddRange(IEnumerable<T> collection);
  • [x] Inserts a range
    public void InsertRange(int index, IEnumerable<T> collection);
  • [x] Removes a range.
    public void RemoveRange(int index, int count);
  • [ ] Removes the first occurence of each item in the specified collection
    public void RemoveRange(IEnumerable<T> collection);
  • [x] Removes only matching items.
    public int RemoveAll(Predicate<T> match);
  • [ ] Removes only matching items, constraining the search to a specific range in the collection.
    public int RemoveAll(int index, int count, Predicate<T> match);
  • [ ] Clears the current collection and replaces it with the specified collection using default eq. comparer.
    public void ReplaceRange(IEnumerable<T> collection);
  • [ ] Clears the current collection and replaces it with the specified collection using specified comparer.
    public void ReplaceRange(IEnumerable<T> collection, IEqualityComparer<T> comparer);
  • [x] Repalces a range with fewer, equal, or more items, reusing equal items in similar indices.
    public void ReplaceRange(int index, int count, IEnumerable<T> collection);
  • [ ] Repalces a range with fewer, equal, or more items, using the specified comparer to skip equal items in equal indices.
    public void ReplaceRange(int index, int count, IEnumerable<T> collection, IEqualityComparer<T> comparer);

Legend:

  • [x] API approved
  • [ ] API proposed

Notes:

  • All methods above are implemented and tested, some not as extensively as they should.
    It's up to you guys to decide what remains here (will add more tests upon API verdict).
  • In addition to the proposed API, some optimizations were added to the existing methods, to reduce chance of raising unnecessary events, for example, adding a range with an empty collection will do nothing. Similarly clearing an empty collection will do nothing, and some more situations.
  • We would also want to decide whether to make some of the methods virtual, or even bring some of them (specifically does existing in List<T>), down to Collection<T> (The base of ObservableCollection<T>).
  • We should decide which of them tunnel down to base and which stand for themselves as virtual/sealed (see this comment).
  • Most of the overloaded methods tunnel down to one of the lower level methods, so code is reused rather then just pasted out. After we decide on what API sticks with us, we maybe want to clean down the code even further.
weitzhandler picture weitzhandler  ·  20 Jan 2018
1

Great, that's useful. Let's first wait on first wave of feedback and when we are in general ok, we can run these new APIs by API review.
Thanks for your PR!

karelz picture karelz  ·  20 Jan 2018
0

@thomaslevesque commented on Sep 16, 2016
public void ReplaceRange(IEnumerable<T> source, IEnumerable<T> replacement) { } //Might not be plausible, but should be attempted.

I'm not sure about this one. It would have to do a linear search for each item in source, which would result in O(m*n) complexity.

First of all, List<T> also exposes a similar method, that also iterates in an O(n) complexity.
Secondly, I think that in ObservableCollection<T> that its purpose was initially introduced for XAML based UIs (if I remember correctly), a more important aspect is saving UI performance, and that is made by raising the least Reset events possible, and as more range events, even with a small extra O(m) cost, that that is developer is aware of (as explained in the docs).

The more I think about it, the less I like the idea of ReplaceRange. We already realized that we had very different ideas of how it should work, and anyway, I don't think replacing items by batch like this is a common scenario (at least I can't think of a single time where I needed that).

I have to disagree with you. I have personally bumped in several scenarios where I needed this functionality. For example maintaining a suggestion drop down collection or a filtered list, or plenty other scenarios where this is essential.

weitzhandler picture weitzhandler  ·  21 Jan 2018
0

First of all, List also exposes a similar method,

List<T>.RemoveAll isn't similar at all to the proposed ReplaceRange method. It takes a predicate, not a list of items to remove, so it can iterate the list in O(n) to find items to remove. That being said, I was mistaken in saying that the complexity would be O(n*m); by turning source and replacement to a dictionary, it could be done in O(n).

Still, I found the proposed API a bit weird; maybe it should directly accept a dictionary, instead of two collections.

thomaslevesque picture thomaslevesque  ·  21 Jan 2018
0

@thomaslevesque commented Jan 21, 2018
List.RemoveAll isn't similar at all to the proposed ReplaceRange method.

Sorry, I was talking about the RemoveAll method, and once that is in, we would want to consider having a ReplaceRange that achieves a similar goal.

Still, I found the proposed API a bit weird; maybe it should directly accept a dictionary, instead of two collections.

Please read this, this is the current implementation of my recent PR (#26482), I implemented and tested them all, for you to decide which remains in, which one gets down to Collection<T> as a virtual method, and which should just become locally virtual.


@thomaslevesque commented on Sep 16, 2016
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:
c# using (collection.DeferCollectionChangedNotifications()) { collection.Add(...); // no event raised collection.Add(...); // no event raised // ... } // event raised here for all changes

Should I implement this in my PR?

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

Should I implement this in my PR?

I don't know, but I certainly make heavy use of that pattern.

jnm2 picture jnm2  ·  22 Jan 2018
0

@thomaslevesque @jnm2
OK it's been addressed: c06d271.

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

Should I implement this in my PR?

I think it's out of scope for this issue. It would certainly be useful, but it should probably be part of a separate proposal.

thomaslevesque picture thomaslevesque  ·  22 Jan 2018
0

@weitzhandler Thomas is probably right. Also, I generally prefer to send a single event no matter what. Here's a mockup I did of a way to leave the event-combining decision in the hands of the user of the collection: https://gist.github.com/jnm2/d950053fd3825818371b87730f208839#file-eventbufferingcollection-cs Not sure how much of that is interesting to most people, though.

jnm2 picture jnm2  ·  22 Jan 2018
0

@jnm2 commented on Jan 22, 2018
Thomas is probably right.

I understand it may be out of scope, but in the other hand, since the new functionality makes use of clusters (here and here), the need for this pattern might be important here. Let's let @karelz decide.

Also, I generally prefer to send a single event no matter what.

You're missing the point. The idea is to raise as less possible events as we can. So in case of a mixed action (for example replacing some and adding more), we want to split up the events to few groups and raise them separately, while notifying the property change (Count, Item[]) only once.

Here's a mockup I did of a way to leave the event-combining decision in the hands of the user.

  1. I'm not sure we would want to give the user this decision.
  2. I personally went for simplicity and minimalism, but let's let the fx team decide on that. Bear in mind that it's just been committed and it's still awaiting some API Review.
weitzhandler picture weitzhandler  ·  22 Jan 2018
-1

You're missing the point. The idea is to raise as less possible events as we can. So in case of a mixed action (for example replacing some and adding more), we want to split up the events to few groups

I think I'm just taking that further by sending either zero or one collection change event; that's even fewer. The intentional tradeoff being, reset events are more likely. Sending a property change events for Count is not something I've ever found useful.

jnm2 picture jnm2  ·  22 Jan 2018
0

I think I'm just taking that further by sending either zero or one collection change event

Imagine you have a collection containing alpha, bravo, charlie. You want to add a new collection bravo, delta, echo, foxtrot starting from 2nd position.
Instead of raising 3 different type of events, it will ignore the equal item bravo, raise a Replace event for charlie, and raise one grouped Add event for echo and foxtrot. And so on (see tests). That's why we do need to separate out the events. Let us not forget that ObservableCollection was made for UI data-binding purposes, and this pattern of grouping is merely to preserve unnecessary UI thread calls and updates, I've been using it in my projects and it vastly improved the UI performance over ObservableRangeCollection offered by the Xamarin team, especially when using very frequent modifications to the collection (such as a filtered view updated upon user key stroke).

Sending a property change events for Count is not something I've ever found useful.

Count may useful in many instances. One of them is you want to style your view based on the count in the collection, or want to generate a new page in the index of the Count property and some more. And there is also the indexer (Item[]), which can come handy too.

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

I get it, but coming from a LOB domain and UI perspective, I would almost always prefer a reset event in the scenario you give rather than multiple grouped events. (Or more accurately, a single replace event which replaces the entire minimally-scoped starting range with the entire minimally-scoped ending range.)

jnm2 picture jnm2  ·  22 Jan 2018
0

I get it, but coming from a LOB domain and UI perspective

That's my top field, I mostly do LoB.

I would almost always prefer a reset event

You wouldn't prefer a Reset. Not if you measured its performance cost. Resets are worst. They immediately clear the UI and build it all anew - not what we want, we want to reuse as many items as we can so the UI doesn't refresh things we don't need to.
Besides, partial Resets with index indication isn't supported, and also according to the documentation, Reset indicates the collection was cleared.

Or more accurately, a single replace event which replaces the entire minimally-scoped starting range with the entire minimally-scoped ending range.

That's what it does. What's out of the existing indices will be Added, not Replaced.

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

Same.

UI perf measurements have shown that ten nonconsecutive adds are worse for performance than a single reset. Of course, replace rather than reset when it's possible to represent that with the API.

jnm2 picture jnm2  ·  22 Jan 2018
0

UI perf measurements have shown that ten nonconsecutive adds are worse for performance than a single reset.

Not the results I've seen. Adds update the items one by one and the view doesn't flicker as it does with Resets. That what actually motivated me to divide the events up to clusters. Of course trying to group the events instead of raising them individually whenever possible if the items are consecutive.

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

The UI controls I was working with did not flicker either way, but the lag due to a reset was similar to the lag caused by a couple of Add events. Full invalidation isn't necessary when you see that the property descriptors are the same and almost the row instances are the same.

jnm2 picture jnm2  ·  22 Jan 2018
0

I was recently working on a Xamarin project and tried various scenarios.
I used the ReplaceRange to update a drop down, and I experienced flickering upon resets, unlike grouped events. WPF is most likely much faster anyway.
Besides, in my implementation, there shouldn't be several consecutive Add events they will all be combined to a single batched Add (for example).

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

That kind of platform sensitivity is what makes me think the batching of events should be at least somewhat configurable. Or then again, maybe I should just continue to keep away from ObservableCollection. 😜

jnm2 picture jnm2  ·  22 Jan 2018
1

The individual events in my implementation will only occur in non-consecutive replacements - not additions, which is good because the UI will only refresh the required indices.

weitzhandler picture weitzhandler  ·  22 Jan 2018
0

@karelz
Can we push this issue into 2.1, this has been a long issue that is rather compelling in almost any UI and MVVM scenario, and improves performance drastically.

The API I proposed is no much other from the API that was already approved. It just added more flexibility and a few more essential options.

weitzhandler picture weitzhandler  ·  15 Mar 2018
2

I don't think it is a good idea to rush this API. Moreover, technically speaking we are past API freeze. Given that the change and the new API shape didn't get enough scrutiny from community / experts tells me it is not as simple as it looks.
If I remember correctly, even the original approval was a bit open-ended with "_we need more implementation validation, before we are fully ok with the API_". Once we ship the API, we will live with it forever. Therefore, we should be fairly sure it is right. I don't have the confidence from the discussion above that we are there yet.

karelz picture karelz  ·  16 Mar 2018
0

@karelz I understand. Thanks for your response tho. I hope we're gonna get there some time soon.
But I'd like to request from you to link the 2nd round API from the original post.

weitzhandler picture weitzhandler  ·  17 Mar 2018
4

The issue is marked api-ready-for-review. It is on our list. We have just been prioritizing review of APIs critical for 2.1. I hope we will be able to get to the Future backlog in 2-3 weeks.

karelz picture karelz  ·  18 Mar 2018
0

@karelz what's the status on this issue?
@weitzhandler just a heads up, I was trying to use your implementation with a grouped CollectionViewSource and it seems that one doesn't pick up on range changes ( with a new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, list, index) it only removes one element at index)

hutterm picture hutterm  ·  4 Jul 2018
1

Well I was developing against the existing tests. I shall try adding tests that verify that. I don't know if we will be able to add tests that make use of CollectionViewSource but I'll try tracing down the error.

weitzhandler picture weitzhandler  ·  4 Jul 2018
0

The proposal sounds good, but we need to have a design that answers these questions:

  • Should the methods be virtual? If no, why not? If yes, how does eventing work and how do derived types work?
  • Should some of these methods be pushed down to Collection<T>?

We are concerned that derived classes that already override the existing virtual methods will end up getting not being notified for removals and/or they are getting the even raised for single items and the bulk operation.

terrajobst picture terrajobst  ·  10 Jul 2018
0

@terrajobst
That's a valid concern indeed, I have been messing with it in the past, making the range-methods non-virtual, see this test, for example.
But if we are to make them virtual, then it should be degraded into Collection<T>, it doesn't really make sense otherwise IMHO.
@redoced It seems to me that CollectionViewSource isn't part of the .NET Core, can you please link the CollectionViewSource you're talking about and let us know how you plugged in the new ObservableCollection<T> with the range functionality?

@karelz all of these concerns were also valid when the suggestion was approved back ago without my small additions. Can we advance this issue? Very compelling for any XAML dev.

weitzhandler picture weitzhandler  ·  17 Sep 2018
2

@weitzhandler I am not sure what you're asking for. If there are concerns about API shape or behavior, then they need to be addressed prior to finalizing & approving the API. If we overlooked some concerns earlier, it does not mean there is a free pass to ignore them now.
It could (and did) happen that approved API gets rejected in PR, because new API concerns arise which were missed during previous API approval review. We are not slaves of process. The process is here to help us come up with solid APIs which developers will use for decades. We want to deliver high-quality APIs and ideally learn from our past experience to make APIs even better now.
And yes, API design is HARD and sometimes TRICKY.

karelz picture karelz  ·  17 Sep 2018
2

cc @danmosemsft @safern @ianhays to see if it would fit our 3.0 plans (with UI stacks coming to .NET Core) - the API had a share of back-and-forth and it could use some love from our team ...

karelz picture karelz  ·  17 Sep 2018
0

Hey @karelz and thanks for your kind and detailed response.
I was just hoping for this issue to get some more attention, I find it really compelling.

weitzhandler picture weitzhandler  ·  17 Sep 2018
11

I've assigned the issue to myself in order to take a deep look into it and decide whether it fits in the framework or not for our 3.0 plans.

safern picture safern  ·  17 Sep 2018
0

@ianhays and I discussed this and we agree to add this 4 APIs for now:

// Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection)

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int 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);

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:

Should the methods be virtual? If no, why not? If yes, how does eventing work and how do derived types work?

Yes, we would like this methods to be virtual to follow what we're currently doing in ObservableCollection.

Should some of these methods be pushed down to Collection?

Yes, and then ObservableCollection could just call the base implementation and then trigger the necessary events.

Marking as ready for review.

safern picture safern  ·  4 Oct 2018
0

@safern

TBH I find it disappointing that the predicate isn't in the API, because there won't be a way to refine the added collection, hence reducing the number of events occurring when modifying the collection instead of combining the entire action to one.
If the approved methods will be virtual, and will be open ended to allow adding the predicate-enabled methods later, that will be bearable.
Just bear in mind that predicate methods exist in List<T>.

Have you read my comment and my PR?

weitzhandler picture weitzhandler  ·  5 Oct 2018
0

From reading the threads extensively I figured that the best thing to do regarding making the methods virtual or not, is that we shouldn't make this virtual methods but follow the Collection pattern where we have underlying protected virtual methods as that allows people to add filtering and custom behavior when inserting/removing items. So then the API would look something like this:

    // 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);
    }

    #region virtual methods
    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
    protected virtual void RemoveItemsRange(int index, int count);
    #endregion

Just bear in mind that predicate methods exist in List.
Have you read my comment and my PR?

I understand your disappointment, but as reviewers have stated previously in this thread, this are complicated APIs and it is always complicated to satisfy all customers out there with exactly what they need, we're trying to handle and expose quality and maintainable APIs, because once we add those APIs, we have to stick with them for the rest of time. So we have to be careful on what we add and how we add them. I would like to keep this proposal as simple as it can be and then for the predicate APIs we can open another issue, focused on those APIs and have the right discussion on what are the problems and how to address them. That minimizes risk and PR would be smaller, easier to review and we would get things right.

I'm not holding back on them, I know they exist in List<T>, but I would like to keep this proposal and addition as simple and useful as possible at the same time. I think that with this first APIs that we're going to bring to the review and most likely would be approved, the most common use cases will be covered.

So from looking at the thread, the complications where brought by the Predicate APIs, so if we want to minimize discussion in this super long/old issue and get APIs approved and going, let's keep this simple and start another discussion focused on the predicate APIs once this issue is approved and APIs are added, how does that sound?

safern picture safern  ·  5 Oct 2018
0

@safern

From reading the threads extensively...
I understand your disappointment

Only wanted to make sure you went through it. Thank you very much for your time. I do understand.

// 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);
}

Kinda missing the point. The goal is raise as less events as possible and avoid unnecessary events. Please take a look at this. This will avoid all unnecessary events when replacing the collection. Disregard the predicate.

weitzhandler picture weitzhandler  ·  6 Oct 2018
8

Looks good, but we should add a ReplaceItemsRange:

public partial class Collection<T>
{
    public void AddRange(IEnumerable<T> collection);

    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);

    // These are override by ObservableCollection<T> to raise the event:

    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);

    protected virtual void RemoveItemsRange(int index, int count);

    protected virtual void ReplaceItemsRange(int index, int count, IEnumerable<T> collection);
}
terrajobst picture terrajobst  ·  9 Oct 2018
0

I'd be happy to be assigned too.

weitzhandler picture weitzhandler  ·  9 Oct 2018
0

All yours 😄

safern picture safern  ·  9 Oct 2018
0

Tx. I'd need a bit of help tho.

Under what solution is Collection.cs?
Looks like it's part of Lib, I'm not sure where to get started.

Please have a look here.

weitzhandler picture weitzhandler  ·  9 Oct 2018
0

You need to make the change first in coreclr for the implementation: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

The reference assembly that exposes Collection is System.Runtime: https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L3944-L3977

safern picture safern  ·  9 Oct 2018
1

Should I first open a separate PR in the coreclr repo for the changes in Collection?

Yes, you can open a simultaneous PR in corefx to expose the APIs in the reference assemblies, add tests and also add the implementation for ObservableCollection. However the CI builds will fail until the coreCLR PR is merged and corefx consumes a new version of the coreclr package.

After adding the APIs in Collection implementation in order to test your changes in CoreFX, once you added the APIs to the reference assembly for Collection you will want to follow this steps to build CoreFX wit a custom version of coreclr: https://github.com/dotnet/corefx/blob/d9193a1bd70eee4320f8dcdd4e237ee9acf689e9/Documentation/building/advanced-inner-loop-testing.md#compile-corefx-with-self-compiled-coreclr-binaries

Please let me know here or in gitter if you need help from my side 😄

safern picture safern  ·  9 Oct 2018
0

Where are the tests for Collection<T> located?

weitzhandler picture weitzhandler  ·  10 Oct 2018
0

Finally some justice for the observable collections :) Wonder though how it would work in real-life existing derived classes with AddRange

alexsorokoletov picture alexsorokoletov  ·  20 Oct 2018
0

FWIW, two other methods I'm missing in IList/ICollection are GetRange and Reverse. The bare-bones interfaces provided mean I have to either give up on returning an interface in my properties or give up the nice extra methods...

adrientetar picture adrientetar  ·  9 Nov 2018
0

@safern Is there a proper guide on adding tests for System.Private.CoreLib (they're added in corefx), so that it runs against the clr on my machine?
I'm reading this but I'm not certain this is the right one, can you please confirm or elaborate?

weitzhandler picture weitzhandler  ·  23 Jan 2019
1

I think following this will be enough: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits

safern picture safern  ·  23 Jan 2019
0

Unless I'm thick and missing something super obvious, this appears to be landing in NetCore as opposed to NetStandard. Does it need to be implemented in both, will this cover NetStandard?

ChaseFlorell picture ChaseFlorell  ·  27 Feb 2019
1

@chaseflorell this repo is for.NET Core as you mention. The dotnet/standard repo is the place where the standard is defined and requests should go in In issues there

danmosemsft picture danmosemsft  ·  27 Feb 2019
0

Because the Collection is in Private Lib in CLR, I find it really hard and time consuming to set up the tests for my changes in the CLR. Gave it a few shots but didn't have much of success, need to find a free afternoon to get it done 💔

weitzhandler picture weitzhandler  ·  27 Feb 2019
0

Gave it a few shots but didn't have much of success, need to find a free afternoon to get it done 💔

Please let me know or ping me on gitter if you get stuck at some point.

safern picture safern  ·  27 Feb 2019
0

Before I realized that the mono corefx implementation is forked from here I got AddRange and InsertRange working locally. Today I spent some time porting it to the main repository to see if I could get it to work with the xUnit tests.

@weitzhandler I would be happy to combine our efforts so we can get this wrapped up as I am able to get the CoreCLR build working with my xUnit tests.

My Current Implementation:
Collection (only):

  • AddRange & xUnit Tests
  • InsertRange & xUnit Tests

Update

I ended up taking this all the way to the point where I can submit a PR to complete this. Looking forward to everyone's feedback

ahoefling picture ahoefling  ·  4 Mar 2019
0

Do we have an open ticket for adding AddRange to the concurrent collections?

Grauenwolf picture Grauenwolf  ·  5 Mar 2019
4

@safern is this on track on being done soonish? If so, we could still add it to .NET Standard.

terrajobst picture terrajobst  ·  8 Mar 2019
2

Awesome. Yeah PRs are our in CoreCLR and CoreFX. Will try to get them in ASAP

safern picture safern  ·  8 Mar 2019
0

@safern @terrajobst What is the process to getting this into the Standard from here? I went ahead before I started dev on this and created an issue to track this in the mono project as well. mono/mono#13265.

I am happy to continue working on anything additional that needs to be done on the other projects to help get this in. This is really going to be a great addition to .NET Standard

ahoefling picture ahoefling  ·  8 Mar 2019
3

WPF developer from a large exchange-listed company here.

It's incredible how much of a difference a proper .AddRange() function will make to the speed of ObservableCollection. Adding items one-by-one with a CollectionChanged event after every add is incredibly slow. Adding lots of items with one CollectionChanged at the end is orders of magnitude faster.

tl;dr AddRange() makes the difference between a huge grid with 10,000 items being fast, fluent and responsive vs. slow, clunky and unresponsive (to the point of being unusable).

So this PR gets my +1 vote. Of all the improvements to grid speed, this is by far the most important.

sharpe5 picture sharpe5  ·  29 Mar 2019
0

FYI: This API caused a significant regression for WPF apps :( - see dotnet/corefx#38085
... just to demonstrate that even things that should be "OK" and simple, are not always that simple when you're at the bottom of the stack ... sigh
cc @onovotny

karelz picture karelz  ·  31 May 2019
0

We had to revert the original PR in dotnet/corefx#38115 for 3.0 Preview 6 (and also the .NET Standard 2.1 PR https://github.com/dotnet/standard/pull/1222)
Let's keep this issue open to figure out what should happen in Preview 7 (or later in .NET 5).

karelz picture karelz  ·  31 May 2019
5

Next steps:

  1. Figure out what we have to do in WPF to accommodate this API. Do we need to make WPF code changes? Large or small changes?
  2. Is there a pattern we should look for in other projects? How many other projects will be impacted?

Note: Preview 7 is RC (go-live) and we won't be able to add new APIs after that. Even if this change is too close to shipping Preview 7, we may have to postpone it to next major version (.NET 5) ... while it would make me sad, we can't simply break WPF developers and make their transition to .NET Core harder because of this.
Let's try to figure out how to deliver on both - great WPF experience and this new API in.

karelz picture karelz  ·  31 May 2019
21

You know, I was thinking about this a lot again this afternoon, and reading another thread on Twitter about API breakage (between @davidfowl and @terrajobst). I really think we're thinking about this the wrong way, and here's why:

The *Range changes are going to be a HUGE breaking change across the entire UI ecosystem. This stems from the fact that ObservableCollection has _always_ been incorrectly designed. NotifyCollectionChangedEventArgs has always had the capability of containing multiple items in the notification, but from a practical standpoint ObservableCollection never actually had the capability of throwing an event with more than one item in the EventArgs, because you could only ever change one item at a time.

There is going to be a metric crap-ton of customer code out there depending on that behavior. I cannot stress this fact enough, and I pointed it out in my very first post under "Obstacles".

This is why there are so many different variations of subclassing out there that inherited from ObservableCollection and rolled their own Range implementation... because the design was bad in the first place. Xamarin had to ship their own ObservableRangeCollection.cs in every VS project template they shipped, for as long as I can remember.

We can fix this across the core frameworks all we want. But customer code is STILL going to have to change, because they will no longer be able to depend on the old behavior. Waiting until .NET 5 isn't going to make that any better... in fact it's actually going to make the problem a LOT worse, because you'll have WPF code actually shipping on .NET Core, and right now you don't.

Besides, we seem to be forgetting that WPF apps are not going to "just run" on .NET Core without recompiling and redeploying. Recompiling the same code without changes on a whole new framework is a nice idea, but should not be prioritized over making bad designs live forever on a brand new platform... otherwise why the hell did Microsoft put the industry through building a new framework in the first place?

The WHOLE point of .NET Core was to be able to start fresh and fix all the bad design decisions of the past, because you can have multiple versions of the framework installed on the same system.

I also feel it important to point out that if I had implemented this API three years ago when I proposed it (instead of spending all my available free time for it on the spec discussion and then getting burnt out), you'd still have this problem now anyways, only you wouldn't be able to back out.

Thinking about the big picture, and what .NET will look like 5 years from now, I really feel like the best way forward is:

  • to warn everyone what is going to happen
  • give the ecosystem a pattern to implement the fix
  • put on our big-kid panties on and suck it up, confident that we did the right thing for the ecosystem long-term

Just because it's going to suck a little doesn't mean it's not worth doing. Otherwise we're screwing the future for years just to save people 10 minutes of fixes on something they're going to have to recompile _anyways_.

robertmclaws picture robertmclaws  ·  1 Jun 2019
0

I'd say we should make NotifyCollectionChangedEventArgs generic as well. Currently non-generic lists are used which is not only inconvenient, but also risks run-time InvalidCastExceptions.

Happypig375 picture Happypig375  ·  2 Jun 2019
0

I wish that would have been part of the spec @Happypig375 because that is a fantastic idea. Generic Lists still implement IList, so I believe that wouldn't be as big of an issue as I initially thought when I considered it. But that would be a lot of extra testing that I'm not sure we have time for... unless this all gets punted to .NET 5, then we have all the time in the world ;).

robertmclaws picture robertmclaws  ·  2 Jun 2019
-1

Generic Lists still implement IList

Not necessarily, since IList<> does not inherit IList.

jnm2 picture jnm2  ·  2 Jun 2019
0

@jnm2 @Happypig375 Curses, foiled again! I thought it implemented IList as the most basic interface, but it implements IEnumerable instead. :(

robertmclaws picture robertmclaws  ·  2 Jun 2019
9

@robertmclaws thanks for you thoughts, unfortunately I have to respectfully disagree with some of your points -- the landscape is a bit more complicated than what you highlighted above. Here are my points:

The WHOLE point of .NET Core was to be able to start fresh and fix all the bad design decisions of the past, because you can have multiple versions of the framework installed on the same system.

Yes, that was the original plan in .NET Core 1.0. And then the reality hit us and had to put back TONS of old APIs in .NET Core 2.0, reverted our Reflection "start-fresh" work, etc. All that mainly because that was the key blocking problem for migration to .NET Core from .NET Framework -- too many different things. Without backing out on our "start-fresh full-blown" idea, .NET Core platform would not have the success it has today.
We are still able to break things and right some wrongs, but we are extremely picky and cautious about which things to do -- the benefit has to be clear, the impact understood, the migration path has to be paved.

Besides, we seem to be forgetting that WPF apps are not going to "just run" on .NET Core without recompiling and redeploying.

While that is true, history shows that people don't like surprises. They love when they recompile and things just work. Whenever they hit issues, especially those that are not super-clear from the error how to address, they lose faith in the platform. It creates anxiety. And it hurts platform adoption.
I am not saying we have to be 100% compatibility, but we have to be super-smart and picky about which things we want to break.
Moreover, WPF is targeting mainly enterprise customers. Enterprise customers are known to like surprises / differences even less than the rest of the developer community.

Recompiling the same code without changes on a whole new framework is a nice idea, but should not be prioritized over making bad designs live forever on a brand new platform... otherwise why the hell did Microsoft put the industry through building a new framework in the first place?

The fact is people expect things to just mostly work. The fact is that we cannot get rid of all the bad designs - we got rid of some, which are super-painful and held the platform innovation back (think AppDomains, remoting). And even those are painful and slow down migrations.
The key value of .NET Core is side-by-side and ability to break a bit over time. In corner-cases. Or in places where the impact is understood and we have clear data it won't prevent people from migrating from .NET Framework or older .NET Core versions, and it won't negatively affect libraries ecosystem.

Waiting until .NET 5 isn't going to make that any better... in fact it's actually going to make the problem a LOT worse, because you'll have WPF code actually shipping on .NET Core, and right now you don't.

The problem exists in the platform for ages. Since its inception. I don't see how waiting one more release is going to make a significant difference. It is not ideal, but far from end of the world IMO.
Yes, we will have more WPF code on .NET Core 3.0, but the key value of waiting is: We will FULLY understand the impact of this change on ecosystem and what has to happen. Right now we are 5 minutes from 12 and that is REALLY BAD time to make drastic changes which may slow down adoption.

I also feel it important to point out that if I had implemented this API three years ago when I proposed it (instead of spending all my available free time for it on the spec discussion and then getting burnt out), you'd still have this problem now anyways, only you wouldn't be able to back out.

First, I am sorry you got burned out on this. On the other hand, I am not surprised, because this is a COMPLEX problem. There are many opinions and the impact is HUGE. Finding the right balance between "right solution" and "compatible enough" is EXTREMELY important here and therefore it takes time and lots of effort. It is a significant challenge. Thank you for driving the effort! Please don't take the current state as invalidation of your effort, contributions and ideas. They are great. The platform and ecosystem needs just a bit more bake time on the plan to make it happen in least-disruptive way.
The key DIFFERENCE between having the changes in now (3.0 RC time frame) vs. in last release is: We would have had the whole .NET Core 3.0 release to discover the problem and address it. You are right we would not be able to back out the changes if they shipped in 2.1, but we would have more runway to address WPF scenarios, think of mitigations, migration path, documentation, Roslyn analyzers, more testing and validation by customers, etc. ... we do NOT have such runway now. We will have it again in .NET 5 though, if we make the change early on.

Thinking about the big picture, and what .NET will look like 5 years from now, I really feel like the best way forward is:
to warn everyone what is going to happen
give the ecosystem a pattern to implement the fix
put on our big-kid panties on and suck it up, confident that we did the right thing for the ecosystem long-term

I agree. These are good arguments to do it in .NET Core. I just disagree that these are arguments for pushing changes into .NET Core 3.0 vs. .NET 5.

Just because it's going to suck a little doesn't mean it's not worth doing. Otherwise we're screwing the future for years just to save people 10 minutes of fixes on something they're going to have to recompile anyways.

Your quote of "10 minutes of fixes" hints that you understand all the implications of this change on WPF code base and other scenarios more than we do (incl. our WPF experts).
That is what I highlighted as next step above: https://github.com/dotnet/corefx/issues/10752#issuecomment-497870471 (see point [1]).

Most of my comments above are about the fact that currently we DO NOT UNDERSTAND all impact on WPF scenarios and given where we are in the release, risk-management kicks in, which leads to ideas like pushing the change out to .NET 5.

If you or anyone can help us with full analysis and understanding of the changes necessary for WPF scenarios (and other UI scenarios), then this may have a chance to get into .NET Core 3.0 still (no promises as the decision is not just up to me).
Sadly, we won't have time to do it ourselves though, as we are focusing on other higher-priority changes and fixes in the code base. From our point of view, this one is nice-to-have, but not critical for the release (just trying to be transparent here).
Once we have full understanding, we can start the discussion, if it is acceptable, if there are better alternatives, better tooling that needs to accompany it, etc. If we get to consensus in time (and we do not know the exact deadline at the moment sadly), then I can see it happening in .NET Core 3.0.

Does it make sense?
Does it help to explain our point of view?

karelz picture karelz  ·  2 Jun 2019
0

The point of view makes sense, and I understand where Microsoft is coming from. I know no one likes surprises, and this was definitely an unintended surprise. My primary point was that, at this moment, you're not breaking shipping apps. So if people looking to migrate can't do so until the first monthly .NET Core update (October instead of September) that doesn't seem like a huge deal, if people are warned about it soon. It's just something to think about as we're looking at the analysis.

I appreciate your comments on my "if we had shipped in 2.1" statement... while it's a disappointment that we're here, I know it will get done at some point. My passion for it comes from the fact that I know how important it is to WPF performance that it happens ASAP, and my concern for how many apps would break in the .NET 5 timeframe.

On the "10 minutes of fixes" part, I think you're going to have to search your code for NotifyCollectionChangedEventArgs and apply a specific pattern to how you handle the event. I think we will be able to publish guidance on what the set of fixes are for different permutations, and fix it in WPF wherever possible. I think we'll have to publish all that in a Docs article, and then get the word out to people wanting to port WPF apps to .NET Core through the official release notes.

I'm willing to see about putting the time in to make it happen, if you can give me a rough drop-dead date for when you need it by in order to get a positive result from the decision-makers. I have several other things I'm working on, but am willing to make the time if there is a bit of a window to work with.

robertmclaws picture robertmclaws  ·  2 Jun 2019
0

@karelz thank you for laying out all points so nicely and precisely.

I was aware to that WPF issue long ago, because I posted an implementation for it on SO that people complained about when using in WPF, some with ability to fix down, but when we discussed the issue here no one was talking about WPF in .NET Core.
BTW, my implementation also takes advantages of mixed insertions and avoids some other unnecessary actions, such as duplications (because ObservableCollection is originally made for UI, what's important is less events, not less comparisons etc.

I do understand this concern about WPF apps. But migrating WPF apps probably won't suffer from those exceptions as they don't make use of the range features anyway, it's only new WPF apps that will have issues, so is there a way to warn WPF consumers to avoid using the range APIs until they're fixed in the WPF repo? Is it the SAME .NET Core for WPF and others, or we can use preprocessor directives to wrap the range methods with warnings? Maybe put those warnings in the docs?

@robertmclaws

(instead of spending all my available free time for it on the spec discussion and then getting burnt out),

LOL I got burnt out when we agreed to implement our changes in Collection which lies in CoreCLR repo, while the tests were still in the CoreFX. I realized that I was spending too much time figuring out the connection between the two when I gave up.

weitzhandler picture weitzhandler  ·  3 Jun 2019
1

But migrating WPF apps probably won't suffer from those exceptions as they don't make use of the range features anyway, it's only new WPF apps that will have issues

If that was the case, I'd agree. Unfortunately, I believe that it's a common pattern that people added an AddRange extension method that did this functionality. I know I did, and it's the logical method name to use. With this change, that method isn't used. It'll compile just fine but then lead to a runtime exception later -- which in some ways is worse than if it just failed to compile. If that was the case, people would know and could fix.

clairernovotny picture clairernovotny  ·  3 Jun 2019
0

which in some ways is worse than if it just failed to compile

Totally agree. So it's only the new-coming WPF apps who are gonna suffer then, right?
Can we issue a warning on the range methods that they're not yet ready to be used with WPF apps?
Alternatively, we can implement a 'hack' in the WPF code that instead of throwing that exception, inserts the range items one by one, can we?

weitzhandler picture weitzhandler  ·  3 Jun 2019
1

@karelz thanks for the detailed explanation, it is much appreciated! Is there a deadline we could agree to as a hard-stop when we would need to get a Pull Request submitted to fix this problem with WPF? If we as a community are going to research this and try to hunt down the problem a hard-stop date will be helpful so we can prioritize our time.

@onovotny do you think you have time to show us on a call/screenshare exactly what is going on with the NuGet Package Explorer? If we can get some time together to outline the problem I can create some detailed documentation for us to further investigate.

I believe this would give anyone interested in digging into this (@robertmclaws, myself and anyone else) details to go on and hopefully enough to solve the problem for Preview 7

ahoefling picture ahoefling  ·  3 Jun 2019
1

BTW, @ahoefling, thanks for your PR!
Would you mind taking a look at my implementation and see if it makes sense? It avoids replacing same item, so that the UI doesn't refresh for them. I did that because I had an ObservableCollection with 15k items that made the UI laggy.

weitzhandler picture weitzhandler  ·  3 Jun 2019
0

I actually had pull-request dotnet/corefx#26482 that was rejected because I introduced another method if I remember correctly. Then the team decided to move it all into CoreCLR which is where I gave up 😢
But I'd be happy if some of my code or its ideas (for instance those clusters) would be incorporated in the .NET.

weitzhandler picture weitzhandler  ·  3 Jun 2019
0

Yep lol, been a while. It's right here above.

weitzhandler picture weitzhandler  ·  3 Jun 2019
1

@weitzhandler I remember seeing your attempt and I remember looking at it when I was working on my PR. I need to review my code and your code to see the differences and refresh my memory on the implementations.

ahoefling picture ahoefling  ·  3 Jun 2019
0

Can we issue a warning on the range methods that they're not yet ready to be used with WPF apps?

No, because it changes the method binding. Instead of binding to the extension method, it'll silently use the version in Collection<T>.

It's pretty simple: any place there's an AddRange on an ObservableCollection in the codebase, there's a potential issue due to binding. Here's the first one I noticed, but there are many others in use in the various editors.
https://github.com/NuGetPackageExplorer/NuGetPackageExplorer/blob/4868073d5a58a390f2c3c4363c2d7af29ffa7261/PackageViewModel/PackageChooser/PackageChooserViewModel.cs#L370

In preview 5, notice that it's binding to an extension method that calls Add() in a loop. Then, in Preview 6, it doesn't.

clairernovotny picture clairernovotny  ·  3 Jun 2019
0

No, because it changes the method binding. Instead of binding to the extension method, it'll silently use the version in Collection.

You're right.

AFAIR when digging after the WPF exception, it's thrown in a single spot and shouldn't be hard to fix, worst case we port back all range-updates into single UI calls (silly but less work).

weitzhandler picture weitzhandler  ·  3 Jun 2019
0

AFAIR when digging after the WPF exception, it's thrown in a single spot and shouldn't be hard to fix, worst case we port back all range-updates into single UI calls (silly but less work).

The unknown question is what else is making assumptions on that behavior? Is it just that ListCollectionView or are there other things that'll break? Hard problem.

clairernovotny picture clairernovotny  ·  3 Jun 2019
1

I wonder if we could build a Roslyn Analyzer to catch extension methods on ObservableCollection so you get a design-time error instead of a runtime one.

robertmclaws picture robertmclaws  ·  3 Jun 2019
0

The unknown question is what else is making assumptions on that behavior? Is it just that ListCollectionView or are there other things that'll break? Hard problem.

You're prob right, but all those exceptions are backed with tests.

weitzhandler picture weitzhandler  ·  3 Jun 2019
7

Guys if we're back here again, how about we discuss those APIs again?

Dear Jesus, no. We were at the finish line. Let's just fix the problem and move on, not overcomplicate it worse.

robertmclaws picture robertmclaws  ·  3 Jun 2019
2

Not trying to over-complicate, just manage risk.

clairernovotny picture clairernovotny  ·  3 Jun 2019
0

I agree with @robertmclaws we are so close to shipping this feature 🚀 🚀 🚀

ahoefling picture ahoefling  ·  3 Jun 2019
2

@onovotny I appreciate your concern as anything we do now will have long term consequences (good or bad). I am going to try and dig through this to better understand the problem to see if we can come up with something that is acceptable.

ahoefling picture ahoefling  ·  3 Jun 2019
0

OK, so I looked at the code that was throwing the error, and it just seems like the ListCollectionView is operating off the assumption I mentioned earlier where you can only ever do one item at a time in an event. It seems like most of the code in ValidateCollectionChangedEventArgs is no longer necessary.

Wouldn't the solution be:
1) Remove any code that throws SRID.RangeActionsNotSupported
2) Modify/remove existing unit tests that expect that Exception
3) Write some new unit tests against ProcessCollectionChanged to make sure it has the expected behavior with Ranges
4) Write an Analyzer to check for any extension methods on ObservableCollection (or anything that inherits from it) that have a method with "Range" in it, or accept an ILIst/IEnumerable that calls "Add/Remove/Replace"

Am I missing anything?

robertmclaws picture robertmclaws  ·  3 Jun 2019
1

Is there a deadline we could agree to as a hard-stop

We just snapped Preview 6 for release in a week or so. Preview 7 is in a month, but that will be the first preview widely supported : we cannot make (or at least can't plan to make) breaking changes after that. If we put this back in Preview 7 then discovered it caused to much pain to keep, we would have to make such a breaking change.

danmosemsft picture danmosemsft  ·  3 Jun 2019
1

To help everyone out here is the snippet in question that @robertmclaws referenced:

```c#
private void ValidateCollectionChangedEventArgs(NotifyCollectionChangedEventArgs e)
{
switch (e.Action)
{
case NotifyCollectionChangedAction.Add:
if (e.NewItems.Count != 1)
throw new NotSupportedException(SR.Get(SRID.RangeActionsNotSupported));
break;

    case NotifyCollectionChangedAction.Remove:
        if (e.OldItems.Count != 1)
            throw new NotSupportedException(SR.Get(SRID.RangeActionsNotSupported));
        break;

    case NotifyCollectionChangedAction.Replace:
        if (e.NewItems.Count != 1 || e.OldItems.Count != 1)
            throw new NotSupportedException(SR.Get(SRID.RangeActionsNotSupported));
        break;

    case NotifyCollectionChangedAction.Move:
        if (e.NewItems.Count != 1)
            throw new NotSupportedException(SR.Get(SRID.RangeActionsNotSupported));
        if (e.NewStartingIndex < 0)
            throw new InvalidOperationException(SR.Get(SRID.CannotMoveToUnknownPosition));
        break;

    case NotifyCollectionChangedAction.Reset:
        break;

    default:
        throw new NotSupportedException(SR.Get(SRID.UnexpectedCollectionChangeAction, e.Action));
}

}
```

After reviewing this code it makes sense to me that the root cause has to deal with this function throwing SRID.RangeActionNotSupported Exceptions. If we remove that code it should resolve the problem.

As a way to be proactive about other platforms in .NET Core that may be affected by this change maybe we could get a list of repositories we can search through for similar code?

ahoefling picture ahoefling  ·  3 Jun 2019
2

That validation was presumably there for a reason. Do we know why? Just removing the exception doesn't necessarily mean things will work as expected. Do the mechanisms in WPF that call that method work correctly with range methods, have they been tested?

clairernovotny picture clairernovotny  ·  3 Jun 2019
0

That validation was presumably there for a reason. Do we know why?

I would assume because of the broken API elements of the EventArgs class that I mentioned earlier. I'm not suggesting that we "_just_ remove the exceptions", we would need to write new unit tests that cover range methods and those events, and see if the behavior lines up with expectations. Then we'd need to pull the potential fixes into an app or two (like NPE) and see if they behave as expected in real-world environments.

As an aside and since there are .NET Management people on this thread, this situation is just one of many examples for why Microsoft should mandate that all internal code (like ValidateCollectionChangedEventArgs) have XML Documentation Comments. Would certainly be great to know exactly why this validator is here, instead of operating off of assumptions.

robertmclaws picture robertmclaws  ·  3 Jun 2019
2

@robertmclaws ... this situation is just one of many examples for why Microsoft should mandate that all internal code (like ValidateCollectionChangedEventArgs) have XML Documentation Comments

I personally do not believe that any amount of XML docs would truly help. First, you would need time-machine and go back 20 years and backfill everything (not realistic) and second, I have near-zero trust in developers, that they would update the docs as we would progress in future and the code and paradigm would change.
Just being very practical here.
Adding XML docs is good idea, but it is not silver bullet that will save us or prevent all/most mishaps. At best it will help a bit. IMO the cost/benefit ratio is very poor. Having better test coverage is IMO much better cost/benefit investment for product, customers, behavior documentation, etc.

rough drop-dead date / hard-stop date

Personally, I would not support any change after 6/14 given my current knowledge of the release schedule (which may change).
Anything after 6/14 is just too close to snap for July release and therefore it creates uncomfortable amount of risk for me. As an engineer and someone who wants to deliver high quality products, I can't support anything later than that.

karelz picture karelz  ·  3 Jun 2019
2

@robertmclaws

My primary point was that, at this moment, you're not breaking shipping apps. So if people looking to migrate can't do so until the first monthly .NET Core update (October instead of September) that doesn't seem like a huge deal, if people are warned about it soon. It's just something to think about as we're looking at the analysis.

I don't fully understand your statement/point here. Can you please rephrase it / clarify for me?

My key viewpoint is: App migration to .NET Core matters A LOT. To the point that if we have to break apps in a way that is not super-easy to detect upfront, I would not support taking the change in ANY future release of .NET Core (at least given current knowledge from this thread which indicates that many apps will fall into this trap).

I know it will get done at some point.

I hope so too!

My passion for it comes from the fact that I know how important it is to WPF performance that it happens ASAP, and my concern for how many apps would break in the .NET 5 timeframe.

If we have to break apps (e.g. in .NET 5), we have to understand it more - having it analyzed from left and right, to understand the IMPACT on apps (how many and how badly are impacted), if we truly cannot MITIGATE the impact more (Roslyn analyzers, magic and unicorns considered) and what is the VALUE customers will get (how much perf benefit, not just "it is better and the right design").
Such analyzes will take weeks and likely months. Not something that can be done for .NET Core 3.0.
So, from my point of view, if we cannot greatly mitigate the breaking change impact by e.g. modifying the PR, some WPF code, or adding smart Roslyn analyzers or something like that, it is automatically out of .NET Core 3.0 for me at this moment (with potential consideration for in .NET 5 after a lengthy discussion and analysis mentioned above).

To be explicitly clear: I am HESITANT to view documentation, release notes, and/or guidance as SUFFICIENT MITIGATION of the breaking change impact which manifests as weird runtime exception. I am curious what others on CoreFX team think abut it (@danmosemsft).

karelz picture karelz  ·  3 Jun 2019
0

@karelz
I totally understand your concern.
Do you think it would be enough relying on the WPF tests + additional tests in the WPF repo to verify range handling, and maybe one or two of OS WPF showcase apps tested against this change, and have this in 3.0?

weitzhandler picture weitzhandler  ·  3 Jun 2019
0

@weitzhandler depends on solution, but keep in mind that WPF tests did NOT flag current problem (as it requires app using common extension), so it his hard to claim it is sufficient test coverage for this case ...

karelz picture karelz  ·  3 Jun 2019
0

I'm not familiar with the WPF testing architecture, but I'm sure it has UI tests that we can add collection-range testing to.

weitzhandler picture weitzhandler  ·  3 Jun 2019
2

I'm sure it has UI tests that we can add collection-range testing to

Just to reiterate: It is the test bed that did NOT discover the problem in the first place. While we can boost it up, the coverage of the scenario will be only what we will add, i.e. skewed by our understanding and minimal. The key will be to come up with good test plan to discover potential downsides of the solution we are going to propose. Ideally some exploratory/end-to-end testing.

karelz picture karelz  ·  3 Jun 2019
-2

Shouldn't the priority here to be to finally make it right? I find it kind of odd that this conversation is directed at supporting folks who had to wire up their own extensions in order to make the API work correctly in the first place. Now that we have a working structure, those folks extensions are inevitably being deprecated.

This seems like an instance of the Needs of the Many Outweigh the Needs of the Few⁉︎

ChaseFlorell picture ChaseFlorell  ·  3 Jun 2019
1

@ChaseFlorell One must remember an extremely important policy of .NET (and many other Microsoft products): Do not break existing products, even with major release. This has been in place for years and will continue to be.

SamuelEnglard picture SamuelEnglard  ·  3 Jun 2019
2

@SamuelEnglard but unless I'm missing something, we're not breaking any existing products, we're _potentially_ breaking other developers workarounds to existing deficiencies.

ChaseFlorell picture ChaseFlorell  ·  3 Jun 2019
2

@karelz Do you have like 15 minutes at some point in the next couple days to chat briefly on Teams? I think we could get on the same page real quick with a face-to-face discussion.

robertmclaws picture robertmclaws  ·  3 Jun 2019
-1

@ChaseFlorell existing apps / assets = existing customers = existing products.

karelz picture karelz  ·  3 Jun 2019
1

@robertmclaws sure, ping on me gitter, Twitter, Skype or email - see my GH profile for links to the info.

karelz picture karelz  ·  3 Jun 2019
1

@ChaseFlorell I should have been clearer:

Do not beak existing products, applications, or code; whether the product, application, or code belongs to Microsoft or not.

Those workarounds are in products, products that are used by people now. And it's not potentially, as the reason this was reopened in the first place was an example of it not working for a developer.

SamuelEnglard picture SamuelEnglard  ·  3 Jun 2019
1

Do not beak existing products, applications, or code; whether the product, application, or code belongs to Microsoft or not.

Those workarounds are in products, products that are used by people now. And it's not potentially, as the reason this was reopened in the first place was an example of it not working for a developer.

But honestly this is a net new method. How can MS possibly account for every extension method ever conceived in history? Sure it's in products that are used, but their extension methods are within their code base that they have control over.

ChaseFlorell picture ChaseFlorell  ·  3 Jun 2019
4

But honestly this is a net new method. How can MS possibly account for every extension method ever conceived in history?

Normally we consider breaking hypothetical extension methods to be an acceptable breaking change, at least in a major version. The concern is whether this is going to affect enough people that it's not.

danmosemsft picture danmosemsft  ·  3 Jun 2019
4

Normally we consider breaking hypothetical extension methods to be an acceptable breaking change, at least in a major version. The concern is whether this is going to affect enough people that it's not.

Right, and reasonable. So what is the threshold for this? Also, is there another avenue to a solution without reverting the code? I feel as though I'm in the same boat whereby we've got a custom ObservableRangeCollection<T> in our source, the idea here is to actually be able to delete it in favor of the code @ahoefling contributed... ie: we _want_ to stop supporting our own custom implementation.

ChaseFlorell picture ChaseFlorell  ·  3 Jun 2019
1

That validation was presumably there for a reason. Do we know why? Just removing the exception doesn't necessarily mean things will work as expected. Do the mechanisms in WPF that call that method work correctly with range methods, have they been tested?

Let's ask @vatsan-madhavan about that

omariom picture omariom  ·  3 Jun 2019
0

Normally we consider breaking hypothetical extension methods to be an acceptable breaking change, at least in a major version. The concern is whether this is going to affect enough people that it's not.

Right, and reasonable. So what is the threshold for this?

Replacing them with something that fails seems something to avoid

benaadams picture benaadams  ·  3 Jun 2019
4

@benaadams The replacement didn't fail. What failed was that a WPF control coded in throwing exceptions when a broken API gained the functionality it originally intended. The Observable's EventArgs were a bad design, and the WPF control was a bad design. We fixed the first, now we have to a) fix the second, and b) find out what else might be broken, c) build a tool that can help developers porting their stuff to find potential problems.

I'm going to try to jump on a call with Karel today so that the people working on a fix + the people running the show can all get on the same page.

robertmclaws picture robertmclaws  ·  3 Jun 2019
11

Here is how I feel about this:

  • This is an acceptable breaking change. It's clearly valuable and completely sensible to support this in UI. It's likely that we can't always support it in the most efficient way, but at the very minimum the receiver can always treat it as individual events or as a reset.

  • The product needs to self-consistent. Even for acceptable breaking changes, we can't ship them if part of the product can't deal with them. Of course, we don't always know this upfront, but we do now know about WPF being broken in 3.0.

  • It's very late in .NET Core 3.0. We only have a few months left and the WPF team still has a ton of work to do. Judging by the number of references from WPF in .NET Framework, this is likely going to require some non-trivial amount of work in WPF. We don't believe we have enough runway in 3.0 to walk this entire list and ensure WPF works consistently.

My position is that we postpone this API to the next major version of .NET (because .NET Core 3.1 is mostly minor fixes).

terrajobst picture terrajobst  ·  3 Jun 2019
0

/cc @rladuca, @grubioe

vatsan-madhavan picture vatsan-madhavan  ·  3 Jun 2019
2

@terrajobst Thank you SO MUCH for that link to the references in WPF. I'm going to put together a triage list and see what might be affected.

robertmclaws picture robertmclaws  ·  3 Jun 2019
0

ObservableCollection Range Changes WPF Triage List

Source List: .NET Reference Source

MS.Internal.Annotations

robertmclaws picture robertmclaws  ·  3 Jun 2019
0

My position is that we postpone this API to the next major version of .NET (because .NET Core 3.1 is mostly minor fixes).

I agree.

danmosemsft picture danmosemsft  ·  3 Jun 2019
4

Just had my call with @karelz. I want to talk briefly with Andrew before I report the results of that conversation on the rest of the thread.

robertmclaws picture robertmclaws  ·  3 Jun 2019
5

Just to clarify: We have decided between architects, managers and PM on CoreFX team, that we WILL NOT take this issue for 3.0. We are fine to take it on the first day of .NET 5 in master (which should be in July ... likely mid-July if I read the tea-leaves right ... the dates are subject to change though).
I discussed it with @robertmclaws over Skype and we came to same consensus, given all restrictions (not much runway, important WPF scenario, plenty of remaining WPF work left).

Moving to Future (will move it to .NET 5, once we have the milestone - at the moment we branch off 3.0 out of master)

karelz picture karelz  ·  3 Jun 2019
-2

OK, now that it's been confirmed (sadly), can we consider adding filtered replace to enable reusing items (good for UI performance, so those items don't have to be rendered again). Imagine a drop-down list or plenty of other scenarios I've used and encountered, that this behavior of reusing items made it slick.

Here's my previous comment summarizing the approved and suggested API, please have a look before you downvote. You can also have a look at PR dotnet/corefx#26482 to see the behavior.

Those filters are not there as a syntax sugar. They're to save UI performance.

weitzhandler picture weitzhandler  ·  4 Jun 2019
21

Well, this is disappointing... but I'm not going to suggest that Microsoft put on its' big-kid panties if I'm not willing to do it myself, so we move forward. :)

I spoke with Karel and Andrew, and we have the beginnings of a plan to tackle this properly in .NET 5... I will open a new issue soon and link back to it here when we're ready for a bigger discussion.

Regarding any API changes, I am going to HIGHLY suggest that we keep pressing forward with completing the solution we have today, and not reopen the API Design Review process. I think the right thing to do @weitzhandler would be to let this process finish out and be merged into the codebase before you propose any new features. Here's why:

1) We need to be respectful of the people that worked so hard to get this as far as they have. Andrew + the reviewers + the risk management team did a crapload of work, and Andrew is the only one (out of 3 people that tried!) to actually get this over the finish line. @weitzhandler, you and I both pushed PRs that weren't accepted... so we should be working to be tailwind for Andrew, not headwind.

2) The original spec had some of those additional method signatures, and Microsoft rejected them in favor of a more simplified approach. We all discussed that (ad-nauseum, I might add) and I'm not sure the situation has changed.

3) Speaking only for myself (and trying to be diplomatic here), but your original PR would have been accepted if you hadn't over-complicated it with stuff that wasn't agreed on. When I first started on this, I had an implementation done and a PR submitted, based on Xamarin's ObservableRangeCollection. That PR was rejected for not having completed an API Design Review first. So I went through the process and opened this Issue like I was supposed to. You kind of jumped in halfway through and took over that process, tried to circumvent the approval by piggybacking on my work, and your PR was ultimately rejected for being off-spec. If you are thinking about being involved moving forward, the right thing to do here is assist in unblocking the current solution, not block it further by trying to again advance a _twice_-rejected agenda. Just my $0.02.

At any rate, Andrew and I will be working together to come up with a detailed mitigation plan, and we're start prepping to hit this hard when the .NET 5 branch is available. Thanks to @karelz @onovotny @danmosemsft @terrajobst and everyone else for your help over the last few weeks, and looking forward to getting this out the door properly. :)

robertmclaws picture robertmclaws  ·  4 Jun 2019
4

@robertmclaws ok. that makes a lot of sense. thanks for being kind, and sorry.

3. tried to circumvent the approval by piggybacking on my work

I'm sorry that you see it this way, I've based my work on a RangeObservableCollection I was using internally in our projects for a long time, and I'm sorry for kicking in, I wouldn't have done it if I hadn't had the feeling you backed off. A thousand apologies. Apart from the work I invested in the filtering, I also think it's very important to UI performance, hence the nudge, sorry everyone about that and let's just move on.

weitzhandler picture weitzhandler  ·  4 Jun 2019
14

@robertmclaws

  1. We need to be respectful of the people that worked so hard to get this as far as they have. Andrew + the reviewers + the risk management team did a crapload of work, and Andrew is the only one (out of 3 people that tried!) to actually get this over the finish line. @weitzhandler, you and I both pushed PRs that weren't accepted... so we should be working to be tailwind for Andrew, not headwind.

This a really well written and reflecting comment on what it means to work together to achieve more. This is truly remarkable and part of the reason why I love our OSS community so much. You all rock. You've all been incredibly patient with us. This API has been sitting there for a long time before we even looked at it.

@weitzhandler

Don't feel too bad. It's quite clear based on your effort and PRs that you care about this API and want the right thing to happen. As I've said before: I prefer working with people who care, because more the key differentiator between a good dev and a great dev isn't necessarily skill, but level of care.

terrajobst picture terrajobst  ·  4 Jun 2019
5

@terrajobst Thanks man. I worked really hard on that post... I had hoped it wouldn't offend anyone. :)

@weitzhandler No need to be sorry man, I know you were helping, and I had a lot of personal stuff going on back then, so my head wasn't in the right place at the time. We've all learned valuable lessons in this process. At this point I just want to see this cross the finish line so that all of our collective passionate investments will have been worth it in the hundreds of apps that will run faster because of this work. 🍻

robertmclaws picture robertmclaws  ·  4 Jun 2019
4

@weitzhandler I definitely think that you should put a proposal together for your additional APIs. I just think it should wait. If you start on your proposal now in your own repo, then you can copy it to this repo after we get this API over the finish line. Just check out the Speclet format in the first post, should give you a great start. Happy to review it in your repo while you're working on it, if you like. :)

robertmclaws picture robertmclaws  ·  4 Jun 2019
5

FYI: Moved to milestone 5.0 as promised.
WPF team will be busy until we ship 3.0, but if we want to take changes into CoreFX that won't break WPF badly in master, we can do it now.

karelz picture karelz  ·  31 Jul 2019
0

Just curious, did you consider keeping the changes but raising NotifyCollectionChangedActionEventArgs which are compatible with the current version of WPF?

Either raising multiple events or just raising with the Action set to Reset once for batch changes. This way the API could be added sooner (maybe even 3.1) without any work on WPF. And the behaviour changed later when the WPF parts are complete.

Daniel-Svensson picture Daniel-Svensson  ·  14 Aug 2019
10

Raising multiple events is what the system already does. The point of these changes is to NOT have the UI repaint on every item insert. This means we need to take the time to do it right, and WPF/Xamarin/UWP will benefit far more from implementing the API properly through the stack than from taking shortcuts to ram it through any given release window.

robertmclaws picture robertmclaws  ·  14 Aug 2019
0

Any news?

kronic picture kronic  ·  13 Feb 2020
6

It would be really nice to get these changes to observable collection!

TonyValenti picture TonyValenti  ·  29 Apr 2020
0

@terrajobst @karelz just to clarify the current status as there's not many recent comments or an update in the initial post.

Is this indeed in the .NET 5 milestone and will land for RTM as the labels and milestone bucket seem to indicate?

I am a bit confused as the linked PR was merged a while ago...?

michael-hawker picture michael-hawker  ·  5 May 2020
0

This is a breaking change that caused a revert before 3.0 release. To prevent this happening again, would it be possible to fix this in a preview soon? This will give everyone time to adapt these components and support range notification changes.

YZahringer picture YZahringer  ·  14 May 2020
1

What is the status of this? The PR was seemingly merged but the issue is still open?

Ghosty141 picture Ghosty141  ·  21 Jun 2020
-2

Unfortunately it was reverted
https://github.com/dotnet/corefx/pull/38115

danmosemsft picture danmosemsft  ·  22 Jun 2020
-16

Unfortunately, the cost of this feature is too high to fit in 5.0 at this stage in the release. The initial implementation in 3.0 had to be reverted due to the change breaking WPF (https://github.com/dotnet/runtime/issues/29721), and it's likely we'll be in a similar situation if we proceed in this release.

layomia picture layomia  ·  27 Jun 2020
4

So why does nobody fix WPF as part of this? :(

Its handling of collection change events is broken, it just should do a reset for anything not supported, not throw an exception.

weltkante picture weltkante  ·  27 Jun 2020
18

@layomia sorry, but I don't understand why. This has been reverted in 3.0-Preview7 to minimize the impact of WPF apps migration to .NET Core 3. This was understandable to allow a simple migration without side effects, it was a priority.

Now .NET 5 is still in preview, not release candidate and breaking changes should still be allowed. Based on Announcing .NET 5.0 Preview 6 blog post, the release will be feature-complete with Preview 8.

The PR is already reviewed and accepted. In my opinion, this should be an acceptable change for 5.0-Preview7.

YZahringer picture YZahringer  ·  27 Jun 2020
0

Wait another year and a half.

kronic picture kronic  ·  29 Jun 2020
0

So why does nobody fix WPF as part of this? :(

Its handling of collection change events is broken, it just should do a reset for anything not supported, not throw an exception.

Chicken and egg. Since Observable doesn't support it, no one has bothered to created an alternate collection class to fix and test WPF.

Since no one fixed and tested WPF, they don't want to add this non-breaking change.

Grauenwolf picture Grauenwolf  ·  29 Jun 2020
0

Adding this has zero impact on WPF migration because no existing WPF application is using it.

Yes, it may not work right for new applications. But that's not the same as a breaking change.

Grauenwolf picture Grauenwolf  ·  29 Jun 2020
0

Chicken and egg. Since Observable doesn't support it, no one has bothered to created an alternate collection class to fix and test WPF.

Since no one fixed and tested WPF, they don't want to add this non-breaking change.

Is there a ticket in the WPF repo?

sharpninja picture sharpninja  ·  30 Jun 2020
0

Is there a ticket in the WPF repo?

dotnet/wpf#1887 ... for what its worth they did mark this for 5.0 but I don't know if they still intend to do it ...

weltkante picture weltkante  ·  30 Jun 2020
0

I'd imagine WinUI probably needs one too as well? FYI @ranjeshj

michael-hawker picture michael-hawker  ·  1 Jul 2020
5

If they implemented the INotifyCollectionChanged protocol correctly, no, they wouldn't need one. Its a generalized interface that already supported bulk operations, just the stock implementation of ObservableCollection didn't make use of it.

WPF has only been broken because they don't implement the protocol of INotifyCollectionChanged correctly, WPF has to fix it regardless of whether this issue is implemented or not, since users can pass their own collection classes along.

I'd imagine if WinUI made similar mistakes that someone already has been complaining about it (in fact there were issues that INotifyCollectionChanged stopped working after adding Desktop support).

weltkante picture weltkante  ·  1 Jul 2020
4

To be more specific about this, WPF has UI controls that throw exceptions if you have more than one object in the INotifyCollectionChanged event. There are some controls that actually depend on this behavior, so at the moment, we're stuck.

Just based on what I know, this is not likely to get into .NET 5.0 (unfortunately, I haven't had the time to spearhead this from the outside, and i believe that .NET 5.0 is feature complete for a September-ish launch), but I think we'll be able to get it into .NET 6..0, which will follow right behind.

To be clear, that is not Microsoft's official stance, just my opinion after trying to shepherd this long overdue feature for the last 4 years.

robertmclaws picture robertmclaws  ·  1 Jul 2020
0

WPF has UI controls that throw exceptions if you have more than one object in the INotifyCollectionChanged event

Thats a weird statement, because in the linked issue its not any control at all, the infrastructure is throwing. Maybe you are wrongly associating your observations to the control you were trying to bind the collection against, but from what I can tell the events never actually reach the control.

There are some controls that actually depend on this behavior

Thats an even more weird statement, as the events never arrive at the controls. If you have concerns that controls wouldn't be able to handle collections changing completely instead of incrementally I have to remind you that the current infrastructure already supports reset events.

Usually though controls don't bother looking at the collection events in the first place, they let the infrastructure and low level controls like ItemsControl handle it. So implementing it efficiently just for collections connected to an ItemsControl and letting everyone else just see reset events would already cover most of the cases in practical applications.

Currently I doubt such special-casing is necessary, but if you have concrete examples or compatibility concerns for the WPF side I'd suggest adding them to dotnet/wpf#1887 instead of here so they are considered when they get arround to fixing it.

weltkante picture weltkante  ·  1 Jul 2020
1

This would be great to have. As you have already noticed in WPF, controls consuming this would likely need updates though to support/take advantage of this. Same would hold for WinUI controls like ListView/GridView/ItemsRepeater etc.

ranjeshj picture ranjeshj  ·  1 Jul 2020
1

@weltkante You are nitpicking about the difference between controls and the underlying infrastructure that is technically a part of the control when it is executing. And you're also nitpicking on controls "depending" on that behavior. Because it's not possible to have the event contain more than one entry (because if it does, an exception is thrown) the controls are not designed with the possibility of multiple entries in mind. Meaning they depend on only one event coming through the system. And just throwing reset events may not be the right approach either. Making sure the ecosystem doesn't get broken in the process will require more testing.

When we get around to producing a build that has both fixes to be able to identify downstream issues, we will add them to the ticket as appropriate.

robertmclaws picture robertmclaws  ·  1 Jul 2020
0

@robertmclaws - This doesn't seem like nitpicking to me. If the base code is broken, then fixing it shouldn't cause changing code in controls, except where the controls are fundamentally "broken" (meaning don't follow the intent of the event passing a collection in the arguments). I have implemented INotifyCollectionChanged handlers in WPF and never experienced any issues when receiving more than one value (I had implemented a composition of ObservableCollection with Range methods).

sharpninja picture sharpninja  ·  17 Jul 2020