From 61210f4a52142cc174e363096c0be843cb800826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Nieto=20S=C3=A1nchez?= Date: Mon, 7 Sep 2015 20:06:43 +0200 Subject: [PATCH] Fixed binding issue with weak references: https://github.com/Perspex/Perspex/issues/107 --- .../ChangeBranchTest.cs | 6 +- .../DataContextChangeSynchronizerTest.cs | 29 ++- .../XamlBindingTest.cs | 2 +- .../Context/PerspexXamlMemberValuePlugin.cs | 4 +- .../ObservablePropertyBranch.cs | 53 +++--- .../DataContextChangeSynchronizer.cs | 171 +++++++++++------- .../DataBinding/PerspexPropertyBinder.cs | 4 +- .../DataBinding/XamlBinding.cs | 41 ++--- 8 files changed, 174 insertions(+), 136 deletions(-) diff --git a/Tests/Perspex.Markup.Xaml.UnitTests/ChangeBranchTest.cs b/Tests/Perspex.Markup.Xaml.UnitTests/ChangeBranchTest.cs index 74aa0b9db3..c9d1a51ead 100644 --- a/Tests/Perspex.Markup.Xaml.UnitTests/ChangeBranchTest.cs +++ b/Tests/Perspex.Markup.Xaml.UnitTests/ChangeBranchTest.cs @@ -58,12 +58,12 @@ var level1 = new Level1(); var branch = new ObservablePropertyBranch(level1, new PropertyPath("Level2.Level3.Property")); - bool hit = false; - ObservableExtensions.Subscribe(branch.Changed, _ => hit = true); + bool received = false; + ObservableExtensions.Subscribe(branch.Values, v => received = ((int)v == 3)); level1.Level2.Level3.Property = 3; - Assert.True(hit); + Assert.True(received); } } } diff --git a/Tests/Perspex.Markup.Xaml.UnitTests/DataContextChangeSynchronizerTest.cs b/Tests/Perspex.Markup.Xaml.UnitTests/DataContextChangeSynchronizerTest.cs index 284227d0ed..1c8bb17cb7 100644 --- a/Tests/Perspex.Markup.Xaml.UnitTests/DataContextChangeSynchronizerTest.cs +++ b/Tests/Perspex.Markup.Xaml.UnitTests/DataContextChangeSynchronizerTest.cs @@ -27,8 +27,8 @@ [Fact] public void SameTypesFromUIToModel() { - var synchronizer = new DataContextChangeSynchronizer(guiObject, SamplePerspexObject.IntProperty, new PropertyPath("IntProp"), viewModel, repo); - synchronizer.SubscribeModelToUI(); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("IntProp"), viewModel), new DataContextChangeSynchronizer.BindingTarget(guiObject, SamplePerspexObject.IntProperty), repo); + synchronizer.StartUpdatingSourceWhenTargetChanges(); const int someValue = 4; guiObject.Int = someValue; @@ -39,8 +39,8 @@ [Fact] public void DifferentTypesFromUIToModel() { - var synchronizer = new DataContextChangeSynchronizer(guiObject, SamplePerspexObject.StringProperty, new PropertyPath("IntProp"), viewModel, repo); - synchronizer.SubscribeModelToUI(); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("IntProp"), viewModel), new DataContextChangeSynchronizer.BindingTarget(guiObject, SamplePerspexObject.StringProperty), repo); + synchronizer.StartUpdatingSourceWhenTargetChanges(); guiObject.String = "2"; @@ -50,8 +50,8 @@ [Fact] public void DifferentTypesAndNonConvertibleValueFromUIToModel() { - var synchronizer = new DataContextChangeSynchronizer(guiObject, SamplePerspexObject.StringProperty, new PropertyPath("IntProp"), viewModel, repo); - synchronizer.SubscribeModelToUI(); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("IntProp"), viewModel), new DataContextChangeSynchronizer.BindingTarget(guiObject, SamplePerspexObject.StringProperty), repo); + synchronizer.StartUpdatingSourceWhenTargetChanges(); guiObject.String = ""; @@ -62,8 +62,8 @@ [Fact] public void DifferentTypesFromModelToUI() { - var synchronizer = new DataContextChangeSynchronizer(guiObject, SamplePerspexObject.StringProperty, new PropertyPath("IntProp"), viewModel, repo); - synchronizer.SubscribeUIToModel(); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("IntProp"), viewModel), new DataContextChangeSynchronizer.BindingTarget(guiObject, SamplePerspexObject.StringProperty), repo); + synchronizer.StartUpdatingTargetWhenSourceChanges(); viewModel.IntProp = 2; @@ -73,8 +73,8 @@ [Fact] public void SameTypesFromModelToUI() { - var synchronizer = new DataContextChangeSynchronizer(guiObject, SamplePerspexObject.IntProperty, new PropertyPath("IntProp"), viewModel, repo); - synchronizer.SubscribeUIToModel(); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("IntProp"), viewModel), new DataContextChangeSynchronizer.BindingTarget(guiObject, SamplePerspexObject.IntProperty), repo); + synchronizer.StartUpdatingTargetWhenSourceChanges(); viewModel.IntProp = 2; @@ -87,14 +87,9 @@ var mainWindowViewModel = new MainWindowViewModel(); var contentControl = new ContentControl(); - var synchronizer = new DataContextChangeSynchronizer( - contentControl, - ContentControl.ContentProperty, - new PropertyPath("Content"), - mainWindowViewModel, - repo); + var synchronizer = new DataContextChangeSynchronizer(new DataContextChangeSynchronizer.BindingSource(new PropertyPath("Content"), mainWindowViewModel), new DataContextChangeSynchronizer.BindingTarget(contentControl, ContentControl.ContentProperty), repo); - synchronizer.SubscribeUIToModel(); + synchronizer.StartUpdatingTargetWhenSourceChanges(); var logInViewModel = new LogInViewModel(); mainWindowViewModel.Content = logInViewModel; diff --git a/Tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs b/Tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs index fd92757e91..f5c59b1f1a 100644 --- a/Tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs +++ b/Tests/Perspex.Markup.Xaml.UnitTests/XamlBindingTest.cs @@ -12,7 +12,7 @@ { var t = new Mock(); var sut = new XamlBinding(t.Object); - sut.Bind(null); + sut.BindToDataContext(null); } } } diff --git a/src/Markup/Perspex.Markup.Xaml/Context/PerspexXamlMemberValuePlugin.cs b/src/Markup/Perspex.Markup.Xaml/Context/PerspexXamlMemberValuePlugin.cs index e283ec63ff..c1ba8213c1 100644 --- a/src/Markup/Perspex.Markup.Xaml/Context/PerspexXamlMemberValuePlugin.cs +++ b/src/Markup/Perspex.Markup.Xaml/Context/PerspexXamlMemberValuePlugin.cs @@ -56,7 +56,7 @@ namespace Perspex.Markup.Xaml.Context private void HandlePerspexProperty(object instance, object value) { var pp = this.PerspexProperty; - var po = (PerspexObject) instance; + var po = (PerspexObject)instance; po.SetValue(pp, value); } @@ -75,7 +75,7 @@ namespace Perspex.Markup.Xaml.Context var dataContext = target.DataContext; var binding = this.propertyBinder.GetBinding(target, definition.TargetProperty); - binding.Bind(dataContext); + binding.BindToDataContext(dataContext); } // ReSharper disable once MemberCanBePrivate.Global diff --git a/src/Markup/Perspex.Markup.Xaml/DataBinding/ChangeTracking/ObservablePropertyBranch.cs b/src/Markup/Perspex.Markup.Xaml/DataBinding/ChangeTracking/ObservablePropertyBranch.cs index 2df9a766f3..6165a69a9e 100644 --- a/src/Markup/Perspex.Markup.Xaml/DataBinding/ChangeTracking/ObservablePropertyBranch.cs +++ b/src/Markup/Perspex.Markup.Xaml/DataBinding/ChangeTracking/ObservablePropertyBranch.cs @@ -16,41 +16,46 @@ namespace Perspex.Markup.Xaml.DataBinding.ChangeTracking public class ObservablePropertyBranch { - private readonly object root; + private readonly object instance; private readonly PropertyPath propertyPath; - private PropertyMountPoint mountPoint; + private readonly PropertyMountPoint mountPoint; - public ObservablePropertyBranch(object root, PropertyPath propertyPath) + public ObservablePropertyBranch(object instance, PropertyPath propertyPath) { - Guard.ThrowIfNull(root, nameof(root)); + Guard.ThrowIfNull(instance, nameof(instance)); Guard.ThrowIfNull(propertyPath, nameof(propertyPath)); - this.root = root; + this.instance = instance; this.propertyPath = propertyPath; - this.mountPoint = new PropertyMountPoint(root, propertyPath); - var subscriptions = this.GetInpcNodes(); - this.Changed = this.CreateObservableFromNodes(subscriptions); + this.mountPoint = new PropertyMountPoint(instance, propertyPath); + var properties = this.GetPropertiesThatRaiseNotifications(); + this.Values = this.CreateUnifiedObservableFromNodes(properties); } - private IObservable CreateObservableFromNodes(IEnumerable subscriptions) + public IObservable Values { get; private set; } + + private IObservable CreateUnifiedObservableFromNodes(IEnumerable subscriptions) { - return subscriptions.Select( - subscription => Observable.FromEventPattern( - ev => subscription.Parent.PropertyChanged += ev, - handler => subscription.Parent.PropertyChanged -= handler) - .Do(_ => this.mountPoint = new PropertyMountPoint(this.root, this.propertyPath)) - .Where(pattern => pattern.EventArgs.PropertyName == subscription.PropertyName)) - .Merge(); + return subscriptions.Select(this.GetObservableFromProperty).Merge(); } - private IEnumerable GetInpcNodes() + private IObservable GetObservableFromProperty(PropertyDefinition subscription) { - return this.GetSubscriptionsRecursive(this.root, this.propertyPath, 0); + return Observable.FromEventPattern( + parentOnPropertyChanged => subscription.Parent.PropertyChanged += parentOnPropertyChanged, + parentOnPropertyChanged => subscription.Parent.PropertyChanged -= parentOnPropertyChanged) + .Where(pattern => pattern.EventArgs.PropertyName == subscription.PropertyName) + .Select(pattern => this.mountPoint.Value); } - private IEnumerable GetSubscriptionsRecursive(object current, PropertyPath propertyPath, int i) + private IEnumerable GetPropertiesThatRaiseNotifications() { - var subscriptions = new List(); + return this.GetSubscriptionsRecursive(this.instance, this.propertyPath, 0); + } + + private IEnumerable GetSubscriptionsRecursive(object current, PropertyPath propertyPath, int i) + { + var subscriptions = new List(); var inpc = current as INotifyPropertyChanged; if (inpc == null) @@ -59,7 +64,7 @@ namespace Perspex.Markup.Xaml.DataBinding.ChangeTracking } var nextPropertyName = propertyPath.Chunks[i]; - subscriptions.Add(new InpcNode(inpc, nextPropertyName)); + subscriptions.Add(new PropertyDefinition(inpc, nextPropertyName)); if (i < this.propertyPath.Chunks.Length) { @@ -76,8 +81,6 @@ namespace Perspex.Markup.Xaml.DataBinding.ChangeTracking return subscriptions; } - public IObservable Changed { get; } - public object Value { get @@ -93,9 +96,9 @@ namespace Perspex.Markup.Xaml.DataBinding.ChangeTracking public Type Type => this.mountPoint.ProperyType; - private class InpcNode + private class PropertyDefinition { - public InpcNode(INotifyPropertyChanged parent, string propertyName) + public PropertyDefinition(INotifyPropertyChanged parent, string propertyName) { this.Parent = parent; this.PropertyName = propertyName; diff --git a/src/Markup/Perspex.Markup.Xaml/DataBinding/DataContextChangeSynchronizer.cs b/src/Markup/Perspex.Markup.Xaml/DataBinding/DataContextChangeSynchronizer.cs index a6ea52e1fe..5526b93edd 100644 --- a/src/Markup/Perspex.Markup.Xaml/DataBinding/DataContextChangeSynchronizer.cs +++ b/src/Markup/Perspex.Markup.Xaml/DataBinding/DataContextChangeSynchronizer.cs @@ -7,8 +7,8 @@ namespace Perspex.Markup.Xaml.DataBinding { using System; - using System.Diagnostics; using System.Globalization; + using System.Reactive.Linq; using System.Reflection; using ChangeTracking; using Glass; @@ -16,104 +16,153 @@ namespace Perspex.Markup.Xaml.DataBinding public class DataContextChangeSynchronizer { + private readonly BindingTarget bindingTarget; private readonly ITypeConverter targetPropertyTypeConverter; private readonly TargetBindingEndpoint bindingEndpoint; private readonly ObservablePropertyBranch sourceEndpoint; - public DataContextChangeSynchronizer(PerspexObject target, PerspexProperty targetProperty, - PropertyPath sourcePropertyPath, object source, ITypeConverterProvider typeConverterProvider) + public DataContextChangeSynchronizer(BindingSource bindingSource, BindingTarget bindingTarget, ITypeConverterProvider typeConverterProvider) { - Guard.ThrowIfNull(target, nameof(target)); - Guard.ThrowIfNull(targetProperty, nameof(targetProperty)); - Guard.ThrowIfNull(sourcePropertyPath, nameof(sourcePropertyPath)); - Guard.ThrowIfNull(source, nameof(source)); + this.bindingTarget = bindingTarget; + Guard.ThrowIfNull(bindingTarget.Object, nameof(bindingTarget.Object)); + Guard.ThrowIfNull(bindingTarget.Property, nameof(bindingTarget.Property)); + Guard.ThrowIfNull(bindingSource.SourcePropertyPath, nameof(bindingSource.SourcePropertyPath)); + Guard.ThrowIfNull(bindingSource.Source, nameof(bindingSource.Source)); Guard.ThrowIfNull(typeConverterProvider, nameof(typeConverterProvider)); - this.bindingEndpoint = new TargetBindingEndpoint(target, targetProperty); - this.sourceEndpoint = new ObservablePropertyBranch(source, sourcePropertyPath); - this.targetPropertyTypeConverter = typeConverterProvider.GetTypeConverter(targetProperty.PropertyType); + this.bindingEndpoint = new TargetBindingEndpoint(bindingTarget.Object, bindingTarget.Property); + this.sourceEndpoint = new ObservablePropertyBranch(bindingSource.Source, bindingSource.SourcePropertyPath); + this.targetPropertyTypeConverter = typeConverterProvider.GetTypeConverter(bindingTarget.Property.PropertyType); } - private bool CanAssignWithoutConversion + public class BindingTarget { - get + private readonly PerspexObject obj; + private readonly PerspexProperty property; + + public BindingTarget(PerspexObject @object, PerspexProperty property) { - var sourceTypeInfo = this.sourceEndpoint.Type.GetTypeInfo(); - var targetTypeInfo = this.bindingEndpoint.Property.PropertyType.GetTypeInfo(); - var compatible = targetTypeInfo.IsAssignableFrom(sourceTypeInfo); - return compatible; + this.obj = @object; + this.property = property; + } + + public PerspexObject Object => obj; + + public PerspexProperty Property => property; + + public object Value + { + get { return obj.GetValue(property); } + set { obj.SetValue(property, value); } } } - public void SubscribeModelToUI() + public class BindingSource { - this.bindingEndpoint.Object.GetObservable(this.bindingEndpoint.Property).Subscribe(this.UpdateModelFromUI); + private readonly PropertyPath sourcePropertyPath; + private readonly object source; + + public BindingSource(PropertyPath sourcePropertyPath, object source) + { + this.sourcePropertyPath = sourcePropertyPath; + this.source = source; + } + + public PropertyPath SourcePropertyPath => this.sourcePropertyPath; + + public object Source => source; } - public void SubscribeUIToModel() + public void StartUpdatingTargetWhenSourceChanges() { - this.sourceEndpoint.Changed.Subscribe(_ => this.UpdateUIFromModel()); - this.UpdateUIFromModel(); + // TODO: commenting out this line will make the existing value to be skipped from the SourceValues. This is not supposed to happen. Is it? + bindingTarget.Value = ConvertedValue(sourceEndpoint.Value, bindingTarget.Property.PropertyType); + + // We use the native Bind method from PerspexObject to subscribe to the SourceValues observable + this.bindingTarget.Object.Bind(this.bindingTarget.Property, this.SourceValues); } - private void UpdateUIFromModel() + public void StartUpdatingSourceWhenTargetChanges() { - object contextGetter = this.sourceEndpoint.Value; - this.SetCompatibleValue(contextGetter, this.bindingEndpoint.Property.PropertyType, o => this.bindingEndpoint.Object.SetValue(this.bindingEndpoint.Property, o)); + // We subscribe to the TargetValues and each time we have a new value, we update the source with it + this.TargetValues.Subscribe(newValue => this.sourceEndpoint.Value = newValue); } - private void SetCompatibleValue(object originalValue, Type targetType, Action setValueFunc) + private IObservable SourceValues { - if (originalValue == null) + get { - setValueFunc(null); + return this.sourceEndpoint.Values.Select(originalValue => this.ConvertedValue(originalValue, this.bindingTarget.Property.PropertyType)); } - else + } + + private IObservable TargetValues + { + get + { + return this.bindingEndpoint.Object + .GetObservable(this.bindingEndpoint.Property).Select(o => this.ConvertedValue(o, this.sourceEndpoint.Type)); + } + } + + private bool CanAssignWithoutConversion + { + get + { + var sourceTypeInfo = this.sourceEndpoint.Type.GetTypeInfo(); + var targetTypeInfo = this.bindingEndpoint.Property.PropertyType.GetTypeInfo(); + var compatible = targetTypeInfo.IsAssignableFrom(sourceTypeInfo); + return compatible; + } + } + + private object ConvertedValue(object originalValue, Type propertyType) + { + object converted; + if (this.TryConvert(originalValue, propertyType, out converted)) + { + return converted; + } + + return null; + } + + private bool TryConvert(object originalValue, Type targetType, out object finalValue) + { + if (originalValue != null) { if (this.CanAssignWithoutConversion) { - setValueFunc(originalValue); + finalValue = originalValue; + return true; } - else - { - var synchronizationOk = false; - if (this.targetPropertyTypeConverter != null) + if (this.targetPropertyTypeConverter != null) + { + if (this.targetPropertyTypeConverter.CanConvertTo(null, targetType)) { - if (this.targetPropertyTypeConverter.CanConvertTo(null, targetType)) + object convertedValue = this.targetPropertyTypeConverter.ConvertTo( + null, + CultureInfo.InvariantCulture, + originalValue, + targetType); + + if (convertedValue != null) { - object convertedValue = this.targetPropertyTypeConverter.ConvertTo(null, CultureInfo.InvariantCulture, originalValue, - targetType); - - if (convertedValue != null) - { - setValueFunc(convertedValue); - synchronizationOk = true; - } + finalValue = convertedValue; + return true; } } - - if (!synchronizationOk) - { - this.LogCannotConvertError(originalValue); - } } } - } - - private void UpdateModelFromUI(object valueFromUI) - { - this.SetCompatibleValue(valueFromUI, this.sourceEndpoint.Type, o => this.sourceEndpoint.Value = o); - } - - private void LogCannotConvertError(object value) - { - Contract.Requires(value != null); - - var loggableValue = value.ToString(); - var valueToWrite = string.IsNullOrWhiteSpace(loggableValue) ? "'(empty/whitespace string)'" : loggableValue; + else + { + finalValue = null; + return true; + } - Debug.WriteLine("Cannot convert value {0} ({1}) to {2}", valueToWrite, value.GetType(), this.bindingEndpoint.Property.PropertyType); + finalValue = null; + return false; } } } \ No newline at end of file diff --git a/src/Markup/Perspex.Markup.Xaml/DataBinding/PerspexPropertyBinder.cs b/src/Markup/Perspex.Markup.Xaml/DataBinding/PerspexPropertyBinder.cs index 3cb8c3f67e..515304f2aa 100644 --- a/src/Markup/Perspex.Markup.Xaml/DataBinding/PerspexPropertyBinder.cs +++ b/src/Markup/Perspex.Markup.Xaml/DataBinding/PerspexPropertyBinder.cs @@ -31,8 +31,8 @@ namespace Perspex.Markup.Xaml.DataBinding public IEnumerable GetBindings(PerspexObject source) { return from binding in this.bindings - where binding.Target == source - select binding; + where binding.Target == source + select binding; } public XamlBinding Create(XamlBindingDefinition xamlBinding) diff --git a/src/Markup/Perspex.Markup.Xaml/DataBinding/XamlBinding.cs b/src/Markup/Perspex.Markup.Xaml/DataBinding/XamlBinding.cs index 1b58726b85..f8b4feea70 100644 --- a/src/Markup/Perspex.Markup.Xaml/DataBinding/XamlBinding.cs +++ b/src/Markup/Perspex.Markup.Xaml/DataBinding/XamlBinding.cs @@ -14,6 +14,12 @@ namespace Perspex.Markup.Xaml.DataBinding public class XamlBinding { private readonly ITypeConverterProvider typeConverterProvider; + private DataContextChangeSynchronizer changeSynchronizer; + + public XamlBinding(ITypeConverterProvider typeConverterProvider) + { + this.typeConverterProvider = typeConverterProvider; + } public PerspexObject Target { get; set; } @@ -23,12 +29,7 @@ namespace Perspex.Markup.Xaml.DataBinding public BindingMode BindingMode { get; set; } - public XamlBinding(ITypeConverterProvider typeConverterProvider) - { - this.typeConverterProvider = typeConverterProvider; - } - - public void Bind(object dataContext) + public void BindToDataContext(object dataContext) { if (dataContext == null) { @@ -37,35 +38,25 @@ namespace Perspex.Markup.Xaml.DataBinding try { + var bindingSource = new DataContextChangeSynchronizer.BindingSource(this.SourcePropertyPath, dataContext); + var bindingTarget = new DataContextChangeSynchronizer.BindingTarget(this.Target, this.TargetProperty); + + this.changeSynchronizer = new DataContextChangeSynchronizer(bindingSource, bindingTarget, this.typeConverterProvider); + if (this.BindingMode == BindingMode.TwoWay) { - var changeSynchronizer = new DataContextChangeSynchronizer( - this.Target, - this.TargetProperty, - this.SourcePropertyPath, - dataContext, - this.typeConverterProvider); - - changeSynchronizer.SubscribeUIToModel(); - changeSynchronizer.SubscribeModelToUI(); + this.changeSynchronizer.StartUpdatingTargetWhenSourceChanges(); + this.changeSynchronizer.StartUpdatingSourceWhenTargetChanges(); } if (this.BindingMode == BindingMode.OneWay) { - var subscriptionHandler = new DataContextChangeSynchronizer( - this.Target, - this.TargetProperty, - this.SourcePropertyPath, - dataContext, - this.typeConverterProvider); - - subscriptionHandler.SubscribeUIToModel(); + this.changeSynchronizer.StartUpdatingTargetWhenSourceChanges(); } if (this.BindingMode == BindingMode.OneWayToSource) { - var subscriptionHandler = new DataContextChangeSynchronizer(this.Target, this.TargetProperty, this.SourcePropertyPath, dataContext, this.typeConverterProvider); - subscriptionHandler.SubscribeModelToUI(); + this.changeSynchronizer.StartUpdatingSourceWhenTargetChanges(); } } catch (Exception e)