[Feature]: Bind operation with Sorting

[dwcullop]

Describe the functionality desired

A new Bind operator that takes a sort comparer that is applied to the resulting collection.

From @RolandPheasant on Slack:

I'd like a new take on Bind which accepts a sort comparer. The reason is that sort is super heavy weight because it supplies an additional list on the change set which is used for binding, visualize and page only. This need would go away if the sort was done inside bind

The steps the functionality will provide

The ability to have a sorted ObservableCollection without having to apply a Sort operator explicitly which is more expensive.

Considerations

Using Sort followed by Bind works, but it would be more performant to have in a single step because it would do away with an intermediate changeset.

[timothylcooke]

I'm going to share some of my experiences with Sorting because I do it very often. A lot of this is probably irrelevant to this issue, but I'm adding it because it is loosely related. Sorry for the wall of text.

In many of my ViewModels, I have an API that returns a list of elements to be displayed in the UI. It is almost boilerplate for me to make a ViewModel with a SourceCache of C# records. When users click "Refresh" (or on a timer) I fetch the latest records from the API, and update the SourceCache with the latest records:

I then bind the SourceCache to the View. When I care about the order in which the records show up in the View, I make the API return a sort key with the record.

So, I often bind to the list with something like this:

mySourceCache
    .Sort(SortExpressionComparer<MyRecordType>.Ascending(x => x.OrderBy))
    .Bind(out var mySortedList)
    .Subscribe();
this.MySortedList = mySortedList;

There are two common cases in which I have to deviate from this pattern:

1. Sometimes, I use specific Controls that require me to use some arbitrary, readonly collection instance (often extending IList<T>, or even ObservableCollection<T>) that is created and managed by the control. In those cases, I must bind to that existing list, rather than using a ReadOnlyObservableCollection<T>.

   Maybe I'm missing something, but I haven't found an instance of Bind that works for that use case, and the built-in operator Clone(ICollection<T>) doesn't seem to handle sorting correctly. I wish there were an overload of Clone(IList<T>) that properly handled sorting. I've implemented my own BindToList operator that I use for this situation:

    public static IObservable<IChangeSet<TObject, TKey>> BindToList<TObject, TKey>(this IObservable<IChangeSet<TObject, TKey>> source, IList<TObject> list)
    {
        return source.ForEachChange(x =>
            {
                // CurrentIndex and PreviousIndex are only >= 0 if we are sorting.
                switch (x.Reason)
                {
                    case ChangeReason.Add:
                        if (x.CurrentIndex < 0)
                            list.Add(x.Current);
                        else
                            list.Insert(x.CurrentIndex, x.Current);
                        break;
                    case ChangeReason.Remove:
                        if (x.CurrentIndex < 0)
                            list.Remove(x.Current);
                        else
                            list.RemoveAt(x.CurrentIndex);
                        break;
                    case ChangeReason.Refresh:
                        // Do nothing!
                        break;
                    case ChangeReason.Moved:
                    case ChangeReason.Update:
                        if (x.PreviousIndex >= 0)
                        {
                            list.RemoveAt(x.PreviousIndex);
                            list.Insert(x.CurrentIndex, x.Current);
                        }
                        else
                        {
                            // This is a relatively new operator, so I'm not sure if these "not implemented" cases ever happen.
    #if DEBUG
                            if (x.CurrentIndex >= 0)
                            {
                                throw new NotImplementedException();
                            }
    #endif
                            if (x.Previous.HasValue)
                            {
                                list.Replace(x.Previous.Value, x.Current);
                            }
    #if DEBUG
                            else
                            {
                                throw new NotImplementedException();
                            }
    #endif
                        }
                        break;
    #if DEBUG
                    default:
                        throw new NotSupportedException($"Unrecognized reason: {x.Reason}");
    #endif
                }
            });
    }
   

2. Sometimes, I need to bind to something more complex than a record. Either I need to transform to ReactiveObject so I can use observables or INotifyPropertyChanged features in my views, or the Control requires a specific type of element (e.g. I have a Kanban view that requires columns to be of type KanbanColumn). Ideally, my code would look something like this:

    mySourceCache
        .Sort(SortExpressionComparer<MyRecordType>.Ascending(x => x.OrderBy))
        .Transform(x => new MoreComplexType(x))
        .Bind(out var mySortedList)
        .Subscribe();
    this.MySortedList = mySortedList;
   

   The problem I run into with this is that the Transform operator does not seem to preserve sorting, so I usually have to add an otherwise-unnecessary OrderBy property to the MoreComplexType so I can do the Sort after the Transform.

In case #1, I'd love it if something like a sorted BindToList were implemented in DD. Perhaps it already is? I always feel more comfortable with library operators than my own cobbled-together implementations.

In case #2, I don't think anything can be done here. I'm not sure if it's worth creating a separate issue or if that's intended behavior. EDIT: Maybe if there were a Bind that did Sort, Transform, and Bind in a single operation, but that feels overly complicated and niche.

[RolandPheasant]

Sometimes, I use specific Controls that require me to use some arbitrary, readonly collection instance (often extending IList, or even ObservableCollection) that is created and managed by the control. In those cases, I must bind to that existing list, rather than using a ReadOnlyObservableCollection.

@timothylcooke The reason Dynamic Data only supports ObservableCollectionExtended and ReadonlyObservableCollection is historic when I wanted Dynamic Data to be optimal out of the box, otherwise I feared no-one would ever take it seriously. In the case of binding, the restriction for using ObservableCollectionExtended is that it is much more optimal to fire a Reset when there are a large number of changes, so code enforces that. With the standard observable collection and with arbitrary lists this is not possible.

