diff --git a/src/Markup/Perspex.Markup.Xaml/Data/Binding.cs b/src/Markup/Perspex.Markup.Xaml/Data/Binding.cs index b220fb4e46..20cab81984 100644 --- a/src/Markup/Perspex.Markup.Xaml/Data/Binding.cs +++ b/src/Markup/Perspex.Markup.Xaml/Data/Binding.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; +using System.Reactive; using System.Reactive.Linq; using System.Reactive.Subjects; using Perspex.Controls; @@ -216,15 +217,13 @@ namespace Perspex.Markup.Xaml.Data if (!targetIsDataContext) { + var update = target.GetObservable(Control.DataContextProperty) + .Skip(1) + .Select(_ => Unit.Default); var result = new ExpressionObserver( () => target.GetValue(Control.DataContextProperty), - path); - - /// TODO: Instead of doing this, make the ExpressionObserver accept an "update" - /// observable as doing it this way can will cause a leak in Binding as this - /// observable is never unsubscribed. - target.GetObservable(Control.DataContextProperty).Subscribe(x => - result.UpdateRoot()); + path, + update); return result; } @@ -245,19 +244,16 @@ namespace Perspex.Markup.Xaml.Data { Contract.Requires(target != null); + var update = target.GetObservable(Control.TemplatedParentProperty) + .Skip(1) + .Where(x => x != null) + .Take(1) + .Select(_ => Unit.Default); + var result = new ExpressionObserver( () => target.GetValue(Control.TemplatedParentProperty), - path); - - if (target.GetValue(Control.TemplatedParentProperty) == null) - { - // TemplatedParent should only be set once, so only listen for the first non-null - // value. - target.GetObservable(Control.TemplatedParentProperty) - .Where(x => x != null) - .Take(1) - .Subscribe(x => result.UpdateRoot()); - } + path, + update); return result; } diff --git a/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs b/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs index 791a788df5..d3c2146bf2 100644 --- a/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs +++ b/src/Markup/Perspex.Markup/Data/ExpressionObserver.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Reactive; using System.Reactive.Disposables; +using System.Reactive.Linq; using System.Reactive.Subjects; using Perspex.Markup.Data.Plugins; @@ -29,10 +30,11 @@ namespace Perspex.Markup.Data private readonly object _root; private readonly Func _rootGetter; private readonly IObservable _rootObservable; + private readonly IObservable _update; private IDisposable _rootObserverSubscription; + private IDisposable _updateSubscription; private int _count; private readonly ExpressionNode _node; - private ISubject _empty; /// /// Initializes a new instance of the class. @@ -78,12 +80,18 @@ namespace Perspex.Markup.Data /// /// A function which gets the root object. /// The expression. - public ExpressionObserver(Func rootGetter, string expression) + /// An observable which triggers a re-read of the getter. + public ExpressionObserver( + Func rootGetter, + string expression, + IObservable update) { Contract.Requires(rootGetter != null); Contract.Requires(expression != null); + Contract.Requires(update != null); _rootGetter = rootGetter; + _update = update; if (!string.IsNullOrWhiteSpace(expression)) { @@ -104,7 +112,11 @@ namespace Perspex.Markup.Data public bool SetValue(object value) { IncrementCount(); - UpdateRoot(); + + if (_rootGetter != null && _node != null) + { + _node.Target = _rootGetter(); + } try { @@ -174,26 +186,6 @@ namespace Perspex.Markup.Data } } - /// - /// Causes the root object to be re-read from the root getter. - /// - /// TODO: Instead of doing this, make the object accept an "update" observable - /// as doing it this way can cause a leak in Binding. - public void UpdateRoot() - { - if (_count > 0 && _rootGetter != null) - { - if (_node != null) - { - _node.Target = _rootGetter(); - } - else - { - _empty?.OnNext(_rootGetter()); - } - } - } - /// protected override IDisposable SubscribeCore(IObserver observer) { @@ -211,12 +203,17 @@ namespace Perspex.Markup.Data } else { - if (_empty == null) + if (_update == null) { - _empty = new BehaviorSubject(_rootGetter()); + return Observable.Never().StartWith(_root).Subscribe(observer); + } + else + { + return _update + .Select(_ => _rootGetter()) + .StartWith(_rootGetter()) + .Subscribe(observer); } - - return _empty.Subscribe(observer); } } @@ -227,6 +224,11 @@ namespace Perspex.Markup.Data if (_rootGetter != null) { _node.Target = _rootGetter(); + + if (_update != null) + { + _updateSubscription = _update.Subscribe(x => _node.Target = _rootGetter()); + } } else if (_rootObservable != null) { @@ -249,6 +251,12 @@ namespace Perspex.Markup.Data _rootObserverSubscription = null; } + if (_updateSubscription != null) + { + _updateSubscription.Dispose(); + _updateSubscription = null; + } + _node.Target = null; } } diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs index 22373193c3..cfbc4bc29f 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs @@ -46,6 +46,25 @@ namespace Perspex.Markup.UnitTests.Data Assert.NotEqual(Subscription.Infinite, source.Subscriptions[0].Unsubscribe); } + [Fact] + public void Should_Unsubscribe_From_Update_Observable() + { + var scheduler = new TestScheduler(); + var update = scheduler.CreateColdObservable(); + var target = new ExpressionObserver(() => new { Foo = "foo" }, "Foo", update); + var result = new List(); + + using (target.Subscribe(x => result.Add(x))) + using (target.Subscribe(_ => { })) + { + scheduler.Start(); + } + + Assert.Equal(new[] { "foo" }, result); + Assert.Equal(1, update.Subscriptions.Count); + Assert.NotEqual(Subscription.Infinite, update.Subscriptions[0].Unsubscribe); + } + [Fact] public void Should_Set_Node_Target_To_Null_On_Unsubscribe() { diff --git a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs index 0d917b15b4..fe9336ebe6 100644 --- a/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs +++ b/tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Reactive; using System.Reactive.Linq; +using System.Reactive.Subjects; using Microsoft.Reactive.Testing; using Perspex.Markup.Data; using Xunit; @@ -193,13 +194,14 @@ namespace Perspex.Markup.UnitTests.Data public void Empty_Expression_Should_Track_Root() { var data = new Class1 { Foo = "foo" }; - var target = new ExpressionObserver(() => data.Foo, ""); + var update = new Subject(); + var target = new ExpressionObserver(() => data.Foo, "", update); var result = new List(); target.Subscribe(x => result.Add(x)); data.Foo = "bar"; - target.UpdateRoot(); + update.OnNext(Unit.Default); Assert.Equal(new[] { "foo", "bar" }, result); } @@ -286,14 +288,15 @@ namespace Perspex.Markup.UnitTests.Data var first = new Class1 { Foo = "foo" }; var second = new Class1 { Foo = "bar" }; var root = first; - var target = new ExpressionObserver(() => root, "Foo"); + var update = new Subject(); + var target = new ExpressionObserver(() => root, "Foo", update); var result = new List(); var sub = target.Subscribe(x => result.Add(x)); root = second; - target.UpdateRoot(); + update.OnNext(Unit.Default); root = null; - target.UpdateRoot(); + update.OnNext(Unit.Default); Assert.Equal(new[] { "foo", "bar", PerspexProperty.UnsetValue }, result); diff --git a/tests/Perspex.Markup.UnitTests/Perspex.Markup.UnitTests.csproj b/tests/Perspex.Markup.UnitTests/Perspex.Markup.UnitTests.csproj index 639142b00e..161e79f6f6 100644 --- a/tests/Perspex.Markup.UnitTests/Perspex.Markup.UnitTests.csproj +++ b/tests/Perspex.Markup.UnitTests/Perspex.Markup.UnitTests.csproj @@ -84,7 +84,7 @@ - +