From 220a9874679be2dd50adee0f6cda6f189a526023 Mon Sep 17 00:00:00 2001 From: donandren Date: Sat, 14 Jan 2017 16:24:58 +0200 Subject: [PATCH 01/24] issue #855 unit test --- .../ListBoxTests_Single.cs | 71 ++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs index c7992fe80f..d95acbdb7f 100644 --- a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs +++ b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs @@ -1,11 +1,15 @@ // 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 System.Collections.Generic; +using System.ComponentModel; using System.Linq; using Avalonia.Controls.Presenters; using Avalonia.Controls.Templates; +using Avalonia.Data; using Avalonia.Input; using Avalonia.LogicalTree; +using Avalonia.Markup.Xaml.Data; using Avalonia.Styling; using Avalonia.VisualTree; using Xunit; @@ -199,6 +203,71 @@ namespace Avalonia.Controls.UnitTests Assert.Equal(1, target.SelectedIndex); } + [Fact] + public void SelectedItem_Should_Not_Cause_StackOverflow() + { + var viewModel = new TestStackOverflowViewModel() + { + Items = new List { "foo", "bar", "baz" } + }; + + var target = new ListBox + { + Template = new FuncControlTemplate(CreateListBoxTemplate), + DataContext = viewModel, + Items = viewModel.Items + }; + + target.Bind(ListBox.SelectedItemProperty, + new Binding("SelectedItem") { Mode = BindingMode.TwoWay }); + + Assert.Equal(0, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 + target.SelectedItem = viewModel.Items[2]; + + Assert.Equal(viewModel.Items[1], target.SelectedItem); + Assert.Equal(1, viewModel.SetterInvokedCount); + } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public List Items { get; set; } + + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private string _selectedItem; + + public event PropertyChangedEventHandler PropertyChanged; + + public string SelectedItem + { + get { return _selectedItem; } + set + { + if (_selectedItem != value) + { + SetterInvokedCount++; + + int index = Items.IndexOf(value); + + if (MaxInvokedCount > SetterInvokedCount && index > 0) + { + _selectedItem = Items[index - 1]; + } + else + { + _selectedItem = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(SelectedItem))); + } + } + } + } + private Control CreateListBoxTemplate(ITemplatedControl parent) { return new ScrollViewer @@ -237,4 +306,4 @@ namespace Avalonia.Controls.UnitTests target.Presenter.ApplyTemplate(); } } -} +} \ No newline at end of file From 3729b9797fbae73bd1b0f30204d3d01d8c8493b4 Mon Sep 17 00:00:00 2001 From: donandren Date: Sun, 15 Jan 2017 16:06:43 +0200 Subject: [PATCH 02/24] another failing test for issue #855 and #824 --- .../Primitives/RangeBaseTests.cs | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index d3ed077cbf..9bccb1986d 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs @@ -2,7 +2,11 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; +using System.ComponentModel; using Avalonia.Controls.Primitives; +using Avalonia.Controls.Templates; +using Avalonia.Data; +using Avalonia.Markup.Xaml.Data; using Xunit; namespace Avalonia.Controls.UnitTests.Primitives @@ -87,8 +91,93 @@ namespace Avalonia.Controls.UnitTests.Primitives Assert.Throws(() => target.Value = double.NegativeInfinity); } + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + Track track = null; + + var target = new TestRange() + { + Template = new FuncControlTemplate(c => + { + return track = new Track() + { + Width = 100, + Orientation = Orientation.Horizontal, + [~~Track.MinimumProperty] = c[~~RangeBase.MinimumProperty], + [~~Track.MaximumProperty] = c[~~RangeBase.MaximumProperty], + [~~Track.ValueProperty] = c[~~RangeBase.ValueProperty], + Name = "PART_Track", + Thumb = new Thumb() + }; + }), + Minimum = 0, + Maximum = 100, + DataContext = viewModel + }; + + target.Bind(TestRange.ValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay }); + + target.ApplyTemplate(); + track.Measure(new Size(100, 0)); + track.Arrange(new Rect(0, 0, 100, 0)); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.Value = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.Value); + Assert.Equal(expected, track.Value); + } + private class TestRange : RangeBase { } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } } \ No newline at end of file From ef81c5596051ac123a46c9165b455c48d3bcb06d Mon Sep 17 00:00:00 2001 From: donandren Date: Fri, 17 Feb 2017 00:32:38 +0200 Subject: [PATCH 03/24] another simple unit tests for issue #855 for direct and styled properties --- .../Avalonia.Base.UnitTests.csproj | 4 + .../AvaloniaObjectTests_Binding.cs | 103 +++++++++++++++-- .../AvaloniaObjectTests_Direct.cs | 106 ++++++++++++++++-- 3 files changed, 191 insertions(+), 22 deletions(-) diff --git a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj index 07ed7f14ca..c712a8bcf9 100644 --- a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj +++ b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj @@ -124,6 +124,10 @@ {B09B78D8-9B26-48B0-9149-D64A2F120F3F} Avalonia.Base + + {3E53A01A-B331-47F3-B828-4A5717E77A24} + Avalonia.Markup.Xaml + {88060192-33d5-4932-b0f9-8bd2763e857d} Avalonia.UnitTests diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 5e286305d2..7186207b92 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -2,22 +2,21 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; -using System.Collections; -using System.Collections.Generic; +using System.ComponentModel; +using System.Reactive.Concurrency; using System.Reactive.Linq; using System.Reactive.Subjects; -using Microsoft.Reactive.Testing; +using System.Threading; +using System.Threading.Tasks; using Avalonia.Data; using Avalonia.Logging; -using Avalonia.UnitTests; -using Xunit; -using System.Threading.Tasks; +using Avalonia.Markup.Xaml.Data; using Avalonia.Platform; -using System.Threading; -using Moq; -using System.Reactive.Disposables; -using System.Reactive.Concurrency; using Avalonia.Threading; +using Avalonia.UnitTests; +using Microsoft.Reactive.Testing; +using Moq; +using Xunit; namespace Avalonia.Base.UnitTests { @@ -363,7 +362,7 @@ namespace Avalonia.Base.UnitTests Assert.True(called); } } - + [Fact] public async void Bind_With_Scheduler_Executes_On_Scheduler() { @@ -384,7 +383,43 @@ namespace Avalonia.Base.UnitTests await Task.Run(() => source.OnNext(6.7)); } + } + + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + 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 }); + + var child = new Class1() + { + [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] + }; + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); } /// @@ -405,6 +440,15 @@ namespace Avalonia.Base.UnitTests public static readonly StyledProperty QuxProperty = AvaloniaProperty.Register("Qux", 5.6); + + public static readonly StyledProperty DoubleValueProperty = + AvaloniaProperty.Register(nameof(DoubleValue)); + + public double DoubleValue + { + get { return GetValue(DoubleValueProperty); } + set { SetValue(DoubleValueProperty, value); } + } } private class Class2 : Class1 @@ -431,5 +475,40 @@ namespace Avalonia.Base.UnitTests return new InstancedBinding(_source, BindingMode.OneTime); } } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } -} +} \ No newline at end of file diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index ecb555252d..b6e5396020 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -3,10 +3,11 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Reactive.Subjects; -using Avalonia; using Avalonia.Data; using Avalonia.Logging; +using Avalonia.Markup.Xaml.Data; using Avalonia.UnitTests; using Xunit; @@ -208,7 +209,7 @@ namespace Avalonia.Base.UnitTests { var target = new Class1(); - Assert.Throws(() => + Assert.Throws(() => target.SetValue(Class1.BarProperty, "newvalue")); } @@ -217,7 +218,7 @@ namespace Avalonia.Base.UnitTests { var target = new Class1(); - Assert.Throws(() => + Assert.Throws(() => target.SetValue((AvaloniaProperty)Class1.BarProperty, "newvalue")); } @@ -227,7 +228,7 @@ namespace Avalonia.Base.UnitTests var target = new Class1(); var source = new Subject(); - Assert.Throws(() => + Assert.Throws(() => target.Bind(Class1.BarProperty, source)); } @@ -439,12 +440,49 @@ namespace Avalonia.Base.UnitTests Assert.Equal(BindingMode.OneWayToSource, bar.GetMetadata().DefaultBindingMode); } + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + 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, Source = viewModel }); + + var child = new Class1() + { + [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] + }; + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + private class Class1 : AvaloniaObject { public static readonly DirectProperty FooProperty = AvaloniaProperty.RegisterDirect( - "Foo", - o => o.Foo, + "Foo", + o => o.Foo, (o, v) => o.Foo = v, unsetValue: "unset"); @@ -453,14 +491,21 @@ namespace Avalonia.Base.UnitTests public static readonly DirectProperty BazProperty = AvaloniaProperty.RegisterDirect( - "Bar", - o => o.Baz, - (o,v) => o.Baz = v, + "Bar", + o => o.Baz, + (o, v) => o.Baz = v, unsetValue: -1); + public static readonly DirectProperty DoubleValueProperty = + AvaloniaProperty.RegisterDirect( + nameof(DoubleValue), + o => o.DoubleValue, + (o, v) => o.DoubleValue = v); + private string _foo = "initial"; private readonly string _bar = "bar"; private int _baz = 5; + private double _doubleValue; public string Foo { @@ -478,6 +523,12 @@ namespace Avalonia.Base.UnitTests get { return _baz; } set { SetAndRaise(BazProperty, ref _baz, value); } } + + public double DoubleValue + { + get { return _doubleValue; } + set { SetAndRaise(DoubleValueProperty, ref _doubleValue, value); } + } } private class Class2 : AvaloniaObject @@ -497,5 +548,40 @@ namespace Avalonia.Base.UnitTests set { SetAndRaise(FooProperty, ref _foo, value); } } } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } -} +} \ No newline at end of file From 8f3ce463f0c1e97642b43d8727525a6f95fcab40 Mon Sep 17 00:00:00 2001 From: donandren Date: Sun, 26 Feb 2017 21:27:44 +0200 Subject: [PATCH 04/24] Slider (RangeBase) test for #824 with Binding which behind scenes is using weakobservable --- .../Primitives/RangeBaseTests.cs | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index 9bccb1986d..0e67581760 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs @@ -7,6 +7,7 @@ using Avalonia.Controls.Primitives; using Avalonia.Controls.Templates; using Avalonia.Data; using Avalonia.Markup.Xaml.Data; +using Avalonia.Styling; using Xunit; namespace Avalonia.Controls.UnitTests.Primitives @@ -91,8 +92,10 @@ namespace Avalonia.Controls.UnitTests.Primitives Assert.Throws(() => target.Value = double.NegativeInfinity); } - [Fact] - public void SetValue_Should_Not_Cause_StackOverflow() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void SetValue_Should_Not_Cause_StackOverflow(bool useXamlBinding) { var viewModel = new TestStackOverflowViewModel() { @@ -105,16 +108,32 @@ namespace Avalonia.Controls.UnitTests.Primitives { Template = new FuncControlTemplate(c => { - return track = new Track() + track = new Track() { Width = 100, Orientation = Orientation.Horizontal, [~~Track.MinimumProperty] = c[~~RangeBase.MinimumProperty], [~~Track.MaximumProperty] = c[~~RangeBase.MaximumProperty], - [~~Track.ValueProperty] = c[~~RangeBase.ValueProperty], + Name = "PART_Track", Thumb = new Thumb() }; + + if (useXamlBinding) + { + track.Bind(Track.ValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = c, + Priority = BindingPriority.Style + }); + } + else + { + track[~~Track.ValueProperty] = c[~~RangeBase.ValueProperty]; + } + + return track; }), Minimum = 0, Maximum = 100, From ce387271cc3f77a6ba622cf66402ef6f9c83de9c Mon Sep 17 00:00:00 2001 From: donandren Date: Sun, 26 Feb 2017 23:54:24 +0200 Subject: [PATCH 05/24] add test parameters to styled/direct binding tests for #855 and #824 --- .../AvaloniaObjectTests_Binding.cs | 26 +++++++++++++---- .../AvaloniaObjectTests_Direct.cs | 29 +++++++++++++++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 7186207b92..95aaa1fa32 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -385,8 +385,10 @@ namespace Avalonia.Base.UnitTests } } - [Fact] - public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values(bool useXamlBinding) { var viewModel = new TestStackOverflowViewModel() { @@ -401,12 +403,24 @@ namespace Avalonia.Base.UnitTests // [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] //}; - target.Bind(Class1.DoubleValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + target.Bind(Class1.DoubleValueProperty, + new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + + var child = new Class1(); - var child = new Class1() + if (useXamlBinding) { - [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] - }; + child.Bind(Class1.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + } + else + { + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; + } Assert.Equal(1, viewModel.SetterInvokedCount); diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index b6e5396020..17af8164b6 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -440,8 +440,10 @@ namespace Avalonia.Base.UnitTests Assert.Equal(BindingMode.OneWayToSource, bar.GetMetadata().DefaultBindingMode); } - [Fact] - public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values(bool useXamlBinding) { var viewModel = new TestStackOverflowViewModel() { @@ -456,12 +458,27 @@ namespace Avalonia.Base.UnitTests // [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] //}; - target.Bind(Class1.DoubleValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + target.Bind(Class1.DoubleValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = viewModel + }); + + var child = new Class1(); - var child = new Class1() + if (useXamlBinding) { - [~~Class1.DoubleValueProperty] = target[~~Class1.DoubleValueProperty] - }; + child.Bind(Class1.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + } + else + { + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; + } Assert.Equal(1, viewModel.SetterInvokedCount); From b5ee3077bcc7f289af825dc1b73ceece8ee94728 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Oct 2017 16:32:37 -0500 Subject: [PATCH 06/24] Fix direct properties by delaying setting values until after any currently running property changed notifications for this property finish running. --- src/Avalonia.Base/AvaloniaObject.cs | 27 +++++++-- src/Avalonia.Base/DelayedSetter.cs | 56 +++++++++++++++++++ .../AvaloniaObjectTests_Direct.cs | 2 +- 3 files changed, 79 insertions(+), 6 deletions(-) create mode 100644 src/Avalonia.Base/DelayedSetter.cs diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index efcbb57244..77d6c20608 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -510,6 +510,7 @@ namespace Avalonia { Contract.Requires(property != null); VerifyAccess(); + delayedSetter.SetNotifying(property, true); AvaloniaPropertyChangedEventArgs e = new AvaloniaPropertyChangedEventArgs( this, @@ -536,9 +537,12 @@ namespace Avalonia finally { property.Notifying?.Invoke(this, false); + delayedSetter.SetNotifying(property, false); } } + private DelayedSetter delayedSetter = new DelayedSetter(); + /// /// Sets the backing field for a direct avalonia property, raising the /// event if the value has changed. @@ -553,15 +557,28 @@ namespace Avalonia protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) { VerifyAccess(); - if (!object.Equals(field, value)) + if (!delayedSetter.IsNotifying(property)) { - var old = field; - field = value; - RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); - return true; + if (!object.Equals(field, value)) + { + var old = field; + field = value; + RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); + + if (delayedSetter.HasPendingSet(property)) + { + SetAndRaise(property, ref field, (T)delayedSetter.GetFirstPendingSet(property)); + } + return true; + } + else + { + return false; + } } else { + delayedSetter.RecordPendingSet(property, value); return false; } } diff --git a/src/Avalonia.Base/DelayedSetter.cs b/src/Avalonia.Base/DelayedSetter.cs new file mode 100644 index 0000000000..df0a8b85f4 --- /dev/null +++ b/src/Avalonia.Base/DelayedSetter.cs @@ -0,0 +1,56 @@ +using System; +using System.Collections.Generic; +using System.Text; + +namespace Avalonia +{ + class DelayedSetter + { + private class SettingStatus + { + public bool Notifying { get; set; } + + private Queue pendingValues; + + public Queue PendingValues + { + get + { + return pendingValues ?? (pendingValues = new Queue()); + } + } + } + + private readonly Dictionary setRecords = new Dictionary(); + + public void SetNotifying(T property, bool notifying) + { + if (!setRecords.ContainsKey(property)) + { + setRecords[property] = new SettingStatus(); + } + setRecords[property].Notifying = notifying; + } + + public bool IsNotifying(T property) => setRecords.TryGetValue(property, out var value) && value.Notifying; + + public void RecordPendingSet(T property, object value) + { + if (!setRecords.ContainsKey(property)) + { + setRecords[property] = new SettingStatus(); + } + setRecords[property].PendingValues.Enqueue(value); + } + + public bool HasPendingSet(T property) + { + return setRecords.ContainsKey(property) && setRecords[property].PendingValues.Count != 0; + } + + public object GetFirstPendingSet(T property) + { + return setRecords[property].PendingValues.Dequeue(); + } + } +} diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index d20ec32893..4123e229dd 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -485,7 +485,7 @@ namespace Avalonia.Base.UnitTests //here in real life stack overflow exception is thrown issue #855 and #824 target.DoubleValue = 51.001; - Assert.Equal(2, viewModel.SetterInvokedCount); + Assert.Equal(3, viewModel.SetterInvokedCount); double expected = 51; From 8eb355e99d626b5dc3c277552449f3fe04997d58 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Oct 2017 21:09:52 -0500 Subject: [PATCH 07/24] Fix SelectedItem stack overflow bug. Changes of this pattern are basically required for any direct property that is not using SetAndRaise. --- src/Avalonia.Base/AvaloniaObject.cs | 24 +++++---- .../{ => Utilities}/DelayedSetter.cs | 23 ++++---- .../Primitives/SelectingItemsControl.cs | 52 ++++++++++++------- 3 files changed, 63 insertions(+), 36 deletions(-) rename src/Avalonia.Base/{ => Utilities}/DelayedSetter.cs (66%) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 77d6c20608..91e087abc0 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -51,6 +51,12 @@ namespace Avalonia /// private EventHandler _propertyChanged; + /// + /// Delayed setter helper for direct properties. Used to fix #855. + /// + protected readonly DelayedSetter directDelayedSetter = new DelayedSetter(); + + /// /// Initializes a new instance of the class. /// @@ -510,7 +516,6 @@ namespace Avalonia { Contract.Requires(property != null); VerifyAccess(); - delayedSetter.SetNotifying(property, true); AvaloniaPropertyChangedEventArgs e = new AvaloniaPropertyChangedEventArgs( this, @@ -537,12 +542,9 @@ namespace Avalonia finally { property.Notifying?.Invoke(this, false); - delayedSetter.SetNotifying(property, false); } } - private DelayedSetter delayedSetter = new DelayedSetter(); - /// /// Sets the backing field for a direct avalonia property, raising the /// event if the value has changed. @@ -557,17 +559,21 @@ namespace Avalonia protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) { VerifyAccess(); - if (!delayedSetter.IsNotifying(property)) + if (!directDelayedSetter.IsNotifying(property)) { if (!object.Equals(field, value)) { var old = field; field = value; - RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); + + using (directDelayedSetter.MarkNotifying(property)) + { + RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); + } - if (delayedSetter.HasPendingSet(property)) + if (directDelayedSetter.HasPendingSet(property)) { - SetAndRaise(property, ref field, (T)delayedSetter.GetFirstPendingSet(property)); + SetAndRaise(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); } return true; } @@ -578,7 +584,7 @@ namespace Avalonia } else { - delayedSetter.RecordPendingSet(property, value); + directDelayedSetter.AddPendingSet(property, value); return false; } } diff --git a/src/Avalonia.Base/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs similarity index 66% rename from src/Avalonia.Base/DelayedSetter.cs rename to src/Avalonia.Base/Utilities/DelayedSetter.cs index df0a8b85f4..c8c9c87d95 100644 --- a/src/Avalonia.Base/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -1,40 +1,45 @@ using System; using System.Collections.Generic; +using System.Reactive.Disposables; using System.Text; -namespace Avalonia +namespace Avalonia.Utilities { - class DelayedSetter + public class DelayedSetter { private class SettingStatus { public bool Notifying { get; set; } - private Queue pendingValues; + private Queue pendingValues; - public Queue PendingValues + public Queue PendingValues { get { - return pendingValues ?? (pendingValues = new Queue()); + return pendingValues ?? (pendingValues = new Queue()); } } } private readonly Dictionary setRecords = new Dictionary(); - public void SetNotifying(T property, bool notifying) + public IDisposable MarkNotifying(T property) { + Contract.Requires(!IsNotifying(property)); + if (!setRecords.ContainsKey(property)) { setRecords[property] = new SettingStatus(); } - setRecords[property].Notifying = notifying; + setRecords[property].Notifying = true; + + return Disposable.Create(() => setRecords[property].Notifying = false); } public bool IsNotifying(T property) => setRecords.TryGetValue(property, out var value) && value.Notifying; - public void RecordPendingSet(T property, object value) + public void AddPendingSet(T property, TValue value) { if (!setRecords.ContainsKey(property)) { @@ -48,7 +53,7 @@ namespace Avalonia return setRecords.ContainsKey(property) && setRecords[property].PendingValues.Count != 0; } - public object GetFirstPendingSet(T property) + public TValue GetFirstPendingSet(T property) { return setRecords[property].PendingValues.Dequeue(); } diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index ab09a4701d..e5f5e8fc30 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -183,30 +183,46 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - var old = SelectedItem; - var index = IndexOf(Items, value); - var effective = index != -1 ? value : null; - - if (!object.Equals(effective, old)) + if (!directDelayedSetter.IsNotifying(SelectedItemProperty)) { - _selectedItem = effective; - RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue); - SelectedIndex = index; + var old = SelectedItem; + var index = IndexOf(Items, value); + var effective = index != -1 ? value : null; - if (effective != null) + if (!object.Equals(effective, old)) { - if (SelectedItems.Count != 1 || SelectedItems[0] != effective) + _selectedItem = effective; + using (directDelayedSetter.MarkNotifying(SelectedItemProperty)) + { + RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue); + } + + SelectedIndex = index; + + if (effective != null) + { + if (SelectedItems.Count != 1 || SelectedItems[0] != effective) + { + _syncingSelectedItems = true; + SelectedItems.Clear(); + SelectedItems.Add(effective); + _syncingSelectedItems = false; + } + } + else if (SelectedItems.Count > 0) { - _syncingSelectedItems = true; SelectedItems.Clear(); - SelectedItems.Add(effective); - _syncingSelectedItems = false; } - } - else if (SelectedItems.Count > 0) - { - SelectedItems.Clear(); - } + + if (directDelayedSetter.HasPendingSet(SelectedItemProperty)) + { + SelectedItem = directDelayedSetter.GetFirstPendingSet(SelectedItemProperty); + } + } + } + else + { + directDelayedSetter.AddPendingSet(SelectedItemProperty, value); } } else From 6486ffc77f2a888cc891e4502c5d0f31441d0431 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Oct 2017 21:14:08 -0500 Subject: [PATCH 08/24] Apply fix to PriorityValue, which works for StyledProperty bindings where the bindings don't have the same priority. --- src/Avalonia.Base/PriorityValue.cs | 83 +++++++++++-------- .../AvaloniaObjectTests_Binding.cs | 2 +- 2 files changed, 51 insertions(+), 34 deletions(-) diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 3726fb7ae5..cc6f9e465c 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -30,6 +30,7 @@ namespace Avalonia private readonly SingleOrDictionary _levels = new SingleOrDictionary(); private object _value; private readonly Func _validate; + private static readonly DelayedSetter delayedSetter = new DelayedSetter(); /// /// Initializes a new instance of the class. @@ -234,51 +235,67 @@ namespace Avalonia /// The priority level that the value came from. private void UpdateValue(object value, int priority) { - var notification = value as BindingNotification; - object castValue; - - if (notification != null) - { - value = (notification.HasValue) ? notification.Value : null; - } - - if (TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) + if (!delayedSetter.IsNotifying(this)) { - var old = _value; + var notification = value as BindingNotification; + object castValue; - if (_validate != null && castValue != AvaloniaProperty.UnsetValue) + if (notification != null) { - castValue = _validate(castValue); + value = (notification.HasValue) ? notification.Value : null; } - ValuePriority = priority; - _value = castValue; - - if (notification?.HasValue == true) + if (!object.Equals(value, _value) && TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) { - notification.SetValue(castValue); - } + var old = _value; - if (notification == null || notification.HasValue) - { - Owner?.Changed(this, old, _value); - } + if (_validate != null && castValue != AvaloniaProperty.UnsetValue) + { + castValue = _validate(castValue); + } - if (notification != null) - { - Owner?.BindingNotificationReceived(this, notification); + ValuePriority = priority; + _value = castValue; + + if (notification?.HasValue == true) + { + notification.SetValue(castValue); + } + + if (notification == null || notification.HasValue) + { + using (delayedSetter.MarkNotifying(this)) + { + Owner?.Changed(this, old, _value); + } + + if (delayedSetter.HasPendingSet(this)) + { + var pendingSet = delayedSetter.GetFirstPendingSet(this); + UpdateValue(pendingSet.value, pendingSet.priority); + } + } + + if (notification != null) + { + Owner?.BindingNotificationReceived(this, notification); + } } + else + { + Logger.Error( + LogArea.Binding, + Owner, + "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", + Property.Name, + _valueType, + value, + value?.GetType()); + } } else { - Logger.Error( - LogArea.Binding, - Owner, - "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", - Property.Name, - _valueType, - value, - value?.GetType()); + delayedSetter.AddPendingSet(this, (value, priority)); } } } diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index ff37509847..b266b44cba 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -428,7 +428,7 @@ namespace Avalonia.Base.UnitTests //here in real life stack overflow exception is thrown issue #855 and #824 target.DoubleValue = 51.001; - Assert.Equal(2, viewModel.SetterInvokedCount); + Assert.Equal(3, viewModel.SetterInvokedCount); double expected = 51; From 440f2cafc566ff3b17865551907cbdbbe80d4b44 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Oct 2017 21:39:11 -0500 Subject: [PATCH 09/24] Fix SelectedItemsControl and RangeBase Stackoverflow bugs. --- src/Avalonia.Base/AvaloniaObject.cs | 26 ++++++++++- .../Primitives/SelectingItemsControl.cs | 44 ++++++++----------- .../Primitives/RangeBaseTests.cs | 2 +- 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 91e087abc0..b5956d681a 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -54,7 +54,7 @@ namespace Avalonia /// /// Delayed setter helper for direct properties. Used to fix #855. /// - protected readonly DelayedSetter directDelayedSetter = new DelayedSetter(); + private readonly DelayedSetter directDelayedSetter = new DelayedSetter(); /// @@ -589,6 +589,30 @@ namespace Avalonia } } + protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Action delayedSet) + { + Contract.Requires(setterCallback != null); + Contract.Requires(delayedSet != null); + if (!directDelayedSetter.IsNotifying(property)) + { + setterCallback(value, notification => + { + using (directDelayedSetter.MarkNotifying(property)) + { + notification(); + } + }); + if (directDelayedSetter.HasPendingSet(property)) + { + delayedSet((T)directDelayedSetter.GetFirstPendingSet(property)); + } + } + else + { + directDelayedSetter.AddPendingSet(property, value); + } + } + /// /// Tries to cast a value to a type, taking into account that the value may be a /// . diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index e5f5e8fc30..294b14b13d 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -151,15 +151,18 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - var old = SelectedIndex; - var effective = (value >= 0 && value < Items?.Cast().Count()) ? value : -1; - - if (old != effective) + SetAndRaise(SelectedIndexProperty, (val, notifierWrapper) => { - _selectedIndex = effective; - RaisePropertyChanged(SelectedIndexProperty, old, effective, BindingPriority.LocalValue); - SelectedItem = ElementAt(Items, effective); - } + var old = SelectedIndex; + var effective = (val >= 0 && val < Items?.Cast().Count()) ? val : -1; + + if (old != effective) + { + _selectedIndex = effective; + notifierWrapper(() => RaisePropertyChanged(SelectedIndexProperty, old, effective, BindingPriority.LocalValue)); + SelectedItem = ElementAt(Items, effective); + } + }, value, val => SelectedIndex = val); } else { @@ -183,19 +186,17 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - if (!directDelayedSetter.IsNotifying(SelectedItemProperty)) + SetAndRaise(SelectedItemProperty, (val, notifyWrapper) => { var old = SelectedItem; - var index = IndexOf(Items, value); - var effective = index != -1 ? value : null; + var index = IndexOf(Items, val); + var effective = index != -1 ? val : null; if (!object.Equals(effective, old)) { _selectedItem = effective; - using (directDelayedSetter.MarkNotifying(SelectedItemProperty)) - { - RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue); - } + + notifyWrapper(() => RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue)); SelectedIndex = index; @@ -213,17 +214,8 @@ namespace Avalonia.Controls.Primitives { SelectedItems.Clear(); } - - if (directDelayedSetter.HasPendingSet(SelectedItemProperty)) - { - SelectedItem = directDelayedSetter.GetFirstPendingSet(SelectedItemProperty); - } - } - } - else - { - directDelayedSetter.AddPendingSet(SelectedItemProperty, value); - } + } + }, value, val => SelectedItem = val); } else { diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index 0e67581760..062ace2648 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs @@ -151,7 +151,7 @@ namespace Avalonia.Controls.UnitTests.Primitives //here in real life stack overflow exception is thrown issue #855 and #824 target.Value = 51.001; - Assert.Equal(2, viewModel.SetterInvokedCount); + Assert.Equal(3, viewModel.SetterInvokedCount); double expected = 51; From da8267ade75006766cbb97438b2ce8ad0584512b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 12:00:17 -0500 Subject: [PATCH 10/24] Fix last binding test. Two-way bindings via the indexer binding should not cause a PriorityBindingEntry to be created in the target. So instead use SetValue + GetObservable (similar to our higher level implementations in Avalonia.Markup). --- src/Avalonia.Base/AvaloniaObject.cs | 23 +-------- src/Avalonia.Base/AvaloniaObjectExtensions.cs | 24 ++------- .../Reactive/AnonymousSubject`1.cs | 16 ------ .../Reactive/AnonymousSubject`2.cs | 49 ------------------- src/Avalonia.Base/Utilities/DelayedSetter.cs | 26 +++++++++- .../AvaloniaObjectTests_Binding.cs | 1 + 6 files changed, 31 insertions(+), 108 deletions(-) delete mode 100644 src/Avalonia.Base/Reactive/AnonymousSubject`1.cs delete mode 100644 src/Avalonia.Base/Reactive/AnonymousSubject`2.cs diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index b5956d681a..3f53a3735c 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -590,28 +590,7 @@ namespace Avalonia } protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Action delayedSet) - { - Contract.Requires(setterCallback != null); - Contract.Requires(delayedSet != null); - if (!directDelayedSetter.IsNotifying(property)) - { - setterCallback(value, notification => - { - using (directDelayedSetter.MarkNotifying(property)) - { - notification(); - } - }); - if (directDelayedSetter.HasPendingSet(property)) - { - delayedSet((T)directDelayedSetter.GetFirstPendingSet(property)); - } - } - else - { - directDelayedSetter.AddPendingSet(property, value); - } - } + => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, val => delayedSet((T)val)); /// /// Tries to cast a value to a type, taking into account that the value may be a diff --git a/src/Avalonia.Base/AvaloniaObjectExtensions.cs b/src/Avalonia.Base/AvaloniaObjectExtensions.cs index 685bf83a75..7967852bfa 100644 --- a/src/Avalonia.Base/AvaloniaObjectExtensions.cs +++ b/src/Avalonia.Base/AvaloniaObjectExtensions.cs @@ -138,17 +138,9 @@ namespace Avalonia AvaloniaProperty property, BindingPriority priority = BindingPriority.LocalValue) { - // TODO: Subject.Create is not yet in stable Rx : once it is, remove the - // AnonymousSubject classes and use Subject.Create. - var output = new Subject(); - var result = new AnonymousSubject( - Observer.Create( - x => output.OnNext(x), - e => output.OnError(e), - () => output.OnCompleted()), + return Subject.Create( + Observer.Create(x => o.SetValue(property, x, priority)), o.GetObservable(property)); - o.Bind(property, output, priority); - return result; } /// @@ -169,17 +161,9 @@ namespace Avalonia AvaloniaProperty property, BindingPriority priority = BindingPriority.LocalValue) { - // TODO: Subject.Create is not yet in stable Rx : once it is, remove the - // AnonymousSubject classes from this file and use Subject.Create. - var output = new Subject(); - var result = new AnonymousSubject( - Observer.Create( - x => output.OnNext(x), - e => output.OnError(e), - () => output.OnCompleted()), + return Subject.Create( + Observer.Create(x => o.SetValue(property, x, priority)), o.GetObservable(property)); - o.Bind(property, output, priority); - return result; } /// diff --git a/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs b/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs deleted file mode 100644 index c7bed15ecb..0000000000 --- a/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs +++ /dev/null @@ -1,16 +0,0 @@ -// 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 System; -using System.Reactive.Subjects; - -namespace Avalonia.Reactive -{ - public class AnonymousSubject : AnonymousSubject, ISubject - { - public AnonymousSubject(IObserver observer, IObservable observable) - : base(observer, observable) - { - } - } -} diff --git a/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs b/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs deleted file mode 100644 index 18551d564e..0000000000 --- a/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs +++ /dev/null @@ -1,49 +0,0 @@ -// 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 System; -using System.Reactive.Subjects; - -namespace Avalonia.Reactive -{ - public class AnonymousSubject : ISubject - { - private readonly IObserver _observer; - private readonly IObservable _observable; - - public AnonymousSubject(IObserver observer, IObservable observable) - { - _observer = observer; - _observable = observable; - } - - public void OnCompleted() - { - _observer.OnCompleted(); - } - - public void OnError(Exception error) - { - if (error == null) - throw new ArgumentNullException("error"); - - _observer.OnError(error); - } - - public void OnNext(T value) - { - _observer.OnNext(value); - } - - public IDisposable Subscribe(IObserver observer) - { - if (observer == null) - throw new ArgumentNullException("observer"); - - // - // [OK] Use of unsafe Subscribe: non-pretentious wrapping of an observable sequence. - // - return _observable.Subscribe/*Unsafe*/(observer); - } - } -} diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs index c8c9c87d95..3abffbdd3d 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -5,7 +5,7 @@ using System.Text; namespace Avalonia.Utilities { - public class DelayedSetter + class DelayedSetter { private class SettingStatus { @@ -57,5 +57,29 @@ namespace Avalonia.Utilities { return setRecords[property].PendingValues.Dequeue(); } + + public void SetAndNotify(T property, Action> setterCallback, TValue value, Action delayedSet) + { + Contract.Requires(setterCallback != null); + Contract.Requires(delayedSet != null); + if (!IsNotifying(property)) + { + setterCallback(value, notification => + { + using (MarkNotifying(property)) + { + notification(); + } + }); + if (HasPendingSet(property)) + { + delayedSet(GetFirstPendingSet(property)); + } + } + else + { + AddPendingSet(property, value); + } + } } } diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index b266b44cba..211c7c985e 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -14,6 +14,7 @@ using Avalonia.Markup.Xaml.Data; using Avalonia.Platform; using Avalonia.Threading; using Avalonia.UnitTests; +using Avalonia.Diagnostics; using Microsoft.Reactive.Testing; using Moq; using Xunit; From 8acf94d3e4f651b34a9480b13ab69f8fabeba266 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 16:52:29 -0500 Subject: [PATCH 11/24] 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; From 36fe461b4a4ad568b0c0df380ad759f8f02e50d2 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 17:22:09 -0500 Subject: [PATCH 12/24] Have PriorityValue use SetAndNotify infrastructure. --- src/Avalonia.Base/PriorityValue.cs | 89 ++++++++------------ src/Avalonia.Base/Utilities/DelayedSetter.cs | 7 +- 2 files changed, 40 insertions(+), 56 deletions(-) diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index df138c88f5..3930b6c224 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -235,75 +235,56 @@ namespace Avalonia /// The priority level that the value came from. private void UpdateValue(object value, int priority) { - if (!delayedSetter.IsNotifying(this)) + delayedSetter.SetAndNotify(this, (update, notify) => { - value = UpdateValueCore(value, priority); + var val = update.value; + var notification = val as BindingNotification; + object castValue; - while (delayedSetter.HasPendingSet(this)) + if (notification != null) { - var pendingSet = delayedSetter.GetFirstPendingSet(this); - UpdateValueCore(pendingSet.value, pendingSet.priority); + val = (notification.HasValue) ? notification.Value : null; } - } - else if(!object.Equals(value, _value)) - { - delayedSetter.AddPendingSet(this, (value, priority)); - } - } - - private object UpdateValueCore(object value, int priority) - { - var notification = value as BindingNotification; - object castValue; - if (notification != null) - { - value = (notification.HasValue) ? notification.Value : null; - } + if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) + { + var old = _value; - if (TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) - { - var old = _value; + if (_validate != null && castValue != AvaloniaProperty.UnsetValue) + { + castValue = _validate(castValue); + } - if (_validate != null && castValue != AvaloniaProperty.UnsetValue) - { - castValue = _validate(castValue); - } + ValuePriority = priority; + _value = castValue; - ValuePriority = priority; - _value = castValue; + if (notification?.HasValue == true) + { + notification.SetValue(castValue); + } - if (notification?.HasValue == true) - { - notification.SetValue(castValue); - } + if (notification == null || notification.HasValue) + { + notify(() => Owner?.Changed(this, old, _value)); + } - if (notification == null || notification.HasValue) - { - using (delayedSetter.MarkNotifying(this)) + if (notification != null) { - Owner?.Changed(this, old, _value); + Owner?.BindingNotificationReceived(this, notification); } } - - if (notification != null) + else { - Owner?.BindingNotificationReceived(this, notification); + Logger.Error( + LogArea.Binding, + Owner, + "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", + Property.Name, + _valueType, + val, + val?.GetType()); } - } - else - { - Logger.Error( - LogArea.Binding, - Owner, - "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", - Property.Name, - _valueType, - value, - value?.GetType()); - } - - return value; + }, (value, priority), val => !object.Equals(val.value, _value)); } } } diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs index 6d611dfa46..155e4509a2 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -58,7 +58,7 @@ namespace Avalonia.Utilities return setRecords[property].PendingValues.Dequeue(); } - public void SetAndNotify(T property, Action> setterCallback, TValue value) + public void SetAndNotify(T property, Action> setterCallback, TValue value, Predicate pendingSetCondition) { Contract.Requires(setterCallback != null); if (!IsNotifying(property)) @@ -81,10 +81,13 @@ namespace Avalonia.Utilities }); } } - else + else if(pendingSetCondition?.Invoke(value) ?? true) { AddPendingSet(property, value); } } + + public void SetAndNotify(T property, Action> setterCallback, TValue value) + => SetAndNotify(property, setterCallback, value, null); } } From a87ee4e49de7b862220da654962624b9c85daf40 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 17:36:17 -0500 Subject: [PATCH 13/24] Clean up DelayedSetter code. --- src/Avalonia.Base/AvaloniaObject.cs | 4 ++-- src/Avalonia.Base/Utilities/DelayedSetter.cs | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 1c4f727161..f0e385cf94 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -590,8 +590,8 @@ namespace Avalonia } } - protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value) - => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value); + protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Predicate pendingSetCondition = null) + => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, o => pendingSetCondition((T)o)); /// /// Tries to cast a value to a type, taking into account that the value may be a diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs index 155e4509a2..a8eb7554be 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -86,8 +86,5 @@ namespace Avalonia.Utilities AddPendingSet(property, value); } } - - public void SetAndNotify(T property, Action> setterCallback, TValue value) - => SetAndNotify(property, setterCallback, value, null); } } From d775d2384ea2155cc1756fdf07ed977fa858765f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 18:04:36 -0500 Subject: [PATCH 14/24] Refactor AvaloniaObject.SetAndRaise and comment DelayedSetter. --- src/Avalonia.Base/AvaloniaObject.cs | 14 +++-- src/Avalonia.Base/Utilities/DelayedSetter.cs | 61 +++++++++++++++++--- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index f0e385cf94..7b978175bd 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -561,16 +561,20 @@ namespace Avalonia VerifyAccess(); if (!directDelayedSetter.IsNotifying(property)) { + var valueChanged = false; if (!object.Equals(field, value)) { SetAndRaiseCore(property, ref field, value); - while (directDelayedSetter.HasPendingSet(property)) - { - SetAndRaiseCore(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); - } - return true; + valueChanged = true; + } + + while (directDelayedSetter.HasPendingSet(property)) + { + SetAndRaiseCore(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); + valueChanged = true; } + return valueChanged; } else if(!object.Equals(field, value)) { diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DelayedSetter.cs index a8eb7554be..4e1a39dd31 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DelayedSetter.cs @@ -5,8 +5,16 @@ using System.Text; namespace Avalonia.Utilities { - class DelayedSetter + /// + /// A utility class to enable deferring assignment until after property-changed notifications are sent. + /// + /// The type of the object that represents the property. + /// The type of value with which to track the delayed assignment. + class DelayedSetter { + /// + /// Information on current setting/notification status of a property. + /// private class SettingStatus { public bool Notifying { get; set; } @@ -22,9 +30,14 @@ namespace Avalonia.Utilities } } - private readonly Dictionary setRecords = new Dictionary(); + private readonly Dictionary setRecords = new Dictionary(); - public IDisposable MarkNotifying(T property) + /// + /// Mark the property as currently notifying. + /// + /// The property to mark as notifying. + /// Returns a disposable that when disposed, marks the property as done notifying. + internal IDisposable MarkNotifying(TProperty property) { Contract.Requires(!IsNotifying(property)); @@ -37,10 +50,22 @@ namespace Avalonia.Utilities return Disposable.Create(() => setRecords[property].Notifying = false); } - public bool IsNotifying(T property) => setRecords.TryGetValue(property, out var value) && value.Notifying; + /// + /// Check if the property is currently notifying listeners. + /// + /// The property. + /// If the property is currently notifying listeners. + internal bool IsNotifying(TProperty property) + => setRecords.TryGetValue(property, out var value) && value.Notifying; - public void AddPendingSet(T property, TValue value) + /// + /// Add a pending assignment for the property. + /// + /// The property. + /// The value to assign. + internal void AddPendingSet(TProperty property, TValue value) { + Contract.Requires(IsNotifying(property)); if (!setRecords.ContainsKey(property)) { setRecords[property] = new SettingStatus(); @@ -48,17 +73,37 @@ namespace Avalonia.Utilities setRecords[property].PendingValues.Enqueue(value); } - public bool HasPendingSet(T property) + /// + /// Checks if there are any pending assignments for the property. + /// + /// The property to check. + /// If the property has any pending assignments. + internal bool HasPendingSet(TProperty property) { return setRecords.ContainsKey(property) && setRecords[property].PendingValues.Count != 0; } - public TValue GetFirstPendingSet(T property) + /// + /// Gets the first pending assignment for the property. + /// + /// The property to check. + /// The first pending assignment for the property. + internal TValue GetFirstPendingSet(TProperty property) { return setRecords[property].PendingValues.Dequeue(); } - public void SetAndNotify(T property, Action> setterCallback, TValue value, Predicate pendingSetCondition) + /// + /// Set the property and notify listeners while ensuring we don't get into a stack overflow as happens with #855 and #824 + /// + /// The property to set. + /// + /// 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. + /// + /// The value to try to set. + /// A predicate to filter what possible values should be added as pending sets (i.e. only values not equal to the current value). + public void SetAndNotify(TProperty property, Action> setterCallback, TValue value, Predicate pendingSetCondition) { Contract.Requires(setterCallback != null); if (!IsNotifying(property)) From e9e0de662ba43d48dbd289db13bb28c2035bc6ac Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 18:08:03 -0500 Subject: [PATCH 15/24] Fix NRE --- src/Avalonia.Base/AvaloniaObject.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 7b978175bd..456db70d77 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -595,7 +595,7 @@ namespace Avalonia } protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Predicate pendingSetCondition = null) - => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, o => pendingSetCondition((T)o)); + => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, o => pendingSetCondition?.Invoke((T)o) ?? true); /// /// Tries to cast a value to a type, taking into account that the value may be a From 4469bafe28a148ac200c28347d8039e3c6718185 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 24 Oct 2017 18:14:04 -0500 Subject: [PATCH 16/24] Fix incorrect assert in RangeBaseTests. --- tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index 062ace2648..0e67581760 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs @@ -151,7 +151,7 @@ namespace Avalonia.Controls.UnitTests.Primitives //here in real life stack overflow exception is thrown issue #855 and #824 target.Value = 51.001; - Assert.Equal(3, viewModel.SetterInvokedCount); + Assert.Equal(2, viewModel.SetterInvokedCount); double expected = 51; From edf2018149018c9bfd7da44d5e01c19a23f1cd78 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 25 Oct 2017 23:50:02 -0500 Subject: [PATCH 17/24] Refactored tests to move XAML Binding class tests into Avalonia.Markup.Xaml.UnitTests. --- .../Avalonia.Base.UnitTests.csproj | 12 -- .../AvaloniaObjectTests_Binding.cs | 20 +-- .../AvaloniaObjectTests_Direct.cs | 20 +-- .../Avalonia.Markup.Xaml.UnitTests.csproj | 1 + .../Data/BindingTests.cs | 139 ++++++++++++++++++ 5 files changed, 146 insertions(+), 46 deletions(-) diff --git a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj index e86aec4be1..8692cdef42 100644 --- a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj +++ b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj @@ -13,18 +13,6 @@ - - {B09B78D8-9B26-48B0-9149-D64A2F120F3F} - Avalonia.Base - - - {3E53A01A-B331-47F3-B828-4A5717E77A24} - Avalonia.Markup.Xaml - - - {88060192-33d5-4932-b0f9-8bd2763e857d} - Avalonia.UnitTests - \ No newline at end of file diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 192f5258ab..529e35d92c 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -387,10 +387,8 @@ namespace Avalonia.Base.UnitTests } } - [Theory] - [InlineData(true)] - [InlineData(false)] - public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values(bool useXamlBinding) + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() { var viewModel = new TestStackOverflowViewModel() { @@ -404,19 +402,7 @@ namespace Avalonia.Base.UnitTests var child = new Class1(); - if (useXamlBinding) - { - child.Bind(Class1.DoubleValueProperty, - new Binding("DoubleValue") - { - Mode = BindingMode.TwoWay, - Source = target - }); - } - else - { - child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; - } + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; Assert.Equal(1, viewModel.SetterInvokedCount); diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index 249528e9b2..ffea0fd2d6 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -440,10 +440,8 @@ namespace Avalonia.Base.UnitTests Assert.Equal(BindingMode.OneWayToSource, bar.GetMetadata().DefaultBindingMode); } - [Theory] - [InlineData(true)] - [InlineData(false)] - public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values(bool useXamlBinding) + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() { var viewModel = new TestStackOverflowViewModel() { @@ -460,19 +458,7 @@ namespace Avalonia.Base.UnitTests var child = new Class1(); - if (useXamlBinding) - { - child.Bind(Class1.DoubleValueProperty, - new Binding("DoubleValue") - { - Mode = BindingMode.TwoWay, - Source = target - }); - } - else - { - child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; - } + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; Assert.Equal(1, viewModel.SetterInvokedCount); diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj b/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj index 03bb71a383..b2d71d7fa8 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj @@ -22,6 +22,7 @@ + diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs index 230e61f300..11a1154b26 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs @@ -337,6 +337,145 @@ namespace Avalonia.Markup.Xaml.UnitTests.Data Assert.Equal("foo", target.Content); } + + [Fact] + public void StyledProperty_SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new StyledPropertyClass(); + + target.Bind(StyledPropertyClass.DoubleValueProperty, + new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + + var child = new StyledPropertyClass(); + + child.Bind(StyledPropertyClass.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new DirectPropertyClass(); + + target.Bind(DirectPropertyClass.DoubleValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = viewModel + }); + + var child = new DirectPropertyClass(); + + child.Bind(DirectPropertyClass.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + + private class StyledPropertyClass : AvaloniaObject + { + public static readonly StyledProperty DoubleValueProperty = + AvaloniaProperty.Register(nameof(DoubleValue)); + + public double DoubleValue + { + get { return GetValue(DoubleValueProperty); } + set { SetValue(DoubleValueProperty, value); } + } + } + + private class DirectPropertyClass : AvaloniaObject + { + public static readonly DirectProperty DoubleValueProperty = + AvaloniaProperty.RegisterDirect( + nameof(DoubleValue), + o => o.DoubleValue, + (o, v) => o.DoubleValue = v); + + private double _doubleValue; + public double DoubleValue + { + get { return _doubleValue; } + set { SetAndRaise(DoubleValueProperty, ref _doubleValue, value); } + } + } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } + private class TwoWayBindingTest : Control { public static readonly StyledProperty TwoWayProperty = From add5f0451b947e2a1523ba3792342bdc52be5168 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Sun, 12 Nov 2017 01:21:25 -0600 Subject: [PATCH 18/24] DelayedSetter -> DeferredSetter --- src/Avalonia.Base/AvaloniaObject.cs | 2 +- src/Avalonia.Base/PriorityValue.cs | 2 +- .../Utilities/{DelayedSetter.cs => DeferredSetter.cs} | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename src/Avalonia.Base/Utilities/{DelayedSetter.cs => DeferredSetter.cs} (99%) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 456db70d77..2583b7ac97 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -54,7 +54,7 @@ namespace Avalonia /// /// Delayed setter helper for direct properties. Used to fix #855. /// - private readonly DelayedSetter directDelayedSetter = new DelayedSetter(); + private readonly DeferredSetter directDelayedSetter = new DeferredSetter(); /// diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 3930b6c224..2a6c5bc2c6 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -30,7 +30,7 @@ namespace Avalonia private readonly SingleOrDictionary _levels = new SingleOrDictionary(); private object _value; private readonly Func _validate; - private static readonly DelayedSetter delayedSetter = new DelayedSetter(); + private static readonly DeferredSetter delayedSetter = new DeferredSetter(); /// /// Initializes a new instance of the class. diff --git a/src/Avalonia.Base/Utilities/DelayedSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs similarity index 99% rename from src/Avalonia.Base/Utilities/DelayedSetter.cs rename to src/Avalonia.Base/Utilities/DeferredSetter.cs index 4e1a39dd31..4b2709c141 100644 --- a/src/Avalonia.Base/Utilities/DelayedSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -10,7 +10,7 @@ namespace Avalonia.Utilities /// /// The type of the object that represents the property. /// The type of value with which to track the delayed assignment. - class DelayedSetter + class DeferredSetter { /// /// Information on current setting/notification status of a property. From 4f8f5e9c719bcf2e4fc336d758bdea997ad6f5b0 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 5 Dec 2017 00:54:37 +0100 Subject: [PATCH 19/24] Added some benchmarks Added some benchmarks to test setting and binding properties. --- .../Avalonia.Benchmarks.csproj | 1 + tests/Avalonia.Benchmarks/Base/Properties.cs | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/Avalonia.Benchmarks/Base/Properties.cs diff --git a/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj b/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj index 1d987e2238..c16b89e0b6 100644 --- a/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj +++ b/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj @@ -49,6 +49,7 @@ + diff --git a/tests/Avalonia.Benchmarks/Base/Properties.cs b/tests/Avalonia.Benchmarks/Base/Properties.cs new file mode 100644 index 0000000000..0a020961d5 --- /dev/null +++ b/tests/Avalonia.Benchmarks/Base/Properties.cs @@ -0,0 +1,43 @@ +using System; +using System.Reactive.Subjects; +using BenchmarkDotNet.Attributes; + +namespace Avalonia.Benchmarks.Base +{ + [MemoryDiagnoser] + public class AvaloniaObjectBenchmark + { + private Class1 target = new Class1(); + private Subject intBinding = new Subject(); + + public AvaloniaObjectBenchmark() + { + target.SetValue(Class1.IntProperty, 123); + } + + [Benchmark] + public void ClearAndSetIntProperty() + { + target.ClearValue(Class1.IntProperty); + target.SetValue(Class1.IntProperty, 123); + } + + [Benchmark] + public void BindIntProperty() + { + using (target.Bind(Class1.IntProperty, intBinding)) + { + for (var i = 0; i < 100; ++i) + { + intBinding.OnNext(i); + } + } + } + + class Class1 : AvaloniaObject + { + public static readonly AvaloniaProperty IntProperty = + AvaloniaProperty.Register("Int"); + } + } +} From d8efff505d82d61650edf7ab0f79773c8e45bc90 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 4 Dec 2017 20:05:10 -0600 Subject: [PATCH 20/24] Clean up formatting. Fix bug in PriorityValue usage of DeferredSetter. Change DeferredSetter to use ConditionalWeakTable to not hold strong references to PriorityValue objects. --- src/Avalonia.Base/AvaloniaObject.cs | 15 +++- src/Avalonia.Base/PriorityValue.cs | 85 ++++++++++--------- src/Avalonia.Base/Utilities/DeferredSetter.cs | 47 ++++++---- .../AvaloniaObjectTests_Binding.cs | 2 +- 4 files changed, 89 insertions(+), 60 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 2583b7ac97..12b7e3d529 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -594,8 +594,19 @@ namespace Avalonia } } - protected void SetAndRaise(AvaloniaProperty property, Action> setterCallback, T value, Predicate pendingSetCondition = null) - => directDelayedSetter.SetAndNotify(property, (val, notify) => setterCallback((T)val, notify), value, o => pendingSetCondition?.Invoke((T)o) ?? true); + protected void SetAndRaise( + AvaloniaProperty property, + Action> setterCallback, + T value, + Predicate pendingSetCondition = null) + { + Contract.Requires(setterCallback != null); + directDelayedSetter.SetAndNotify( + property, + (val, notify) => setterCallback((T)val, notify), + value, + pendingSetCondition != null ? o => pendingSetCondition((T)o) : (Predicate)null); + } /// /// 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 2a6c5bc2c6..5d223c3cf8 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -235,56 +235,61 @@ namespace Avalonia /// The priority level that the value came from. private void UpdateValue(object value, int priority) { - delayedSetter.SetAndNotify(this, (update, notify) => + delayedSetter.SetAndNotify(this, + UpdateCore, + (value, priority), + val => !object.Equals(val.value, _value)); + } + + private void UpdateCore((object value, int priority) update, Action notify) + { + var val = update.value; + var notification = val as BindingNotification; + object castValue; + + if (notification != null) { - var val = update.value; - var notification = val as BindingNotification; - object castValue; + val = (notification.HasValue) ? notification.Value : null; + } - if (notification != null) - { - val = (notification.HasValue) ? notification.Value : null; - } + if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) + { + var old = _value; - if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) + if (_validate != null && castValue != AvaloniaProperty.UnsetValue) { - var old = _value; - - if (_validate != null && castValue != AvaloniaProperty.UnsetValue) - { - castValue = _validate(castValue); - } + castValue = _validate(castValue); + } - ValuePriority = priority; - _value = castValue; + ValuePriority = update.priority; + _value = castValue; - if (notification?.HasValue == true) - { - notification.SetValue(castValue); - } - - if (notification == null || notification.HasValue) - { - notify(() => Owner?.Changed(this, old, _value)); - } + if (notification?.HasValue == true) + { + notification.SetValue(castValue); + } - if (notification != null) - { - Owner?.BindingNotificationReceived(this, notification); - } + if (notification == null || notification.HasValue) + { + notify(() => 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, - val, - val?.GetType()); + Owner?.BindingNotificationReceived(this, notification); } - }, (value, priority), val => !object.Equals(val.value, _value)); + } + else + { + Logger.Error( + LogArea.Binding, + Owner, + "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", + Property.Name, + _valueType, + val, + val?.GetType()); + } } } } diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index 4b2709c141..b87711c1d9 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Reactive.Disposables; +using System.Runtime.CompilerServices; using System.Text; namespace Avalonia.Utilities @@ -11,7 +12,24 @@ namespace Avalonia.Utilities /// The type of the object that represents the property. /// The type of value with which to track the delayed assignment. class DeferredSetter + where TProperty: class { + private struct NotifyDisposable : IDisposable + { + private readonly SettingStatus status; + + public NotifyDisposable(SettingStatus status) + { + this.status = status; + status.Notifying = true; + } + + public void Dispose() + { + status.Notifying = false; + } + } + /// /// Information on current setting/notification status of a property. /// @@ -30,7 +48,7 @@ namespace Avalonia.Utilities } } - private readonly Dictionary setRecords = new Dictionary(); + private readonly ConditionalWeakTable setRecords = new ConditionalWeakTable(); /// /// Mark the property as currently notifying. @@ -40,14 +58,8 @@ namespace Avalonia.Utilities internal IDisposable MarkNotifying(TProperty property) { Contract.Requires(!IsNotifying(property)); - - if (!setRecords.ContainsKey(property)) - { - setRecords[property] = new SettingStatus(); - } - setRecords[property].Notifying = true; - - return Disposable.Create(() => setRecords[property].Notifying = false); + + return new NotifyDisposable(setRecords.GetOrCreateValue(property)); } /// @@ -66,11 +78,8 @@ namespace Avalonia.Utilities internal void AddPendingSet(TProperty property, TValue value) { Contract.Requires(IsNotifying(property)); - if (!setRecords.ContainsKey(property)) - { - setRecords[property] = new SettingStatus(); - } - setRecords[property].PendingValues.Enqueue(value); + + setRecords.GetOrCreateValue(property).PendingValues.Enqueue(value); } /// @@ -80,7 +89,7 @@ namespace Avalonia.Utilities /// If the property has any pending assignments. internal bool HasPendingSet(TProperty property) { - return setRecords.ContainsKey(property) && setRecords[property].PendingValues.Count != 0; + return setRecords.TryGetValue(property, out var status) && status.PendingValues.Count != 0; } /// @@ -90,7 +99,7 @@ namespace Avalonia.Utilities /// The first pending assignment for the property. internal TValue GetFirstPendingSet(TProperty property) { - return setRecords[property].PendingValues.Dequeue(); + return setRecords.GetOrCreateValue(property).PendingValues.Dequeue(); } /// @@ -103,7 +112,11 @@ namespace Avalonia.Utilities /// /// The value to try to set. /// A predicate to filter what possible values should be added as pending sets (i.e. only values not equal to the current value). - public void SetAndNotify(TProperty property, Action> setterCallback, TValue value, Predicate pendingSetCondition) + public void SetAndNotify( + TProperty property, + Action> setterCallback, + TValue value, + Predicate pendingSetCondition) { Contract.Requires(setterCallback != null); if (!IsNotifying(property)) diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 529e35d92c..cb61a663a1 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -406,7 +406,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); From ca9a4c4128a3a1ef4f3c92b4500f3074f56a3689 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 5 Dec 2017 17:18:32 -0600 Subject: [PATCH 21/24] PR Feedback. --- src/Avalonia.Base/AvaloniaObject.cs | 28 +++++++++++++------ src/Avalonia.Base/PriorityValue.cs | 24 ++++++---------- src/Avalonia.Base/Utilities/DeferredSetter.cs | 11 ++++---- .../Primitives/SelectingItemsControl.cs | 20 +++++++++---- .../ListBoxTests_Single.cs | 2 +- 5 files changed, 50 insertions(+), 35 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 12b7e3d529..8506414289 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -51,10 +51,19 @@ namespace Avalonia /// private EventHandler _propertyChanged; + private DeferredSetter _directDeferredSetter; + /// /// Delayed setter helper for direct properties. Used to fix #855. /// - private readonly DeferredSetter directDelayedSetter = new DeferredSetter(); + private DeferredSetter DirectDelayedSetter + { + get + { + return _directDeferredSetter ?? + (_directDeferredSetter = new DeferredSetter()); + } + } /// @@ -559,7 +568,7 @@ namespace Avalonia protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) { VerifyAccess(); - if (!directDelayedSetter.IsNotifying(property)) + if (!DirectDelayedSetter.IsNotifying(property)) { var valueChanged = false; if (!object.Equals(field, value)) @@ -569,16 +578,16 @@ namespace Avalonia valueChanged = true; } - while (directDelayedSetter.HasPendingSet(property)) + while (DirectDelayedSetter.HasPendingSet(property)) { - SetAndRaiseCore(property, ref field, (T)directDelayedSetter.GetFirstPendingSet(property)); + SetAndRaiseCore(property, ref field, (T)DirectDelayedSetter.GetFirstPendingSet(property)); valueChanged = true; } return valueChanged; } else if(!object.Equals(field, value)) { - directDelayedSetter.AddPendingSet(property, value); + DirectDelayedSetter.AddPendingSet(property, value); } return false; } @@ -588,7 +597,7 @@ namespace Avalonia var old = field; field = value; - using (directDelayedSetter.MarkNotifying(property)) + using (DirectDelayedSetter.MarkNotifying(property)) { RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); } @@ -598,14 +607,15 @@ namespace Avalonia AvaloniaProperty property, Action> setterCallback, T value, - Predicate pendingSetCondition = null) + Predicate pendingSetCondition) { Contract.Requires(setterCallback != null); - directDelayedSetter.SetAndNotify( + Contract.Requires(pendingSetCondition != null); + DirectDelayedSetter.SetAndNotify( property, (val, notify) => setterCallback((T)val, notify), value, - pendingSetCondition != null ? o => pendingSetCondition((T)o) : (Predicate)null); + o => pendingSetCondition((T)o)); } /// diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 5d223c3cf8..ea10427b10 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -28,9 +28,10 @@ namespace Avalonia { private readonly Type _valueType; private readonly SingleOrDictionary _levels = new SingleOrDictionary(); - private object _value; + private readonly Func _validate; private static readonly DeferredSetter delayedSetter = new DeferredSetter(); + private (object value, int priority) _value; /// /// Initializes a new instance of the class. @@ -47,9 +48,7 @@ namespace Avalonia { Owner = owner; Property = property; - _valueType = valueType; - _value = AvaloniaProperty.UnsetValue; - ValuePriority = int.MaxValue; + _value = (AvaloniaProperty.UnsetValue, int.MaxValue); _validate = validate; } @@ -66,16 +65,12 @@ namespace Avalonia /// /// Gets the current value. /// - public object Value => _value; + public object Value => _value.value; /// /// Gets the priority of the binding that is currently active. /// - public int ValuePriority - { - get; - private set; - } + public int ValuePriority => _value.priority; /// /// Adds a new binding. @@ -238,7 +233,7 @@ namespace Avalonia delayedSetter.SetAndNotify(this, UpdateCore, (value, priority), - val => !object.Equals(val.value, _value)); + val => !object.Equals(val.value, val.value)); } private void UpdateCore((object value, int priority) update, Action notify) @@ -254,15 +249,14 @@ namespace Avalonia if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) { - var old = _value; + var old = this._value.value; if (_validate != null && castValue != AvaloniaProperty.UnsetValue) { castValue = _validate(castValue); } - ValuePriority = update.priority; - _value = castValue; + this._value = (castValue, update.priority); if (notification?.HasValue == true) { @@ -271,7 +265,7 @@ namespace Avalonia if (notification == null || notification.HasValue) { - notify(() => Owner?.Changed(this, old, _value)); + notify(() => Owner?.Changed(this, old, Value)); } if (notification != null) diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index b87711c1d9..b42ca0bf3d 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -14,11 +14,11 @@ namespace Avalonia.Utilities class DeferredSetter where TProperty: class { - private struct NotifyDisposable : IDisposable + internal struct NotifyDisposable : IDisposable { private readonly SettingStatus status; - public NotifyDisposable(SettingStatus status) + internal NotifyDisposable(SettingStatus status) { this.status = status; status.Notifying = true; @@ -33,7 +33,7 @@ namespace Avalonia.Utilities /// /// Information on current setting/notification status of a property. /// - private class SettingStatus + internal class SettingStatus { public bool Notifying { get; set; } @@ -55,7 +55,7 @@ namespace Avalonia.Utilities /// /// The property to mark as notifying. /// Returns a disposable that when disposed, marks the property as done notifying. - internal IDisposable MarkNotifying(TProperty property) + internal NotifyDisposable MarkNotifying(TProperty property) { Contract.Requires(!IsNotifying(property)); @@ -119,6 +119,7 @@ namespace Avalonia.Utilities Predicate pendingSetCondition) { Contract.Requires(setterCallback != null); + Contract.Requires(pendingSetCondition != null); if (!IsNotifying(property)) { setterCallback(value, notification => @@ -139,7 +140,7 @@ namespace Avalonia.Utilities }); } } - else if(pendingSetCondition?.Invoke(value) ?? true) + else if(pendingSetCondition(value)) { AddPendingSet(property, value); } diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index 1a401c82bc..3d34bae7eb 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -151,7 +151,7 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - SetAndRaise(SelectedIndexProperty, (val, notifierWrapper) => + SetAndRaise(SelectedIndexProperty, (val, notifyWrapper) => { var old = SelectedIndex; var effective = (val >= 0 && val < Items?.Cast().Count()) ? val : -1; @@ -159,10 +159,15 @@ namespace Avalonia.Controls.Primitives if (old != effective) { _selectedIndex = effective; - notifierWrapper(() => RaisePropertyChanged(SelectedIndexProperty, old, effective, BindingPriority.LocalValue)); + notifyWrapper(() => + RaisePropertyChanged( + SelectedIndexProperty, + old, + effective, + BindingPriority.LocalValue)); SelectedItem = ElementAt(Items, effective); } - }, value); + }, value, val => val != SelectedIndex); } else { @@ -196,7 +201,12 @@ namespace Avalonia.Controls.Primitives { _selectedItem = effective; - notifyWrapper(() => RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue)); + notifyWrapper(() => + RaisePropertyChanged( + SelectedItemProperty, + old, + effective, + BindingPriority.LocalValue)); SelectedIndex = index; @@ -215,7 +225,7 @@ namespace Avalonia.Controls.Primitives SelectedItems.Clear(); } } - }, value); + }, value, val => val != SelectedItem); } else { diff --git a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs index d95acbdb7f..fee4994ee3 100644 --- a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs +++ b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs @@ -223,7 +223,7 @@ namespace Avalonia.Controls.UnitTests Assert.Equal(0, viewModel.SetterInvokedCount); - //here in real life stack overflow exception is thrown issue #855 + // In Issue #855, a Stackoverflow occured here. target.SelectedItem = viewModel.Items[2]; Assert.Equal(viewModel.Items[1], target.SelectedItem); From 05eda280de538a815324032fd65c4ba403e70777 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 5 Dec 2017 19:35:33 -0600 Subject: [PATCH 22/24] 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. --- src/Avalonia.Base/AvaloniaObject.cs | 104 ++++++++++-------- src/Avalonia.Base/PriorityValue.cs | 15 ++- src/Avalonia.Base/Utilities/DeferredSetter.cs | 57 ++++++---- .../Primitives/SelectingItemsControl.cs | 16 +-- .../AvaloniaObjectTests_Direct.cs | 2 +- .../Primitives/RangeBaseTests.cs | 2 +- 6 files changed, 116 insertions(+), 80 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 8506414289..2035b6f2aa 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -56,7 +56,7 @@ namespace Avalonia /// /// Delayed setter helper for direct properties. Used to fix #855. /// - private DeferredSetter DirectDelayedSetter + private DeferredSetter DirectPropertyDeferredSetter { get { @@ -554,6 +554,15 @@ namespace Avalonia } } + /// + /// A callback type for encapsulating complex logic for setting direct properties. + /// + /// The type of the property. + /// The value to which to set the property. + /// The backing field for the property. + /// A wrapper for the property-changed notification. + protected delegate void SetAndRaiseCallback(T value, ref T field, Action notifyWrapper); + /// /// Sets the backing field for a direct avalonia property, raising the /// event if the value has changed. @@ -561,61 +570,70 @@ namespace Avalonia /// The type of the property. /// The property. /// The backing field. + /// A callback called to actually set the value to the backing field. /// The value. /// /// True if the value changed, otherwise false. /// - protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) + protected bool SetAndRaise( + AvaloniaProperty property, + ref T field, + SetAndRaiseCallback setterCallback, + T value) { - VerifyAccess(); - if (!DirectDelayedSetter.IsNotifying(property)) - { - var valueChanged = false; - if (!object.Equals(field, value)) + Contract.Requires(setterCallback != null); + return DirectPropertyDeferredSetter.SetAndNotify( + property, + ref field, + (object val, ref T backing, 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; + /// + /// Sets the backing field for a direct avalonia property, raising the + /// event if the value has changed. + /// + /// The type of the property. + /// The property. + /// The backing field. + /// The value. + /// + /// True if the value changed, otherwise false. + /// + protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) + { + VerifyAccess(); + return SetAndRaise( + property, + ref field, + (T val, ref T backing, Action notifyWrapper) + => SetAndRaiseCore(property, ref backing, val, notifyWrapper), + value); } - private void SetAndRaiseCore(AvaloniaProperty property, ref T field, T value) + /// + /// Default assignment logic for SetAndRaise. + /// + /// The type of the property. + /// The property. + /// The backing field. + /// The value. + /// A wrapper for the property-changed notification. + /// + /// True if the value changed, otherwise false. + /// + private bool SetAndRaiseCore(AvaloniaProperty property, ref T field, T value, Action notifyWrapper) { var old = field; field = value; - using (DirectDelayedSetter.MarkNotifying(property)) - { - RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); - } - } - - protected void SetAndRaise( - AvaloniaProperty property, - Action> setterCallback, - T value, - Predicate pendingSetCondition) - { - Contract.Requires(setterCallback != null); - Contract.Requires(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; } /// diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index ea10427b10..cad86b5dea 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/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 notify) + private bool UpdateCore( + (object value, int priority) update, + ref (object value, int priority) backing, + 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; } } } diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index b42ca0bf3d..2c8c7f8887 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/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. /// /// The type of the object that represents the property. - /// The type of value with which to track the delayed assignment. - class DeferredSetter + /// The type of value with which to track the delayed assignment. + class DeferredSetter where TProperty: class { - internal struct NotifyDisposable : IDisposable + private struct NotifyDisposable : IDisposable { private readonly SettingStatus status; @@ -33,17 +33,17 @@ namespace Avalonia.Utilities /// /// Information on current setting/notification status of a property. /// - internal class SettingStatus + private class SettingStatus { public bool Notifying { get; set; } - private Queue pendingValues; + private Queue pendingValues; - public Queue PendingValues + public Queue PendingValues { get { - return pendingValues ?? (pendingValues = new Queue()); + return pendingValues ?? (pendingValues = new Queue()); } } } @@ -55,7 +55,7 @@ namespace Avalonia.Utilities /// /// The property to mark as notifying. /// Returns a disposable that when disposed, marks the property as done notifying. - internal NotifyDisposable MarkNotifying(TProperty property) + private NotifyDisposable MarkNotifying(TProperty property) { Contract.Requires(!IsNotifying(property)); @@ -67,7 +67,7 @@ namespace Avalonia.Utilities /// /// The property. /// If the property is currently notifying listeners. - internal bool IsNotifying(TProperty property) + private bool IsNotifying(TProperty property) => setRecords.TryGetValue(property, out var value) && value.Notifying; /// @@ -75,7 +75,7 @@ namespace Avalonia.Utilities /// /// The property. /// The value to assign. - internal void AddPendingSet(TProperty property, TValue value) + private void AddPendingSet(TProperty property, TSetRecord value) { Contract.Requires(IsNotifying(property)); @@ -87,7 +87,7 @@ namespace Avalonia.Utilities /// /// The property to check. /// If the property has any pending assignments. - 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 /// /// The property to check. /// The first pending assignment for the property. - internal TValue GetFirstPendingSet(TProperty property) + private TSetRecord GetFirstPendingSet(TProperty property) { return setRecords.GetOrCreateValue(property).PendingValues.Dequeue(); } + public delegate bool SetterDelegate(TSetRecord record, ref TValue backing, Action notifyCallback); + public delegate bool PendingSetPredicate(TSetRecord record, ref TValue backing); + /// /// Set the property and notify listeners while ensuring we don't get into a stack overflow as happens with #855 and #824 /// /// The property to set. + /// The backing field for the property /// /// 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. /// /// The value to try to set. /// A predicate to filter what possible values should be added as pending sets (i.e. only values not equal to the current value). - public void SetAndNotify( + public bool SetAndNotify( TProperty property, - Action> setterCallback, - TValue value, - Predicate pendingSetCondition) + ref TValue backing, + SetterDelegate setterCallback, + TSetRecord value, + PendingSetPredicate pendingSetCondition) { Contract.Requires(setterCallback != null); Contract.Requires(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; } } } diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index 3d34bae7eb..2e668fda95 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/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 notifyWrapper) => { - var old = SelectedIndex; + var old = backing; var effective = (val >= 0 && val < Items?.Cast().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 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 { diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index ffea0fd2d6..6d10f50276 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/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); diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index 0e67581760..2dfb30a9f0 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/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); From a02515fe19fd4161904b620effc75d1c6094499c Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 6 Dec 2017 22:19:50 -0600 Subject: [PATCH 23/24] Make pending set condition always !object.Equals since the other condition in PriorityValue didn't match expected behavior as per new unit tests. --- src/Avalonia.Base/AvaloniaObject.cs | 3 +-- src/Avalonia.Base/PriorityValue.cs | 4 +--- src/Avalonia.Base/Utilities/DeferredSetter.cs | 9 +++------ 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 624274cc96..34278c397f 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -603,8 +603,7 @@ namespace Avalonia setterCallback((T)val, ref backing, notify); return true; }, - value, - (object o, ref T backing) => !object.Equals(o, backing)); + value); } /// diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 6b82b202be..9b5318083a 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -246,9 +246,7 @@ namespace Avalonia delayedSetter.SetAndNotify(this, ref _value, UpdateCore, - (value, priority), - ((object value, int) val, ref (object value, int) backing) - => !object.Equals(val.value, backing.value)); + (value, priority)); } private bool UpdateCore( diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index 2c8c7f8887..957bcdc942 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -103,7 +103,6 @@ namespace Avalonia.Utilities } public delegate bool SetterDelegate(TSetRecord record, ref TValue backing, Action notifyCallback); - public delegate bool PendingSetPredicate(TSetRecord record, ref TValue backing); /// /// Set the property and notify listeners while ensuring we don't get into a stack overflow as happens with #855 and #824 @@ -120,15 +119,13 @@ namespace Avalonia.Utilities TProperty property, ref TValue backing, SetterDelegate setterCallback, - TSetRecord value, - PendingSetPredicate pendingSetCondition) + TSetRecord value) { Contract.Requires(setterCallback != null); - Contract.Requires(pendingSetCondition != null); if (!IsNotifying(property)) { bool updated = false; - if (pendingSetCondition(value, ref backing)) + if (!object.Equals(value, backing)) { updated = setterCallback(value, ref backing, notification => { @@ -150,7 +147,7 @@ namespace Avalonia.Utilities } return updated; } - else if(pendingSetCondition(value, ref backing)) + else if(!object.Equals(value, backing)) { AddPendingSet(property, value); } From 1cfc9fe47fe88323a50748d93dadbbecba0feb1d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 7 Dec 2017 12:48:47 -0600 Subject: [PATCH 24/24] Remove extraneous documentation comment. --- src/Avalonia.Base/Utilities/DeferredSetter.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs index 957bcdc942..fdfa160134 100644 --- a/src/Avalonia.Base/Utilities/DeferredSetter.cs +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -114,7 +114,6 @@ namespace Avalonia.Utilities /// The first parameter is the value to set, and the second is a wrapper that takes a callback that sends the property-changed notification. /// /// The value to try to set. - /// A predicate to filter what possible values should be added as pending sets (i.e. only values not equal to the current value). public bool SetAndNotify( TProperty property, ref TValue backing,