Add a combine extension method to ReactiveList<T> #531

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Member

tberman commented Feb 28, 2014

Tried a simple test, and it worked without issues.

@niik niik commented on the diff Feb 28, 2014

ReactiveUI/ReactiveListMixins.cs
+ list.AddRange(ea.NewItems.Cast<T>());
+ break;
+ case NotifyCollectionChangedAction.Remove:
+ list.RemoveAll(ea.OldItems.Cast<T>());
+ break;
+ case NotifyCollectionChangedAction.Replace:
+ list.RemoveAll(ea.OldItems.Cast<T>());
+ list.AddRange(ea.NewItems.Cast<T>());
+ break;
+ case NotifyCollectionChangedAction.Reset:
+ using (list.SuppressChangeNotifications()) {
+ list.Clear();
+ list.AddRange(listOne);
+ list.AddRange(listTwo);
+ }
+ list.Reset();
@niik

niik Feb 28, 2014

Member

SuppressChangeNotifications will automatically signal a reset when the scope is disposed

Member

niik commented Feb 28, 2014

@tberman Thanks for your submission!

I'd like to see some unit tests for this. My main concern is that this code ignores indices from the source lists meaning that it'll only work for reference types not implementing custom equality comparisons. Ie, the Remove(T) will remove everything that's considered equal to the T instance you're sending in, a replace notification from one of the source lists could very well end up removing more than one item.

As a consequence it also doesn't concern itself with ordering at all which would be fine if the combined list was considered a set but since it's returning a list I don't think that's going to fly.

If this is to be a thing I think the only way is to guarantee that the items for the first list lies first in the combined list (and in the same order). You're going to have to do some heavy lifting with index calculations here.

As a final note I think this extension method should return an IReactiveDerivedList<T> instead of returning an IDisposable and exporting a read-write list (than consumers can modify freely) through an out-variable. IReactiveDerivedList is read-only, implements IDisposable and communicates that it's a projection of something.

Have a look at onSourceCollectionChanged in ReactiveDerivedCollection for some inspiration.

@niik niik commented on the diff Feb 28, 2014

ReactiveUI/ReactiveListMixins.cs
+ combinedList = list;
+
+ combinedList.AddRange(listOne);
+ combinedList.AddRange(listTwo);
+
+ Action<NotifyCollectionChangedEventArgs> listChanged = (ea) => {
+ switch (ea.Action) {
+ case NotifyCollectionChangedAction.Add:
+ list.AddRange(ea.NewItems.Cast<T>());
+ break;
+ case NotifyCollectionChangedAction.Remove:
+ list.RemoveAll(ea.OldItems.Cast<T>());
+ break;
+ case NotifyCollectionChangedAction.Replace:
+ list.RemoveAll(ea.OldItems.Cast<T>());
+ list.AddRange(ea.NewItems.Cast<T>());
@niik

niik Feb 28, 2014

Member

This actually breaks the notification chain, a Replace from the source will now trigger two notifications from the combined. This makes the replace implementation non-atomic and as such also non-reentrant.

@niik niik commented on the diff Feb 28, 2014

ReactiveUI/ReactiveListMixins.cs
+ if (listOne == null) {
+ throw new ArgumentNullException("listOne");
+ }
+
+ if (listTwo == null) {
+ throw new ArgumentNullException("listTwo");
+ }
+
+ var list = new ReactiveList<T>();
+
+ combinedList = list;
+
+ combinedList.AddRange(listOne);
+ combinedList.AddRange(listTwo);
+
+ Action<NotifyCollectionChangedEventArgs> listChanged = (ea) => {
@niik

niik Feb 28, 2014

Member

Notifications can come from either of the lists from any thread at any time so the change handler has to serialize access to the resulting list.

Member

tberman commented Feb 28, 2014

Thanks for your feedback, couple questions before I mess with this more

  1. If we renamed Combine (which is an ambiguous name for the operator) Union, does the Set-like mechanics make sense? If not, I would probably rename it Concat if we implemented the behavior you are talking about, as that is much clearer.

  2. I avoided a derived projection because I was afraid of the scheduling issues if you derive another collection from it. Imagine listOne.Combine(listTwo).CreateDerivedCollection(), do you think that will pose an issue, or not?

  3. Finally, if the goal here is set mechanics, is it time (shudder) for a ReactiveSet? If so, I'd want to build this off of #448.

Member

niik commented Feb 28, 2014

If we renamed Combine (which is an ambiguous name for the operator) Union, does the Set-like mechanics make sense? If not, I would probably rename it Concat if we implemented the behavior you are talking about, as that is much clearer.

Yeah I think we should match the terminology of LINQ as far as possible so it'd be either Concat or Union depending on which route you'd like to take.

I avoided a derived projection because I was afraid of the scheduling issues if you derive another collection from it. Imagine listOne.Combine(listTwo).CreateDerivedCollection(), do you think that will pose an issue, or not?

Yeah, you're wise to be afraid but unfortunately scheduling issues are unavoidable. Remember that you can already create a derived collection from the list you create today. The main issue is to as far as possible match the notifications from the source lists. One thing that bit me hard when I wrote the current ReactiveDerivedList implementation was when I tried to cheat by publishing two notifications for one notification from the source. Notifications need to be atomic, ie when someone downstream gets a notification that can't be in the middle of the list doing its work so signalling has to be the last thing your change handler does before returning and there can only be one notification.

Finally, if the goal here is set mechanics, is it time (shudder) for a ReactiveSet? If so, I'd want to build this off of #448.

I don't see a great need for a ReactiveSet personally and I think it might actually be even harder to do because now you have to essentially keep shadow copies of both collections and act on them accordingly (a shadow collection is actually what powers ReactiveDerivedCollection today but that has more to do with it tracking item changes as part of its filter operation).

All that said I think the value of a ReactiveSet is probably best answered by @paulcbetts.

Owner

paulcbetts commented Mar 2, 2014

Lemme Contemplate this and see what I come up with

Owner

ghuntley commented Dec 21, 2015

Doing some PR cleanup, @paulcbetts 👍 or 👎 or 🔥 /cc: @niik and @tberman

ghuntley added the rxui label Dec 21, 2015

Member

tberman commented Feb 15, 2016

🔥 it was a terrible idea every time we used it.

tberman closed this Feb 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment