From d2cc993a3eed06cbbd157c772372d5443590f115 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 15 Oct 2015 21:12:34 +0200 Subject: [PATCH] Fixed DataContext binding problem. Described by XamlBindingTest.Should_Not_Write_To_Old_DataContext. Problem still remains with ListBox.SelectedItems however, that's going to require something more. --- .../Binding/XamlBinding.cs | 27 +++---- .../Binding/ExpressionObserver.cs | 53 ++++++------- .../PerspexObjectTests_Binding.cs | 17 ++++ .../ExpressionObserverTests_Property.cs | 14 ++-- .../XamlBindingTest.cs | 77 +++++++++++++++++-- 5 files changed, 133 insertions(+), 55 deletions(-) diff --git a/src/Markup/Perspex.Markup.Xaml/Binding/XamlBinding.cs b/src/Markup/Perspex.Markup.Xaml/Binding/XamlBinding.cs index a77b4b4c2a..61b6206f4e 100644 --- a/src/Markup/Perspex.Markup.Xaml/Binding/XamlBinding.cs +++ b/src/Markup/Perspex.Markup.Xaml/Binding/XamlBinding.cs @@ -43,26 +43,17 @@ namespace Perspex.Markup.Xaml.Binding IObservablePropertyBag instance, PerspexProperty property) { - IObservable dataContext = null; + var dataContextHost = property != Control.DataContextProperty ? + instance : + instance.InheritanceParent as IObservablePropertyBag; - if (property != Control.DataContextProperty) + if (dataContextHost != null) { - dataContext = instance.GetObservable(Control.DataContextProperty); - } - else - { - var parent = instance.InheritanceParent as IObservablePropertyBag; - - if (parent != null) - { - dataContext = parent.GetObservable(Control.DataContextProperty); - } - } - - if (dataContext != null) - { - var result = new ExpressionObserver(null, SourcePropertyPath); - dataContext.Subscribe(x => result.Root = x); + var result = new ExpressionObserver( + () => dataContextHost.GetValue(Control.DataContextProperty), + SourcePropertyPath); + dataContextHost.GetObservable(Control.DataContextProperty).Subscribe(x => + result.UpdateRoot()); return result; } diff --git a/src/Markup/Perspex.Markup/Binding/ExpressionObserver.cs b/src/Markup/Perspex.Markup/Binding/ExpressionObserver.cs index e3141c5d4c..f70106e632 100644 --- a/src/Markup/Perspex.Markup/Binding/ExpressionObserver.cs +++ b/src/Markup/Perspex.Markup/Binding/ExpressionObserver.cs @@ -12,7 +12,7 @@ namespace Perspex.Markup.Binding /// public class ExpressionObserver : ObservableBase, IDescription { - private object _root; + private Func _root; private int _count; private ExpressionNode _node; @@ -22,7 +22,20 @@ namespace Perspex.Markup.Binding /// The root object. /// The expression. public ExpressionObserver(object root, string expression) + : this(() => root, expression) { + } + + /// + /// Initializes a new instance of the class. + /// + /// A function which gets the root object. + /// The expression. + public ExpressionObserver(Func root, string expression) + { + Contract.Requires(root != null); + Contract.Requires(expression != null); + _root = root; _node = ExpressionNodeBuilder.Build(expression); Expression = expression; @@ -39,6 +52,7 @@ namespace Perspex.Markup.Binding public bool SetValue(object value) { IncrementCount(); + UpdateRoot(); try { @@ -55,30 +69,6 @@ namespace Perspex.Markup.Binding /// public string Expression { get; } - /// - /// Gets or sets the root object that the expression is being observed on. - /// - public object Root - { - get - { - return _root; - } - - set - { - if (_root != value) - { - _root = value; - - if (_count > 0) - { - _node.Target = _root; - } - } - } - } - /// /// Gets the type of the expression result or null if the expression could not be /// evaluated. @@ -116,6 +106,17 @@ namespace Perspex.Markup.Binding } } + /// + /// Causes the root object to be re-read. + /// + public void UpdateRoot() + { + if (_count > 0) + { + _node.Target = _root(); + } + } + /// protected override IDisposable SubscribeCore(IObserver observer) { @@ -134,7 +135,7 @@ namespace Perspex.Markup.Binding { if (_count++ == 0) { - _node.Target = Root; + _node.Target = _root(); } } diff --git a/tests/Perspex.Base.UnitTests/PerspexObjectTests_Binding.cs b/tests/Perspex.Base.UnitTests/PerspexObjectTests_Binding.cs index 49534082a9..e54d4089a5 100644 --- a/tests/Perspex.Base.UnitTests/PerspexObjectTests_Binding.cs +++ b/tests/Perspex.Base.UnitTests/PerspexObjectTests_Binding.cs @@ -34,6 +34,20 @@ namespace Perspex.Base.UnitTests Assert.Equal("initial", target.GetValue(Class1.FooProperty)); } + [Fact] + public void Bind_To_ValueType_Accepts_UnsetValue() + { + var target = new Class1(); + var source = new Subject(); + + target.Bind(Class1.QuxProperty, source); + source.OnNext(6.7); + source.OnNext(PerspexProperty.UnsetValue); + + Assert.Equal(5.6, target.GetValue(Class1.QuxProperty)); + Assert.False(target.IsSet(Class1.QuxProperty)); + } + [Fact] public void Bind_Throws_Exception_For_Unregistered_Property() { @@ -277,6 +291,9 @@ namespace Perspex.Base.UnitTests public static readonly PerspexProperty BazProperty = PerspexProperty.Register("Baz", "bazdefault", true); + + public static readonly PerspexProperty QuxProperty = + PerspexProperty.Register("Qux", 5.6); } private class Class2 : Class1 diff --git a/tests/Perspex.Markup.UnitTests/Binding/ExpressionObserverTests_Property.cs b/tests/Perspex.Markup.UnitTests/Binding/ExpressionObserverTests_Property.cs index 2e1608a4b6..072a9faa74 100644 --- a/tests/Perspex.Markup.UnitTests/Binding/ExpressionObserverTests_Property.cs +++ b/tests/Perspex.Markup.UnitTests/Binding/ExpressionObserverTests_Property.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Reactive.Linq; +using System.Reactive.Subjects; using Perspex.Markup.Binding; using Xunit; @@ -227,7 +228,7 @@ namespace Perspex.Markup.UnitTests.Binding [Fact] public async void Should_Handle_Null_Root() { - var target = new ExpressionObserver(null, "Foo"); + var target = new ExpressionObserver((object)null, "Foo"); var result = await target.Take(1); Assert.Equal(PerspexProperty.UnsetValue, result); @@ -238,12 +239,15 @@ namespace Perspex.Markup.UnitTests.Binding { var first = new Class1 { Foo = "foo" }; var second = new Class1 { Foo = "bar" }; - var target = new ExpressionObserver(first, "Foo"); + var root = first; + var target = new ExpressionObserver(() => root, "Foo"); var result = new List(); - var sub = target.Subscribe(x => result.Add(x)); - target.Root = second; - target.Root = null; + + root = second; + target.UpdateRoot(); + root = null; + target.UpdateRoot(); Assert.Equal(new[] { "foo", "bar", PerspexProperty.UnsetValue }, result); diff --git a/tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs b/tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs index 6e0d14d5cf..ef286107ee 100644 --- a/tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs +++ b/tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs @@ -1,21 +1,86 @@ // Copyright (c) The Perspex Project. All rights reserved. // Licensed under the MIT license. See licence.md file in the project root for full license information. -using Moq; +using System; +using Perspex.Controls; using Perspex.Markup.Xaml.Binding; -using OmniXaml.TypeConversion; using Xunit; namespace Perspex.Xaml.Base.UnitTest { public class XamlBindingTest { + /// + /// Tests a problem discovered with ListBox with selection. + /// + /// + /// - Items is bound to DataContext first, followed by say SelectedIndex + /// - When the ListBox is removed from the visual tree, DataContext becomes null (as it's + /// inherited) + /// - This changes Items to null, which changes SelectedIndex to null as there are no + /// longer any items + /// - However, the news that DataContext is now null hasn't yet reached the SelectedIndex + /// binding and so the unselection is sent back to the ViewModel + /// [Fact] - public void TestNullDataContext() + public void Should_Not_Write_To_Old_DataContext() { - //var t = new Mock(); - //var sut = new XamlBinding(t.Object); - //sut.BindTo(null); + var vm = new OldDataContextViewModel(); + var target = new OldDataContextTest(); + + var fooBinding = new XamlBinding + { + SourcePropertyPath = "Foo", + BindingMode = BindingMode.TwoWay, + }; + + var barBinding = new XamlBinding + { + SourcePropertyPath = "Bar", + BindingMode = BindingMode.TwoWay, + }; + + // Bind Foo and Bar to the VM. + fooBinding.Bind(target, OldDataContextTest.FooProperty); + barBinding.Bind(target, OldDataContextTest.BarProperty); + target.DataContext = vm; + + // Make sure the control's Foo and Bar properties are read from the VM + Assert.Equal(1, target.GetValue(OldDataContextTest.FooProperty)); + Assert.Equal(2, target.GetValue(OldDataContextTest.BarProperty)); + + // Set DataContext to null. + target.DataContext = null; + + // Foo and Bar are no longer bound so they return 0, their default value. + Assert.Equal(0, target.GetValue(OldDataContextTest.FooProperty)); + Assert.Equal(0, target.GetValue(OldDataContextTest.BarProperty)); + + // The problem was here - DataContext is now null, setting Foo to 0. Bar is bound to + // Foo so Bar also gets set to 0. However the Bar binding still had a reference to + // the VM and so vm.Bar was set to 0 erroneously. + Assert.Equal(1, vm.Foo); + Assert.Equal(2, vm.Bar); + } + + private class OldDataContextViewModel + { + public int Foo { get; set; } = 1; + public int Bar { get; set; } = 2; + } + + private class OldDataContextTest : Control + { + public static readonly PerspexProperty FooProperty = + PerspexProperty.Register("Foo"); + + public static readonly PerspexProperty BarProperty = + PerspexProperty.Register("Bar"); + + public OldDataContextTest() + { + Bind(BarProperty, GetObservable(FooProperty)); + } } } }