That said I think the global community is well trained enough to automatically use the fast version, even if we were to introduce additional bind methods. Therefore I'd agree that BindToList would be a good addition to the lib.

The problem I run into with this is that the Transform operator does not seem to preserve sorting, so I usually have to add an otherwise-unnecessary OrderBy property to the MoreComplexType so I can do the Sort after the Transform.

You're right, this is a tricky scenario. The nature of the the cache side of DD is that it does not maintain order as it stores state via keys only. However what may be worth trying is to convert it to a list first:

 mySourceCache
     .RemoveKey() // Makes it an observable list which does maintain order (mostly but not for dynamic filtering)
     .Sort(SortExpressionComparer<MyRecordType>.Ascending(x => x.OrderBy))
     .Transform(x => new MoreComplexType(x))
     .Bind(out var mySortedList)
     .Subscribe();

[timothylcooke]

@RolandPheasant As always, thanks for the great feedback. I'm excited to try out .RemoveKey().

For the purposes of this issue, none of my above feedback is relevant unless you decide to:

1. Implement BindToList in the first place (documentation comments should discourage using it whenever possible)
2. Implement BindToList at the same time as you add Bind operations with sorting.

FWIW, I'm in no way married to the BindToList name. It could probably just be Bind, unless you want to give it a unique name to try to discourage its use.

[dwcullop]

@timothylcooke Since @RolandPheasant likes your idea for a new operator, then I think you should open a new issue for it so someone will work on it. Better yet, you could follow it up with a PR since you already have an implementation. All you'll need is some unit tests.

I would suggest a slightly different prototype for it though:

public static IObservable<IChangeSet<TObject, TKey>> Bind<TObject, TKey>(this IObservable<IChangeSet<TObject, TKey>> source, out IReadOnlyList<TObject> list);

You don't want something outside of the operator to be able to change the list because it would break the Indexing.

Would it work for your scenario if the list were IReadOnlyList?

[timothylcooke]

No, It would have to be IList<TObject> to work more generally. I see your point about you wouldn't want it to be modified outside of the Bind, but unfortunately, that's a necessary evil.

[RolandPheasant]

I wonder whether we could introduce an analyser which warms about the dangers of binding to an amendable list. The consumer can turn it off, but at least they know.

[JakenVeina]

There's probably a variety of usage suggestions we could give, through an analyzer. I believe it can be bundled into the package as a dev-only dependency as well.

[RolandPheasant]

@timothylcooke #878 is my proposed solution, which also binds to IList but of course it will also need a non-sorted bind which can handle iList too.

[timothylcooke]

@RolandPheasant Definitely looks great! I'm excited to try it out. It really feels like this is the way to handle this.

So, for the times when I'm forced by some pesky control to use an IList<T>, I think every case is now covered. When binding from IChangeSet<TObject>, I can just use Clone, which works with or without sorting. When binding from IChangeSet<TObject,TKey>, I stopped using my BindToList and started using RemoveKey().Sort(comparer).Clone(list) once I found out about RemoveKey (this does have some side effects), but now it looks like I'll just be able to use .Clone(list) when order does not matter and BindAndSort(list, comparer) when it does. It seems my BindToList operator is now completely unnecessary.

Are you planning on making an equivalent operator on IChangeSet<TObject>? Would that provide a similar performance improvement?

Do you plan on adding any sort of analyzers to suggest not using the IList overload?

[RolandPheasant]

@timothylcooke can you proof read this for me please https://github.com/reactivemarbles/DynamicData/wiki/Sort-And-Bind-Operator

As well as my points in the wiki I should also include a bind overload that accepts and arbitrary instance of iList from the cache side, and probably rename ObservableList.Clone to be just Bind so we have consistency across the board.

[RolandPheasant]

SortAndBind has been release in v8.4.1

[timothylcooke]

@timothylcooke can you proof read this for me please https://github.com/reactivemarbles/DynamicData/wiki/Sort-And-Bind-Operator

Looks great. I made some tiny fixes, but all-in-all, I think it's very clear and easy to follow. Good work.

[RolandPheasant]

Thanks @timothylcooke. I am working on a version of SortAndBind which accepts an observable of comparers, meaning sort can be switched. When that's done, the new bind and sort will have functional parity with the prior version.

[timothylcooke]

@RolandPheasant I went through one of my projects yesterday and changed a lot of my cache.Sort().Bind() calls to cache.SortAndBind(). There was still one instance where I couldn't use the new operator because I needed to transform between sort and bind. I ended up having to keep it it was before: cache.RemoveKey().Sort().Transform().Bind(). I know I had previously suggested doing Sort, Transform, and Bind all in a single operation (perhaps you could add a Func<TIn, TOut> transform to an overload). I still think it's pretty niche, but if it's not too difficult to implement, it would be helpful in edge cases.

Maybe if there were a Bind that did Sort, Transform, and Bind in a single operation, but that feels overly complicated and niche.

[RolandPheasant]

That's great to hear. If you have any issues I'll get on the case asap.

RemoveKey() implies a list sort. List does not suffer the same issue as cache sort as items are stored by index and sorted state is preserved between operators. It therefore does not require a copy of the sorted state to be transmitted in the change set. In short, list sort will continue to exist / be supported but I also think for efficiency it could additionally have a bind and sort operator.

Regarding combing operators, I think it is generally a good idea. For example, I always transform a grouping immediately after creating a group. So surely better to have an overload on group that takes a transform.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.