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

src/Avalonia.Animation/Animatable.cs

@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Linq;
 using Avalonia.Data;
 
 #nullable enable
@@ -93,16 +94,35 @@ protected sealed override void OnPropertyChangedCore<T>(AvaloniaPropertyChangedE
                 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 @@ _transitionState is object &&
                 {
                     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 @@ public void Dispose()
         {
             _subscription?.Dispose();
             _subscription = null;
-            _isSubscribed = false;
             OnCompleted();
         }
 
@@ -74,6 +73,7 @@ public void OnCompleted()
             var oldValue = _value;
             _value = default;
             Priority = BindingPriority.Unset;
+            _isSubscribed = false;
             _sink.Completed(Property, this, oldValue);
         }
 
@@ -104,8 +104,11 @@ public void OnNext(BindingValue<T> value)
         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.
-            if (!_isSubscribed && (!_batchUpdate || ignoreBatchUpdate))
+            // until Subscribe has finished, which will be too late to prevent reentrancy. In addition
+            // 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 @@
 
 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 @@ public AvaloniaPropertyValueStore()
             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 @@ public void EndBatchUpdate()
 
         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 @@ public bool IsAnimating(AvaloniaProperty property)
 
         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 @@ public bool TryGetValue<T>(
             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 @@ public bool TryGetValue<T>(
 
             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 @@ public IDisposable AddBinding<T>(
             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 @@ public IDisposable AddBinding<T>(
 
         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 @@ public void ClearLocalValue<T>(StyledPropertyBase<T> property)
                     // 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 @@ public void ClearLocalValue<T>(StyledPropertyBase<T> property)
 
         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 @@ public void CoerceValue<T>(StyledPropertyBase<T> property)
 
         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 @@ void IValueSink.Completed<T>(
             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 @@ void IValueSink.Completed<T>(
                 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 @@ private IDisposable BindExisting<T>(
             {
                 priorityValue = new PriorityValue<T>(_owner, property, this, e);
 
-                if (_batchUpdate is object)
+                if (IsBatchUpdating())
                 {
                     priorityValue.BeginBatchUpdate();
                 }
@@ -338,7 +340,7 @@ private IDisposable BindExisting<T>(
         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 @@ private void NotifyValueChanged<T>(
             }
         }
 
+        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 @@ private class BatchUpdate
 
             public BatchUpdate(ValueStore owner) => _owner = owner;
 
+            public bool IsBatchUpdating => _batchUpdateCount > 0;
+
             public void Begin()
             {
                 if (_batchUpdateCount++ == 0)
@@ -437,8 +456,10 @@ public bool End()
 
                             // 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.
-                            if (slot.Priority == BindingPriority.Unset)
+                            // their priority to Unset. We need to re-read the slot here because raising ValueChanged
+                            // 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 @@ public void Replacing_Transitions_During_Animation_Does_Not_Throw_KeyNotFound()
             }
         }
 
+        [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);