Skip to content

Commit

Permalink
Fix BindToObservableList: Previous values not being removed & sorting…
Browse files Browse the repository at this point in the history 
… 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 <[email protected]>
  • Loading branch information
@ryanholden8
ryanholden8 and Ryan Holden authored Sep 3, 2020
1 parent 6594137 commit fb578dd
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 36 deletions.
92 changes: 78 additions & 14 deletions src/DynamicData.Tests/Binding/IObservableListBindCacheSortedFixture.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -11,20 +13,23 @@ namespace DynamicData.Tests.Binding

public class IObservableListBindCacheSortedFixture : IDisposable
{
private static readonly IComparer<Person> _comparerAgeAscThanNameAsc = SortExpressionComparer<Person>.Ascending(p => p.Age).ThenByAscending(p => p.Name);
private static readonly IComparer<Person> _comparerNameDesc = SortExpressionComparer<Person>.Descending(p => p.Name);

private readonly IObservableList<Person> _list;
private readonly ChangeSetAggregator<Person> _listNotifications;
private readonly ISourceCache<Person, string> _source;
private readonly SortedChangeSetAggregator<Person, string> _sourceCacheNotifications;
private readonly RandomPersonGenerator _generator = new RandomPersonGenerator();
private readonly IComparer<Person> _comparer = SortExpressionComparer<Person>.Ascending(p => p.Age);
private readonly BehaviorSubject<IComparer<Person>> _comparer = new BehaviorSubject<IComparer<Person>>(_comparerAgeAscThanNameAsc);

public IObservableListBindCacheSortedFixture()
{
_source = new SourceCache<Person, string>(p => p.Name);
_sourceCacheNotifications = _source
.Connect()
.AutoRefresh()
.Sort(_comparer, resetThreshold: 25)
.Sort(_comparer, resetThreshold: 10)
.BindToObservableList(out _list)
.AsAggregator();

Expand All @@ -38,6 +43,38 @@ public void Dispose()
_source.Dispose();
}

[Fact]
public void InitialBindWithExistingData()
{
var source = new SourceCache<Person, string>(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()
{
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand Down
23 changes: 22 additions & 1 deletion src/DynamicData/Binding/IObservableListEx.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Linq;
using System.Reactive.Linq;

namespace DynamicData.Binding
Expand Down Expand Up @@ -102,7 +103,27 @@ public static IObservable<ISortedChangeSet<TObject, TKey>> BindToObservableList<
return Observable.Create<ISortedChangeSet<TObject, TKey>>(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);
});
Expand Down
25 changes: 4 additions & 21 deletions src/DynamicData/Cache/Internal/RemoveKeyEnumerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public IEnumerator<Change<TObject>> 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<TObject>(ListChangeReason.Refresh, current: change.Current, index: index);
}
Expand All @@ -65,28 +65,11 @@ public IEnumerator<Change<TObject>> GetEnumerator()
yield return new Change<TObject>(change.Current, change.CurrentIndex, change.PreviousIndex);
break;
case ChangeReason.Update:
// If not sorted
if (change.CurrentIndex == -1)
{
yield return new Change<TObject>(ListChangeReason.Remove, change.Previous.Value);
yield return new Change<TObject>(ListChangeReason.Add, change.Current);
}
else
{
yield return new Change<TObject>(ListChangeReason.Remove, change.Current, index: change.CurrentIndex);
yield return new Change<TObject>(ListChangeReason.Add, change.Current, index: change.CurrentIndex);
}
yield return new Change<TObject>(ListChangeReason.Remove, change.Previous.Value, index: change.PreviousIndex);
yield return new Change<TObject>(ListChangeReason.Add, change.Current, index: change.CurrentIndex);
break;
case ChangeReason.Remove:
// If not sorted
if (change.CurrentIndex == -1)
{
yield return new Change<TObject>(ListChangeReason.Remove, change.Current);
}
else
{
yield return new Change<TObject>(ListChangeReason.Remove, change.Current, index: change.CurrentIndex);
}
yield return new Change<TObject>(ListChangeReason.Remove, change.Current, index: change.CurrentIndex);
break;
}
}
Expand Down

0 comments on commit fb578dd

Please sign in to comment.