From fb578dd7ee636440d68070d514f776829b59d04a Mon Sep 17 00:00:00 2001 From: Ryan Holden Date: Thu, 3 Sep 2020 11:35:08 -0400 Subject: [PATCH] Fix BindToObservableList: Previous values not being removed & sorting resets being ignored. (#401) * [Fix] Previous value not being removed when receiving an update change when the change is at a new index due to sorting. * Removed unnecessary CurrentIndex checks in RemoveKeyEnumerator * [Fix] Sorted change set with a reset not updating the `IObservableList` via `BindToObservableList` Co-authored-by: Ryan Holden --- .../IObservableListBindCacheSortedFixture.cs | 92 ++++++++++++++++--- src/DynamicData/Binding/IObservableListEx.cs | 23 ++++- .../Cache/Internal/RemoveKeyEnumerator.cs | 25 +---- 3 files changed, 104 insertions(+), 36 deletions(-) diff --git a/src/DynamicData.Tests/Binding/IObservableListBindCacheSortedFixture.cs b/src/DynamicData.Tests/Binding/IObservableListBindCacheSortedFixture.cs index ff4e27563..2963887db 100644 --- a/src/DynamicData.Tests/Binding/IObservableListBindCacheSortedFixture.cs +++ b/src/DynamicData.Tests/Binding/IObservableListBindCacheSortedFixture.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reactive.Linq; +using System.Reactive.Subjects; using DynamicData.Binding; using DynamicData.Tests.Domain; using FluentAssertions; @@ -11,12 +13,15 @@ namespace DynamicData.Tests.Binding public class IObservableListBindCacheSortedFixture : IDisposable { + private static readonly IComparer _comparerAgeAscThanNameAsc = SortExpressionComparer.Ascending(p => p.Age).ThenByAscending(p => p.Name); + private static readonly IComparer _comparerNameDesc = SortExpressionComparer.Descending(p => p.Name); + private readonly IObservableList _list; private readonly ChangeSetAggregator _listNotifications; private readonly ISourceCache _source; private readonly SortedChangeSetAggregator _sourceCacheNotifications; private readonly RandomPersonGenerator _generator = new RandomPersonGenerator(); - private readonly IComparer _comparer = SortExpressionComparer.Ascending(p => p.Age); + private readonly BehaviorSubject> _comparer = new BehaviorSubject>(_comparerAgeAscThanNameAsc); public IObservableListBindCacheSortedFixture() { @@ -24,7 +29,7 @@ public IObservableListBindCacheSortedFixture() _sourceCacheNotifications = _source .Connect() .AutoRefresh() - .Sort(_comparer, resetThreshold: 25) + .Sort(_comparer, resetThreshold: 10) .BindToObservableList(out _list) .AsAggregator(); @@ -38,6 +43,38 @@ public void Dispose() _source.Dispose(); } + [Fact] + public void InitialBindWithExistingData() + { + var source = new SourceCache(p => p.Name); + + // Populate source before binding + var person1 = new Person("Adult1", 20); + var person2 = new Person("Adult2", 30); + source.AddOrUpdate(person2); // Add out of order to assert intial order + source.AddOrUpdate(person1); + + var sourceCacheNotifications = source + .Connect() + .AutoRefresh() + .Sort(_comparer, resetThreshold: 10) + .BindToObservableList(out var list) + .AsAggregator(); + + var listNotifications = list.Connect().AsAggregator(); + + // Assert + listNotifications.Messages.Count().Should().Be(1); + listNotifications.Messages.First().First().Reason.Should().Be(ListChangeReason.AddRange); + list.Items.Should().Equal(new Person[] { person1, person2 }); + + // Clean up + source.Dispose(); + sourceCacheNotifications.Dispose(); + listNotifications.Dispose(); + list.Dispose(); + } + [Fact] public void AddToSourceAddsToDestination() { @@ -51,13 +88,18 @@ public void AddToSourceAddsToDestination() [Fact] public void UpdateToSourceUpdatesTheDestination() { - var person = new Person("Adult1", 50); - var personUpdated = new Person("Adult1", 51); - _source.AddOrUpdate(person); - _source.AddOrUpdate(personUpdated); + var person1 = new Person("Adult1", 20); + var person2 = new Person("Adult2", 30); + var personUpdated1 = new Person("Adult1", 40); - _list.Count.Should().Be(1, "Should be 1 item in the collection"); - _list.Items.First().Should().Be(personUpdated, "Should be updated person"); + _source.AddOrUpdate(person1); + _source.AddOrUpdate(person2); + + _list.Items.Should().Equal(new Person[] { person1, person2 }); + + _source.AddOrUpdate(personUpdated1); + + _list.Items.Should().Equal(new Person[] { person2, personUpdated1 }); } [Fact] @@ -73,11 +115,13 @@ public void RemoveSourceRemovesFromTheDestination() [Fact] public void BatchAdd() { - var people = _generator.Take(100).ToList(); + var people = _generator.Take(15).ToList(); _source.AddOrUpdate(people); - _list.Count.Should().Be(100, "Should be 100 items in the collection"); - _list.Should().BeEquivalentTo(_list, "Collections should be equivalent"); + var sorted = people.OrderBy(p => p, _comparerAgeAscThanNameAsc).ToList(); + + _list.Count.Should().Be(15, "Should be 15 items in the collection"); + _list.Items.Should().Equal(sorted, "Collections should be equivalent"); } [Fact] @@ -86,15 +130,35 @@ public void BatchRemove() var people = _generator.Take(100).ToList(); _source.AddOrUpdate(people); _source.Clear(); - _list.Count.Should().Be(0, "Should be 100 items in the collection"); + _list.Count.Should().Be(0, "Should be 0 items in the collection"); } [Fact] public void CollectionIsInSortOrder() { _source.AddOrUpdate(_generator.Take(100)); - var sorted = _source.Items.OrderBy(p => p, _comparer).ToList(); - sorted.Should().BeEquivalentTo(_list.Items); + var sorted = _source.Items.OrderBy(p => p, _comparerAgeAscThanNameAsc).ToList(); + sorted.Should().Equal(_list.Items); + } + + [Fact] + public void Reset() + { + var people = Enumerable.Range(1, 100).Select(i => new Person("P" + i, i)).ToArray(); + + _source.AddOrUpdate(people); + + _comparer.OnNext(_comparerNameDesc); + + var sorted = people.OrderBy(p => p, _comparerNameDesc).ToList(); + + _list.Items.Should().Equal(sorted); + + _listNotifications.Messages.Count().Should().Be(2); // Initial loading change set and a reset change due to a change over the reset threshold. + _listNotifications.Messages[0].First().Reason.Should().Be(ListChangeReason.AddRange);// initial loading + _listNotifications.Messages[1].Count.Should().Be(2);// Reset + _listNotifications.Messages[1].First().Reason.Should().Be(ListChangeReason.Clear); // reset + _listNotifications.Messages[1].Last().Reason.Should().Be(ListChangeReason.AddRange); // reset } [Fact] diff --git a/src/DynamicData/Binding/IObservableListEx.cs b/src/DynamicData/Binding/IObservableListEx.cs index d96d337bc..671b347da 100644 --- a/src/DynamicData/Binding/IObservableListEx.cs +++ b/src/DynamicData/Binding/IObservableListEx.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Reactive.Linq; namespace DynamicData.Binding @@ -102,7 +103,27 @@ public static IObservable> BindToObservableList< return Observable.Create>(observer => { return source - .Do(changes => sourceList.Edit(editor => editor.Clone(changes.RemoveKey(editor)))) + .Do(changes => + { + switch (changes.SortedItems.SortReason) + { + case SortReason.InitialLoad: + sourceList.AddRange(changes.SortedItems.Select(kv => kv.Value)); + break; + case SortReason.ComparerChanged: + case SortReason.DataChanged: + case SortReason.Reorder: + sourceList.Edit(editor => editor.Clone(changes.RemoveKey(editor))); + break; + case SortReason.Reset: + sourceList.Edit(editor => + { + editor.Clear(); + editor.AddRange(changes.SortedItems.Select(kv => kv.Value)); + }); + break; + } + }) .Finally(() => sourceList.Dispose()) .SubscribeSafe(observer); }); diff --git a/src/DynamicData/Cache/Internal/RemoveKeyEnumerator.cs b/src/DynamicData/Cache/Internal/RemoveKeyEnumerator.cs index e1b68118c..17ea4e090 100644 --- a/src/DynamicData/Cache/Internal/RemoveKeyEnumerator.cs +++ b/src/DynamicData/Cache/Internal/RemoveKeyEnumerator.cs @@ -50,7 +50,7 @@ public IEnumerator> GetEnumerator() // Thus, currentIndex will not be available here where as other changes like add and remove do have indexes if coming from a sorted changeset. // In order to properly handle a refresh and map to an index on a list, we need to use the source list (within the edit method so that it's thread safe) - if (_list != null && _list.IndexOf(change.Current) is int index && index >= 0) + if (_list?.IndexOf(change.Current) is int index && index >= 0) { yield return new Change(ListChangeReason.Refresh, current: change.Current, index: index); } @@ -65,28 +65,11 @@ public IEnumerator> GetEnumerator() yield return new Change(change.Current, change.CurrentIndex, change.PreviousIndex); break; case ChangeReason.Update: - // If not sorted - if (change.CurrentIndex == -1) - { - yield return new Change(ListChangeReason.Remove, change.Previous.Value); - yield return new Change(ListChangeReason.Add, change.Current); - } - else - { - yield return new Change(ListChangeReason.Remove, change.Current, index: change.CurrentIndex); - yield return new Change(ListChangeReason.Add, change.Current, index: change.CurrentIndex); - } + yield return new Change(ListChangeReason.Remove, change.Previous.Value, index: change.PreviousIndex); + yield return new Change(ListChangeReason.Add, change.Current, index: change.CurrentIndex); break; case ChangeReason.Remove: - // If not sorted - if (change.CurrentIndex == -1) - { - yield return new Change(ListChangeReason.Remove, change.Current); - } - else - { - yield return new Change(ListChangeReason.Remove, change.Current, index: change.CurrentIndex); - } + yield return new Change(ListChangeReason.Remove, change.Current, index: change.CurrentIndex); break; } }