Merge pull request #5686 from AvaloniaUI/fixes/batchupdate-set-cleared-value-end-update

Fix batch updates to allow cleared values to be re-set

src/Avalonia.Animation/Animatable.cs

@@ -2,6 +2,7 @@ using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Linq;
 using Avalonia.Data;
 
 #nullable enable
@@ -93,16 +94,35 @@ namespace Avalonia.Animation
                 var oldTransitions = change.OldValue.GetValueOrDefault<Transitions>();
                 var newTransitions = change.NewValue.GetValueOrDefault<Transitions>();
 
+                // When transitions are replaced, we add the new transitions before removing the old
+                // transitions, so that when the old transition being disposed causes the value to
+                // change, there is a corresponding entry in `_transitionStates`. This means that we
+                // need to account for any transitions present in both the old and new transitions
+                // collections.
                 if (newTransitions is object)
                 {
+                    var toAdd = (IList)newTransitions;
+
+                    if (newTransitions.Count > 0 && oldTransitions?.Count > 0)
+                    {
+                        toAdd = newTransitions.Except(oldTransitions).ToList();
+                    }
+
                     newTransitions.CollectionChanged += TransitionsCollectionChanged;
-                    AddTransitions(newTransitions);
+                    AddTransitions(toAdd);
                 }
 
                 if (oldTransitions is object)
                 {
+                    var toRemove = (IList)oldTransitions;
+
+                    if (oldTransitions.Count > 0 && newTransitions?.Count > 0)
+                    {
+                        toRemove = oldTransitions.Except(newTransitions).ToList();
+                    }
+
                     oldTransitions.CollectionChanged -= TransitionsCollectionChanged;
-                    RemoveTransitions(oldTransitions);
+                    RemoveTransitions(toRemove);
                 }
             }
             else if (_transitionsEnabled &&
@@ -115,9 +135,9 @@ namespace Avalonia.Animation
                 {
                     var transition = Transitions[i];
 
-                    if (transition.Property == change.Property)
+                    if (transition.Property == change.Property &&
+                        _transitionState.TryGetValue(transition, out var state))
                     {
-                        var state = _transitionState[transition];
                         var oldValue = state.BaseValue;
                         var newValue = GetAnimationBaseValue(transition.Property);
 

src/Avalonia.Base/PropertyStore/BindingEntry.cs

@@ -65,7 +65,6 @@ namespace Avalonia.PropertyStore
         {
             _subscription?.Dispose();
             _subscription = null;
-            _isSubscribed = false;
             OnCompleted();
         }
 
@@ -74,6 +73,7 @@ namespace Avalonia.PropertyStore
             var oldValue = _value;
             _value = default;
             Priority = BindingPriority.Unset;
+            _isSubscribed = false;
             _sink.Completed(Property, this, oldValue);
         }
 
@@ -104,8 +104,11 @@ namespace Avalonia.PropertyStore
         public void Start(bool ignoreBatchUpdate)
         {
             // We can't use _subscription to check whether we're subscribed because it won't be set
-            // until Subscribe has finished, which will be too late to prevent reentrancy.
+            // until Subscribe has finished, which will be too late to prevent reentrancy. In addition
-            if (!_isSubscribed && (!_batchUpdate || ignoreBatchUpdate))
+            // don't re-subscribe to completed/disposed bindings (indicated by Unset priority).
+            if (!_isSubscribed &&
+                Priority != BindingPriority.Unset &&
+                (!_batchUpdate || ignoreBatchUpdate))
             {
                 _isSubscribed = true;
                 _subscription = Source.Subscribe(this);

src/Avalonia.Base/PropertyStore/ConstantValueEntry.cs

@@ -6,12 +6,19 @@ using Avalonia.Data;
 
 namespace Avalonia.PropertyStore
 {
+    /// <summary>
+    /// Represents an untyped interface to <see cref="ConstantValueEntry{T}"/>.
+    /// </summary>
+    internal interface IConstantValueEntry : IPriorityValueEntry, IDisposable
+    {
+    }
+
     /// <summary>
     /// Stores a value with a priority in a <see cref="ValueStore"/> or
     /// <see cref="PriorityValue{T}"/>.
     /// </summary>
     /// <typeparam name="T">The property type.</typeparam>
-    internal class ConstantValueEntry<T> : IPriorityValueEntry<T>, IDisposable
+    internal class ConstantValueEntry<T> : IPriorityValueEntry<T>, IConstantValueEntry
     {
         private IValueSink _sink;
         private Optional<T> _value;

src/Avalonia.Base/Utilities/AvaloniaPropertyValueStore.cs

@@ -94,7 +94,7 @@ namespace Avalonia.Utilities
             return (0, false);
         }
 
-        public bool TryGetValue(AvaloniaProperty property, [MaybeNull] out TValue value)
+        public bool TryGetValue(AvaloniaProperty property, [MaybeNullWhen(false)] out TValue value)
         {
             (int index, bool found) = TryFindEntry(property.Id);
             if (!found)

src/Avalonia.Base/ValueStore.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using Avalonia.Data;
 using Avalonia.PropertyStore;
 using Avalonia.Utilities;
@@ -56,7 +57,7 @@ namespace Avalonia
 
         public bool IsAnimating(AvaloniaProperty property)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 return slot.Priority < BindingPriority.LocalValue;
             }
@@ -66,7 +67,7 @@ namespace Avalonia
 
         public bool IsSet(AvaloniaProperty property)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 return slot.GetValue().HasValue;
             }
@@ -79,7 +80,7 @@ namespace Avalonia
             BindingPriority maxPriority,
             out T value)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 var v = ((IValue<T>)slot).GetValue(maxPriority);
 
@@ -103,7 +104,7 @@ namespace Avalonia
 
             IDisposable? result = null;
 
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 result = SetExisting(slot, property, value, priority);
             }
@@ -138,7 +139,7 @@ namespace Avalonia
             IObservable<BindingValue<T>> source,
             BindingPriority priority)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 return BindExisting(slot, property, source, priority);
             }
