From f2c3805d6a0086ce27ac9ecb55680def9f31b9ea Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 1 Feb 2023 18:08:13 +0100 Subject: [PATCH 1/6] Update ncrunch config. --- .ncrunch/Avalonia.UnitTests.v3.ncrunchproject | 5 +++++ .ncrunch/GpuInterop.v3.ncrunchproject | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .ncrunch/Avalonia.UnitTests.v3.ncrunchproject create mode 100644 .ncrunch/GpuInterop.v3.ncrunchproject diff --git a/.ncrunch/Avalonia.UnitTests.v3.ncrunchproject b/.ncrunch/Avalonia.UnitTests.v3.ncrunchproject new file mode 100644 index 0000000000..cff5044edf --- /dev/null +++ b/.ncrunch/Avalonia.UnitTests.v3.ncrunchproject @@ -0,0 +1,5 @@ + + + False + + \ No newline at end of file diff --git a/.ncrunch/GpuInterop.v3.ncrunchproject b/.ncrunch/GpuInterop.v3.ncrunchproject new file mode 100644 index 0000000000..319cd523ce --- /dev/null +++ b/.ncrunch/GpuInterop.v3.ncrunchproject @@ -0,0 +1,5 @@ + + + True + + \ No newline at end of file From 1e926dac72b4c666d4c213cd72693d4cbf551e27 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 1 Feb 2023 18:19:40 +0100 Subject: [PATCH 2/6] Added failing tests for #10110. --- .../AvaloniaObjectTests_Binding.cs | 18 +++++++++++++ .../Xaml/ResourceDictionaryTests.cs | 25 ++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 030d6ba215..f0386615c5 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -1285,6 +1285,24 @@ namespace Avalonia.Base.UnitTests subscription.Dispose(); } + [Theory] + [InlineData(BindingPriority.LocalValue)] + [InlineData(BindingPriority.Style)] + public void Binding_Producing_UnsetValue_Does_Not_Cause_Unsubscribe(BindingPriority priority) + { + var target = new Class1(); + var source = new Subject>(); + + target.Bind(Class1.FooProperty, source, priority); + + source.OnNext("foo"); + Assert.Equal("foo", target.GetValue(Class1.FooProperty)); + source.OnNext(BindingValue.Unset); + Assert.Equal("foodefault", target.GetValue(Class1.FooProperty)); + source.OnNext("bar"); + Assert.Equal("bar", target.GetValue(Class1.FooProperty)); + } + [Fact] public void Produces_Correct_Values_And_Base_Values_With_Multiple_Animation_Bindings() { diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Xaml/ResourceDictionaryTests.cs b/tests/Avalonia.Markup.Xaml.UnitTests/Xaml/ResourceDictionaryTests.cs index 6cab83751f..bea6bc4dc8 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Xaml/ResourceDictionaryTests.cs +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Xaml/ResourceDictionaryTests.cs @@ -307,7 +307,30 @@ namespace Avalonia.Markup.Xaml.UnitTests.Xaml Assert.True(buttonResources.ContainsDeferredKey("Red")); } } - + + [Fact] + public void Dynamically_Changing_Referenced_Resources_Works_With_DynamicResource() + { + var xaml = @" + + + Red + + +"; + + var userControl = (UserControl)AvaloniaRuntimeXamlLoader.Load(xaml); + + Assert.Equal(Colors.Red, ((ISolidColorBrush)userControl.FindResource("brush")!).Color); + + userControl.Resources.Remove("color"); + Assert.Equal(default, ((ISolidColorBrush)userControl.FindResource("brush")!).Color); + + userControl.Resources.Add("color", Colors.Blue); + Assert.Equal(Colors.Blue, ((ISolidColorBrush)userControl.FindResource("brush")!).Color); + } + private IDisposable StyledWindow(params (string, string)[] assets) { var services = TestServices.StyledWindow.With( From 7b00ef69892358650c10e0f27fb0d5dd1477e3f5 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 2 Feb 2023 12:00:40 +0100 Subject: [PATCH 3/6] Make untyped bindings produce default value. This changes the behavior of bindings slightly, in that previously a binding which produced `UnsetValue` (or a binding error) caused the value of any lower priority value of value to take effect. After this change, the binding reverts to the property's default value. This behavior more closely matches WPF and fixes #10110. --- src/Avalonia.Base/AvaloniaObject.cs | 4 +- .../PropertyStore/BindingEntryBase.cs | 47 +++++++++---------- .../LocalValueBindingObserver.cs | 42 ++++++++++++----- .../LocalValueUntypedBindingObserver.cs | 25 +++++++--- .../SourceUntypedBindingEntry.cs | 2 + .../PropertyStore/TypedBindingEntry.cs | 2 + .../PropertyStore/UntypedBindingEntry.cs | 5 ++ .../AvaloniaObjectTests_Binding.cs | 18 ++++++- .../AvaloniaObjectTests_Validation.cs | 4 +- 9 files changed, 99 insertions(+), 50 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 1946d4ba5c..db6dbe98e0 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -270,8 +270,8 @@ namespace Avalonia /// The property. /// True if the property is set, otherwise false. /// - /// Checks whether a value is assigned to the property, or that there is a binding to the - /// property that is producing a value other than . + /// Returns true if is a styled property which has a value + /// assigned to it or a binding targeting it; otherwise false. /// public bool IsSet(AvaloniaProperty property) { diff --git a/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs b/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs index 11dc80ef8f..e1ff0970c2 100644 --- a/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs +++ b/src/Avalonia.Base/PropertyStore/BindingEntryBase.cs @@ -16,6 +16,8 @@ namespace Avalonia.PropertyStore private IDisposable? _subscription; private bool _hasValue; private TValue? _value; + private TValue? _defaultValue; + private bool _isDefaultValueInitialized; protected BindingEntryBase( ValueFrame frame, @@ -89,6 +91,7 @@ namespace Avalonia.PropertyStore protected abstract BindingValue ConvertAndValidate(TSource value); protected abstract BindingValue ConvertAndValidate(BindingValue value); + protected abstract TValue GetDefaultValue(Type ownerType); protected virtual void Start(bool produceValue) { @@ -104,17 +107,6 @@ namespace Avalonia.PropertyStore }; } - private void ClearValue() - { - if (_hasValue) - { - _hasValue = false; - _value = default; - if (_subscription is not null) - Frame.Owner?.OnBindingValueCleared(Property, Frame.Priority); - } - } - private void SetValue(BindingValue value) { static void Execute(BindingEntryBase instance, BindingValue value) @@ -124,24 +116,20 @@ namespace Avalonia.PropertyStore LoggingUtils.LogIfNecessary(instance.Frame.Owner.Owner, instance.Property, value); - if (value.HasValue) - { - if (!instance._hasValue || !EqualityComparer.Default.Equals(instance._value, value.Value)) - { - instance._value = value.Value; - instance._hasValue = true; - if (instance._subscription is not null && instance._subscription != s_creatingQuiet) - instance.Frame.Owner?.OnBindingValueChanged(instance, instance.Frame.Priority); - } - } - else if (value.Type != BindingValueType.DoNothing) + var effectiveValue = value.HasValue ? value.Value : instance.GetCachedDefaultValue(); + + if (!instance._hasValue || !EqualityComparer.Default.Equals(instance._value, effectiveValue)) { - instance.ClearValue(); + instance._value = effectiveValue; + instance._hasValue = true; if (instance._subscription is not null && instance._subscription != s_creatingQuiet) - instance.Frame.Owner?.OnBindingValueCleared(instance.Property, instance.Frame.Priority); + instance.Frame.Owner?.OnBindingValueChanged(instance, instance.Frame.Priority); } } + if (value.Type == BindingValueType.DoNothing) + return; + if (Dispatcher.UIThread.CheckAccess()) { Execute(this, value); @@ -161,5 +149,16 @@ namespace Avalonia.PropertyStore _subscription = null; Frame.OnBindingCompleted(this); } + + private TValue GetCachedDefaultValue() + { + if (!_isDefaultValueInitialized) + { + _defaultValue = GetDefaultValue(Frame.Owner!.Owner.GetType()); + _isDefaultValueInitialized = true; + } + + return _defaultValue!; + } } } diff --git a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs index f89cb029b6..0771ade647 100644 --- a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs +++ b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs @@ -10,6 +10,8 @@ namespace Avalonia.PropertyStore { private readonly ValueStore _owner; private IDisposable? _subscription; + private T? _defaultValue; + private bool _isDefaultValueInitialized; public LocalValueBindingObserver(ValueStore owner, StyledProperty property) { @@ -41,26 +43,28 @@ namespace Avalonia.PropertyStore public void OnNext(T value) { - static void Execute(ValueStore owner, StyledProperty property, T value) + static void Execute(LocalValueBindingObserver instance, T value) { - if (property.ValidateValue?.Invoke(value) != false) - owner.SetValue(property, value, BindingPriority.LocalValue); - else - owner.ClearLocalValue(property); + var owner = instance._owner; + var property = instance.Property; + + if (property.ValidateValue?.Invoke(value) == false) + value = instance.GetCachedDefaultValue(); + + owner.SetValue(property, value, BindingPriority.LocalValue); } if (Dispatcher.UIThread.CheckAccess()) { - Execute(_owner, Property, value); + Execute(this, value); } else { // To avoid allocating closure in the outer scope we need to capture variables // locally. This allows us to skip most of the allocations when on UI thread. - var instance = _owner; - var property = Property; + var instance = this; var newValue = value; - Dispatcher.UIThread.Post(() => Execute(instance, property, newValue)); + Dispatcher.UIThread.Post(() => Execute(instance, newValue)); } } @@ -73,12 +77,13 @@ namespace Avalonia.PropertyStore LoggingUtils.LogIfNecessary(owner.Owner, property, value); - if (value.HasValue) - owner.SetValue(property, value.Value, BindingPriority.LocalValue); - else if (value.Type != BindingValueType.DataValidationError) - owner.ClearLocalValue(property); + var effectiveValue = value.HasValue ? value.Value : instance.GetCachedDefaultValue(); + owner.SetValue(property, effectiveValue, BindingPriority.LocalValue); } + if (value.Type is BindingValueType.DoNothing or BindingValueType.DataValidationError) + return; + if (Dispatcher.UIThread.CheckAccess()) { Execute(this, value); @@ -92,5 +97,16 @@ namespace Avalonia.PropertyStore Dispatcher.UIThread.Post(() => Execute(instance, newValue)); } } + + private T GetCachedDefaultValue() + { + if (!_isDefaultValueInitialized) + { + _defaultValue = Property.GetDefaultValue(_owner.Owner.GetType()); + _isDefaultValueInitialized = true; + } + + return _defaultValue!; + } } } diff --git a/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs b/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs index 2d157b2519..46e6ed810a 100644 --- a/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs +++ b/src/Avalonia.Base/PropertyStore/LocalValueUntypedBindingObserver.cs @@ -1,5 +1,4 @@ using System; -using System.Security.Cryptography; using Avalonia.Data; using Avalonia.Threading; @@ -10,6 +9,8 @@ namespace Avalonia.PropertyStore { private readonly ValueStore _owner; private IDisposable? _subscription; + private T? _defaultValue; + private bool _isDefaultValueInitialized; public LocalValueUntypedBindingObserver(ValueStore owner, StyledProperty property) { @@ -49,11 +50,7 @@ namespace Avalonia.PropertyStore if (value == AvaloniaProperty.UnsetValue) { - owner.ClearLocalValue(property); - } - else if (value == BindingOperations.DoNothing) - { - // Do nothing! + owner.SetValue(property, instance.GetCachedDefaultValue(), BindingPriority.LocalValue); } else if (UntypedValueUtils.TryConvertAndValidate(property, value, out var typedValue)) { @@ -61,11 +58,14 @@ namespace Avalonia.PropertyStore } else { - owner.ClearLocalValue(property); + owner.SetValue(property, instance.GetCachedDefaultValue(), BindingPriority.LocalValue); LoggingUtils.LogInvalidValue(owner.Owner, property, typeof(T), value); } } + if (value == BindingOperations.DoNothing) + return; + if (Dispatcher.UIThread.CheckAccess()) { Execute(this, value); @@ -79,5 +79,16 @@ namespace Avalonia.PropertyStore Dispatcher.UIThread.Post(() => Execute(instance, newValue)); } } + + private T GetCachedDefaultValue() + { + if (!_isDefaultValueInitialized) + { + _defaultValue = Property.GetDefaultValue(_owner.Owner.GetType()); + _isDefaultValueInitialized = true; + } + + return _defaultValue!; + } } } diff --git a/src/Avalonia.Base/PropertyStore/SourceUntypedBindingEntry.cs b/src/Avalonia.Base/PropertyStore/SourceUntypedBindingEntry.cs index b56d0d4529..b82714817b 100644 --- a/src/Avalonia.Base/PropertyStore/SourceUntypedBindingEntry.cs +++ b/src/Avalonia.Base/PropertyStore/SourceUntypedBindingEntry.cs @@ -31,5 +31,7 @@ namespace Avalonia.PropertyStore { throw new NotSupportedException(); } + + protected override TTarget GetDefaultValue(Type ownerType) => Property.GetDefaultValue(ownerType); } } diff --git a/src/Avalonia.Base/PropertyStore/TypedBindingEntry.cs b/src/Avalonia.Base/PropertyStore/TypedBindingEntry.cs index 697725c87b..550f5c0001 100644 --- a/src/Avalonia.Base/PropertyStore/TypedBindingEntry.cs +++ b/src/Avalonia.Base/PropertyStore/TypedBindingEntry.cs @@ -48,5 +48,7 @@ namespace Avalonia.PropertyStore return value; } + + protected override T GetDefaultValue(Type ownerType) => Property.GetDefaultValue(ownerType); } } diff --git a/src/Avalonia.Base/PropertyStore/UntypedBindingEntry.cs b/src/Avalonia.Base/PropertyStore/UntypedBindingEntry.cs index f8becb2e06..a77d7fddb6 100644 --- a/src/Avalonia.Base/PropertyStore/UntypedBindingEntry.cs +++ b/src/Avalonia.Base/PropertyStore/UntypedBindingEntry.cs @@ -29,5 +29,10 @@ namespace Avalonia.PropertyStore { throw new NotSupportedException(); } + + protected override object? GetDefaultValue(Type ownerType) + { + return ((IStyledPropertyMetadata)Property.GetMetadata(ownerType)).DefaultValue; + } } } diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index f0386615c5..baaed5104d 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -372,6 +372,20 @@ namespace Avalonia.Base.UnitTests Assert.Null(target.GetValue(property)); } + [Fact] + public void LocalValue_Bind_Generic_To_ValueType_Accepts_UnsetValue() + { + var target = new Class1(); + var source = new Subject>(); + + target.Bind(Class1.QuxProperty, source); + source.OnNext(6.7); + source.OnNext(BindingValue.Unset); + + Assert.Equal(5.6, target.GetValue(Class1.QuxProperty)); + Assert.True(target.IsSet(Class1.QuxProperty)); + } + [Fact] public void LocalValue_Bind_NonGeneric_To_ValueType_Accepts_UnsetValue() { @@ -383,7 +397,7 @@ namespace Avalonia.Base.UnitTests source.OnNext(AvaloniaProperty.UnsetValue); Assert.Equal(5.6, target.GetValue(Class1.QuxProperty)); - Assert.False(target.IsSet(Class1.QuxProperty)); + Assert.True(target.IsSet(Class1.QuxProperty)); } [Fact] @@ -397,7 +411,7 @@ namespace Avalonia.Base.UnitTests source.OnNext(AvaloniaProperty.UnsetValue); Assert.Equal(5.6, target.GetValue(Class1.QuxProperty)); - Assert.False(target.IsSet(Class1.QuxProperty)); + Assert.True(target.IsSet(Class1.QuxProperty)); } [Fact] diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs index e8175cf477..3fc950573d 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs @@ -66,7 +66,7 @@ namespace Avalonia.Base.UnitTests } [Fact] - public void Reverts_To_Lower_Priority_If_Style_Binding_Fails_Validation() + public void Reverts_To_DefaultValue_If_Style_Binding_Fails_Validation_2() { var target = new Class1(); var source = new Subject(); @@ -75,7 +75,7 @@ namespace Avalonia.Base.UnitTests target.Bind(Class1.FooProperty, source, BindingPriority.StyleTrigger); source.OnNext(150); - Assert.Equal(10, target.GetValue(Class1.FooProperty)); + Assert.Equal(11, target.GetValue(Class1.FooProperty)); } [Fact] From bca57ea0e9c65c5b17422303261b1a7a058d842b Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 2 Feb 2023 16:27:48 +0100 Subject: [PATCH 4/6] Added additional failing test. --- .../AvaloniaObjectTests_Validation.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs index 3fc950573d..513aeb65ab 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Validation.cs @@ -78,6 +78,20 @@ namespace Avalonia.Base.UnitTests Assert.Equal(11, target.GetValue(Class1.FooProperty)); } + [Theory] + [InlineData(BindingPriority.LocalValue)] + [InlineData(BindingPriority.Style)] + public void Reverts_To_DefaultValue_If_Style_Binding_Fails_Validation_3(BindingPriority priority) + { + var target = new Class1(); + var source = new Subject>(); + + target.Bind(Class1.FooProperty, source, priority); + source.OnNext(150); + + Assert.Equal(11, target.GetValue(Class1.FooProperty)); + } + [Fact] public void Reverts_To_DefaultValue_Even_In_Presence_Of_Other_Bindings() { From b2c1d80f5cfdf3c0dccbed07cb908b69f7db3e2d Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 2 Feb 2023 16:32:02 +0100 Subject: [PATCH 5/6] Fix validation of local value bindings. Local value bindings with a type of `BindingValue` were not being validated. --- .../PropertyStore/LocalValueBindingObserver.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs index 0771ade647..5908d9e535 100644 --- a/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs +++ b/src/Avalonia.Base/PropertyStore/LocalValueBindingObserver.cs @@ -77,8 +77,17 @@ namespace Avalonia.PropertyStore LoggingUtils.LogIfNecessary(owner.Owner, property, value); - var effectiveValue = value.HasValue ? value.Value : instance.GetCachedDefaultValue(); - owner.SetValue(property, effectiveValue, BindingPriority.LocalValue); + if (value.HasValue) + { + var effectiveValue = value.Value; + if (property.ValidateValue?.Invoke(effectiveValue) == false) + effectiveValue = instance.GetCachedDefaultValue(); + owner.SetValue(property, effectiveValue, BindingPriority.LocalValue); + } + else + { + owner.SetValue(property, instance.GetCachedDefaultValue(), BindingPriority.LocalValue); + } } if (value.Type is BindingValueType.DoNothing or BindingValueType.DataValidationError) From 467ef300bf559123f3dc355185ad78efdf0675ef Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Fri, 3 Feb 2023 10:43:26 +0100 Subject: [PATCH 6/6] Remove unneeded method. --- src/Avalonia.Base/PropertyStore/ValueStore.cs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/Avalonia.Base/PropertyStore/ValueStore.cs b/src/Avalonia.Base/PropertyStore/ValueStore.cs index f36a96992b..a758360545 100644 --- a/src/Avalonia.Base/PropertyStore/ValueStore.cs +++ b/src/Avalonia.Base/PropertyStore/ValueStore.cs @@ -380,23 +380,6 @@ namespace Avalonia.PropertyStore } } - /// - /// Called by non-LocalValue binding entries to re-evaluate the effective value when the - /// binding produces an unset value. - /// - /// The bound property. - /// The priority of binding which produced a new value. - public void OnBindingValueCleared(AvaloniaProperty property, BindingPriority priority) - { - Debug.Assert(priority != BindingPriority.LocalValue); - - if (TryGetEffectiveValue(property, out var existing)) - { - if (priority <= existing.Priority) - ReevaluateEffectiveValue(property, existing); - } - } - /// /// Called by a when its /// state changes.