Profile editor - Fix undoing profile editor commands

- Undoing a change to an existing keyframe's value restores the value instead of deleting the keyframe
- Undoing "remove effect" puts the effect back in its place instead of below the next one
- Undoing "toggle keyframes" restores what it was before
- Undoing a brush change on a layer that had no brush removes the brush again, using the new Layer.RemoveLayerBrush
- Undoing "add element" disables the element again
- Undoing an event path change back to no path goes back to an event start node, like a new condition
- MoveProfileElement says "Move folder" for folders
- UpdateLayerProperty without a time no longer calls itself "Add ... keyframe"

src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs

@@ -167,15 +167,13 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
     /// </summary>
     public void UpdateEventNode(bool updateScript)
     {
-        if (EventPath == null)
+        Type? pathType = EventPath?.GetPropertyType();
+        if (EventPath != null && pathType == null)
             return;
 
-        Type? pathType = EventPath.GetPropertyType();
+        // Create an event node if the path type is a data model event, without a path go back to the event node a new
-        if (pathType == null)
+        // condition starts with
-            return;
+        if (pathType == null || pathType.IsAssignableTo(typeof(IDataModelEvent)))
-
-        // Create an event node if the path type is a data model event
-        if (pathType.IsAssignableTo(typeof(IDataModelEvent)))
         {
             EventConditionEventStartNode eventNode;
             // Ensure the start node is an event node

src/Artemis.Core/Models/Profile/Layer.cs

@@ -847,6 +847,24 @@ public sealed class Layer : RenderProfileElement
         ActivateLayerBrush();
     }
 
+    /// <summary>
+    ///     Removes the current layer brush, leaving the layer without a brush
+    /// </summary>
+    public void RemoveLayerBrush()
+    {
+        if (LayerBrush == null)
+            return;
+
+        BaseLayerBrush oldLayerBrush = LayerBrush;
+        General.BrushReference.SetCurrentValue(null!);
+        LayerBrush = null;
+
+        // Don't dispose the brush, only disable it
+        // That way an undo-action does not need to worry about disposed brushes
+        oldLayerBrush.InternalDisable();
+        ActivateLayerBrush();
+    }
+
     private void ActivateLayerBrush()
     {
         try

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ConditionCommandTests.cs

@@ -180,7 +180,7 @@ public class ConditionCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void UpdateEventConditionPath_Undo_ToNoPath_KeepsTheNewStartNode()
+    public void UpdateEventConditionPath_Undo_ToNoPath_GoesBackToAnEventStartNode()
     {
         TestModule module = AddTestModule();
         EventCondition condition = new(_layer);
@@ -191,11 +191,11 @@ public class ConditionCommandTests : ProfileEditorTestBase
         command.Execute();
         command.Undo();
 
-        // Pinned as-is: UpdateEventNode does nothing without a path, so undoing back to "no path" keeps the start node
+        // Like a new condition, rather than keeping the start node made for the path that was undone
-        // made for the path that was undone
         condition.EventPath.ShouldBeNull();
-        condition.GetStartNode().ShouldNotBe(originalStartNode);
+        condition.GetStartNode().GetType().ShouldBe(originalStartNode.GetType());
-        condition.GetStartNode().GetType().Name.ShouldBe("EventConditionValueChangedStartNode");
+        condition.Script.Nodes.ShouldContain(condition.GetStartNode());
+        condition.Script.Nodes.ShouldNotContain(n => n.GetType().Name == "EventConditionValueChangedStartNode");
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/KeyframeCommandTests.cs

@@ -219,15 +219,15 @@ public class KeyframeCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void ToggleLayerPropertyKeyframes_Undo_DoesNotRememberWhatItWasBefore()
+    public void ToggleLayerPropertyKeyframes_Undo_RestoresWhatItWasBefore()
     {
         ToggleLayerPropertyKeyframes<float> enable = new(_opacity, true, TimeSpan.Zero);
 
         enable.Execute();
         enable.Undo();
 
-        // Pinned as-is: undo sets the opposite of what the command did rather than restoring the earlier state, so
+        // "Enabling" keyframes that were already enabled leaves them enabled when undone
-        // "enabling" keyframes that were already enabled turns them off when undone
+        _opacity.KeyframesEnabled.ShouldBeTrue();
-        _opacity.KeyframesEnabled.ShouldBeFalse();
+        _opacity.Keyframes.ShouldBe([_first, _second]);
     }
 }

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerBrushAndLedCommandTests.cs

@@ -67,7 +67,7 @@ public class LayerBrushAndLedCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void ChangeLayerBrush_Undo_OnALayerWithoutBrush_KeepsTheNewBrush()
+    public void ChangeLayerBrush_Undo_OnALayerWithoutBrush_RemovesTheNewBrush()
     {
         Layer layer = new(Root, "Layer without brush");
         Root.AddChild(layer);
@@ -76,8 +76,8 @@ public class LayerBrushAndLedCommandTests : ProfileEditorTestBase
         command.Execute();
         command.Undo();
 
-        // Pinned as-is: with no brush to go back to, undo leaves the new brush (and the brush reference) in place
+        layer.LayerBrush.ShouldBeNull();
-        layer.LayerBrush.ShouldBeOfType<GradientTestBrush>();
+        layer.General.BrushReference.CurrentValue.ShouldBeNull();
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerEffectCommandTests.cs

@@ -111,7 +111,7 @@ public class LayerEffectCommandTests : ProfileEditorTestBase
     [Theory]
     [InlineData(0)]
     [InlineData(1)]
-    public void RemoveLayerEffect_Undo_OfAnEffectAboveAnother_MovesItDown(int index)
+    public void RemoveLayerEffect_Undo_OfAnEffectAboveAnother_PutsItBackInItsPlace(int index)
     {
         Harness.AddEffect<TintTestEffect>(_layer);
         Harness.AddEffect<OpacityTestEffect>(_layer);
@@ -124,13 +124,10 @@ public class LayerEffectCommandTests : ProfileEditorTestBase
         command.Execute();
         command.Undo();
 
-        // Pinned as-is: removing renumbers the remaining effects, so the effect below takes over the removed effect's
+        // Effects apply in order, so this keeps the layer looking the same
-        // order. Adding it back sorts it after the effect that now has the same order, swapping the two. Since effects
-        // apply in order, undo can change what the layer looks like
-        effects.RemoveAt(index);
-        effects.Insert(index + 1, removed);
         _layer.LayerEffects.ShouldBe(effects);
-        Snapshot().ShouldNotBe(before);
+        _layer.LayerEffects.Select(e => e.Order).ShouldBe([1, 2, 3]);
+        Snapshot().ShouldBe(before);
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerPropertyCommandTests.cs

@@ -114,7 +114,7 @@ public class LayerPropertyCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void UpdateLayerProperty_WithOriginalValue_OnAKeyframe_RemovesTheKeyframeOnUndo()
+    public void UpdateLayerProperty_WithOriginalValue_OnAKeyframe_RestoresTheKeyframeOnUndo()
     {
         AddKeyframe(0, 0);
         LayerPropertyKeyframe<float> keyframe = AddKeyframe(60, 1);
@@ -124,12 +124,10 @@ public class LayerPropertyCommandTests : ProfileEditorTestBase
         keyframe.Value.ShouldBe(40);
         command.Undo();
 
-        // Pinned as-is: this constructor never checks whether there already is a keyframe at the time. When keyframes are
+        _opacity.Keyframes.ShouldContain(keyframe);
-        // enabled, Execute treats the existing keyframe that SetCurrentValue updated as a new one, and Undo removes it
+        _opacity.Keyframes.Count.ShouldBe(2);
-        // instead of restoring its value. Changing a keyframe through a property input and undoing deletes the keyframe
+        keyframe.Value.ShouldBe(60);
-        _opacity.Keyframes.ShouldNotContain(keyframe);
+        command.DisplayName.ShouldBe("Update Opacity");
-        _opacity.Keyframes.Count.ShouldBe(1);
-        command.DisplayName.ShouldBe("Add Opacity keyframe");
     }
 
     [Fact]
@@ -142,9 +140,7 @@ public class LayerPropertyCommandTests : ProfileEditorTestBase
 
         _opacity.BaseValue.ShouldBe(40);
         _opacity.Keyframes.Count.ShouldBe(1);
-        // Pinned as-is: with keyframes enabled the command expects to add a keyframe and renames itself, even though
+        command.DisplayName.ShouldBe("Update Opacity");
-        // without a time no keyframe is added
-        command.DisplayName.ShouldBe("Add Opacity keyframe");
 
         command.Undo();
         _opacity.BaseValue.ShouldBe(0, 0.01f);

src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ProfileElementCommandTests.cs

@@ -53,7 +53,7 @@ public class ProfileElementCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void AddProfileElement_Undo_LeavesTheElementEnabled()
+    public void AddProfileElement_Undo_DisablesTheElement()
     {
         Layer layer = new(_folder, "New layer");
         AddProfileElement command = new(layer, _folder, 0);
@@ -61,9 +61,8 @@ public class ProfileElementCommandTests : ProfileEditorTestBase
         command.Execute();
         command.Undo();
 
-        // Pinned as-is: Execute enables the element but Undo does not disable it again. It is no longer in the profile
+        // Like RemoveProfileElement, so its brush and effects don't stay enabled
-        // so it doesn't render, but its brush and effects stay enabled (unlike RemoveProfileElement, which disables)
+        layer.Enabled.ShouldBeFalse();
-        layer.Enabled.ShouldBeTrue();
     }
 
     [Fact]
@@ -212,12 +211,11 @@ public class ProfileElementCommandTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void MoveProfileElement_DisplayName_AlwaysSaysLayer()
+    public void MoveProfileElement_DisplayName_SaysWhatIsMoved()
     {
         Folder other = Harness.AddFolder(Root, "Other folder");
 
-        // Pinned as-is: the constructor sets "Move folder" for folders and then always overwrites it with "Move layer"
+        new MoveProfileElement(_folder, other, 0).DisplayName.ShouldBe("Move folder");
-        new MoveProfileElement(_folder, other, 0).DisplayName.ShouldBe("Move layer");
         new MoveProfileElement(Root, _first, 0).DisplayName.ShouldBe("Move layer");
     }
 

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/AddProfileElement.cs

@@ -55,6 +55,7 @@ public class AddProfileElement : IProfileEditorCommand, IDisposable
     public void Undo()
     {
         _isAdded = false;
+        _subject.Disable();
         _target.RemoveChild(_subject);
     }
 

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ChangeLayerBrush.cs

@@ -56,6 +56,8 @@ public class ChangeLayerBrush : IProfileEditorCommand, IDisposable
     {
         if (_previousBrush != null)
             _layer.ChangeLayerBrush(_previousBrush);
+        else
+            _layer.RemoveLayerBrush();
         _executed = false;
     }
 

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/MoveProfileElement.cs

@@ -29,9 +29,7 @@ public class MoveProfileElement : IProfileEditorCommand
             _originalIndex = _subject.Parent.Children.IndexOf(_subject);
         }
 
-        if (subject is Folder)
+        DisplayName = subject is Folder ? "Move folder" : "Move layer";
-            DisplayName = "Move folder";
-        DisplayName = "Move layer";
     }
 
     #region Implementation of IProfileEditorCommand

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/RemoveLayerEffect.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Linq;
 using Artemis.Core;
 using Artemis.Core.LayerEffects;
 
@@ -12,6 +13,7 @@ public class RemoveLayerEffect : IProfileEditorCommand, IDisposable
     private readonly BaseLayerEffect _layerEffect;
     private readonly RenderProfileElement _renderProfileElement;
     private bool _executed;
+    private int _order;
 
     /// <summary>
     ///     Creates a new instance of the <see cref="RemoveLayerEffect" /> class.
@@ -35,6 +37,7 @@ public class RemoveLayerEffect : IProfileEditorCommand, IDisposable
     /// <inheritdoc />
     public void Execute()
     {
+        _order = _layerEffect.Order;
         _renderProfileElement.RemoveLayerEffect(_layerEffect);
         _executed = true;
     }
@@ -42,6 +45,11 @@ public class RemoveLayerEffect : IProfileEditorCommand, IDisposable
     /// <inheritdoc />
     public void Undo()
     {
+        // Removing the effect renumbered the effects below it, make room so it goes back in its old place
+        foreach (BaseLayerEffect layerEffect in _renderProfileElement.LayerEffects.Where(e => e.Order >= _order))
+            layerEffect.Order++;
+        _layerEffect.Order = _order;
+
         _renderProfileElement.AddLayerEffect(_layerEffect);
         _executed = false;
     }

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ToggleLayerPropertyKeyframes.cs

@@ -14,6 +14,8 @@ public class ToggleLayerPropertyKeyframes<T> : IProfileEditorCommand
     private readonly LayerProperty<T> _layerProperty;
     private readonly TimeSpan _time;
     private LayerPropertyKeyframe<T>? _keyframe;
+    private bool _addedKeyframe;
+    private bool _wasEnabled;
 
     /// <summary>
     ///     Creates a new instance of the <see cref="ToggleLayerPropertyKeyframes{T}" /> class.
@@ -33,37 +35,26 @@ public class ToggleLayerPropertyKeyframes<T> : IProfileEditorCommand
     /// <inheritdoc />
     public void Execute()
     {
-        if (_enable)
+        _wasEnabled = _layerProperty.KeyframesEnabled;
-        {
+        _addedKeyframe = false;
-            _layerProperty.KeyframesEnabled = true;
+        _layerProperty.KeyframesEnabled = _enable;
 
-            // If there weren't any keyframes yet, add one with the current value at the current time
+        // If there weren't any keyframes yet, add one with the current value at the current time
-            if (!_layerProperty.Keyframes.Any())
+        if (_enable && !_layerProperty.Keyframes.Any())
-            {
-                // If executed before, reuse the previous keyframe
-                _keyframe ??= new LayerPropertyKeyframe<T>(_layerProperty.CurrentValue, _time, Easings.Functions.Linear, _layerProperty);
-                _layerProperty.AddKeyframe(_keyframe);
-            }
-        }
-        else
         {
-            _layerProperty.KeyframesEnabled = false;
+            // If executed before, reuse the previous keyframe
+            _keyframe ??= new LayerPropertyKeyframe<T>(_layerProperty.CurrentValue, _time, Easings.Functions.Linear, _layerProperty);
+            _layerProperty.AddKeyframe(_keyframe);
+            _addedKeyframe = true;
         }
     }
 
     /// <inheritdoc />
     public void Undo()
     {
-        if (_enable)
+        if (_addedKeyframe && _keyframe != null)
-        {
+            _layerProperty.RemoveKeyframe(_keyframe);
-            if (_keyframe != null)
+        _layerProperty.KeyframesEnabled = _wasEnabled;
-                _layerProperty.RemoveKeyframe(_keyframe);
-            _layerProperty.KeyframesEnabled = false;
-        }
-        else
-        {
-            _layerProperty.KeyframesEnabled = true;
-        }
     }
 
     #endregion

src/Artemis.UI.Shared/Services/ProfileEditor/Commands/UpdateLayerProperty.cs

@@ -39,6 +39,7 @@ public class UpdateLayerProperty<T> : IProfileEditorCommand
         _originalValue = originalValue;
         _newValue = newValue;
         _time = time;
+        _hasKeyframe = _layerProperty.Keyframes.Any(k => k.Position == time);
 
         DisplayName = $"Update {_layerProperty.PropertyDescription.Name ?? "property"}";
     }
@@ -58,8 +59,9 @@ public class UpdateLayerProperty<T> : IProfileEditorCommand
             return;
         }
 
-        // If the layer had no keyframe yet but keyframes are enabled, a new keyframe will be returned
+        // If the layer had no keyframe yet but keyframes are enabled, a new keyframe will be returned. Without a time the
-        if (!_hasKeyframe && _layerProperty.KeyframesEnabled)
+        // base value is changed instead
+        if (!_hasKeyframe && _time != null && _layerProperty.KeyframesEnabled)
         {
             _newKeyframe = _layerProperty.SetCurrentValue(_newValue, _time);
             DisplayName = $"Add {_layerProperty.PropertyDescription.Name ?? "property"} keyframe";