@@ -160,7 +161,7 @@ namespace Avalonia
 
         public void ClearLocalValue<T>(StyledPropertyBase<T> property)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 if (slot is PriorityValue<T> p)
                 {
@@ -173,7 +174,7 @@ namespace Avalonia
                     // During batch update values can't be removed immediately because they're needed to raise
                     // a correctly-typed _sink.ValueChanged notification. They instead mark themselves for removal
                     // by setting their priority to Unset.
-                    if (_batchUpdate is null)
+                    if (!IsBatchUpdating())
                     {
                         _values.Remove(property);
                     }
@@ -198,7 +199,7 @@ namespace Avalonia
 
         public void CoerceValue<T>(StyledPropertyBase<T> property)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 if (slot is PriorityValue<T> p)
                 {
@@ -209,7 +210,7 @@ namespace Avalonia
 
         public Diagnostics.AvaloniaPropertyValue? GetDiagnostic(AvaloniaProperty property)
         {
-            if (_values.TryGetValue(property, out var slot))
+            if (TryGetValue(property, out var slot))
             {
                 var slotValue = slot.GetValue();
                 return new Diagnostics.AvaloniaPropertyValue(
@@ -242,6 +243,7 @@ namespace Avalonia
             IPriorityValueEntry entry,
             Optional<T> oldValue)
         {
+            // We need to include remove sentinels here so call `_values.TryGetValue` directly.
             if (_values.TryGetValue(property, out var slot) && slot == entry)
             {
                 if (_batchUpdate is null)
@@ -285,7 +287,7 @@ namespace Avalonia
                 else
                 {
                     var priorityValue = new PriorityValue<T>(_owner, property, this, l);
-                    if (_batchUpdate is object)
+                    if (IsBatchUpdating())
                         priorityValue.BeginBatchUpdate();
                     result = priorityValue.SetValue(value, priority);
                     _values.SetValue(property, priorityValue);
@@ -311,7 +313,7 @@ namespace Avalonia
             {
                 priorityValue = new PriorityValue<T>(_owner, property, this, e);
 
-                if (_batchUpdate is object)
+                if (IsBatchUpdating())
                 {
                     priorityValue.BeginBatchUpdate();
                 }
@@ -338,7 +340,7 @@ namespace Avalonia
         private void AddValue(AvaloniaProperty property, IValue value)
         {
             _values.AddValue(property, value);
-            if (_batchUpdate is object && value is IBatchUpdate batch)
+            if (IsBatchUpdating() && value is IBatchUpdate batch)
                 batch.BeginBatchUpdate();
             value.Start();
         }
@@ -364,6 +366,21 @@ namespace Avalonia
             }
         }
 
+        private bool IsBatchUpdating() => _batchUpdate?.IsBatchUpdating == true;
+
+        private bool TryGetValue(AvaloniaProperty property, [MaybeNullWhen(false)] out IValue value)
+        {
+            return _values.TryGetValue(property, out value) && !IsRemoveSentinel(value);
+        }
+
+        private static bool IsRemoveSentinel(IValue value)
+        {
+            // Local value entries are optimized and contain only a single value field to save space,
+            // so there's no way to mark them for removal at the end of a batch update. Instead a
+            // ConstantValueEntry with a priority of Unset is used as a sentinel value.
+            return value is IConstantValueEntry t && t.Priority == BindingPriority.Unset;
+        }
+
         private class BatchUpdate
         {
             private ValueStore _owner;
@@ -373,6 +390,8 @@ namespace Avalonia
 
             public BatchUpdate(ValueStore owner) => _owner = owner;
 
+            public bool IsBatchUpdating => _batchUpdateCount > 0;
+
             public void Begin()
             {
                 if (_batchUpdateCount++ == 0)
@@ -437,8 +456,10 @@ namespace Avalonia
 
                             // During batch update values can't be removed immediately because they're needed to raise
                             // the _sink.ValueChanged notification. They instead mark themselves for removal by setting
-                            // their priority to Unset.
+                            // their priority to Unset. We need to re-read the slot here because raising ValueChanged
-                            if (slot.Priority == BindingPriority.Unset)
+                            // could have caused it to be updated.
+                            if (values.TryGetValue(entry.property, out var updatedSlot) &&
+                                updatedSlot.Priority == BindingPriority.Unset)
                             {
                                 values.Remove(entry.property);
                             }

tests/Avalonia.Animation.UnitTests/AnimatableTests.cs

@@ -330,6 +330,37 @@ namespace Avalonia.Animation.UnitTests
             }
         }
 
+        [Fact]
+        public void Transitions_Can_Be_Changed_To_Collection_That_Contains_The_Same_Transitions()
+        {
+            var target = CreateTarget();
+            var control = CreateControl(target.Object);
+
+            control.Transitions = new Transitions { target.Object };
+        }
+
+        [Fact]
+        public void Transitions_Can_Re_Set_During_Batch_Update()
+        {
+            var target = CreateTarget();
+            var control = CreateControl(target.Object);
+
+            // Assigning and then clearing Transitions ensures we have a transition state
+            // collection created.
+            control.Transitions = null;
+
+            control.BeginBatchUpdate();
+
+            // Setting opacity then Transitions means that we receive the Transitions change
+            // after the Opacity change when EndBatchUpdate is called.
+            control.Opacity = 0.5;
+            control.Transitions = new Transitions { target.Object };
+
+            // Which means that the transition state hasn't been initialized with the new
+            // Transitions when the Opacity change notification gets raised here.
+            control.EndBatchUpdate();
+        }
+
         private static Mock<ITransition> CreateTarget()
         {
             return CreateTransition(Visual.OpacityProperty);

tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_BatchUpdate.cs

@@ -53,6 +53,21 @@ namespace Avalonia.Base.UnitTests
             Assert.Empty(raised);
         }
 
+        [Fact]
+        public void Binding_Disposal_Should_Not_Raise_Property_Changes_During_Batch_Update()
+        {
+            var target = new TestClass();
+            var observable = new TestObservable<string>("foo");
+            var raised = new List<string>();
+
+            var sub = target.Bind(TestClass.FooProperty, observable, BindingPriority.LocalValue);
+            target.GetObservable(TestClass.FooProperty).Skip(1).Subscribe(x => raised.Add(x));
+            target.BeginBatchUpdate();
+            sub.Dispose();
+
+            Assert.Empty(raised);
+        }
+
         [Fact]
         public void SetValue_Change_Should_Be_Raised_After_Batch_Update_1()
         {
@@ -240,6 +255,27 @@ namespace Avalonia.Base.UnitTests
             Assert.Equal(BindingPriority.Unset, raised[0].Priority);
         }
 
+        [Fact]
+        public void Binding_Disposal_Should_Be_Raised_After_Batch_Update()
+        {
+            var target = new TestClass();
+            var observable = new TestObservable<string>("foo");
+            var raised = new List<AvaloniaPropertyChangedEventArgs>();
+
+            var sub = target.Bind(TestClass.FooProperty, observable, BindingPriority.LocalValue);
+            target.PropertyChanged += (s, e) => raised.Add(e);
+
+            target.BeginBatchUpdate();
+            sub.Dispose();
+            target.EndBatchUpdate();
+
+            Assert.Equal(1, raised.Count);
+            Assert.Null(target.Foo);
+            Assert.Equal("foo", raised[0].OldValue);
+            Assert.Null(raised[0].NewValue);
+            Assert.Equal(BindingPriority.Unset, raised[0].Priority);
+        }
+
         [Fact]
         public void ClearValue_Change_Should_Be_Raised_After_Batch_Update_1()
         {
@@ -449,6 +485,92 @@ namespace Avalonia.Base.UnitTests
             Assert.Null(raised[1].NewValue);
         }
 
+        [Fact]
+        public void Can_Set_Cleared_Value_When_Ending_Batch_Update()
+        {
+            var target = new TestClass();
+            var raised = 0;
+
+            target.Foo = "foo";
+
+            target.BeginBatchUpdate();
+            target.ClearValue(TestClass.FooProperty);
+            target.PropertyChanged += (sender, e) =>
+            {
+                if (e.Property == TestClass.FooProperty && e.NewValue is null)
+                {
+                    target.Foo = "bar";
+                    ++raised;
+                }
+            };
+            target.EndBatchUpdate();
+
+            Assert.Equal("bar", target.Foo);
+            Assert.Equal(1, raised);
+        }
+
+        [Fact]
+        public void Can_Bind_Cleared_Value_When_Ending_Batch_Update()
+        {
+            var target = new TestClass();
+            var raised = 0;
+            var notifications = new List<AvaloniaPropertyChangedEventArgs>();
+
+            target.Foo = "foo";
+
+            target.BeginBatchUpdate();
+            target.ClearValue(TestClass.FooProperty);
+            target.PropertyChanged += (sender, e) =>
+            {
+                if (e.Property == TestClass.FooProperty && e.NewValue is null)
+                {
+                    target.Bind(TestClass.FooProperty, new TestObservable<string>("bar"));
+                    ++raised;
+                }
+
+                notifications.Add(e);
+            };
+            target.EndBatchUpdate();
+
+            Assert.Equal("bar", target.Foo);
+            Assert.Equal(1, raised);
+            Assert.Equal(2, notifications.Count);
+            Assert.Equal(null, notifications[0].NewValue);
+            Assert.Equal("bar", notifications[1].NewValue);
+        }
+
+        [Fact]
+        public void Can_Bind_Completed_Binding_Back_To_Original_Value_When_Ending_Batch_Update()
+        {
+            var target = new TestClass();
+            var raised = 0;
+            var notifications = new List<AvaloniaPropertyChangedEventArgs>();
+            var observable1 = new TestObservable<string>("foo");
+            var observable2 = new TestObservable<string>("foo");
+
+            target.Bind(TestClass.FooProperty, observable1);
+
+            target.BeginBatchUpdate();
+            observable1.OnCompleted();
+            target.PropertyChanged += (sender, e) =>
+            {
+                if (e.Property == TestClass.FooProperty && e.NewValue is null)
+                {
+                    target.Bind(TestClass.FooProperty, observable2);
+                    ++raised;
+                }
+
+                notifications.Add(e);
+            };
+            target.EndBatchUpdate();
+
+            Assert.Equal("foo", target.Foo);
+            Assert.Equal(1, raised);
+            Assert.Equal(2, notifications.Count);
+            Assert.Equal(null, notifications[0].NewValue);
+            Assert.Equal("foo", notifications[1].NewValue);
+        }
+
         public class TestClass : AvaloniaObject
         {
             public static readonly StyledProperty<string> FooProperty =