From 18537addd78b01bb14528cdb2420397b2776a023 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 28 Feb 2020 00:49:04 +0100 Subject: [PATCH] Fix problems with setter binding priorities. We need to make sure to set up the bindings as soon as the setter is instanced, even if it's not activated immediately. --- .../SingleSubscriberObservableBase.cs | 2 +- src/Avalonia.Styling/Styling/ISetter.cs | 5 +- .../Styling/ISetterInstance.cs | 22 ++- .../Styling/PropertySetterBindingInstance.cs | 141 ++++++++++++++++-- .../Styling/PropertySetterInstance.cs | 72 ++++++--- src/Avalonia.Styling/Styling/Setter.cs | 75 +++++----- src/Avalonia.Styling/Styling/StyleInstance.cs | 25 +++- .../StyleTests.cs | 2 +- .../Avalonia.Styling.UnitTests/SetterTests.cs | 28 ++-- .../Avalonia.Styling.UnitTests/StyleTests.cs | 78 ++++++++++ 10 files changed, 360 insertions(+), 90 deletions(-) diff --git a/src/Avalonia.Base/Reactive/SingleSubscriberObservableBase.cs b/src/Avalonia.Base/Reactive/SingleSubscriberObservableBase.cs index cd8ce2cd80..d3ac3dac8a 100644 --- a/src/Avalonia.Base/Reactive/SingleSubscriberObservableBase.cs +++ b/src/Avalonia.Base/Reactive/SingleSubscriberObservableBase.cs @@ -36,7 +36,7 @@ namespace Avalonia.Reactive return this; } - void IDisposable.Dispose() + public virtual void Dispose() { Unsubscribed(); _observer = null; diff --git a/src/Avalonia.Styling/Styling/ISetter.cs b/src/Avalonia.Styling/Styling/ISetter.cs index 44e43caf85..3d945c6772 100644 --- a/src/Avalonia.Styling/Styling/ISetter.cs +++ b/src/Avalonia.Styling/Styling/ISetter.cs @@ -16,13 +16,12 @@ namespace Avalonia.Styling /// Instances a setter on a control. /// /// The control. - /// Whether the parent style has an activator. /// An . /// /// This method should return an which can be used to apply /// the setter to the specified control. Note that it should not apply the setter value - /// until is called. + /// until is called. /// - ISetterInstance Instance(IStyleable target, bool hasActivator); + ISetterInstance Instance(IStyleable target); } } diff --git a/src/Avalonia.Styling/Styling/ISetterInstance.cs b/src/Avalonia.Styling/Styling/ISetterInstance.cs index ebfc227d12..a299a87b64 100644 --- a/src/Avalonia.Styling/Styling/ISetterInstance.cs +++ b/src/Avalonia.Styling/Styling/ISetterInstance.cs @@ -1,20 +1,40 @@ #nullable enable +using System; + namespace Avalonia.Styling { /// /// Represents a setter that has been instanced on a control. /// - public interface ISetterInstance + public interface ISetterInstance : IDisposable { + /// + /// Starts the setter instance. + /// + /// Whether the parent style has an activator. + /// + /// If is false then the setter should be immediately + /// applied and and should not be called. + /// If true, then bindings etc should be initiated but not produce a value until + /// called. + /// + public void Start(bool hasActivator); + /// /// Activates the setter. /// + /// + /// Should only be called if hasActivator was true when was called. + /// public void Activate(); /// /// Deactivates the setter. /// + /// + /// Should only be called if hasActivator was true when was called. + /// public void Deactivate(); } } diff --git a/src/Avalonia.Styling/Styling/PropertySetterBindingInstance.cs b/src/Avalonia.Styling/Styling/PropertySetterBindingInstance.cs index 74d7f98398..12055446a5 100644 --- a/src/Avalonia.Styling/Styling/PropertySetterBindingInstance.cs +++ b/src/Avalonia.Styling/Styling/PropertySetterBindingInstance.cs @@ -1,37 +1,85 @@ using System; +using System.Reactive.Subjects; using Avalonia.Data; +using Avalonia.Reactive; #nullable enable namespace Avalonia.Styling { - internal class PropertySetterBindingInstance : ISetterInstance + internal class PropertySetterBindingInstance : SingleSubscriberObservableBase>, + ISubject>, + ISetterInstance { private readonly IStyleable _target; - private readonly AvaloniaProperty _property; - private readonly BindingPriority _priority; + private readonly StyledPropertyBase? _styledProperty; + private readonly DirectPropertyBase? _directProperty; private readonly InstancedBinding _binding; + private readonly Inner _inner; + private BindingValue _value; private IDisposable? _subscription; + private IDisposable? _subscriptionTwoWay; private bool _isActive; public PropertySetterBindingInstance( IStyleable target, - AvaloniaProperty property, - BindingPriority priority, + StyledPropertyBase property, IBinding binding) { _target = target; - _property = property; - _priority = priority; - _binding = binding.Initiate(target, property).WithPriority(priority); + _styledProperty = property; + _binding = binding.Initiate(_target, property); + _inner = new Inner(this); + } + + public PropertySetterBindingInstance( + IStyleable target, + DirectPropertyBase property, + IBinding binding) + { + _target = target; + _directProperty = property; + _binding = binding.Initiate(_target, property); + _inner = new Inner(this); + } + + public void Start(bool hasActivator) + { + _isActive = !hasActivator; + + if (_styledProperty is object) + { + if (_binding.Mode != BindingMode.OneWayToSource) + { + var priority = hasActivator ? BindingPriority.StyleTrigger : BindingPriority.Style; + _subscription = _target.Bind(_styledProperty, this, priority); + } + + if (_binding.Mode == BindingMode.TwoWay) + { + _subscriptionTwoWay = _target.GetBindingObservable(_styledProperty).Subscribe(this); + } + } + else + { + if (_binding.Mode != BindingMode.OneWayToSource) + { + _subscription = _target.Bind(_directProperty!, this); + } + + if (_binding.Mode == BindingMode.TwoWay) + { + _subscriptionTwoWay = _target.GetBindingObservable(_directProperty!).Subscribe(this); + } + } } public void Activate() { if (!_isActive) { - _subscription = BindingOperations.Apply(_target, _property, _binding, null); _isActive = true; + PublishNext(); } } @@ -39,10 +87,81 @@ namespace Avalonia.Styling { if (_isActive) { - _subscription?.Dispose(); - _subscription = null; _isActive = false; + PublishNext(); + } + } + + public override void Dispose() + { + if (_subscription is object) + { + var sub = _subscription; + _subscription = null; + sub.Dispose(); + } + + if (_subscriptionTwoWay is object) + { + var sub = _subscriptionTwoWay; + _subscriptionTwoWay = null; + sub.Dispose(); + } + + base.Dispose(); + } + + void IObserver>.OnCompleted() + { + // This is the observable coming from the target control. It should not complete. + } + + void IObserver>.OnError(Exception error) + { + // This is the observable coming from the target control. It should not error. + } + + void IObserver>.OnNext(BindingValue value) + { + if (value.HasValue && _isActive) + { + _binding.Subject.OnNext(value.Value); } } + + protected override void Subscribed() + { + _subscription = _binding.Observable.Subscribe(_inner); + } + + protected override void Unsubscribed() + { + _subscription?.Dispose(); + _subscription = null; + } + + private void PublishNext() + { + PublishNext(_isActive ? _value : default); + } + + private void ConvertAndPublishNext(object? value) + { + _value = value is T v ? v : BindingValue.FromUntyped(value).Convert(); + + if (_isActive) + { + PublishNext(); + } + } + + private class Inner : IObserver + { + private readonly PropertySetterBindingInstance _owner; + public Inner(PropertySetterBindingInstance owner) => _owner = owner; + public void OnCompleted() => _owner.PublishCompleted(); + public void OnError(Exception error) => _owner.PublishError(error); + public void OnNext(object? value) => _owner.ConvertAndPublishNext(value); + } } } diff --git a/src/Avalonia.Styling/Styling/PropertySetterInstance.cs b/src/Avalonia.Styling/Styling/PropertySetterInstance.cs index 284ca8cdd0..530a09a01a 100644 --- a/src/Avalonia.Styling/Styling/PropertySetterInstance.cs +++ b/src/Avalonia.Styling/Styling/PropertySetterInstance.cs @@ -1,16 +1,17 @@ using System; using Avalonia.Data; +using Avalonia.Reactive; #nullable enable namespace Avalonia.Styling { - internal class PropertySetterInstance : ISetterInstance + internal class PropertySetterInstance : SingleSubscriberObservableBase>, + ISetterInstance { private readonly IStyleable _target; private readonly StyledPropertyBase? _styledProperty; private readonly DirectPropertyBase? _directProperty; - private readonly BindingPriority _priority; private readonly T _value; private IDisposable? _subscription; private bool _isActive; @@ -18,41 +19,55 @@ namespace Avalonia.Styling public PropertySetterInstance( IStyleable target, StyledPropertyBase property, - BindingPriority priority, T value) { _target = target; _styledProperty = property; - _priority = priority; _value = value; } public PropertySetterInstance( IStyleable target, DirectPropertyBase property, - BindingPriority priority, T value) { _target = target; _directProperty = property; - _priority = priority; _value = value; } - public void Activate() + public void Start(bool hasActivator) { - if (!_isActive) + if (hasActivator) { if (_styledProperty is object) { - _subscription = _target.SetValue(_styledProperty, _value, _priority); + _subscription = _target.Bind(_styledProperty, this, BindingPriority.StyleTrigger); + } + else + { + _subscription = _target.Bind(_directProperty, this); + } + } + else + { + if (_styledProperty is object) + { + _subscription = _target.SetValue(_styledProperty, _value, BindingPriority.Style); } else { _target.SetValue(_directProperty!, _value); } + } + } + public void Activate() + { + if (!_isActive) + { _isActive = true; + PublishNext(); } } @@ -60,23 +75,40 @@ namespace Avalonia.Styling { if (_isActive) { - if (_subscription is null) + _isActive = false; + PublishNext(); + } + } + + public override void Dispose() + { + if (_subscription is object) + { + var sub = _subscription; + _subscription = null; + sub.Dispose(); + } + else if (_isActive) + { + if (_styledProperty is object) { - if (_styledProperty is object) - { - _target.ClearValue(_styledProperty); - } - else - { - _target.ClearValue(_directProperty!); - } + _target.ClearValue(_styledProperty); } else { - _subscription.Dispose(); - _subscription = null; + _target.ClearValue(_directProperty); } } + + base.Dispose(); + } + + protected override void Subscribed() => PublishNext(); + protected override void Unsubscribed() { } + + private void PublishNext() + { + PublishNext(_isActive ? new BindingValue(_value) : default); } } } diff --git a/src/Avalonia.Styling/Styling/Setter.cs b/src/Avalonia.Styling/Styling/Setter.cs index 08e8a699b8..691a3087a5 100644 --- a/src/Avalonia.Styling/Styling/Setter.cs +++ b/src/Avalonia.Styling/Styling/Setter.cs @@ -61,7 +61,7 @@ namespace Avalonia.Styling } } - public ISetterInstance Instance(IStyleable target, bool hasActivator) + public ISetterInstance Instance(IStyleable target) { target = target ?? throw new ArgumentNullException(nameof(target)); @@ -70,60 +70,67 @@ namespace Avalonia.Styling throw new InvalidOperationException("Setter.Property must be set."); } - var priority = hasActivator ? BindingPriority.StyleTrigger : BindingPriority.Style; + var value = Value; - if (Value is IBinding binding) + if (value is ITemplate template && + !typeof(ITemplate).IsAssignableFrom(Property.PropertyType)) { - return new PropertySetterBindingInstance(target, Property, priority, binding); + value = template.Build(); } - else + + var data = new SetterVisitorData { - var value = Value; - - if (value is ITemplate template && - !typeof(ITemplate).IsAssignableFrom(Property.PropertyType)) - { - value = template.Build(); - } - - var data = new SetterVisitorData - { - target = target, - priority = priority, - value = value, - }; - - Property.Accept(this, ref data); - return data.result!; - } + target = target, + value = value, + }; + + Property.Accept(this, ref data); + return data.result!; } void IAvaloniaPropertyVisitor.Visit( StyledPropertyBase property, ref SetterVisitorData data) { - data.result = new PropertySetterInstance( - data.target, - property, - data.priority, - (T)data.value); + if (data.value is IBinding binding) + { + data.result = new PropertySetterBindingInstance( + data.target, + property, + binding); + } + else + { + data.result = new PropertySetterInstance( + data.target, + property, + (T)data.value); + } } void IAvaloniaPropertyVisitor.Visit( DirectPropertyBase property, ref SetterVisitorData data) { - data.result = new PropertySetterInstance( - data.target, - property, - data.priority, - (T)data.value); + if (data.value is IBinding binding) + { + data.result = new PropertySetterBindingInstance( + data.target, + property, + binding); + } + else + { + data.result = new PropertySetterInstance( + data.target, + property, + (T)data.value); + } } private struct SetterVisitorData { public IStyleable target; - public BindingPriority priority; public object? value; public ISetterInstance? result; } diff --git a/src/Avalonia.Styling/Styling/StyleInstance.cs b/src/Avalonia.Styling/Styling/StyleInstance.cs index 6977f19f59..a1ad8b6477 100644 --- a/src/Avalonia.Styling/Styling/StyleInstance.cs +++ b/src/Avalonia.Styling/Styling/StyleInstance.cs @@ -23,12 +23,14 @@ namespace Avalonia.Styling Source = source ?? throw new ArgumentNullException(nameof(source)); Target = target ?? throw new ArgumentNullException(nameof(target)); - _setters = new List(setters.Count); + var setterCount = setters.Count; + + _setters = new List(setterCount); _activator = activator; - foreach (var setter in setters) + for (var i = 0; i < setterCount; ++i) { - _setters.Add(setter.Instance(target, activator is object)); + _setters.Add(setters[i].Instance(Target)); } } @@ -37,19 +39,26 @@ namespace Avalonia.Styling public void Start() { - if (_activator == null) + var hasActivator = _activator is object; + + foreach (var setter in _setters) { - ActivatorChanged(true); + setter.Start(hasActivator); } - else + + if (hasActivator) { - _activator.Subscribe(this, 0); + _activator!.Subscribe(this, 0); } } public void Dispose() { - ActivatorChanged(false); + foreach (var setter in _setters) + { + setter.Dispose(); + } + _activator?.Dispose(); } diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/StyleTests.cs b/tests/Avalonia.Markup.Xaml.UnitTests/StyleTests.cs index 44a40af93d..7069c1035f 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/StyleTests.cs +++ b/tests/Avalonia.Markup.Xaml.UnitTests/StyleTests.cs @@ -53,7 +53,7 @@ namespace Avalonia.Markup.Xaml.UnitTests } }; - setter.Instance(control, false).Activate(); + setter.Instance(control).Start(false); Assert.Equal("foo", control.Text); control.Text = "bar"; diff --git a/tests/Avalonia.Styling.UnitTests/SetterTests.cs b/tests/Avalonia.Styling.UnitTests/SetterTests.cs index de0882f6ff..a9a40fd29f 100644 --- a/tests/Avalonia.Styling.UnitTests/SetterTests.cs +++ b/tests/Avalonia.Styling.UnitTests/SetterTests.cs @@ -33,7 +33,7 @@ namespace Avalonia.Styling.UnitTests var style = Mock.Of(); var setter = new Setter(TextBlock.TextProperty, binding); - setter.Instance(control, false).Activate(); + setter.Instance(control).Start(false); Assert.Equal("foo", control.Text); } @@ -46,7 +46,7 @@ namespace Avalonia.Styling.UnitTests var style = Mock.Of(); var setter = new Setter(Decorator.ChildProperty, template); - setter.Instance(control, false).Activate(); + setter.Instance(control).Start(false); Assert.IsType(control.Child); } @@ -63,8 +63,10 @@ namespace Avalonia.Styling.UnitTests }; var setter = new Setter(Decorator.TagProperty, binding); - var instance = setter.Instance(control, true); + var instance = setter.Instance(control); + instance.Start(true); instance.Activate(); + Assert.Equal("foobar", control.Tag); // Issue #1218 caused TestConverter.ConvertBack to throw here. @@ -79,7 +81,7 @@ namespace Avalonia.Styling.UnitTests var style = Mock.Of