Fix sort exception when using binary search (#984)

* Fix sort exception when using binary search

Author: RolandPheasant

src/DynamicData.Tests/Cache/SortAndBindFixture.cs

@@ -91,17 +91,57 @@ protected override (ChangeSetAggregator<Person, string> Aggregrator, IList<Perso
 }
 
 // Bind to a readonly observable collection using binary search
-public sealed class SortAndBindWithBinarySearch : SortAndBindFixture
+public sealed class SortAndBindWithBinarySearch1 : SortAndBindFixture
 {
     protected override (ChangeSetAggregator<Person, string> Aggregrator, IList<Person> List) SetUpTests()
     {
-        var options = new SortAndBindOptions { UseBinarySearch = true };
+        var options = new SortAndBindOptions { UseBinarySearch = true, UseReplaceForUpdates = false};
         var aggregator = _source.Connect().SortAndBind(out var list, _comparer, options).AsAggregator();
 
         return (aggregator, list);
     }
 }
 
+public sealed class SortAndBindWithBinarySearch2 : SortAndBindFixture
+{
+    protected override (ChangeSetAggregator<Person, string> Aggregrator, IList<Person> List) SetUpTests()
+    {
+        var options = new SortAndBindOptions { UseBinarySearch = true, UseReplaceForUpdates = true };
+        var aggregator = _source.Connect().SortAndBind(out var list, _comparer, options).AsAggregator();
+
+        return (aggregator, list);
+    }
+}
+
+public class SortAndBindBinarySearch_ForSameKeyAndObjectValues: IDisposable
+{
+    private readonly List<int> _target = new();
+    private readonly SourceCache<int, int> _strings = new(i=> i);
+
+    [Theory]
+    [InlineData(false)]
+    [InlineData(true)]
+    public void UpdateAnyWhereShouldNotBreak(bool useReplaceForUpdates)
+    {
+        var options = new SortAndBindOptions { UseBinarySearch = true, UseReplaceForUpdates = useReplaceForUpdates };
+
+        using var subscription = _strings.Connect().SortAndBind(_target, SortExpressionComparer<int>.Ascending(i=>i), options).Subscribe();
+
+        var items = Enumerable.Range(1, 10).ToList();
+
+        _strings.AddOrUpdate(items);
+        _strings.AddOrUpdate(1);
+        _strings.AddOrUpdate(5);
+        _strings.AddOrUpdate(10);
+
+        _target.SequenceEqual(items).Should().BeTrue();
+    }
+
+    public void Dispose() => _strings.Dispose();
+}
+
+
+
 // Bind to a readonly observable collection - using default comparer
 public sealed class SortAndBindToReadOnlyObservableCollectionDefaultComparer : SortAndBindFixture
 {
@@ -291,9 +331,9 @@ public void InsertAtEnd()
         last.Should().Be(toInsert);
 
         _boundList.SequenceEqual(_source.Items.OrderBy(p => p, _comparer)).Should().BeTrue();
-
     }
 
+
     [Fact]
     public void InsertInMiddle()
     {
@@ -568,8 +608,6 @@ public void UpdateFirst()
     [Fact]
     public void UpdateLast()
     {
-        //TODO: fixed Text
-
         var people = _generator.Take(100).ToList();
         _source.AddOrUpdate(people);
 
@@ -582,9 +620,6 @@ public void UpdateLast()
         int IndexFromKey(string key) => people.FindIndex(p => p.Key == key);
 
         people.OrderBy(p => p, _comparer).SequenceEqual(_boundList).Should().BeTrue();
-
-
-
     }
 
 

src/DynamicData/Binding/SortAndBind.cs

@@ -180,21 +180,35 @@ private void ApplyChanges(IChangeSet<TObject, TKey> changes)
                         break;
                     case ChangeReason.Update:
                         {
-                            var currentIndex = GetCurrentPosition(change.Previous.Value);
-                            var updatedIndex = GetInsertPosition(item);
+                            if (!options.UseReplaceForUpdates)
+                            {
+                                // If using binary search, it works best when we remove then add,
+                                // so let's optimise for that first.
 
-                            // We need to recalibrate as GetCurrentPosition includes the current item
-                            updatedIndex = currentIndex < updatedIndex ? updatedIndex - 1 : updatedIndex;
+                                var currentIndex = GetCurrentPosition(change.Previous.Value);
+                                target.RemoveAt(currentIndex);
 
-                            // Some control suites and platforms do not support replace, whiles others do, so we opt in.
-                            if (options.UseReplaceForUpdates && currentIndex == updatedIndex)
-                            {
-                                target[currentIndex] = item;
+                                var updatedIndex = GetInsertPosition(item);
+                                target.Insert(updatedIndex, item);
                             }
                             else
                             {
-                                target.RemoveAt(currentIndex);
-                                target.Insert(updatedIndex, item);
+                                var currentIndex = GetCurrentPosition(change.Previous.Value);
+                                var updatedIndex = GetInsertPosition(item);
+
+                                // We need to recalibrate as GetCurrentPosition includes the current item
+                                updatedIndex = currentIndex < updatedIndex ? updatedIndex - 1 : updatedIndex;
+
+                                // Some control suites and platforms do not support replace, whiles others do, so we opt in.
+                                if (currentIndex == updatedIndex)
+                                {
+                                    target[currentIndex] = item;
+                                }
+                                else
+                                {
+                                    target.RemoveAt(currentIndex);
+                                    target.Insert(updatedIndex, item);
+                                }
                             }
                         }
                         break;

src/DynamicData/Cache/Internal/SortExtensions.cs

@@ -35,6 +35,17 @@ public static int GetInsertPositionBinary<TItem>(this IList<TItem> list, TItem t
         // sort is not returning uniqueness
         if (insertIndex < 0)
         {
+            /*
+             * Binary search should not strictly already contain the item (or an item with the same value) when
+             * attempting to find the insert position (it does for updates).  This can result in the insert position not being found.
+             * In this case revert to linear search.
+             */
+            index = list.GetInsertPositionLinear(t, c);
+            if (index >= 0)
+            {
+                return index;
+            }
+
             throw new SortException("Binary search has been specified, yet the sort does not yield uniqueness");
         }