Skip to content

Commit

Permalink
Fixed that non-observable objects within a property expression used b…
Browse files Browse the repository at this point in the history
…y `.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.
  • Loading branch information
JakenVeina authored Dec 5, 2023
1 parent 1716838 commit bb8b5dc
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Reactive.Linq;
Expand Down Expand Up @@ -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<PropertyValue<ClassC, int>>();
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<PropertyValue<ClassC, int>>();
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()
{
Expand Down Expand Up @@ -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}";
}
}
17 changes: 4 additions & 13 deletions src/DynamicData/Binding/ExpressionBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -33,20 +34,10 @@ internal static Func<object, IObservable<Unit>> CreatePropertyChangedFactory(thi

var notifyPropertyChanged = typeof(INotifyPropertyChanged).GetTypeInfo().IsAssignableFrom(property.DeclaringType.GetTypeInfo());

return t =>
{
if (t is null)
{
return Observable<Unit>.Never;
}

if (!notifyPropertyChanged)
{
return Observable.Return(Unit.Default);
}
return t => ((t is null) || !notifyPropertyChanged)
? Observable<Unit>.Never

return Observable.FromEventPattern<PropertyChangedEventHandler, PropertyChangedEventArgs>(handler => ((INotifyPropertyChanged)t).PropertyChanged += handler, handler => ((INotifyPropertyChanged)t).PropertyChanged -= handler).Where(args => args.EventArgs.PropertyName == property.Name).Select(_ => Unit.Default);
};
: Observable.FromEventPattern<PropertyChangedEventHandler, PropertyChangedEventArgs>(handler => ((INotifyPropertyChanged)t).PropertyChanged += handler, handler => ((INotifyPropertyChanged)t).PropertyChanged -= handler).Where(args => args.EventArgs.PropertyName == property.Name).Select(_ => Unit.Default);
}

internal static Func<object, object> CreateValueAccessor(this MemberExpression source)
Expand Down

0 comments on commit bb8b5dc

Please sign in to comment.