From bb8b5dc60567e10a83800c79c4c15d77d877009b Mon Sep 17 00:00:00 2001 From: Jake Meiergerd Date: Mon, 4 Dec 2023 21:39:02 -0600 Subject: [PATCH] Fixed that non-observable objects within a property expression used by `.WhenPropertyChanged()` were incorrectly simulating notifications of property changes, immediately upon subscription, resulting in infinite looping, as a side-effect of the implementation for capturing object changes by re-subscribing to the inner change stream upon any changes in the expression. (#774) Fixes #671. --- ...eeplyNestedNotifyPropertyChangedFixture.cs | 111 ++++++++++++++++++ src/DynamicData/Binding/ExpressionBuilder.cs | 17 +-- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/DynamicData.Tests/Binding/DeeplyNestedNotifyPropertyChangedFixture.cs b/src/DynamicData.Tests/Binding/DeeplyNestedNotifyPropertyChangedFixture.cs index 90a820e6d..c39d31b6c 100644 --- a/src/DynamicData.Tests/Binding/DeeplyNestedNotifyPropertyChangedFixture.cs +++ b/src/DynamicData.Tests/Binding/DeeplyNestedNotifyPropertyChangedFixture.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Reactive.Linq; @@ -35,6 +36,94 @@ public void DepthOfOne() result.Should().Be("NotNull"); } + // Covers https://github.com/reactivemarbles/DynamicData/issues/671 + [Fact] + public void NonObservableChildWithInitialValue() + { + var source = new ClassC() + { + Child = new() + { + Value = 1 + } + }; + + var notifications = new List>(); + var error = null as Exception; + var isCompleted = false; + + using var subscription = source + .WhenPropertyChanged(x => x.Child!.Value, notifyOnInitialValue: true) + .Subscribe( + onNext: notifications.Add, + onError: e => error = e, + onCompleted: () => isCompleted = true); + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Count.Should().Be(1, "a notification was requested for the initial value"); + notifications[0].Value.Should().Be(source.Child!.Value, "the child object's data value should have been published"); + + source.Child.Value = 2; + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Count.Should().Be(1, "the object that was changed does not publish notifications"); + + source.Child = new() + { + Value = 3 + }; + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Count.Should().Be(2, "the parent object should have published a notification for its child being changed"); + notifications[1].Value.Should().Be(source.Child!.Value, "the child object's data value should have been published"); + } + + [Fact] + public void NonObservableChildWithoutInitialValue() + { + var source = new ClassC() + { + Child = new() + { + Value = 1 + } + }; + + var notifications = new List>(); + var error = null as Exception; + var isCompleted = false; + + using var subscription = source + .WhenPropertyChanged(x => x.Child!.Value, notifyOnInitialValue: false) + .Subscribe( + onNext: notifications.Add, + onError: e => error = e, + onCompleted: () => isCompleted = true); + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Should().BeEmpty("no changes have been made"); + + source.Child.Value = 2; + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Should().BeEmpty("the object that was changed does not publish notifications"); + + source.Child = new() + { + Value = 3 + }; + + error.Should().BeNull(); + isCompleted.Should().BeFalse(); + notifications.Count.Should().Be(1, "the parent object should have published a notification for its child being changed"); + notifications[0].Value.Should().Be(source.Child!.Value, "the child object's data value should have been published"); + } + [Fact] public void NotifiesInitialValue_WithFallback() { @@ -347,4 +436,26 @@ public override string ToString() return $"{nameof(Age)}: {Age}"; } } + + public class ClassC : AbstractNotifyPropertyChanged + { + private ClassD? _classD; + + public ClassD? Child + { + get => _classD; + set => SetAndRaise(ref _classD, value); + } + + public override string ToString() + => $"ClassC: {nameof(Child)}={Child}"; + } + + public class ClassD + { + public int Value { get; set; } + + public override string ToString() + => $"ClassD: {nameof(Value)}={Value}"; + } } diff --git a/src/DynamicData/Binding/ExpressionBuilder.cs b/src/DynamicData/Binding/ExpressionBuilder.cs index e966e2fa1..96373c0c9 100644 --- a/src/DynamicData/Binding/ExpressionBuilder.cs +++ b/src/DynamicData/Binding/ExpressionBuilder.cs @@ -5,6 +5,7 @@ using System.ComponentModel; using System.Linq.Expressions; using System.Reactive; +using System.Reactive.Disposables; using System.Reactive.Linq; using System.Reflection; @@ -33,20 +34,10 @@ internal static Func> CreatePropertyChangedFactory(thi var notifyPropertyChanged = typeof(INotifyPropertyChanged).GetTypeInfo().IsAssignableFrom(property.DeclaringType.GetTypeInfo()); - return t => - { - if (t is null) - { - return Observable.Never; - } - - if (!notifyPropertyChanged) - { - return Observable.Return(Unit.Default); - } + return t => ((t is null) || !notifyPropertyChanged) + ? Observable.Never - return Observable.FromEventPattern(handler => ((INotifyPropertyChanged)t).PropertyChanged += handler, handler => ((INotifyPropertyChanged)t).PropertyChanged -= handler).Where(args => args.EventArgs.PropertyName == property.Name).Select(_ => Unit.Default); - }; + : Observable.FromEventPattern(handler => ((INotifyPropertyChanged)t).PropertyChanged += handler, handler => ((INotifyPropertyChanged)t).PropertyChanged -= handler).Where(args => args.EventArgs.PropertyName == property.Name).Select(_ => Unit.Default); } internal static Func CreateValueAccessor(this MemberExpression source)