From d3179622d6b5c5ee30bfe1c73aa7e2f6ee958b89 Mon Sep 17 00:00:00 2001 From: Roland Pheasant Date: Tue, 12 Jul 2022 11:59:47 +0100 Subject: [PATCH] Fix OnBeingRemoved and option as to whether OnItemRemoved should be invoked upon subscription. Fixes #613 (#616) --- src/DynamicData.Tests/Cache/OnItemFixture.cs | 75 ++++++++++++++++++- .../Cache/Internal/OnBeingRemoved.cs | 17 ++--- src/DynamicData/Cache/ObservableCacheEx.cs | 7 +- .../List/Internal/OnBeingRemoved.cs | 10 ++- src/DynamicData/List/ObservableListEx.cs | 5 +- 5 files changed, 96 insertions(+), 18 deletions(-) diff --git a/src/DynamicData.Tests/Cache/OnItemFixture.cs b/src/DynamicData.Tests/Cache/OnItemFixture.cs index 6e2a704c5..52ec04bca 100644 --- a/src/DynamicData.Tests/Cache/OnItemFixture.cs +++ b/src/DynamicData.Tests/Cache/OnItemFixture.cs @@ -1,7 +1,10 @@ using System; - +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.ComponentModel; +using DynamicData.Binding; using DynamicData.Tests.Domain; - +using FluentAssertions; using Xunit; namespace DynamicData.Tests.Cache; @@ -52,6 +55,21 @@ public void OnItemRemovedCalled() Assert.True(called); } + [Fact] + [Description("Test for https://github.com/reactivemarbles/DynamicData/issues/613")] + public void OnItemRemovedNotCalledForUpdate() + { + var called = false; + var source = new SourceCache(x => x.Age); + + source.Connect().OnItemRemoved(_ => called = true).Subscribe(); + + source.AddOrUpdate(new Person("A", 1)); + source.AddOrUpdate(new Person("A", 2)); + + called.Should().Be(false); + } + [Fact] public void OnItemUpdatedCalled() { @@ -66,4 +84,57 @@ public void OnItemUpdatedCalled() source.AddOrUpdate(update); Assert.True(called); } + + [Fact] + [Description("Test for https://github.com/reactivemarbles/DynamicData/issues/268")] + public void ListAndCacheShouldHaveEquivalentBehaviour() + { + var source = new ObservableCollection + { + new Item { Id = 1 }, + new Item { Id = 2 } + }; + + var list = source.ToObservableChangeSet() + .Transform(item => new Proxy { Item = item }) + .OnItemAdded(proxy => proxy.Active = true) + .OnItemRemoved(proxy => proxy.Active = false) + .Bind(out var listOutput) + .Subscribe(); + + var cache = source.ToObservableChangeSet(item => item.Id) + .Transform(item => new Proxy { Item = item }) + .OnItemAdded(proxy => proxy.Active = true) + .OnItemRemoved(proxy => proxy.Active = false) + .Bind(out var cacheOutput) + .Subscribe(); + + Assert.Equal(listOutput, cacheOutput, new ProxyEqualityComparer()); + + list.Dispose(); + cache.Dispose(); + + Assert.Equal(listOutput, cacheOutput, new ProxyEqualityComparer()); + } + + public class Item + { + public int Id { get; set; } + } + + public class Proxy + { + public Item Item { get; set; } + + public bool? Active { get; set; } + + + } + + public class ProxyEqualityComparer : IEqualityComparer + { + public bool Equals(Proxy x, Proxy y) => x.Item.Id == y.Item.Id && x.Active == y.Active; + + public int GetHashCode(Proxy obj) => HashCode.Combine(obj.Active, obj.Item); + } } diff --git a/src/DynamicData/Cache/Internal/OnBeingRemoved.cs b/src/DynamicData/Cache/Internal/OnBeingRemoved.cs index 94b7f23d9..2fb7aace2 100644 --- a/src/DynamicData/Cache/Internal/OnBeingRemoved.cs +++ b/src/DynamicData/Cache/Internal/OnBeingRemoved.cs @@ -2,8 +2,6 @@ // Roland Pheasant licenses this file to you under the MIT license. // See the LICENSE file in the project root for full license information. -using System; -using System.Collections.Generic; using System.Reactive.Disposables; using System.Reactive.Linq; @@ -15,13 +13,15 @@ internal sealed class OnBeingRemoved where TKey : notnull { private readonly Action _removeAction; + private readonly bool _invokeOnUnsubscribe; private readonly IObservable> _source; - public OnBeingRemoved(IObservable> source, Action removeAction) + public OnBeingRemoved(IObservable> source, Action removeAction, bool invokeOnUnsubscribe) { _source = source ?? throw new ArgumentNullException(nameof(source)); _removeAction = removeAction ?? throw new ArgumentNullException(nameof(removeAction)); + _invokeOnUnsubscribe = invokeOnUnsubscribe; } public IObservable> Run() @@ -40,7 +40,11 @@ public IObservable> Run() lock (locker) { - cache.Items.ForEach(t => _removeAction(t)); + if (_invokeOnUnsubscribe) + { + cache.Items.ForEach(t => _removeAction(t)); + } + cache.Clear(); } }); @@ -54,11 +58,6 @@ private void RegisterForRemoval(IChangeSet changes, Cache _removeAction(t)); - break; - case ChangeReason.Remove: // ReSharper disable once InconsistentlySynchronizedField _removeAction(change.Current); diff --git a/src/DynamicData/Cache/ObservableCacheEx.cs b/src/DynamicData/Cache/ObservableCacheEx.cs index 23fcbf855..02473d886 100644 --- a/src/DynamicData/Cache/ObservableCacheEx.cs +++ b/src/DynamicData/Cache/ObservableCacheEx.cs @@ -1129,7 +1129,7 @@ public static IObservable> DisposeMany( throw new ArgumentNullException(nameof(source)); } - return new OnBeingRemoved( + return new DisposeMany( source, t => { @@ -2874,19 +2874,20 @@ public static IObservable> OnItemRefreshedThe type of the key. /// The source. /// The remove action. + /// Should the remove action be invoked when the subscription is disposed. /// An observable which emits a change set with items being removed. /// /// source /// or /// removeAction. /// - public static IObservable> OnItemRemoved(this IObservable> source, Action removeAction) + public static IObservable> OnItemRemoved(this IObservable> source, Action removeAction, bool invokeOnUnsubscribe = true) where TKey : notnull { if (source is null) throw new ArgumentNullException(nameof(source)); if (removeAction is null) throw new ArgumentNullException(nameof(removeAction)); - return new OnBeingRemoved(source, removeAction).Run(); + return new OnBeingRemoved(source, removeAction, invokeOnUnsubscribe).Run(); } /// diff --git a/src/DynamicData/List/Internal/OnBeingRemoved.cs b/src/DynamicData/List/Internal/OnBeingRemoved.cs index 2d4c86e6f..2b66cedc0 100644 --- a/src/DynamicData/List/Internal/OnBeingRemoved.cs +++ b/src/DynamicData/List/Internal/OnBeingRemoved.cs @@ -14,13 +14,15 @@ namespace DynamicData.List.Internal; internal sealed class OnBeingRemoved { private readonly Action _callback; + private readonly bool _invokeOnUnsubscribe; private readonly IObservable> _source; - public OnBeingRemoved(IObservable> source, Action callback) + public OnBeingRemoved(IObservable> source, Action callback, bool invokeOnUnsubscribe) { _source = source ?? throw new ArgumentNullException(nameof(source)); _callback = callback ?? throw new ArgumentNullException(nameof(callback)); + _invokeOnUnsubscribe = invokeOnUnsubscribe; } public IObservable> Run() @@ -36,7 +38,11 @@ public IObservable> Run() () => { subscriber.Dispose(); - items.ForEach(t => _callback(t)); + + if (_invokeOnUnsubscribe) + { + items.ForEach(t => _callback(t)); + } }); }); } diff --git a/src/DynamicData/List/ObservableListEx.cs b/src/DynamicData/List/ObservableListEx.cs index aa86b4370..b1e33ead6 100644 --- a/src/DynamicData/List/ObservableListEx.cs +++ b/src/DynamicData/List/ObservableListEx.cs @@ -1150,13 +1150,14 @@ public static IObservable> OnItemRefreshed(this IOb /// The type of the object. /// The source. /// The remove action. + /// Should the remove action be invoked when the subscription is disposed. /// An observable which emits the change set. /// /// source /// or /// removeAction. /// - public static IObservable> OnItemRemoved(this IObservable> source, Action removeAction) + public static IObservable> OnItemRemoved(this IObservable> source, Action removeAction, bool invokeOnUnsubscribe = true) { if (source is null) { @@ -1168,7 +1169,7 @@ public static IObservable> OnItemRemoved(this IObservable(source, removeAction).Run(); + return new OnBeingRemoved(source, removeAction, invokeOnUnsubscribe).Run(); } ///