From 09d0c3ae3eab56dc80778774c7d9ed963e94ffb5 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 20 Jul 2022 12:09:08 +0200 Subject: [PATCH] Refactored style activators. - Always evaluate the active state from current information, don't rely on subscriptions to fire as the current state may not be up-to-date - Don't notify the `IStyleActivatorSink` of a change immediately on subscription --- .../Styling/Activators/AndActivator.cs | 55 ++++++------------- .../Styling/Activators/IStyleActivator.cs | 14 +++++ .../Styling/Activators/NotActivator.cs | 8 +-- .../Styling/Activators/NthChildActivator.cs | 19 +++---- .../Styling/Activators/OrActivator.cs | 49 ++++------------- .../Activators/PropertyEqualsActivator.cs | 13 ++--- .../Styling/Activators/StyleActivatorBase.cs | 54 ++++++++++++++---- .../Styling/Activators/StyleClassActivator.cs | 25 ++------- src/Avalonia.Base/Styling/StyleInstance.cs | 16 +----- .../Styling/SelectorTests_Nesting.cs | 8 ++- .../Styling/StyleActivatorExtensions.cs | 6 ++ 11 files changed, 117 insertions(+), 150 deletions(-) diff --git a/src/Avalonia.Base/Styling/Activators/AndActivator.cs b/src/Avalonia.Base/Styling/Activators/AndActivator.cs index 953d0a4953..a189072dbc 100644 --- a/src/Avalonia.Base/Styling/Activators/AndActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/AndActivator.cs @@ -1,6 +1,4 @@ -#nullable enable - -using System.Collections.Generic; +using System.Collections.Generic; namespace Avalonia.Styling.Activators { @@ -11,49 +9,35 @@ namespace Avalonia.Styling.Activators internal class AndActivator : StyleActivatorBase, IStyleActivatorSink { private List? _sources; - private ulong _flags; - private ulong _mask; public int Count => _sources?.Count ?? 0; - public override bool IsActive - { - get - { - if (_sources is null) - return false; - - foreach (var source in _sources) - { - if (!source.IsActive) - return false; - } - - return true; - } - } - public void Add(IStyleActivator activator) { + if (IsSubscribed) + throw new AvaloniaInternalException("AndActivator is already subscribed."); _sources ??= new List(); _sources.Add(activator); } - void IStyleActivatorSink.OnNext(bool value, int tag) + void IStyleActivatorSink.OnNext(bool value, int tag) => ReevaluateIsActive(); + + protected override bool EvaluateIsActive() { - if (value) - { - _flags |= 1ul << tag; - } - else - { - _flags &= ~(1ul << tag); - } + if (_sources is null || _sources.Count == 0) + return true; + + var count = _sources.Count; + var mask = (1ul << count) - 1; + var flags = 0UL; - if (_mask != 0) + for (var i = 0; i < count; ++i) { - PublishNext(_flags == _mask); + if (_sources[i].IsActive) + flags |= 1ul << i; } + + return flags == mask; } protected override void Initialize() @@ -66,9 +50,6 @@ namespace Avalonia.Styling.Activators { source.Subscribe(this, i++); } - - _mask = (1ul << Count) - 1; - PublishNext(_flags == _mask); } } @@ -81,8 +62,6 @@ namespace Avalonia.Styling.Activators source.Unsubscribe(this); } } - - _mask = 0; } } } diff --git a/src/Avalonia.Base/Styling/Activators/IStyleActivator.cs b/src/Avalonia.Base/Styling/Activators/IStyleActivator.cs index 3dee6aaab6..f4b307177f 100644 --- a/src/Avalonia.Base/Styling/Activators/IStyleActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/IStyleActivator.cs @@ -21,13 +21,27 @@ namespace Avalonia.Styling.Activators /// /// Gets a value indicating whether the style is activated. /// + /// + /// This property should read directly from its inputs and not rely on any subscriptions + /// to fire in order to be up-to-date. If a change in active state occurs when reading + /// this property then any subscribed should not be + /// notified of the change. + /// bool IsActive { get; } + /// + /// Gets a value indicating whether the style is subscribed. + /// + bool IsSubscribed { get; } + /// /// Subscribes to the activator. /// /// The listener. /// An optional tag. + /// + /// This method should not call . + /// void Subscribe(IStyleActivatorSink sink, int tag = 0); /// diff --git a/src/Avalonia.Base/Styling/Activators/NotActivator.cs b/src/Avalonia.Base/Styling/Activators/NotActivator.cs index 1735b265d1..06a2bfd2c7 100644 --- a/src/Avalonia.Base/Styling/Activators/NotActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/NotActivator.cs @@ -1,6 +1,4 @@ -#nullable enable - -namespace Avalonia.Styling.Activators +namespace Avalonia.Styling.Activators { /// /// An which inverts the state of an input activator. @@ -9,8 +7,8 @@ namespace Avalonia.Styling.Activators { private readonly IStyleActivator _source; public NotActivator(IStyleActivator source) => _source = source; - public override bool IsActive => !_source.IsActive; - void IStyleActivatorSink.OnNext(bool value, int tag) => PublishNext(!value); + void IStyleActivatorSink.OnNext(bool value, int tag) => ReevaluateIsActive(); + protected override bool EvaluateIsActive() => !_source.IsActive; protected override void Initialize() => _source.Subscribe(this, 0); protected override void Deinitialize() => _source.Unsubscribe(this); } diff --git a/src/Avalonia.Base/Styling/Activators/NthChildActivator.cs b/src/Avalonia.Base/Styling/Activators/NthChildActivator.cs index 87f181b884..33d4cd0824 100644 --- a/src/Avalonia.Base/Styling/Activators/NthChildActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/NthChildActivator.cs @@ -26,30 +26,25 @@ namespace Avalonia.Styling.Activators _reversed = reversed; } - public override bool IsActive => NthChildSelector.Evaluate(_control, _provider, _step, _offset, _reversed).IsMatch; - - protected override void Initialize() + protected override bool EvaluateIsActive() { - PublishNext(IsActive); - _provider.ChildIndexChanged += ChildIndexChanged; + return NthChildSelector.Evaluate(_control, _provider, _step, _offset, _reversed).IsMatch; } - protected override void Deinitialize() - { - _provider.ChildIndexChanged -= ChildIndexChanged; - } + protected override void Initialize() => _provider.ChildIndexChanged += ChildIndexChanged; + protected override void Deinitialize() => _provider.ChildIndexChanged -= ChildIndexChanged; private void ChildIndexChanged(object? sender, ChildIndexChangedEventArgs e) { // Run matching again if: // 1. Selector is reversed, so other item insertion/deletion might affect total count without changing subscribed item index. - // 2. e.Child is null, when all children indeces were changed. + // 2. e.Child is null, when all children indices were changed. // 3. Subscribed child index was changed. if (_reversed - || e.Child is null + || e.Child is null || e.Child == _control) { - PublishNext(IsActive); + ReevaluateIsActive(); } } } diff --git a/src/Avalonia.Base/Styling/Activators/OrActivator.cs b/src/Avalonia.Base/Styling/Activators/OrActivator.cs index 7b206a1d34..52f25c36c4 100644 --- a/src/Avalonia.Base/Styling/Activators/OrActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/OrActivator.cs @@ -1,6 +1,4 @@ -#nullable enable - -using System.Collections.Generic; +using System.Collections.Generic; namespace Avalonia.Styling.Activators { @@ -11,49 +9,29 @@ namespace Avalonia.Styling.Activators internal class OrActivator : StyleActivatorBase, IStyleActivatorSink { private List? _sources; - private ulong _flags; - private bool _initializing; public int Count => _sources?.Count ?? 0; - public override bool IsActive - { - get - { - if (_sources is null) - return false; - - foreach (var source in _sources) - { - if (source.IsActive) - return true; - } - - return false; - } - } - public void Add(IStyleActivator activator) { _sources ??= new List(); _sources.Add(activator); } - void IStyleActivatorSink.OnNext(bool value, int tag) + void IStyleActivatorSink.OnNext(bool value, int tag) => ReevaluateIsActive(); + + protected override bool EvaluateIsActive() { - if (value) - { - _flags |= 1ul << tag; - } - else - { - _flags &= ~(1ul << tag); - } + if (_sources is null || _sources.Count == 0) + return true; - if (!_initializing) + foreach (var source in _sources) { - PublishNext(_flags != 0); + if (source.IsActive) + return true; } + + return false; } protected override void Initialize() @@ -62,15 +40,10 @@ namespace Avalonia.Styling.Activators { var i = 0; - _initializing = true; - foreach (var source in _sources) { source.Subscribe(this, i++); } - - _initializing = false; - PublishNext(_flags != 0); } } diff --git a/src/Avalonia.Base/Styling/Activators/PropertyEqualsActivator.cs b/src/Avalonia.Base/Styling/Activators/PropertyEqualsActivator.cs index d98959d40b..388671f3f7 100644 --- a/src/Avalonia.Base/Styling/Activators/PropertyEqualsActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/PropertyEqualsActivator.cs @@ -1,7 +1,5 @@ using System; -#nullable enable - namespace Avalonia.Styling.Activators { /// @@ -24,13 +22,10 @@ namespace Avalonia.Styling.Activators _value = value; } - public override bool IsActive + protected override bool EvaluateIsActive() { - get - { - var value = _control.GetValue(_property); - return PropertyEqualsSelector.Compare(_property.PropertyType, value, _value); - } + var value = _control.GetValue(_property); + return PropertyEqualsSelector.Compare(_property.PropertyType, value, _value); } protected override void Initialize() @@ -42,6 +37,6 @@ namespace Avalonia.Styling.Activators void IObserver.OnCompleted() { } void IObserver.OnError(Exception error) { } - void IObserver.OnNext(object? value) => PublishNext(IsActive); + void IObserver.OnNext(object? value) => ReevaluateIsActive(); } } diff --git a/src/Avalonia.Base/Styling/Activators/StyleActivatorBase.cs b/src/Avalonia.Base/Styling/Activators/StyleActivatorBase.cs index c9645dcbc2..a4639b8feb 100644 --- a/src/Avalonia.Base/Styling/Activators/StyleActivatorBase.cs +++ b/src/Avalonia.Base/Styling/Activators/StyleActivatorBase.cs @@ -7,22 +7,23 @@ namespace Avalonia.Styling.Activators { private IStyleActivatorSink? _sink; private int _tag; - private bool? _value; + private bool _value; - public abstract bool IsActive { get; } + public bool IsActive => _value = EvaluateIsActive(); + + public bool IsSubscribed => _sink is not null; public void Subscribe(IStyleActivatorSink sink, int tag = 0) { if (_sink is null) { + Initialize(); _sink = sink; _tag = tag; - _value = null; - Initialize(); } else { - throw new AvaloniaInternalException("Cannot subscribe to a StyleActivator more than once."); + throw new AvaloniaInternalException("StyleActivator is already subscribed."); } } @@ -37,22 +38,51 @@ namespace Avalonia.Styling.Activators Deinitialize(); } - public void PublishNext(bool value) + public void Dispose() { - if (_value != value) + _sink = null; + Deinitialize(); + } + + /// + /// Evaluates the value. + /// + /// + /// This method should read directly from its inputs and not rely on any subscriptions to + /// fire in order to be up-to-date. + /// + protected abstract bool EvaluateIsActive(); + + /// + /// Called from a derived class when the state should be re-evaluated + /// and the subscriber notified of any change. + /// + /// + /// The evaluated active state; + /// + protected bool ReevaluateIsActive() + { + var value = EvaluateIsActive(); + + if (value != _value) { _value = value; _sink?.OnNext(value, _tag); } - } - public void Dispose() - { - _sink = null; - Deinitialize(); + return value; } + /// + /// Called in response to a to allow the + /// derived class to set up any necessary subscriptions. + /// protected abstract void Initialize(); + + /// + /// Called in response to an or + /// to allow the derived class to dispose any active subscriptions. + /// protected abstract void Deinitialize(); } } diff --git a/src/Avalonia.Base/Styling/Activators/StyleClassActivator.cs b/src/Avalonia.Base/Styling/Activators/StyleClassActivator.cs index de27e797ea..faeacfe0f9 100644 --- a/src/Avalonia.Base/Styling/Activators/StyleClassActivator.cs +++ b/src/Avalonia.Base/Styling/Activators/StyleClassActivator.cs @@ -1,10 +1,6 @@ using System.Collections.Generic; -using System.Collections.Specialized; -using Avalonia.Collections; using Avalonia.Controls; -#nullable enable - namespace Avalonia.Styling.Activators { /// @@ -22,8 +18,6 @@ namespace Avalonia.Styling.Activators _match = match; } - public override bool IsActive => AreClassesMatching(_classes, _match); - public static bool AreClassesMatching(IReadOnlyList classes, IList toMatch) { int remainingMatches = toMatch.Count; @@ -54,20 +48,9 @@ namespace Avalonia.Styling.Activators return remainingMatches == 0; } - void IClassesChangedListener.Changed() - { - PublishNext(IsActive); - } - - protected override void Initialize() - { - PublishNext(IsActive); - _classes.AddListener(this); - } - - protected override void Deinitialize() - { - _classes.RemoveListener(this); - } + void IClassesChangedListener.Changed() => ReevaluateIsActive(); + protected override bool EvaluateIsActive() => AreClassesMatching(_classes, _match); + protected override void Initialize() => _classes.AddListener(this); + protected override void Deinitialize() => _classes.RemoveListener(this); } } diff --git a/src/Avalonia.Base/Styling/StyleInstance.cs b/src/Avalonia.Base/Styling/StyleInstance.cs index 377bf31a32..a6ca2421cd 100644 --- a/src/Avalonia.Base/Styling/StyleInstance.cs +++ b/src/Avalonia.Base/Styling/StyleInstance.cs @@ -20,8 +20,6 @@ namespace Avalonia.Styling { private readonly IStyleActivator? _activator; private List? _setters; - private bool _isActivatorInitializing; - private bool _isActivatorSubscribed; public StyleInstance(IStyle style, IStyleActivator? activator) { @@ -36,14 +34,8 @@ namespace Avalonia.Styling { get { - if (_activator is object && !_isActivatorSubscribed) - { - _isActivatorInitializing = true; + if (_activator?.IsSubscribed == false) _activator.Subscribe(this); - _isActivatorInitializing = false; - _isActivatorSubscribed = true; - } - return _activator?.IsActive ?? true; } } @@ -65,10 +57,6 @@ namespace Avalonia.Styling _activator?.Dispose(); } - void IStyleActivatorSink.OnNext(bool value, int tag) - { - if (!_isActivatorInitializing) - Owner?.OnFrameActivationChanged(this); - } + void IStyleActivatorSink.OnNext(bool value, int tag) => Owner?.OnFrameActivationChanged(this); } } diff --git a/tests/Avalonia.Base.UnitTests/Styling/SelectorTests_Nesting.cs b/tests/Avalonia.Base.UnitTests/Styling/SelectorTests_Nesting.cs index 9048b488b6..b46389d71f 100644 --- a/tests/Avalonia.Base.UnitTests/Styling/SelectorTests_Nesting.cs +++ b/tests/Avalonia.Base.UnitTests/Styling/SelectorTests_Nesting.cs @@ -292,7 +292,13 @@ namespace Avalonia.Base.UnitTests.Styling private class ActivatorSink : IStyleActivatorSink { - public ActivatorSink(IStyleActivator source) => source.Subscribe(this); + public ActivatorSink(IStyleActivator source) + { + source.Subscribe(this); + Active = source.IsActive; + } + + public bool Active { get; private set; } public void OnNext(bool value, int tag) => Active = value; } diff --git a/tests/Avalonia.Base.UnitTests/Styling/StyleActivatorExtensions.cs b/tests/Avalonia.Base.UnitTests/Styling/StyleActivatorExtensions.cs index f5b3bb40a3..af75513df9 100644 --- a/tests/Avalonia.Base.UnitTests/Styling/StyleActivatorExtensions.cs +++ b/tests/Avalonia.Base.UnitTests/Styling/StyleActivatorExtensions.cs @@ -33,8 +33,14 @@ namespace Avalonia.Base.UnitTests.Styling private readonly IStyleActivator _source; public ObservableAdapter(IStyleActivator source) => _source = source; + protected override void Initialize() => _source.Subscribe(this); protected override void Deinitialize() => _source.Unsubscribe(this); + + protected override void Subscribed(IObserver observer, bool first) + { + observer.OnNext(_source.IsActive); + } void IStyleActivatorSink.OnNext(bool value, int tag) {