1
0
Эх сурвалжийг харах

Refactor DeferredSetter logic to allow passing references to backing fields and utilizing that in all usages. Refactor SetAndRaise logic to be combined and simplified between the two use cases. Now the common case delegates to the less specialized case.

Jeremy Koritzinsky 8 жил өмнө
parent
commit
05eda280de

+ 61 - 43
src/Avalonia.Base/AvaloniaObject.cs

@@ -56,7 +56,7 @@ namespace Avalonia
         /// <summary>
         /// Delayed setter helper for direct properties. Used to fix #855.
         /// </summary>
-        private DeferredSetter<AvaloniaProperty, object> DirectDelayedSetter
+        private DeferredSetter<AvaloniaProperty, object> DirectPropertyDeferredSetter
         {
             get
             {
@@ -554,6 +554,15 @@ namespace Avalonia
             }
         }
 
+        /// <summary>
+        /// A callback type for encapsulating complex logic for setting direct properties.
+        /// </summary>
+        /// <typeparam name="T">The type of the property.</typeparam>
+        /// <param name="value">The value to which to set the property.</param>
+        /// <param name="field">The backing field for the property.</param>
+        /// <param name="notifyWrapper">A wrapper for the property-changed notification.</param>
+        protected delegate void SetAndRaiseCallback<T>(T value, ref T field, Action<Action> notifyWrapper);
+
         /// <summary>
         /// Sets the backing field for a direct avalonia property, raising the 
         /// <see cref="PropertyChanged"/> event if the value has changed.
@@ -561,61 +570,70 @@ namespace Avalonia
         /// <typeparam name="T">The type of the property.</typeparam>
         /// <param name="property">The property.</param>
         /// <param name="field">The backing field.</param>
+        /// <param name="setterCallback">A callback called to actually set the value to the backing field.</param>
         /// <param name="value">The value.</param>
         /// <returns>
         /// True if the value changed, otherwise false.
         /// </returns>
-        protected bool SetAndRaise<T>(AvaloniaProperty<T> property, ref T field, T value)
+        protected bool SetAndRaise<T>(
+            AvaloniaProperty<T> property,
+            ref T field,
+            SetAndRaiseCallback<T> setterCallback,
+            T value)
         {
-            VerifyAccess();
-            if (!DirectDelayedSetter.IsNotifying(property))
-            {
-                var valueChanged = false;
-                if (!object.Equals(field, value))
+            Contract.Requires<ArgumentNullException>(setterCallback != null);
+            return DirectPropertyDeferredSetter.SetAndNotify(
+                property,
+                ref field,
+                (object val, ref T backing, Action<Action> notify) =>
                 {
-                    SetAndRaiseCore(property, ref field, value);
-
-                    valueChanged = true;
-                }
+                    setterCallback((T)val, ref backing, notify);
+                    return true;
+                },
+                value,
+                (object o, ref T backing) => !object.Equals(o, backing));
+        }
 
-                while (DirectDelayedSetter.HasPendingSet(property))
-                {
-                    SetAndRaiseCore(property, ref field, (T)DirectDelayedSetter.GetFirstPendingSet(property));
-                    valueChanged = true;
-                }
-                return valueChanged;
-            }
-            else if(!object.Equals(field, value))
-            {
-                DirectDelayedSetter.AddPendingSet(property, value);
-            }
-            return false;
+        /// <summary>
+        /// Sets the backing field for a direct avalonia property, raising the 
+        /// <see cref="PropertyChanged"/> event if the value has changed.
+        /// </summary>
+        /// <typeparam name="T">The type of the property.</typeparam>
+        /// <param name="property">The property.</param>
+        /// <param name="field">The backing field.</param>
+        /// <param name="value">The value.</param>
+        /// <returns>
+        /// True if the value changed, otherwise false.
+        /// </returns>
+        protected bool SetAndRaise<T>(AvaloniaProperty<T> property, ref T field, T value)
+        {
+            VerifyAccess();
+            return SetAndRaise(
+                property,
+                ref field,
+                (T val, ref T backing, Action<Action> notifyWrapper)
+                    => SetAndRaiseCore(property, ref backing, val, notifyWrapper),
+                value);
         }
 
