From 00f8dfabc050d5e65071062e5bc18ebd4e3c73bf Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 3 Jul 2018 16:09:23 -0500 Subject: [PATCH] Use SingleOrQueue instead of Queue in DeferredSetter. Make sure tests pass. --- src/Avalonia.Base/AvaloniaObject.cs | 49 +++++------------ src/Avalonia.Base/IPriorityValueOwner.cs | 2 +- src/Avalonia.Base/PriorityValue.cs | 9 +++- src/Avalonia.Base/Utilities/DeferredSetter.cs | 9 ++-- src/Avalonia.Base/Utilities/SingleOrQueue.cs | 54 +++++++++++++++++++ src/Avalonia.Base/ValueStore.cs | 8 +-- .../PriorityValueTests.cs | 52 ++++++++++-------- .../Utilities/SingleOrQueueTests.cs | 50 +++++++++++++++++ 8 files changed, 165 insertions(+), 68 deletions(-) create mode 100644 src/Avalonia.Base/Utilities/SingleOrQueue.cs create mode 100644 tests/Avalonia.Base.UnitTests/Utilities/SingleOrQueueTests.cs diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 9fd2acbe5f..db4e52ee0f 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -45,21 +45,8 @@ namespace Avalonia /// private EventHandler _propertyChanged; - private DeferredSetter _directDeferredSetter; private ValueStore _values; - - /// - /// Delayed setter helper for direct properties. Used to fix #855. - /// - private DeferredSetter DirectPropertyDeferredSetter - { - get - { - return _directDeferredSetter ?? - (_directDeferredSetter = new DeferredSetter()); - } - } - + private ValueStore Values => _values ?? (_values = new ValueStore(this)); /// /// Initializes a new instance of the class. @@ -223,9 +210,9 @@ namespace Avalonia { return ((IDirectPropertyAccessor)GetRegistered(property)).GetValue(this); } - else if (_values != null) + else if (Values != null) { - var result = _values.GetValue(property); + var result = Values.GetValue(property); if (result == AvaloniaProperty.UnsetValue) { @@ -263,7 +250,7 @@ namespace Avalonia Contract.Requires(property != null); VerifyAccess(); - return _values?.IsAnimating(property) ?? false; + return Values?.IsAnimating(property) ?? false; } /// @@ -280,7 +267,7 @@ namespace Avalonia Contract.Requires(property != null); VerifyAccess(); - return _values?.IsSet(property) ?? false; + return Values?.IsSet(property) ?? false; } /// @@ -376,12 +363,7 @@ namespace Avalonia description, priority); - if (_values == null) - { - _values = new ValueStore(this); - } - - return _values.AddBinding(property, source, priority); + return Values.AddBinding(property, source, priority); } } @@ -412,7 +394,7 @@ namespace Avalonia public void Revalidate(AvaloniaProperty property) { VerifyAccess(); - _values?.Revalidate(property); + Values?.Revalidate(property); } internal void PriorityValueChanged(AvaloniaProperty property, int priority, object oldValue, object newValue) @@ -454,7 +436,7 @@ namespace Avalonia /// Gets all priority values set on the object. /// /// A collection of property/value tuples. - internal IDictionary GetSetValues() => _values?.GetSetValues(); + internal IDictionary GetSetValues() => Values?.GetSetValues(); /// /// Forces revalidation of properties when a property value changes. @@ -564,15 +546,15 @@ namespace Avalonia T value) { Contract.Requires(setterCallback != null); - return _values.Setter.SetAndNotify( + return Values.Setter.SetAndNotify( property, ref field, - ((object value, int) update, ref T backing, Action notify) => + (object update, ref T backing, Action notify) => { - setterCallback((T)update.value, ref backing, notify); + setterCallback((T)update, ref backing, notify); return true; }, - (value, 0)); + value); } /// @@ -735,13 +717,8 @@ namespace Avalonia originalValue?.GetType().FullName ?? "(null)")); } - if (_values == null) - { - _values = new ValueStore(this); - } - LogPropertySet(property, value, priority); - _values.AddValue(property, value, (int)priority); + Values.AddValue(property, value, (int)priority); } /// diff --git a/src/Avalonia.Base/IPriorityValueOwner.cs b/src/Avalonia.Base/IPriorityValueOwner.cs index 3a931d6b35..8eaefac4f2 100644 --- a/src/Avalonia.Base/IPriorityValueOwner.cs +++ b/src/Avalonia.Base/IPriorityValueOwner.cs @@ -33,6 +33,6 @@ namespace Avalonia /// void VerifyAccess(); - DeferredSetter Setter { get; } + DeferredSetter Setter { get; } } } diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index bc09a41e5f..03094e2236 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -248,6 +248,12 @@ namespace Avalonia (value, priority)); } + private bool UpdateCore( + object update, + ref (object value, int priority) backing, + Action notify) + => UpdateCore(((object, int))update, ref backing, notify); + private bool UpdateCore( (object value, int priority) update, ref (object value, int priority) backing, @@ -255,13 +261,14 @@ namespace Avalonia { var val = update.value; var notification = val as BindingNotification; + object castValue; if (notification != null) { val = (notification.HasValue) ? notification.Value : null; } - if (TypeUtilities.TryConvertImplicit(_valueType, val, out object castValue)) + if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) { var old = backing.value; diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index fdfa160134..9e3cf023c7 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -8,6 +8,7 @@ namespace Avalonia.Utilities { /// /// A utility class to enable deferring assignment until after property-changed notifications are sent. + /// Used to fix #855. /// /// The type of the object that represents the property. /// The type of value with which to track the delayed assignment. @@ -37,13 +38,13 @@ namespace Avalonia.Utilities { public bool Notifying { get; set; } - private Queue pendingValues; + private SingleOrQueue pendingValues; - public Queue PendingValues + public SingleOrQueue PendingValues { get { - return pendingValues ?? (pendingValues = new Queue()); + return pendingValues ?? (pendingValues = new SingleOrQueue()); } } } @@ -89,7 +90,7 @@ namespace Avalonia.Utilities /// If the property has any pending assignments. private bool HasPendingSet(TProperty property) { - return setRecords.TryGetValue(property, out var status) && status.PendingValues.Count != 0; + return setRecords.TryGetValue(property, out var status) && !status.PendingValues.Empty; } /// diff --git a/src/Avalonia.Base/Utilities/SingleOrQueue.cs b/src/Avalonia.Base/Utilities/SingleOrQueue.cs new file mode 100644 index 0000000000..f33adaff37 --- /dev/null +++ b/src/Avalonia.Base/Utilities/SingleOrQueue.cs @@ -0,0 +1,54 @@ +using System; +using System.Collections.Generic; +using System.Text; + +namespace Avalonia.Utilities +{ + public class SingleOrQueue + { + private T _head; + private Queue _tail; + + private Queue Tail => _tail ?? (_tail = new Queue()); + + private bool HasTail => _tail != null; + + public bool Empty { get; private set; } = true; + + public void Enqueue(T value) + { + if (Empty) + { + _head = value; + } + else + { + Tail.Enqueue(value); + } + + Empty = false; + } + + public T Dequeue() + { + if (Empty) + { + throw new InvalidOperationException("Cannot dequeue from an empty queue!"); + } + + var result = _head; + + if (HasTail && Tail.Count != 0) + { + _head = Tail.Dequeue(); + } + else + { + _head = default; + Empty = true; + } + + return result; + } + } +} diff --git a/src/Avalonia.Base/ValueStore.cs b/src/Avalonia.Base/ValueStore.cs index 5e34e8a43f..0e404500a1 100644 --- a/src/Avalonia.Base/ValueStore.cs +++ b/src/Avalonia.Base/ValueStore.cs @@ -116,7 +116,7 @@ namespace Avalonia public bool IsAnimating(AvaloniaProperty property) { - return _values.TryGetValue(property, out var value) ? (value as PriorityValue)?.IsAnimating ?? false : false; + return _values.TryGetValue(property, out var value) && value is PriorityValue priority && priority.IsAnimating; } public bool IsSet(AvaloniaProperty property) @@ -168,14 +168,14 @@ namespace Avalonia return value; } - private DeferredSetter _defferedSetter; + private DeferredSetter _defferedSetter; - public DeferredSetter Setter + public DeferredSetter Setter { get { return _defferedSetter ?? - (_defferedSetter = new DeferredSetter()); + (_defferedSetter = new DeferredSetter()); } } } diff --git a/tests/Avalonia.Base.UnitTests/PriorityValueTests.cs b/tests/Avalonia.Base.UnitTests/PriorityValueTests.cs index d6650aa104..61ba9bc463 100644 --- a/tests/Avalonia.Base.UnitTests/PriorityValueTests.cs +++ b/tests/Avalonia.Base.UnitTests/PriorityValueTests.cs @@ -1,11 +1,12 @@ // 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 Avalonia.Utilities; +using Moq; using System; using System.Linq; using System.Reactive.Linq; using System.Reactive.Subjects; -using Moq; using Xunit; namespace Avalonia.Base.UnitTests @@ -21,7 +22,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Initial_Value_Should_Be_UnsetValue() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); Assert.Same(AvaloniaProperty.UnsetValue, target.Value); } @@ -29,7 +30,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void First_Binding_Sets_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 0); @@ -39,7 +40,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Changing_Binding_Should_Set_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var subject = new BehaviorSubject("foo"); target.Add(subject, 0); @@ -51,7 +52,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Setting_Direct_Value_Should_Override_Binding() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 0); target.SetValue("bar", 0); @@ -62,7 +63,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Binding_Firing_Should_Override_Direct_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var source = new BehaviorSubject("initial"); target.Add(source, 0); @@ -76,7 +77,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Earlier_Binding_Firing_Should_Not_Override_Later() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var nonActive = new BehaviorSubject("na"); var source = new BehaviorSubject("initial"); @@ -92,7 +93,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Binding_Completing_Should_Revert_To_Direct_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var source = new BehaviorSubject("initial"); target.Add(source, 0); @@ -108,7 +109,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Binding_With_Lower_Priority_Has_Precedence() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 1); target.Add(Single("bar"), 0); @@ -120,7 +121,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Later_Binding_With_Same_Priority_Should_Take_Precedence() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 1); target.Add(Single("bar"), 0); @@ -133,7 +134,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Changing_Binding_With_Lower_Priority_Should_Set_Not_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var subject = new BehaviorSubject("bar"); target.Add(Single("foo"), 0); @@ -146,7 +147,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void UnsetValue_Should_Fall_Back_To_Next_Binding() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var subject = new BehaviorSubject("bar"); target.Add(subject, 0); @@ -162,7 +163,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Adding_Value_Should_Call_OnNext() { - var owner = new Mock(); + var owner = GetMockOwner(); var target = new PriorityValue(owner.Object, TestProperty, typeof(string)); target.Add(Single("foo"), 0); @@ -173,7 +174,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Changing_Value_Should_Call_OnNext() { - var owner = new Mock(); + var owner = GetMockOwner(); var target = new PriorityValue(owner.Object, TestProperty, typeof(string)); var subject = new BehaviorSubject("foo"); @@ -186,7 +187,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Disposing_A_Binding_Should_Revert_To_Next_Value() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 0); var disposable = target.Add(Single("bar"), 0); @@ -199,7 +200,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Disposing_A_Binding_Should_Remove_BindingEntry() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); target.Add(Single("foo"), 0); var disposable = target.Add(Single("bar"), 0); @@ -212,7 +213,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Completing_A_Binding_Should_Revert_To_Previous_Binding() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var source = new BehaviorSubject("bar"); target.Add(Single("foo"), 0); @@ -226,7 +227,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Completing_A_Binding_Should_Revert_To_Lower_Priority() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var source = new BehaviorSubject("bar"); target.Add(Single("foo"), 1); @@ -240,7 +241,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Completing_A_Binding_Should_Remove_BindingEntry() { - var target = new PriorityValue(null, TestProperty, typeof(string)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(string)); var subject = new BehaviorSubject("bar"); target.Add(Single("foo"), 0); @@ -254,7 +255,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Direct_Value_Should_Be_Coerced() { - var target = new PriorityValue(null, TestProperty, typeof(int), x => Math.Min((int)x, 10)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(int), x => Math.Min((int)x, 10)); target.SetValue(5, 0); Assert.Equal(5, target.Value); @@ -265,7 +266,7 @@ namespace Avalonia.Base.UnitTests [Fact] public void Bound_Value_Should_Be_Coerced() { - var target = new PriorityValue(null, TestProperty, typeof(int), x => Math.Min((int)x, 10)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(int), x => Math.Min((int)x, 10)); var source = new Subject(); target.Add(source, 0); @@ -279,7 +280,7 @@ namespace Avalonia.Base.UnitTests public void Revalidate_Should_ReCoerce_Value() { var max = 10; - var target = new PriorityValue(null, TestProperty, typeof(int), x => Math.Min((int)x, max)); + var target = new PriorityValue(GetMockOwner().Object, TestProperty, typeof(int), x => Math.Min((int)x, max)); var source = new Subject(); target.Add(source, 0); @@ -302,5 +303,12 @@ namespace Avalonia.Base.UnitTests { return Observable.Never().StartWith(value); } + + private static Mock GetMockOwner() + { + var owner = new Mock(); + owner.SetupGet(o => o.Setter).Returns(new DeferredSetter()); + return owner; + } } } diff --git a/tests/Avalonia.Base.UnitTests/Utilities/SingleOrQueueTests.cs b/tests/Avalonia.Base.UnitTests/Utilities/SingleOrQueueTests.cs new file mode 100644 index 0000000000..f1ab265bc9 --- /dev/null +++ b/tests/Avalonia.Base.UnitTests/Utilities/SingleOrQueueTests.cs @@ -0,0 +1,50 @@ +using Avalonia.Utilities; +using System; +using System.Collections.Generic; +using System.Text; +using Xunit; + +namespace Avalonia.Base.UnitTests.Utilities +{ + public class SingleOrQueueTests + { + [Fact] + public void New_SingleOrQueue_Is_Empty() + { + Assert.True(new SingleOrQueue().Empty); + } + + [Fact] + public void Dequeue_Throws_When_Empty() + { + var queue = new SingleOrQueue(); + + Assert.Throws(() => queue.Dequeue()); + } + + [Fact] + public void Enqueue_Adds_Element() + { + var queue = new SingleOrQueue(); + + queue.Enqueue(1); + + Assert.False(queue.Empty); + + Assert.Equal(1, queue.Dequeue()); + } + + [Fact] + public void Multiple_Elements_Dequeued_In_Correct_Order() + { + var queue = new SingleOrQueue(); + + queue.Enqueue(1); + queue.Enqueue(2); + queue.Enqueue(3); + Assert.Equal(1, queue.Dequeue()); + Assert.Equal(2, queue.Dequeue()); + Assert.Equal(3, queue.Dequeue()); + } + } +}