[Regression] Exception when using .Sort with comparerChanged overload and AddRange

[bruzkovsky]

After upgrading from DynamicData 6.x to 7.x, my ListViews (Xamarin.Forms) were suddenly empty. Fortunately there was an Exception thrown and with that information I could narrow the problem down to the following:

When using .Sort(IObservable<IComparer<T>> comparerChanged), then an initial .AddRange will basically be ignored, unless observerChanged ticked at least one value or T implements IComparable.

You can repro this with the following small example:
ReproDynamicDataSort.zip

var source = new SourceList<ListItem>();
var sorter = new Subject<IComparer<ListItem>>();

source.Connect()
    .Sort(sorter)
    .Bind(out var bound)
    .Subscribe();

source.AddRange(Enumerable.Range(1, 10).Select(i => new ListItem(i)));
sorter.OnNext(SortExpressionComparer<ListItem>.Descending(x => x.Number));

The sample is really simple, it creates a SourceList, passes the sorter and then adds 10 items. The key part is that the sorter does not tick before the items are added, e.g. switching the last 2 lines will make the code work. Unfortunately, I have cases where I do not know the order beforehand, so I cannot do something like this in my production code.

It seems that since DynamicData 7.x, the internal comparer is initialized with Comparer<T>.Default, which needs the items to implement IComparable. Whereas in DynamicData 6.x, there was a fallback for when there is no comparer available. From my point of view, it should be sufficient to implement that fallback again.

[RolandPheasant]

I was not aware that anything has changed with sort for quite some time. I'll take a look at the source code history. Also it has always been by design to not tick until a comparer is specified. The easy solution is to use BehaviorSubject.

[bruzkovsky]

Maybe that's because we are really slow at updating dependencies :-D The package is available since November 2020...

Also it has always been by design to not tick until a comparer is specified.

That's how it was in 6.17.14, but in 7.1.1 it throws an Exception and there are no items ticked, even after the comparer is specified. When you run the sample, there are no items written to the console at the end.

For this sample, when using a BehaviorSubject and a pseudo-comparer, I get the same behavior as with 6.17.14 without using a BehaviorSubject:

ReproDynamicDataSort.zip

var sorter = new BehaviorSubject<IComparer<ListItem>>(new NullComparer());
//...
internal class NullComparer : IComparer<ListItem>
{
    public int Compare(ListItem x, ListItem y)
    {
        return 0;
    }
}

Output

Hello World!

Before sort:
Item 1
Item 2
Item 3
Item 4
Item 5
Item 6
Item 7
Item 8
Item 9
Item 10

After sort:
Item 10
Item 9
Item 8
Item 7
Item 6
Item 5
Item 4
Item 3
Item 2
Item 1

[bruzkovsky]

I tried to fix this with a BehaviorSubject in our production app, but it still failed. I updated the repro slightly to highlight the issue.:
ReproDynamicDataSort.zip

When adding the items before connecting, even the BehaviorSubject is "too late" to tick the comparer and the Exception is thrown again. We create dynamic groups (based on the XamarinFormsGrouping Snippet) and pass the comparer inside to also sort the items inside the group. The list is always filled before the group is created, so I don't think we can do much else on this front.

I also tried implementing my own .Sort operator with a IComparer<T> fallbackComparer, but the Sort class is internal....

[RolandPheasant]

Thanks for the reproduction.

The reason as you suggest for the error is timing. Within the sort code, the changes are subscribed to before the comparer changed stream, which means that the code tries to sort using the default object comparer which expects the object to implement IComparable.

A simple solution around this would be to implement IComparable:

    internal class ListItem: IComparable<ListItem>
    {
        public static readonly IComparer<ListItem> DefaultComparer = SortExpressionComparer<ListItem>.Ascending(x => x.Number);

        public int Number { get; }

        public ListItem(int number)
        {
            Number = number;
        }

        public int CompareTo(ListItem other) => DefaultComparer.Compare(this, other);
    }

The fix within Dynamic Data will be to subscribe to the comparer changes before subscribing to item changes. I'll try this out and if it works will merge it.

[RolandPheasant]

Doh, I did the investigation after reading the first comment. Now I have scrolled down and seen that you had worked out the same. In case you did not know the "pseduo-comparer" method is exactly what all primitives implement. That's why for the error would not occur if you are sorting built in structures.

What I am still unsure of is why the error only occurs for you after an upgrade,

[bruzkovsky]

the "pseduo-comparer" method is exactly what all primitives implement

Interesting, I didn't know that! I'll try implementing IComparable then.

It looks like this line is the culprit: src/DynamicData/List/Internal/Sort.cs#L34

Also, this seems to be only regarding List, not Cache: src/DynamicData/Cache/Internal/Sort.cs

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