-        private void SetAndRaiseCore<T>(AvaloniaProperty<T> property, ref T field, T value)
+        /// <summary>
+        /// Default assignment logic for SetAndRaise.
+        /// </summary>
+        /// <typeparam name="T">The type of the property.</typeparam>
+        /// <param name="property">The property.</param>
+        /// <param name="field">The backing field.</param>
+        /// <param name="value">The value.</param>
+        /// <param name="notifyWrapper">A wrapper for the property-changed notification.</param>
+        /// <returns>
+        /// True if the value changed, otherwise false.
+        /// </returns>
+        private bool SetAndRaiseCore<T>(AvaloniaProperty property, ref T field, T value, Action<Action> notifyWrapper)
         {
             var old = field;
             field = value;
 
-            using (DirectDelayedSetter.MarkNotifying(property))
-            {
-                RaisePropertyChanged(property, old, value, BindingPriority.LocalValue);
-            }
-        }
-
-        protected void SetAndRaise<T>(
-            AvaloniaProperty<T> property,
-            Action<T, Action<Action>> setterCallback,
-            T value,
-            Predicate<T> pendingSetCondition)
-        {
-            Contract.Requires<ArgumentNullException>(setterCallback != null);
-            Contract.Requires<ArgumentNullException>(pendingSetCondition != null);
-            DirectDelayedSetter.SetAndNotify(
-                property,
-                (val, notify) => setterCallback((T)val, notify),
-                value,
-                o => pendingSetCondition((T)o));
+            notifyWrapper(() => RaisePropertyChanged(property, old, value, BindingPriority.LocalValue));
+            return true;
         }
 
         /// <summary>

+ 11 - 4
src/Avalonia.Base/PriorityValue.cs

@@ -48,6 +48,7 @@ namespace Avalonia
         {
             Owner = owner;
             Property = property;
+            _valueType = valueType;
             _value = (AvaloniaProperty.UnsetValue, int.MaxValue);
             _validate = validate;
         }
@@ -231,12 +232,17 @@ namespace Avalonia
         private void UpdateValue(object value, int priority)
         {
             delayedSetter.SetAndNotify(this,
+                ref _value,
                 UpdateCore,
                 (value, priority),
-                val => !object.Equals(val.value, val.value));
+                ((object value, int) val, ref (object value, int) backing)
+                    => !object.Equals(val.value, backing.value));
         }
 
-        private void UpdateCore((object value, int priority) update, Action<Action> notify)
+        private bool UpdateCore(
+            (object value, int priority) update,
+            ref (object value, int priority) backing,
+            Action<Action> notify)
         {
             var val = update.value;
             var notification = val as BindingNotification;
@@ -249,14 +255,14 @@ namespace Avalonia
 
             if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue))
             {
-                var old = this._value.value;
+                var old = backing.value;
 
                 if (_validate != null && castValue != AvaloniaProperty.UnsetValue)
                 {
                     castValue = _validate(castValue);
                 }
 
-                this._value = (castValue, update.priority);
+                backing = (castValue, update.priority);
 
                 if (notification?.HasValue == true)
                 {
@@ -284,6 +290,7 @@ namespace Avalonia
                     val,
                     val?.GetType());
             }
+            return true;
         }
     }
 }

+ 34 - 23
src/Avalonia.Base/Utilities/DeferredSetter.cs

@@ -10,11 +10,11 @@ namespace Avalonia.Utilities
     /// A utility class to enable deferring assignment until after property-changed notifications are sent.
     /// </summary>
     /// <typeparam name="TProperty">The type of the object that represents the property.</typeparam>
