From 8acf94d3e4f651b34a9480b13ab69f8fabeba266 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 16:52:29 -0500 Subject: [PATCH] Fix bug in DelayedSetter and change recursive delayed setting to prevent possible issues down the road. --- src/Avalonia.Base/AvaloniaObject.cs | 35 +++---- src/Avalonia.Base/PriorityValue.cs | 91 ++++++++++--------- src/Avalonia.Base/Utilities/DelayedSetter.cs | 13 ++- .../Primitives/SelectingItemsControl.cs | 4 +- .../AvaloniaObjectTests_Binding.cs | 8 +- .../AvaloniaObjectTests_Direct.cs | 8 +- 6 files changed, 80 insertions(+), 79 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 3f53a3735c..1c4f727161 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -563,34 +563,35 @@ namespace Avalonia { if (!object.Equals(field, value)) { - var old = field; - field = value; - - using (directDelayedSetter.MarkNotifying(property)) - { - RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); - } + SetAndRaiseCore(property, ref field, value); - if (directDelayedSetter.HasPendingSet(property)) + while (directDelayedSetter.HasPendingSet(property)) { - SetAndRaise(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); + SetAndRaiseCore(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); } return true; } - else - { - return false; - } } - else + else if(!object.Equals(field, value)) { directDelayedSetter.AddPendingSet(property, value); - return false; + } + return false; + } + + private void SetAndRaiseCore(AvaloniaProperty property, ref T field, T value) + { + var old = field; + field = value; + + using (directDelayedSetter.MarkNotifying(property)) + { + RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); } } - protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Action delayedSet) - => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, val => delayedSet((T)val)); + protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value) + => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value); /// /// Tries to cast a value to a type, taking into account that the value may be a diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index cc6f9e465c..df138c88f5 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -237,66 +237,73 @@ namespace Avalonia { if (!delayedSetter.IsNotifying(this)) { - var notification = value as BindingNotification; - object castValue; + value = UpdateValueCore(value, priority); - if (notification != null) + while (delayedSetter.HasPendingSet(this)) { - value = (notification.HasValue) ? notification.Value : null; + var pendingSet = delayedSetter.GetFirstPendingSet(this); + UpdateValueCore(pendingSet.value, pendingSet.priority); } + } + else if(!object.Equals(value, _value)) + { + delayedSetter.AddPendingSet(this, (value, priority)); + } + } - if (!object.Equals(value, _value) && TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) - { - var old = _value; + private object UpdateValueCore(object value, int priority) + { + var notification = value as BindingNotification; + object castValue; - if (_validate != null && castValue != AvaloniaProperty.UnsetValue) - { - castValue = _validate(castValue); - } + if (notification != null) + { + value = (notification.HasValue) ? notification.Value : null; + } - ValuePriority = priority; - _value = castValue; + if (TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) + { + var old = _value; - if (notification?.HasValue == true) - { - notification.SetValue(castValue); - } + if (_validate != null && castValue != AvaloniaProperty.UnsetValue) + { + castValue = _validate(castValue); + } - if (notification == null || notification.HasValue) - { - using (delayedSetter.MarkNotifying(this)) - { - Owner?.Changed(this, old, _value); - } + ValuePriority = priority; + _value = castValue; - if (delayedSetter.HasPendingSet(this)) - { - var pendingSet = delayedSetter.GetFirstPendingSet(this); - UpdateValue(pendingSet.value, pendingSet.priority); - } - } + if (notification?.HasValue == true) + { + notification.SetValue(castValue); + } - if (notification != null) + if (notification == null || notification.HasValue) + { + using (delayedSetter.MarkNotifying(this)) { - Owner?.BindingNotificationReceived(this, notification); + Owner?.Changed(this, old, _value); } } - else + + if (notification != null) { - Logger.Error( - LogArea.Binding, - Owner, - "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", - Property.Name, - _valueType, - value, - value?.GetType()); - } + Owner?.BindingNotificationReceived(this, notification); + } } else { - delayedSetter.AddPendingSet(this, (value, priority)); + Logger.Error( + LogArea.Binding, + Owner, + "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", + Property.Name, + _valueType, + value, + value?.GetType()); } + + return value; } } } diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs index 3abffbdd3d..6d611dfa46 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -58,10 +58,9 @@ namespace Avalonia.Utilities return setRecords[property].PendingValues.Dequeue(); } - public void SetAndNotify(T property, Action> setterCallback, TValue value, Action delayedSet) + public void SetAndNotify(T property, Action> setterCallback, TValue value) { Contract.Requires(setterCallback != null); - Contract.Requires(delayedSet != null); if (!IsNotifying(property)) { setterCallback(value, notification => @@ -71,9 +70,15 @@ namespace Avalonia.Utilities notification(); } }); - if (HasPendingSet(property)) + while (HasPendingSet(property)) { - delayedSet(GetFirstPendingSet(property)); + setterCallback(GetFirstPendingSet(property), notification => + { + using (MarkNotifying(property)) + { + notification(); + } + }); } } else diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index 294b14b13d..13d8480978 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -162,7 +162,7 @@ namespace Avalonia.Controls.Primitives notifierWrapper(() => RaisePropertyChanged(SelectedIndexProperty, old, effective, BindingPriority.LocalValue)); SelectedItem = ElementAt(Items, effective); } - }, value, val => SelectedIndex = val); + }, value); } else { @@ -215,7 +215,7 @@ namespace Avalonia.Controls.Primitives SelectedItems.Clear(); } } - }, value, val => SelectedItem = val); + }, value); } else { diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 211c7c985e..192f5258ab 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -399,12 +399,6 @@ namespace Avalonia.Base.UnitTests var target = new Class1(); - //note: if the initialization of the child binding is here target/child binding work fine!!! - //var child = new Class1() - //{ - // [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] - //}; - target.Bind(Class1.DoubleValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); @@ -429,7 +423,7 @@ namespace Avalonia.Base.UnitTests //here in real life stack overflow exception is thrown issue #855 and #824 target.DoubleValue = 51.001; - Assert.Equal(3, viewModel.SetterInvokedCount); + Assert.Equal(2, viewModel.SetterInvokedCount); double expected = 51; diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index 4123e229dd..249528e9b2 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -452,12 +452,6 @@ namespace Avalonia.Base.UnitTests var target = new Class1(); - //note: if the initialization of the child binding is here there is no stackoverflow!!! - //var child = new Class1() - //{ - // [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] - //}; - target.Bind(Class1.DoubleValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay, @@ -485,7 +479,7 @@ namespace Avalonia.Base.UnitTests //here in real life stack overflow exception is thrown issue #855 and #824 target.DoubleValue = 51.001; - Assert.Equal(3, viewModel.SetterInvokedCount); + Assert.Equal(2, viewModel.SetterInvokedCount); double expected = 51;