From 2f7229565789ca2e6c202d98d79bd4dffa21358e Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 10 Aug 2016 11:12:31 +0200 Subject: [PATCH] Started refactor of ExpressionObserver. Tying to be more "rx", but also will allow us to move forward on BindingNotification changes. --- src/Avalonia.Base/Avalonia.Base.csproj | 1 + src/Avalonia.Base/Utilities/WeakObservable.cs | 54 +++++ .../Templates/MemberSelector.cs | 51 ++--- .../Avalonia.Markup/Avalonia.Markup.csproj | 1 + .../Data/EmptyExpressionNode.cs | 16 ++ .../Avalonia.Markup/Data/ExpressionNode.cs | 153 +++++++++----- .../Data/ExpressionObserver.cs | 196 ++++-------------- .../Avalonia.Markup/Data/IndexerNode.cs | 187 +++++++---------- .../Avalonia.Markup/Data/LogicalNotNode.cs | 28 ++- .../Data/PropertyAccessorNode.cs | 127 ++---------- .../Avalonia.Markup.UnitTests.csproj | 2 +- .../ExpressionObserverTests_DataValidation.cs | 58 +++++- .../Data/ExpressionObserverTests_Indexer.cs | 2 +- .../Data/ExpressionObserverTests_Lifetime.cs | 44 ++-- .../Data/ExpressionObserverTests_Negation.cs | 17 +- .../Data/ExpressionObserverTests_Property.cs | 109 +++++++++- .../Data/ExpressionObserverTests_SetValue.cs | 10 +- 17 files changed, 564 insertions(+), 492 deletions(-) create mode 100644 src/Avalonia.Base/Utilities/WeakObservable.cs create mode 100644 src/Markup/Avalonia.Markup/Data/EmptyExpressionNode.cs diff --git a/src/Avalonia.Base/Avalonia.Base.csproj b/src/Avalonia.Base/Avalonia.Base.csproj index d9375e30ee..963726c158 100644 --- a/src/Avalonia.Base/Avalonia.Base.csproj +++ b/src/Avalonia.Base/Avalonia.Base.csproj @@ -117,6 +117,7 @@ + diff --git a/src/Avalonia.Base/Utilities/WeakObservable.cs b/src/Avalonia.Base/Utilities/WeakObservable.cs new file mode 100644 index 0000000000..c261cc0520 --- /dev/null +++ b/src/Avalonia.Base/Utilities/WeakObservable.cs @@ -0,0 +1,54 @@ +// Copyright (c) The Avalonia Project. All rights reserved. +// 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; + +namespace Avalonia.Utilities +{ + /// + /// Provides extension methods for working with weak event handlers. + /// + public static class WeakObservable + { + /// + /// Converts a .NET event conforming to the standard .NET event pattern into an observable + /// sequence, subscribing weakly. + /// + /// The type of the event args. + /// Object instance that exposes the event to convert. + /// Name of the event to convert. + /// + public static IObservable> FromEventPattern( + object target, + string eventName) + where TEventArgs : EventArgs + { + Contract.Requires(target != null); + Contract.Requires(eventName != null); + + return Observable.Create>(observer => + { + var handler = new Handler(observer); + WeakSubscriptionManager.Subscribe(target, eventName, handler); + return () => WeakSubscriptionManager.Unsubscribe(target, eventName, handler); + }).Publish().RefCount(); + } + + private class Handler : IWeakSubscriber where TEventArgs : EventArgs + { + private IObserver> _observer; + + public Handler(IObserver> observer) + { + _observer = observer; + } + + public void OnEvent(object sender, TEventArgs e) + { + _observer.OnNext(new EventPattern(sender, e)); + } + } + } +} diff --git a/src/Markup/Avalonia.Markup.Xaml/Templates/MemberSelector.cs b/src/Markup/Avalonia.Markup.Xaml/Templates/MemberSelector.cs index 84ba432753..7e9d7c00b8 100644 --- a/src/Markup/Avalonia.Markup.Xaml/Templates/MemberSelector.cs +++ b/src/Markup/Avalonia.Markup.Xaml/Templates/MemberSelector.cs @@ -30,39 +30,40 @@ namespace Avalonia.Markup.Xaml.Templates public object Select(object o) { - if (string.IsNullOrEmpty(MemberName)) - { - return o; - } + throw new NotImplementedException(); + ////if (string.IsNullOrEmpty(MemberName)) + ////{ + //// return o; + ////} - if (_expressionNode == null) - { - _expressionNode = ExpressionNodeBuilder.Build(MemberName); + ////if (_expressionNode == null) + ////{ + //// _expressionNode = ExpressionNodeBuilder.Build(MemberName); - _memberValueNode = _expressionNode; + //// _memberValueNode = _expressionNode; - while (_memberValueNode.Next != null) - { - _memberValueNode = _memberValueNode.Next; - } - } + //// while (_memberValueNode.Next != null) + //// { + //// _memberValueNode = _memberValueNode.Next; + //// } + ////} - _expressionNode.Target = new WeakReference(o); + ////_expressionNode.Target = new WeakReference(o); - object result = _memberValueNode.CurrentValue.Target; + ////object result = _memberValueNode.CurrentValue.Target; - _expressionNode.Target = null; + ////_expressionNode.Target = null; - if (result == AvaloniaProperty.UnsetValue) - { - return null; - } - else if (result is BindingNotification) - { - return null; - } + ////if (result == AvaloniaProperty.UnsetValue) + ////{ + //// return null; + ////} + ////else if (result is BindingNotification) + ////{ + //// return null; + ////} - return result; + ////return result; } } } \ No newline at end of file diff --git a/src/Markup/Avalonia.Markup/Avalonia.Markup.csproj b/src/Markup/Avalonia.Markup/Avalonia.Markup.csproj index f26931d24e..9324503cb0 100644 --- a/src/Markup/Avalonia.Markup/Avalonia.Markup.csproj +++ b/src/Markup/Avalonia.Markup/Avalonia.Markup.csproj @@ -42,6 +42,7 @@ Properties\SharedAssemblyInfo.cs + diff --git a/src/Markup/Avalonia.Markup/Data/EmptyExpressionNode.cs b/src/Markup/Avalonia.Markup/Data/EmptyExpressionNode.cs new file mode 100644 index 0000000000..d0133f161e --- /dev/null +++ b/src/Markup/Avalonia.Markup/Data/EmptyExpressionNode.cs @@ -0,0 +1,16 @@ +// Copyright (c) The Avalonia Project. All rights reserved. +// Licensed under the MIT license. See licence.md file in the project root for full license information. + +using System; +using System.Reactive.Linq; + +namespace Avalonia.Markup.Data +{ + internal class EmptyExpressionNode : ExpressionNode + { + protected override IObservable StartListening(WeakReference reference) + { + return Observable.Return(reference.Target); + } + } +} diff --git a/src/Markup/Avalonia.Markup/Data/ExpressionNode.cs b/src/Markup/Avalonia.Markup/Data/ExpressionNode.cs index 622a5f1029..26e1234d34 100644 --- a/src/Markup/Avalonia.Markup/Data/ExpressionNode.cs +++ b/src/Markup/Avalonia.Markup/Data/ExpressionNode.cs @@ -2,111 +2,154 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; +using System.Reactive.Disposables; +using System.Reactive.Linq; using System.Reactive.Subjects; using Avalonia.Data; namespace Avalonia.Markup.Data { - internal abstract class ExpressionNode : IObservable + internal abstract class ExpressionNode : ISubject { protected static readonly WeakReference UnsetReference = new WeakReference(AvaloniaProperty.UnsetValue); - private WeakReference _target; - - private Subject _subject; - - private WeakReference _value = UnsetReference; + private WeakReference _target = UnsetReference; + private IDisposable _valueSubscription; + private IObserver _observer; public ExpressionNode Next { get; set; } public WeakReference Target { - get - { - return _target; - } + get { return _target; } set { - var newInstance = value?.Target; - var oldInstance = _target?.Target; + Contract.Requires(value != null); - if (!object.Equals(oldInstance, newInstance)) - { - if (oldInstance != null) - { - Unsubscribe(oldInstance); - } + var oldTarget = _target?.Target; + var newTarget = value.Target; + var running = _valueSubscription != null; + if (!ReferenceEquals(oldTarget, newTarget)) + { + _valueSubscription?.Dispose(); + _valueSubscription = null; _target = value; - if (newInstance != null) - { - SubscribeAndUpdate(_target); - } - else - { - CurrentValue = UnsetReference; - } - - if (Next != null) + if (running) { - Next.Target = _value; + _valueSubscription = StartListeningCore(); } } } } - public WeakReference CurrentValue + public IDisposable Subscribe(IObserver observer) { - get + if (_observer != null) { - return _value; + throw new AvaloniaInternalException("ExpressionNode can only be subscribed once."); } - set + _observer = observer; + var nextSubscription = Next?.Subscribe(this); + _valueSubscription = StartListeningCore(); + + return Disposable.Create(() => { - _value = value; + _valueSubscription?.Dispose(); + _valueSubscription = null; + nextSubscription?.Dispose(); + _observer = null; + }); + } - if (Next != null) - { - Next.Target = value; - } + void IObserver.OnCompleted() + { + throw new AvaloniaInternalException("ExpressionNode.OnCompleted should not be called."); + } - _subject?.OnNext(value.Target); - } + void IObserver.OnError(Exception error) + { + throw new AvaloniaInternalException("ExpressionNode.OnError should not be called."); } - public virtual bool SetValue(object value, BindingPriority priority) + void IObserver.OnNext(object value) { - return Next?.SetValue(value, priority) ?? false; + NextValueChanged(value); } - public virtual IDisposable Subscribe(IObserver observer) + protected virtual IObservable StartListening(WeakReference reference) { - if (Next != null) + return Observable.Return(reference.Target); + } + + protected virtual void NextValueChanged(object value) + { + _observer.OnNext(value); + } + + private IDisposable StartListeningCore() + { + var target = _target.Target; + IObservable source; + + if (target == null) + { + source = Observable.Return(TargetNullNotification()); + } + else if (target == AvaloniaProperty.UnsetValue) { - return Next.Subscribe(observer); + source = Observable.Empty(); } else { - if (_subject == null) - { - _subject = new Subject(); - } - - observer.OnNext(CurrentValue.Target); - return _subject.Subscribe(observer); + source = StartListening(_target); } + + return source.Subscribe(TargetValueChanged); } - protected virtual void SubscribeAndUpdate(WeakReference reference) + private void TargetValueChanged(object value) { - CurrentValue = reference; + var notification = value as BindingNotification; + + if (notification == null) + { + if (Next != null) + { + Next.Target = new WeakReference(value); + } + else + { + _observer.OnNext(value); + } + } + else + { + if (notification.Error != null) + { + _observer.OnNext(notification); + } + else if (notification.HasValue) + { + if (Next != null) + { + Next.Target = new WeakReference(notification.Value); + } + else + { + _observer.OnNext(value); + } + } + } } - protected virtual void Unsubscribe(object target) + private BindingNotification TargetNullNotification() { + // TODO: Work out a way to give a more useful error message here. + return new BindingNotification(new NullReferenceException(), BindingErrorType.Error); } } } diff --git a/src/Markup/Avalonia.Markup/Data/ExpressionObserver.cs b/src/Markup/Avalonia.Markup/Data/ExpressionObserver.cs index 83a0628c84..2e4c98fe82 100644 --- a/src/Markup/Avalonia.Markup/Data/ExpressionObserver.cs +++ b/src/Markup/Avalonia.Markup/Data/ExpressionObserver.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Reactive; using System.Reactive.Disposables; using System.Reactive.Linq; +using System.Reactive.Subjects; using Avalonia.Data; using Avalonia.Markup.Data.Plugins; @@ -38,15 +39,11 @@ namespace Avalonia.Markup.Data ExceptionValidationPlugin.Instance, }; - private readonly WeakReference _root; - private readonly Func _rootGetter; - private readonly IObservable _rootObservable; - private readonly IObservable _update; - private IDisposable _rootObserverSubscription; - private IDisposable _updateSubscription; - private int _count; + private static readonly object UninitializedValue = new object(); private readonly ExpressionNode _node; - private bool _enableDataValidation; + private readonly Subject _finished; + private readonly object _root; + private IObservable _result; /// /// Initializes a new instance of the class. @@ -58,15 +55,9 @@ namespace Avalonia.Markup.Data { Contract.Requires(expression != null); - _root = new WeakReference(root); - _enableDataValidation = enableDataValidation; - - if (!string.IsNullOrWhiteSpace(expression)) - { - _node = ExpressionNodeBuilder.Build(expression, enableDataValidation); - } - Expression = expression; + _node = Parse(expression, enableDataValidation); + _root = new WeakReference(root); } /// @@ -83,15 +74,10 @@ namespace Avalonia.Markup.Data Contract.Requires(rootObservable != null); Contract.Requires(expression != null); - _rootObservable = rootObservable; - _enableDataValidation = enableDataValidation; - - if (!string.IsNullOrWhiteSpace(expression)) - { - _node = ExpressionNodeBuilder.Build(expression, enableDataValidation); - } - Expression = expression; + _node = Parse(expression, enableDataValidation); + _finished = new Subject(); + _root = rootObservable; } /// @@ -111,16 +97,12 @@ namespace Avalonia.Markup.Data Contract.Requires(expression != null); Contract.Requires(update != null); - _rootGetter = rootGetter; - _update = update; - _enableDataValidation = enableDataValidation; - - if (!string.IsNullOrWhiteSpace(expression)) - { - _node = ExpressionNodeBuilder.Build(expression, enableDataValidation); - } - Expression = expression; + _node = Parse(expression, enableDataValidation); + _finished = new Subject(); + + _node.Target = new WeakReference(rootGetter()); + _root = update.Select(x => rootGetter()); } /// @@ -134,21 +116,7 @@ namespace Avalonia.Markup.Data /// public bool SetValue(object value, BindingPriority priority = BindingPriority.LocalValue) { - IncrementCount(); - - if (_rootGetter != null && _node != null) - { - _node.Target = new WeakReference(_rootGetter()); - } - - try - { - return _node?.SetValue(value, priority) ?? false; - } - finally - { - DecrementCount(); - } + return (Leaf as PropertyAccessorNode)?.SetTargetValue(value, priority) ?? false; } /// @@ -160,42 +128,11 @@ namespace Avalonia.Markup.Data /// Gets the type of the expression result or null if the expression could not be /// evaluated. /// - public Type ResultType - { - get - { - IncrementCount(); - - try - { - if (_node != null) - { - return (Leaf as PropertyAccessorNode)?.PropertyType; - } - else if (_rootGetter != null) - { - return _rootGetter()?.GetType(); - } - else - { - return _root.Target?.GetType(); - } - } - finally - { - DecrementCount(); - } - } - } + public Type ResultType => (Leaf as PropertyAccessorNode)?.PropertyType; /// string IDescription.Description => Expression; - /// - /// Gets the root expression node. Used for testing. - /// - internal ExpressionNode Node => _node; - /// /// Gets the leaf node. /// @@ -212,94 +149,51 @@ namespace Avalonia.Markup.Data /// protected override IDisposable SubscribeCore(IObserver observer) { - IncrementCount(); - - if (_node != null) + if (_result == null) { - IObservable source = _node; + var source = (IObservable)_node; - if (_rootObservable != null) + if (_finished != null) { - source = source.TakeUntil(_rootObservable.LastOrDefaultAsync()); + source = source.TakeUntil(_finished); } - else if (_update != null) - { - source = source.TakeUntil(_update.LastOrDefaultAsync()); - } - - var subscription = source.Subscribe(observer); - return Disposable.Create(() => - { - DecrementCount(); - subscription.Dispose(); - }); + _result = Observable.Using(StartRoot, _ => source) + .Publish(UninitializedValue) + .RefCount() + .Where(x => x != UninitializedValue); } - else if (_rootObservable != null) + + return _result.Subscribe(observer); + } + + private static ExpressionNode Parse(string expression, bool enableDataValidation) + { + if (!string.IsNullOrWhiteSpace(expression)) { - return _rootObservable.Subscribe(observer); + return ExpressionNodeBuilder.Build(expression, enableDataValidation); } else { - if (_update == null) - { - return Observable.Never() - .StartWith(_root.Target) - .Subscribe(observer); - } - else - { - return _update - .Select(_ => _rootGetter()) - .StartWith(_rootGetter()) - .Subscribe(observer); - } + return new EmptyExpressionNode(); } } - private void IncrementCount() + private IDisposable StartRoot() { - if (_count++ == 0 && _node != null) - { - if (_rootGetter != null) - { - _node.Target = new WeakReference(_rootGetter()); + var observable = _root as IObservable; - if (_update != null) - { - _updateSubscription = _update.Subscribe(x => - _node.Target = new WeakReference(_rootGetter())); - } - } - else if (_rootObservable != null) - { - _rootObserverSubscription = _rootObservable.Subscribe(x => - _node.Target = new WeakReference(x)); - } - else - { - _node.Target = _root; - } + if (observable != null) + { + return observable.Subscribe( + x => _node.Target = new WeakReference(x), + _ => _finished.OnNext(Unit.Default), + () => _finished.OnNext(Unit.Default)); } - } - - private void DecrementCount() - { - if (--_count == 0 && _node != null) + else { - if (_rootObserverSubscription != null) - { - _rootObserverSubscription.Dispose(); - _rootObserverSubscription = null; - } - - if (_updateSubscription != null) - { - _updateSubscription.Dispose(); - _updateSubscription = null; - } - - _node.Target = null; + _node.Target = (WeakReference)_root; + return Disposable.Empty; } } } diff --git a/src/Markup/Avalonia.Markup/Data/IndexerNode.cs b/src/Markup/Avalonia.Markup/Data/IndexerNode.cs index 8849e7edbc..f9615ee804 100644 --- a/src/Markup/Avalonia.Markup/Data/IndexerNode.cs +++ b/src/Markup/Avalonia.Markup/Data/IndexerNode.cs @@ -10,130 +10,47 @@ using System.ComponentModel; using System.Globalization; using System.Linq; using System.Reflection; +using System.Reactive.Linq; namespace Avalonia.Markup.Data { - internal class IndexerNode : ExpressionNode, - IWeakSubscriber, - IWeakSubscriber + internal class IndexerNode : ExpressionNode { public IndexerNode(IList arguments) { Arguments = arguments; } - public IList Arguments { get; } - - void IWeakSubscriber.OnEvent(object sender, NotifyCollectionChangedEventArgs e) - { - var update = false; - if (sender is IList) - { - object indexObject; - if (!TypeUtilities.TryConvert(typeof(int), Arguments[0], CultureInfo.InvariantCulture, out indexObject)) - { - return; - } - var index = (int)indexObject; - switch (e.Action) - { - case NotifyCollectionChangedAction.Add: - update = index >= e.NewStartingIndex; - break; - case NotifyCollectionChangedAction.Remove: - update = index >= e.OldStartingIndex; - break; - case NotifyCollectionChangedAction.Replace: - update = index >= e.NewStartingIndex && - index < e.NewStartingIndex + e.NewItems.Count; - break; - case NotifyCollectionChangedAction.Move: - update = (index >= e.NewStartingIndex && - index < e.NewStartingIndex + e.NewItems.Count) || - (index >= e.OldStartingIndex && - index < e.OldStartingIndex + e.OldItems.Count); - break; - case NotifyCollectionChangedAction.Reset: - update = true; - break; - } - } - else - { - update = true; - } - - if (update) - { - CurrentValue = new WeakReference(GetValue(sender)); - } - } - - void IWeakSubscriber.OnEvent(object sender, PropertyChangedEventArgs e) + protected override IObservable StartListening(WeakReference reference) { - var typeInfo = sender.GetType().GetTypeInfo(); - - if (typeInfo.GetDeclaredProperty(e.PropertyName) == null) - { - return; - } - - if (typeInfo.GetDeclaredProperty(e.PropertyName).GetIndexParameters().Any()) - { - CurrentValue = new WeakReference(GetValue(sender)); - } - } - - protected override void SubscribeAndUpdate(WeakReference reference) - { - object target = reference.Target; - - CurrentValue = new WeakReference(GetValue(target)); - + var target = reference.Target; var incc = target as INotifyCollectionChanged; - - if (incc != null) - { - WeakSubscriptionManager.Subscribe( - incc, - nameof(incc.CollectionChanged), - this); - } - var inpc = target as INotifyPropertyChanged; - - if (inpc != null) - { - WeakSubscriptionManager.Subscribe( - inpc, - nameof(inpc.PropertyChanged), - this); - } - } - - protected override void Unsubscribe(object target) - { - var incc = target as INotifyCollectionChanged; + var inputs = new List>(); if (incc != null) { - WeakSubscriptionManager.Unsubscribe( - incc, - nameof(incc.CollectionChanged), - this); + inputs.Add(WeakObservable.FromEventPattern( + target, + nameof(incc.CollectionChanged)) + .Where(x => ShouldUpdate(x.Sender, x.EventArgs)) + .Select(_ => GetValue(target))); } - var inpc = target as INotifyPropertyChanged; - if (inpc != null) { - WeakSubscriptionManager.Unsubscribe( - inpc, - nameof(inpc.PropertyChanged), - this); + inputs.Add(WeakObservable.FromEventPattern( + target, + nameof(inpc.PropertyChanged)) + .Where(x => ShouldUpdate(x.Sender, x.EventArgs)) + .Select(_ => GetValue(target))); } + + return Observable.Merge(inputs).StartWith(GetValue(target)); } + public IList Arguments { get; } + private object GetValue(object target) { var typeInfo = target.GetType().GetTypeInfo(); @@ -141,18 +58,23 @@ namespace Avalonia.Markup.Data var dictionary = target as IDictionary; var indexerProperty = GetIndexer(typeInfo); var indexerParameters = indexerProperty?.GetIndexParameters(); + if (indexerProperty != null && indexerParameters.Length == Arguments.Count) { var convertedObjectArray = new object[indexerParameters.Length]; + for (int i = 0; i < Arguments.Count; i++) { object temp = null; + if (!TypeUtilities.TryConvert(indexerParameters[i].ParameterType, Arguments[i], CultureInfo.InvariantCulture, out temp)) { return AvaloniaProperty.UnsetValue; } + convertedObjectArray[i] = temp; } + var intArgs = convertedObjectArray.OfType().ToArray(); // Try special cases where we can validate indicies @@ -166,16 +88,18 @@ namespace Avalonia.Markup.Data { if (intArgs.Length == Arguments.Count && intArgs[0] >= 0 && intArgs[0] < list.Count) { - return list[intArgs[0]]; + return list[intArgs[0]]; } + return AvaloniaProperty.UnsetValue; } else if (dictionary != null) { if (dictionary.Contains(convertedObjectArray[0])) { - return dictionary[convertedObjectArray[0]]; + return dictionary[convertedObjectArray[0]]; } + return AvaloniaProperty.UnsetValue; } else @@ -187,11 +111,11 @@ namespace Avalonia.Markup.Data else { // Fallback to unchecked access - return indexerProperty.GetValue(target, convertedObjectArray); + return indexerProperty.GetValue(target, convertedObjectArray); } } // Multidimensional arrays end up here because the indexer search picks up the IList indexer instead of the - // multidimensional indexer, which doesn't take the same number of arguments + // multidimensional indexer, which doesn't take the same number of arguments else if (typeInfo.IsArray) { return GetValueFromArray((Array)target); @@ -220,13 +144,16 @@ namespace Avalonia.Markup.Data private bool ConvertArgumentsToInts(out int[] intArgs) { intArgs = new int[Arguments.Count]; + for (int i = 0; i < Arguments.Count; ++i) { object value; + if (!TypeUtilities.TryConvert(typeof(int), Arguments[i], CultureInfo.InvariantCulture, out value)) { return false; } + intArgs[i] = (int)value; } return true; @@ -235,7 +162,8 @@ namespace Avalonia.Markup.Data private static PropertyInfo GetIndexer(TypeInfo typeInfo) { PropertyInfo indexer; - for (;typeInfo != null; typeInfo = typeInfo.BaseType?.GetTypeInfo()) + + for (; typeInfo != null; typeInfo = typeInfo.BaseType?.GetTypeInfo()) { // Check for the default indexer name first to make this faster. // This will only be false when a class in VB has a custom indexer name. @@ -243,14 +171,16 @@ namespace Avalonia.Markup.Data { return indexer; } + foreach (var property in typeInfo.DeclaredProperties) { if (property.GetIndexParameters().Any()) { return property; } - } + } } + return null; } @@ -273,5 +203,46 @@ namespace Avalonia.Markup.Data return false; } } + + private bool ShouldUpdate(object sender, NotifyCollectionChangedEventArgs e) + { + if (sender is IList) + { + object indexObject; + + if (!TypeUtilities.TryConvert(typeof(int), Arguments[0], CultureInfo.InvariantCulture, out indexObject)) + { + return false; + } + + var index = (int)indexObject; + + switch (e.Action) + { + case NotifyCollectionChangedAction.Add: + return index >= e.NewStartingIndex; + case NotifyCollectionChangedAction.Remove: + return index >= e.OldStartingIndex; + case NotifyCollectionChangedAction.Replace: + return index >= e.NewStartingIndex && + index < e.NewStartingIndex + e.NewItems.Count; + case NotifyCollectionChangedAction.Move: + return (index >= e.NewStartingIndex && + index < e.NewStartingIndex + e.NewItems.Count) || + (index >= e.OldStartingIndex && + index < e.OldStartingIndex + e.OldItems.Count); + case NotifyCollectionChangedAction.Reset: + return true; + } + } + + return false; + } + + private bool ShouldUpdate(object sender, PropertyChangedEventArgs e) + { + var typeInfo = sender.GetType().GetTypeInfo(); + return typeInfo.GetDeclaredProperty(e.PropertyName)?.GetIndexParameters().Any() ?? false; + } } } diff --git a/src/Markup/Avalonia.Markup/Data/LogicalNotNode.cs b/src/Markup/Avalonia.Markup/Data/LogicalNotNode.cs index 29fa842439..0d96830024 100644 --- a/src/Markup/Avalonia.Markup/Data/LogicalNotNode.cs +++ b/src/Markup/Avalonia.Markup/Data/LogicalNotNode.cs @@ -3,21 +3,15 @@ using System; using System.Globalization; -using System.Reactive.Linq; using Avalonia.Data; namespace Avalonia.Markup.Data { internal class LogicalNotNode : ExpressionNode { - public override bool SetValue(object value, BindingPriority priority) + protected override void NextValueChanged(object value) { - return false; - } - - public override IDisposable Subscribe(IObserver observer) - { - return Next.Select(Negate).Subscribe(observer); + base.NextValueChanged(Negate(value)); } private static object Negate(object v) @@ -34,6 +28,12 @@ namespace Avalonia.Markup.Data { return !result; } + else + { + return new BindingNotification( + new InvalidCastException($"Unable to convert '{s}' to bool."), + BindingErrorType.Error); + } } else { @@ -42,9 +42,17 @@ namespace Avalonia.Markup.Data var boolean = Convert.ToBoolean(v, CultureInfo.InvariantCulture); return !boolean; } - catch + catch (InvalidCastException) + { + // The error message here is "Unable to cast object of type 'System.Object' + // to type 'System.IConvertible'" which is kinda useless so provide our own. + return new BindingNotification( + new InvalidCastException($"Unable to convert '{v}' to bool."), + BindingErrorType.Error); + } + catch (Exception e) { - // TODO: Maybe should log something here. + return new BindingNotification(e, BindingErrorType.Error); } } } diff --git a/src/Markup/Avalonia.Markup/Data/PropertyAccessorNode.cs b/src/Markup/Avalonia.Markup/Data/PropertyAccessorNode.cs index 8fcaa85e25..35fc444ec1 100644 --- a/src/Markup/Avalonia.Markup/Data/PropertyAccessorNode.cs +++ b/src/Markup/Avalonia.Markup/Data/PropertyAccessorNode.cs @@ -3,22 +3,17 @@ using System; using System.Linq; +using System.Reactive.Disposables; using System.Reactive.Linq; -using System.Reflection; -using System.Threading; -using System.Threading.Tasks; -using System.Windows.Input; using Avalonia.Data; -using Avalonia.Logging; using Avalonia.Markup.Data.Plugins; namespace Avalonia.Markup.Data { - internal class PropertyAccessorNode : ExpressionNode, IObserver + internal class PropertyAccessorNode : ExpressionNode { - private IPropertyAccessor _accessor; - private IDisposable _subscription; private bool _enableValidation; + private IPropertyAccessor _accessor; public PropertyAccessorNode(string propertyName, bool enableValidation) { @@ -30,120 +25,40 @@ namespace Avalonia.Markup.Data public Type PropertyType => _accessor?.PropertyType; - public override bool SetValue(object value, BindingPriority priority) + public bool SetTargetValue(object value, BindingPriority priority) { - if (Next != null) + if (_accessor != null) { - return Next.SetValue(value, priority); - } - else - { - if (_accessor != null) - { - try { return _accessor.SetValue(value, priority); } catch { } - } - - return false; + try { return _accessor.SetValue(value, priority); } catch { } } - } - - void IObserver.OnCompleted() - { - // Should not be called by IPropertyAccessor. - } - void IObserver.OnError(Exception error) - { - // Should not be called by IPropertyAccessor. - } - - void IObserver.OnNext(object value) - { - SetCurrentValue(value); + return false; } - protected override void SubscribeAndUpdate(WeakReference reference) + protected override IObservable StartListening(WeakReference reference) { - var instance = reference.Target; + var plugin = ExpressionObserver.PropertyAccessors.FirstOrDefault(x => x.Match(reference)); + var accessor = plugin?.Start(reference, PropertyName); - if (instance != null && instance != AvaloniaProperty.UnsetValue) + if (_enableValidation && Next == null) { - var plugin = ExpressionObserver.PropertyAccessors.FirstOrDefault(x => x.Match(reference)); - var accessor = plugin?.Start(reference, PropertyName); - - if (_enableValidation) + foreach (var validator in ExpressionObserver.DataValidators) { - foreach (var validator in ExpressionObserver.DataValidators) + if (validator.Match(reference)) { - if (validator.Match(reference)) - { - accessor = validator.Start(reference, PropertyName, accessor); - } + accessor = validator.Start(reference, PropertyName, accessor); } } - - _accessor = accessor; - _accessor.Subscribe(this); - } - else - { - CurrentValue = UnsetReference; - } - } - - protected override void Unsubscribe(object target) - { - _accessor?.Dispose(); - _accessor = null; - } - - private void SetCurrentValue(object value) - { - var observable = value as IObservable; - var command = value as ICommand; - var task = value as Task; - bool set = false; - - // HACK: ReactiveCommand is an IObservable but we want to bind to it, not its value. - // We may need to make this a more general solution. - if (observable != null && command == null) - { - CurrentValue = UnsetReference; - set = true; - _subscription = observable - .ObserveOn(SynchronizationContext.Current) - .Subscribe(x => CurrentValue = new WeakReference(x)); } - else if (task != null) - { - var resultProperty = task.GetType().GetTypeInfo().GetDeclaredProperty("Result"); - if (resultProperty != null) + // Ensure that _accessor is set for the duration of the subscription. + return Observable.Using( + () => { - if (task.Status == TaskStatus.RanToCompletion) - { - CurrentValue = new WeakReference(resultProperty.GetValue(task)); - set = true; - } - else - { - task.ContinueWith( - x => CurrentValue = new WeakReference(resultProperty.GetValue(task)), - TaskScheduler.FromCurrentSynchronizationContext()) - .ConfigureAwait(false); - } - } - } - else - { - CurrentValue = new WeakReference(value); - set = true; - } - - if (!set) - { - CurrentValue = UnsetReference; - } + _accessor = accessor; + return Disposable.Create(() => _accessor = null); + }, + _ => accessor); } } } diff --git a/tests/Avalonia.Markup.UnitTests/Avalonia.Markup.UnitTests.csproj b/tests/Avalonia.Markup.UnitTests/Avalonia.Markup.UnitTests.csproj index 045951882e..6f92e88337 100644 --- a/tests/Avalonia.Markup.UnitTests/Avalonia.Markup.UnitTests.csproj +++ b/tests/Avalonia.Markup.UnitTests/Avalonia.Markup.UnitTests.csproj @@ -85,6 +85,7 @@ + @@ -100,7 +101,6 @@ - diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_DataValidation.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_DataValidation.cs index f8cb702cb9..8789862b3a 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_DataValidation.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_DataValidation.cs @@ -4,10 +4,8 @@ using System; using System.Collections; using System.Collections.Generic; -using System.ComponentModel; using System.Linq; using System.Reactive.Linq; -using System.Runtime.CompilerServices; using Avalonia.Data; using Avalonia.Markup.Data; using Avalonia.UnitTests; @@ -106,6 +104,48 @@ namespace Avalonia.Markup.UnitTests.Data }, result); } + [Fact] + public void Doesnt_Subscribe_To_Indei_Of_Intermediate_Object_In_Chain() + { + var data = new Container + { + Inner = new IndeiTest() + }; + + var observer = new ExpressionObserver( + data, + $"{nameof(Container.Inner)}.{nameof(IndeiTest.MustBePositive)}", + true); + + observer.Subscribe(_ => { }); + + // We may want to change this but I've never seen an example of data validation on an + // intermediate object in a chain so for the moment I'm not sure what the result of + // validating such a thing should look like. + Assert.Equal(0, data.ErrorsChangedSubscriptionCount); + Assert.Equal(1, ((IndeiTest)data.Inner).ErrorsChangedSubscriptionCount); + } + + [Fact] + public void Sends_Correct_Notifications_With_Property_Chain() + { + var container = new Container(); + var inner = new IndeiTest(); + + var observer = new ExpressionObserver( + container, + $"{nameof(Container.Inner)}.{nameof(IndeiTest.MustBePositive)}", + true); + var result = new List(); + + observer.Subscribe(x => result.Add(x)); + + Assert.Equal(new[] + { + new BindingNotification(new NullReferenceException(), BindingErrorType.Error), + }, result); + } + public class ExceptionTest : NotifyingBase { private int _mustBePositive; @@ -161,5 +201,19 @@ namespace Avalonia.Markup.UnitTests.Data return result; } } + + private class Container : IndeiBase + { + private object _inner; + + public object Inner + { + get { return _inner; } + set { _inner = value; RaisePropertyChanged(); } + } + + public override bool HasErrors => false; + public override IEnumerable GetErrors(string propertyName) => null; + } } } diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs index 524cabb096..75cf606042 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Indexer.cs @@ -215,7 +215,7 @@ namespace Avalonia.Markup.UnitTests.Data { Func> run = () => { - var source = new NonIntegerIndexer(); + var source = new { Foo = new NonIntegerIndexer() }; var target = new ExpressionObserver(source, "Foo"); return Tuple.Create(target, new WeakReference(source)); }; diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs index 9fa753917c..2a2bf06bf1 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs @@ -27,6 +27,19 @@ namespace Avalonia.Markup.UnitTests.Data Assert.True(completed); } + [Fact] + public void Should_Complete_When_Source_Observable_Errors() + { + var source = new BehaviorSubject(1); + var target = new ExpressionObserver(source, "Foo"); + var completed = false; + + target.Subscribe(_ => { }, () => completed = true); + source.OnError(new Exception()); + + Assert.True(completed); + } + [Fact] public void Should_Complete_When_Update_Observable_Completes() { @@ -40,6 +53,19 @@ namespace Avalonia.Markup.UnitTests.Data Assert.True(completed); } + [Fact] + public void Should_Complete_When_Update_Observable_Errors() + { + var update = new Subject(); + var target = new ExpressionObserver(() => 1, "Foo", update); + var completed = false; + + target.Subscribe(_ => { }, () => completed = true); + update.OnError(new Exception()); + + Assert.True(completed); + } + [Fact] public void Should_Unsubscribe_From_Source_Observable() { @@ -55,7 +81,7 @@ namespace Avalonia.Markup.UnitTests.Data scheduler.Start(); } - Assert.Equal(new[] { AvaloniaProperty.UnsetValue, "foo" }, result); + Assert.Equal(new[] { "foo" }, result); Assert.All(source.Subscriptions, x => Assert.NotEqual(Subscription.Infinite, x.Unsubscribe)); } @@ -77,22 +103,6 @@ namespace Avalonia.Markup.UnitTests.Data Assert.All(update.Subscriptions, x => Assert.NotEqual(Subscription.Infinite, x.Unsubscribe)); } - [Fact] - public void Should_Set_Node_Target_To_Null_On_Unsubscribe() - { - var target = new ExpressionObserver(new { Foo = "foo" }, "Foo"); - var result = new List(); - - using (target.Subscribe(x => result.Add(x))) - using (target.Subscribe(_ => { })) - { - Assert.NotNull(target.Node.Target); - } - - Assert.Equal(new[] { "foo" }, result); - Assert.Null(target.Node.Target); - } - private Recorded> OnNext(long time, object value) { return new Recorded>(time, Notification.CreateOnNext(value)); diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Negation.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Negation.cs index b3046118be..6bee0d10f4 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Negation.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Negation.cs @@ -3,6 +3,7 @@ using System; using System.Reactive.Linq; +using Avalonia.Data; using Avalonia.Markup.Data; using Xunit; @@ -61,23 +62,31 @@ namespace Avalonia.Markup.UnitTests.Data } [Fact] - public async void Should_Return_UnsetValue_For_String_Not_Convertible_To_Boolean() + public async void Should_Return_BindingNotification_For_String_Not_Convertible_To_Boolean() { var data = new { Foo = "foo" }; var target = new ExpressionObserver(data, "!Foo"); var result = await target.Take(1); - Assert.Equal(AvaloniaProperty.UnsetValue, result); + Assert.Equal( + new BindingNotification( + new InvalidCastException($"Unable to convert 'foo' to bool."), + BindingErrorType.Error), + result); } [Fact] - public async void Should_Return_Empty_For_Value_Not_Convertible_To_Boolean() + public async void Should_Return_BindingNotification_For_Value_Not_Convertible_To_Boolean() { var data = new { Foo = new object() }; var target = new ExpressionObserver(data, "!Foo"); var result = await target.Take(1); - Assert.Equal(AvaloniaProperty.UnsetValue, result); + Assert.Equal( + new BindingNotification( + new InvalidCastException($"Unable to convert 'System.Object' to bool."), + BindingErrorType.Error), + result); } [Fact] diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs index 4e65723c75..043e85cae3 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs @@ -32,6 +32,8 @@ namespace Avalonia.Markup.UnitTests.Data var data = new { Foo = "foo" }; var target = new ExpressionObserver(data, "Foo"); + target.Subscribe(_ => { }); + Assert.Equal(typeof(string), target.ResultType); } @@ -71,6 +73,8 @@ namespace Avalonia.Markup.UnitTests.Data var data = new { Foo = new { Bar = new { Baz = "baz" } } }; var target = new ExpressionObserver(data, "Foo.Bar.Baz"); + target.Subscribe(_ => { }); + Assert.Equal(typeof(string), target.ResultType); } @@ -88,6 +92,23 @@ namespace Avalonia.Markup.UnitTests.Data Assert.Equal("Could not find CLR property 'Baz' on '1'", error.Error.Message); } + [Fact] + public void Should_Return_BindingNotification_Error_For_Chain_With_Null_Value() + { + var data = new { Foo = default(object) }; + var target = new ExpressionObserver(data, "Foo.Bar.Baz"); + var result = new List(); + + target.Subscribe(x => result.Add(x)); + + Assert.Equal(1, result.Count); + Assert.IsType(result[0]); + + var error = result[0] as BindingNotification; + Assert.IsType(error.Error); + Assert.Equal("Object reference not set to an instance of an object.", error.Error.Message); + } + [Fact] public void Should_Have_Null_ResultType_For_Broken_Chain() { @@ -151,8 +172,9 @@ namespace Avalonia.Markup.UnitTests.Data var sub = target.Subscribe(x => result.Add(x)); ((Class2)data.Next).Bar = "baz"; + ((Class2)data.Next).Bar = null; - Assert.Equal(new[] { "bar", "baz" }, result); + Assert.Equal(new[] { "bar", "baz", null }, result); sub.Dispose(); @@ -170,8 +192,9 @@ namespace Avalonia.Markup.UnitTests.Data var sub = target.Subscribe(x => result.Add(x)); var old = data.Next; data.Next = new Class2 { Bar = "baz" }; + data.Next = new Class2 { Bar = null }; - Assert.Equal(new[] { "bar", "baz" }, result); + Assert.Equal(new[] { "bar", "baz", null }, result); sub.Dispose(); @@ -192,7 +215,14 @@ namespace Avalonia.Markup.UnitTests.Data data.Next = null; data.Next = new Class2 { Bar = "baz" }; - Assert.Equal(new[] { "bar", AvaloniaProperty.UnsetValue, "baz" }, result); + Assert.Equal( + new object[] + { + "bar", + new BindingNotification(new NullReferenceException(), BindingErrorType.Error), + "baz" + }, + result); sub.Dispose(); @@ -258,17 +288,59 @@ namespace Avalonia.Markup.UnitTests.Data scheduler.Start(); } - Assert.Equal(new[] { AvaloniaProperty.UnsetValue, "foo", "bar" }, result); + Assert.Equal(new[] { "foo", "bar" }, result); Assert.All(source.Subscriptions, x => Assert.NotEqual(Subscription.Infinite, x.Unsubscribe)); } + [Fact] + public void Subscribing_Multiple_Times_Should_Return_Values_To_All() + { + var data = new Class1 { Foo = "foo" }; + var target = new ExpressionObserver(data, "Foo"); + var result1 = new List(); + var result2 = new List(); + var result3 = new List(); + + target.Subscribe(x => result1.Add(x)); + target.Subscribe(x => result2.Add(x)); + + data.Foo = "bar"; + + target.Subscribe(x => result3.Add(x)); + + Assert.Equal(new[] { "foo", "bar" }, result1); + Assert.Equal(new[] { "foo", "bar" }, result2); + Assert.Equal(new[] { "bar" }, result3); + } + + [Fact] + public void Subscribing_Multiple_Times_Should_Only_Add_PropertyChanged_Handlers_Once() + { + var data = new Class1 { Foo = "foo" }; + var target = new ExpressionObserver(data, "Foo"); + + var sub1 = target.Subscribe(x => { }); + var sub2 = target.Subscribe(x => { }); + + Assert.Equal(1, data.PropertyChangedSubscriptionCount); + + sub1.Dispose(); + sub2.Dispose(); + + Assert.Equal(0, data.PropertyChangedSubscriptionCount); + } + [Fact] public void SetValue_Should_Set_Simple_Property_Value() { var data = new Class1 { Foo = "foo" }; var target = new ExpressionObserver(data, "Foo"); - Assert.True(target.SetValue("bar")); + using (target.Subscribe(_ => { })) + { + Assert.True(target.SetValue("bar")); + } + Assert.Equal("bar", data.Foo); } @@ -278,7 +350,11 @@ namespace Avalonia.Markup.UnitTests.Data var data = new Class1 { Next = new Class2 { Bar = "bar" } }; var target = new ExpressionObserver(data, "Next.Bar"); - Assert.True(target.SetValue("baz")); + using (target.Subscribe(_ => { })) + { + Assert.True(target.SetValue("baz")); + } + Assert.Equal("baz", ((Class2)data.Next).Bar); } @@ -288,7 +364,10 @@ namespace Avalonia.Markup.UnitTests.Data var data = new Class1 { Next = new WithoutBar()}; var target = new ExpressionObserver(data, "Next.Bar"); - Assert.False(target.SetValue("baz")); + using (target.Subscribe(_ => { })) + { + Assert.False(target.SetValue("baz")); + } } [Fact] @@ -297,7 +376,10 @@ namespace Avalonia.Markup.UnitTests.Data var data = new Class1(); var target = new ExpressionObserver(data, "Next.Bar"); - Assert.False(target.SetValue("baz")); + using (target.Subscribe(_ => { })) + { + Assert.False(target.SetValue("baz")); + } } [Fact] @@ -306,7 +388,7 @@ namespace Avalonia.Markup.UnitTests.Data var target = new ExpressionObserver((object)null, "Foo"); var result = await target.Take(1); - Assert.Equal(AvaloniaProperty.UnsetValue, result); + Assert.Equal(new BindingNotification(new NullReferenceException(), BindingErrorType.Error), result); } [Fact] @@ -325,7 +407,14 @@ namespace Avalonia.Markup.UnitTests.Data root = null; update.OnNext(Unit.Default); - Assert.Equal(new[] { "foo", "bar", AvaloniaProperty.UnsetValue }, result); + Assert.Equal( + new object[] + { + "foo", + "bar", + new BindingNotification(new NullReferenceException(), BindingErrorType.Error) + }, + result); Assert.Equal(0, first.PropertyChangedSubscriptionCount); Assert.Equal(0, second.PropertyChangedSubscriptionCount); diff --git a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_SetValue.cs b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_SetValue.cs index 4dabd34460..3238435841 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_SetValue.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/ExpressionObserverTests_SetValue.cs @@ -18,7 +18,10 @@ namespace Avalonia.Markup.UnitTests.Data var data = new { Foo = "foo" }; var target = new ExpressionObserver(data, "Foo"); - target.SetValue("bar"); + using (target.Subscribe(_ => { })) + { + target.SetValue("bar"); + } Assert.Equal("foo", data.Foo); } @@ -29,7 +32,10 @@ namespace Avalonia.Markup.UnitTests.Data var data = new Class1 { Foo = new Class2 { Bar = "bar" } }; var target = new ExpressionObserver(data, "Foo.Bar"); - target.SetValue("foo"); + using (target.Subscribe(_ => { })) + { + target.SetValue("foo"); + } Assert.Equal("foo", data.Foo.Bar); }