-    /// <typeparam name="TValue">The type of value with which to track the delayed assignment.</typeparam>
-    class DeferredSetter<TProperty, TValue>
+    /// <typeparam name="TSetRecord">The type of value with which to track the delayed assignment.</typeparam>
+    class DeferredSetter<TProperty, TSetRecord>
         where TProperty: class
     {
-        internal struct NotifyDisposable : IDisposable
+        private struct NotifyDisposable : IDisposable
         {
             private readonly SettingStatus status;
 
@@ -33,17 +33,17 @@ namespace Avalonia.Utilities
         /// <summary>
         /// Information on current setting/notification status of a property.
         /// </summary>
-        internal class SettingStatus
+        private class SettingStatus
         {
             public bool Notifying { get; set; }
 
-            private Queue<TValue> pendingValues;
+            private Queue<TSetRecord> pendingValues;
             
-            public Queue<TValue> PendingValues
+            public Queue<TSetRecord> PendingValues
             {
                 get
                 {
-                    return pendingValues ?? (pendingValues = new Queue<TValue>());
+                    return pendingValues ?? (pendingValues = new Queue<TSetRecord>());
                 }
             }
         }
@@ -55,7 +55,7 @@ namespace Avalonia.Utilities
         /// </summary>
         /// <param name="property">The property to mark as notifying.</param>
         /// <returns>Returns a disposable that when disposed, marks the property as done notifying.</returns>
-        internal NotifyDisposable MarkNotifying(TProperty property)
+        private NotifyDisposable MarkNotifying(TProperty property)
         {
             Contract.Requires<InvalidOperationException>(!IsNotifying(property));
             
@@ -67,7 +67,7 @@ namespace Avalonia.Utilities
         /// </summary>
         /// <param name="property">The property.</param>
         /// <returns>If the property is currently notifying listeners.</returns>
-        internal bool IsNotifying(TProperty property)
+        private bool IsNotifying(TProperty property)
             => setRecords.TryGetValue(property, out var value) && value.Notifying;
 
         /// <summary>
@@ -75,7 +75,7 @@ namespace Avalonia.Utilities
         /// </summary>
         /// <param name="property">The property.</param>
         /// <param name="value">The value to assign.</param>
-        internal void AddPendingSet(TProperty property, TValue value)
+        private void AddPendingSet(TProperty property, TSetRecord value)
         {
             Contract.Requires<InvalidOperationException>(IsNotifying(property));
 
@@ -87,7 +87,7 @@ namespace Avalonia.Utilities
         /// </summary>
         /// <param name="property">The property to check.</param>
         /// <returns>If the property has any pending assignments.</returns>
-        internal bool HasPendingSet(TProperty property)
+        private bool HasPendingSet(TProperty property)
         {
             return setRecords.TryGetValue(property, out var status) && status.PendingValues.Count != 0;
         }
@@ -97,41 +97,50 @@ namespace Avalonia.Utilities
         /// </summary>
         /// <param name="property">The property to check.</param>
         /// <returns>The first pending assignment for the property.</returns>
-        internal TValue GetFirstPendingSet(TProperty property)
+        private TSetRecord GetFirstPendingSet(TProperty property)
         {
             return setRecords.GetOrCreateValue(property).PendingValues.Dequeue();
         }
 
+        public delegate bool SetterDelegate<TValue>(TSetRecord record, ref TValue backing, Action<Action> notifyCallback);
+        public delegate bool PendingSetPredicate<TValue>(TSetRecord record, ref TValue backing);
+
         /// <summary>
         /// Set the property and notify listeners while ensuring we don't get into a stack overflow as happens with #855 and #824
         /// </summary>
         /// <param name="property">The property to set.</param>
+        /// <param name="backing">The backing field for the property</param>
         /// <param name="setterCallback">
         /// A callback that actually sets the property.
         /// The first parameter is the value to set, and the second is a wrapper that takes a callback that sends the property-changed notification.
         /// </param>
         /// <param name="value">The value to try to set.</param>
         /// <param name="pendingSetCondition">A predicate to filter what possible values should be added as pending sets (i.e. only values not equal to the current value).</param>
-        public void SetAndNotify(
+        public bool SetAndNotify<TValue>(
             TProperty property,
-            Action<TValue, Action<Action>> setterCallback,
-            TValue value,
-            Predicate<TValue> pendingSetCondition)
+            ref TValue backing,
+            SetterDelegate<TValue> setterCallback,
+            TSetRecord value,
+            PendingSetPredicate<TValue> pendingSetCondition)
         {
             Contract.Requires<ArgumentNullException>(setterCallback != null);
             Contract.Requires<ArgumentNullException>(pendingSetCondition != null);
             if (!IsNotifying(property))
             {
-                setterCallback(value, notification =>
+                bool updated = false;
+                if (pendingSetCondition(value, ref backing))
                 {
-                    using (MarkNotifying(property))
+                    updated = setterCallback(value, ref backing, notification =>
                     {
-                        notification();
-                    }
-                });
+                        using (MarkNotifying(property))
+                        {
+                            notification();
+                        }
+                    });
+                }
                 while (HasPendingSet(property))
                 {
-                    setterCallback(GetFirstPendingSet(property), notification =>
+                    updated |= setterCallback(GetFirstPendingSet(property), ref backing, notification =>
                     {
                         using (MarkNotifying(property))
                         {
@@ -139,11 +148,13 @@ namespace Avalonia.Utilities
                         }
                     });
                 }
+                return updated;
             }
-            else if(pendingSetCondition(value))
+            else if(pendingSetCondition(value, ref backing))
             {
                 AddPendingSet(property, value);
             }
+            return false;
         }
     }
 }

+ 8 - 8
src/Avalonia.Controls/Primitives/SelectingItemsControl.cs

@@ -151,14 +151,14 @@ namespace Avalonia.Controls.Primitives
             {
                 if (_updateCount == 0)
                 {
-                    SetAndRaise(SelectedIndexProperty, (val, notifyWrapper) =>
+                    SetAndRaise(SelectedIndexProperty, ref _selectedIndex, (int val, ref int backing, Action<Action> notifyWrapper) =>
                     {
-                        var old = SelectedIndex;
+                        var old = backing;
                         var effective = (val >= 0 && val < Items?.Cast<object>().Count()) ? val : -1;
 
                         if (old != effective)
                         {
-                            _selectedIndex = effective;
+                            backing = effective;
                             notifyWrapper(() =>
                                 RaisePropertyChanged(
                                     SelectedIndexProperty,
@@ -167,7 +167,7 @@ namespace Avalonia.Controls.Primitives
                                     BindingPriority.LocalValue));
                             SelectedItem = ElementAt(Items, effective);
                         }
-                    }, value, val => val != SelectedIndex);
+                    }, value);
                 }
                 else
                 {
@@ -191,15 +191,15 @@ namespace Avalonia.Controls.Primitives
             {
                 if (_updateCount == 0)
                 {
-                    SetAndRaise(SelectedItemProperty, (val, notifyWrapper) =>
+                    SetAndRaise(SelectedItemProperty, ref _selectedItem, (object val, ref object backing, Action<Action> notifyWrapper) =>
                     {
-                        var old = SelectedItem;
+                        var old = backing;
                         var index = IndexOf(Items, val);
                         var effective = index != -1 ? val : null;
 
                         if (!object.Equals(effective, old))
                         {
-                            _selectedItem = effective;
+                            backing = effective;
 
                             notifyWrapper(() =>
                                 RaisePropertyChanged(
@@ -225,7 +225,7 @@ namespace Avalonia.Controls.Primitives
                                 SelectedItems.Clear();
                             }
                         }
-                    }, value, val => val != SelectedItem);
+                    }, value);
                 }
                 else
                 {

+ 1 - 1
tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs

@@ -462,7 +462,7 @@ namespace Avalonia.Base.UnitTests
 
             Assert.Equal(1, viewModel.SetterInvokedCount);
 
-            //here in real life stack overflow exception is thrown issue #855 and #824
+            // Issues #855 and #824 were causing a StackOverflowException at this point.
             target.DoubleValue = 51.001;
 
             Assert.Equal(2, viewModel.SetterInvokedCount);

+ 1 - 1
tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs

@@ -148,7 +148,7 @@ namespace Avalonia.Controls.UnitTests.Primitives
 
             Assert.Equal(1, viewModel.SetterInvokedCount);
 
-            //here in real life stack overflow exception is thrown issue #855 and #824
+            // Issues #855 and #824 were causing a StackOverflowException at this point.
             target.Value = 51.001;
 
             Assert.Equal(2, viewModel.SetterInvokedCount);