Profile editor - Fix undo and editor service bugs found by the tests (#913)

* 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"

* Profile editor - Fix auto-saving, failing commands, suspended editing and smaller editor issues

- A failed auto-save no longer stops auto-saving for the rest of the session
- A failing command is no longer also reported to ReactiveUI as unhandled, on top of the error dialog
- Resuming editing applies the editor's time again right away
- Nothing is focused while editing is suspended
- A layer created on another layer gets a name that isn't in use next to it
- CanUndo and CanRedo start out false, so new subscribers know the state right away
- An empty command scope doesn't add an undo step
- New subscribers to History get the current history once
- Expanding the keyframe selection with an already selected keyframe doesn't add it twice
- Snapping can snap to a snap time of zero

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

@@ -173,15 +173,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();
-        if (pathType == null)
-            return;
-
-        // Create an event node if the path type is a data model event
-        if (pathType.IsAssignableTo(typeof(IDataModelEvent)))
+        // Create an event node if the path type is a data model event, without a path go back to the event node a new
+        // condition starts with
+        if (pathType == null || pathType.IsAssignableTo(typeof(IDataModelEvent)))
         {
             EventConditionEventStartNode eventNode;
             // Ensure the start node is an event node

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

@@ -857,6 +857,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
-        // made for the path that was undone
+        // Like a new condition, rather than keeping the start node made for the path that was undone
         condition.EventPath.ShouldBeNull();
-        condition.GetStartNode().ShouldNotBe(originalStartNode);
-        condition.GetStartNode().GetType().Name.ShouldBe("EventConditionValueChangedStartNode");
+        condition.GetStartNode().GetType().ShouldBe(originalStartNode.GetType());
+        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 turns them off when undone
-        _opacity.KeyframesEnabled.ShouldBeFalse();
+        // "Enabling" keyframes that were already enabled leaves them enabled when undone
+        _opacity.KeyframesEnabled.ShouldBeTrue();
+        _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.ShouldBeOfType<GradientTestBrush>();
+        layer.LayerBrush.ShouldBeNull();
+        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
-        // 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);
+        // Effects apply in order, so this keeps the layer looking the same
         _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
-        // enabled, Execute treats the existing keyframe that SetCurrentValue updated as a new one, and Undo removes it
-        // instead of restoring its value. Changing a keyframe through a property input and undoing deletes the keyframe
-        _opacity.Keyframes.ShouldNotContain(keyframe);
-        _opacity.Keyframes.Count.ShouldBe(1);
-        command.DisplayName.ShouldBe("Add Opacity keyframe");
+        _opacity.Keyframes.ShouldContain(keyframe);
+        _opacity.Keyframes.Count.ShouldBe(2);
+        keyframe.Value.ShouldBe(60);
+        command.DisplayName.ShouldBe("Update Opacity");
     }
 
     [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
-        // without a time no keyframe is added
-        command.DisplayName.ShouldBe("Add Opacity keyframe");
+        command.DisplayName.ShouldBe("Update Opacity");
 
         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
-        // so it doesn't render, but its brush and effects stay enabled (unlike RemoveProfileElement, which disables)
-        layer.Enabled.ShouldBeTrue();
+        // Like RemoveProfileElement, so its brush and effects don't stay enabled
+        layer.Enabled.ShouldBeFalse();
     }
 
     [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 layer");
+        new MoveProfileElement(_folder, other, 0).DisplayName.ShouldBe("Move folder");
         new MoveProfileElement(Root, _first, 0).DisplayName.ShouldBe("Move layer");
     }
 

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorCreateElementTests.cs

@@ -98,17 +98,16 @@ public class ProfileEditorCreateElementTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task CreateAndAddLayer_OnALayer_CanRepeatANameAlreadyInUse()
+    public async Task CreateAndAddLayer_OnALayer_PicksANameNotInUseNextToIt()
     {
         Layer existing = AddLayer("New layer", null, LedId.Keyboard_A);
         await StartEditing();
 
         Layer layer = Service.CreateAndAddLayer(existing);
 
-        // Pinned as-is: the name is picked from the children of the target layer (which has none) rather than from its
-        // parent, so the new layer gets the same name as its sibling. CreateAndAddFolder uses the parent.
-        layer.Name.ShouldBe("New layer");
-        Root.Children.Count(c => c.Name == "New layer").ShouldBe(2);
+        layer.Parent.ShouldBe(Root);
+        layer.Name.ShouldNotBe("New layer");
+        Root.Children.Count(c => c.Name == "New layer").ShouldBe(1);
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorFocusModeTests.cs

@@ -89,7 +89,7 @@ public class ProfileEditorFocusModeTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task SuspendedEditing_StillFocusesTheSelection()
+    public async Task SuspendedEditing_DoesNotFocusAnything()
     {
         await StartEditing();
         Service.ChangeCurrentProfileElement(_layer);
@@ -98,10 +98,8 @@ public class ProfileEditorFocusModeTests : ProfileEditorTestBase
 
         Service.ChangeSuspendedEditing(true);
 
-        // Pinned as-is: while editing is suspended ApplyFocusMode clears the focus, but then immediately sets it again
-        // from the focus mode, so the clear has no effect. It looks like the intention was to not focus anything while
-        // the profile plays normally.
-        ProfileService.FocusProfileElementChanges.ShouldBe([null, _layer]);
-        ProfileService.FocusProfileElement.ShouldBe(_layer);
+        // The profile plays normally while editing is suspended
+        ProfileService.FocusProfileElementChanges.ShouldBe([null]);
+        ProfileService.FocusProfileElement.ShouldBeNull();
     }
 }

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorHistoryTests.cs

@@ -34,11 +34,11 @@ public class ProfileEditorHistoryTests : ProfileEditorTestBase
         Service.ExecuteCommand(new RecordingCommand("A", _log));
 
         _log.ShouldBe(["execute A"]);
-        canUndo.ShouldBe([true]);
+        canUndo.ShouldBe([false, true]);
     }
 
     [Fact]
-    public async Task CanUndoAndCanRedo_SayNothingUntilTheHistoryChanges()
+    public async Task CanUndoAndCanRedo_StartOutFalse()
     {
         await StartEditing();
         List<bool> canUndo = [];
@@ -47,10 +47,8 @@ public class ProfileEditorHistoryTests : ProfileEditorTestBase
         using IDisposable undoSubscription = CurrentHistory.CanUndo.Subscribe(canUndo.Add);
         using IDisposable redoSubscription = CurrentHistory.CanRedo.Subscribe(canRedo.Add);
 
-        // Pinned as-is: the subjects behind CanUndo and CanRedo have no initial value, a new subscriber only hears about
-        // the next change. Bindings made after a change show the wrong state until the next one
-        canUndo.ShouldBeEmpty();
-        canRedo.ShouldBeEmpty();
+        canUndo.ShouldBe([false]);
+        canRedo.ShouldBe([false]);
     }
 
     [Fact]
@@ -92,7 +90,7 @@ public class ProfileEditorHistoryTests : ProfileEditorTestBase
         Undo();
         Redo();
 
-        canUndo.ShouldBe([true, false, true]);
+        canUndo.ShouldBe([false, true, false, true]);
         canRedo.ShouldBe([false, true, false]);
     }
 
@@ -152,10 +150,8 @@ public class ProfileEditorHistoryTests : ProfileEditorTestBase
         }
 
         WindowService.ExceptionDialogs.ShouldHaveSingleItem().Title.ShouldBe("Editor command failed");
-        // Pinned as-is: besides the error dialog, ReactiveUI treats the failure as unhandled because nothing subscribes to
-        // the ThrownExceptions of the history's Execute command. In the app that means a failing command is reported as
-        // a crash on top of the dialog.
-        unhandled.ShouldHaveSingleItem().ShouldBeOfType<InvalidOperationException>();
+        // The dialog is enough, ReactiveUI doesn't also treat the failure as unhandled (a crash in the app)
+        unhandled.ShouldBeEmpty();
         Undo().ShouldBeNull();
     }
 
@@ -217,14 +213,13 @@ public class ProfileEditorHistoryTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task CommandScope_WithoutCommands_StillAddsAnEntryToTheHistory()
+    public async Task CommandScope_WithoutCommands_DoesNotAddAnEntryToTheHistory()
     {
         await StartEditing();
 
         Service.CreateCommandScope("Empty").Dispose();
 
-        // Pinned as-is: an empty scope leaves an undo step that does nothing
-        Undo().ShouldBeOfType<CompositeCommand>().DisplayName.ShouldBe("Empty");
+        Undo().ShouldBeNull();
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorServiceTests.cs

@@ -155,18 +155,14 @@ public class ProfileEditorServiceTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task History_GivesNewSubscribersTheCurrentHistoryTwice()
+    public async Task History_GivesNewSubscribersTheCurrentHistoryOnce()
     {
         await StartEditing();
         List<ProfileEditorHistory?> received = [];
 
         using IDisposable _ = Service.History.Subscribe(received.Add);
 
-        // Pinned as-is: History first returns the current history and then subscribes to ProfileConfiguration, which
-        // repeats the current configuration. Harmless for the UI, but every subscriber gets the same history twice
-        received.Count.ShouldBe(2);
-        received.ShouldAllBe(h => h == received[0]);
-        received[0]!.ProfileConfiguration.ShouldBe(Configuration);
+        received.ShouldHaveSingleItem()!.ProfileConfiguration.ShouldBe(Configuration);
     }
 
     [Fact]
@@ -259,7 +255,7 @@ public class ProfileEditorServiceTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task AutoSave_AfterAFailedSave_StopsSaving()
+    public async Task AutoSave_AfterAFailedSave_KeepsSaving()
     {
         Layer layer = AddLayer("Layer", null, LedId.Keyboard_A);
         await StartEditing();
@@ -271,12 +267,13 @@ public class ProfileEditorServiceTests : ProfileEditorTestBase
 
         ProfileService.SaveException = null;
         Service.ExecuteCommand(new RenameProfileElement(layer, "Second"));
-        await Task.Delay(2000, TestContext.Current.CancellationToken);
 
-        // Pinned as-is: the auto-save rethrows the exception after showing the dialog, which ends the observable that
-        // drives auto-saving. Nothing is auto-saved anymore for as long as the app runs, without telling the user
-        lock (ProfileService.SavedProfiles)
-            ProfileService.SavedProfiles.Count.ShouldBe(1);
+        await WaitUntil(() =>
+        {
+            lock (ProfileService.SavedProfiles)
+                return ProfileService.SavedProfiles.Count > 1;
+        });
+        layer.Name.ShouldBe("Second");
     }
 
     [Fact]

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimeTests.cs

@@ -131,7 +131,7 @@ public class ProfileEditorTimeTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public async Task ResumingEditing_TakesBackControlButLeavesTheTimelinesAtTheStart()
+    public async Task ResumingEditing_TakesBackControlAndAppliesTheTime()
     {
         Layer layer = AddLayer("Layer", null, LedId.Keyboard_A);
         await StartEditing();
@@ -142,13 +142,7 @@ public class ProfileEditorTimeTests : ProfileEditorTestBase
 
         Current(Service.SuspendedEditing).ShouldBeFalse();
         ProfileService.UpdateFocusProfile.ShouldBeFalse();
-        // Pinned as-is: resuming calls Tick to apply the editor's time again, but does so before marking editing as
-        // resumed, and Tick skips suspended editing. So the profile stays where suspending left it (at the start) until
-        // the time changes, while the editor shows 2 seconds
-        layer.Timeline.Position.ShouldBe(TimeSpan.Zero);
         Current(Service.Time).ShouldBe(TimeSpan.FromSeconds(2));
-
-        Service.ChangeTime(TimeSpan.FromSeconds(2));
         layer.Timeline.Position.ShouldBe(TimeSpan.FromSeconds(2));
     }
 

src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimelineHelperTests.cs

@@ -45,14 +45,12 @@ public class ProfileEditorTimelineHelperTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void SelectKeyframe_ExpandingWithAKeyframeAlreadySelected_SelectsItTwice()
+    public void SelectKeyframe_ExpandingWithAKeyframeAlreadySelected_SelectsItOnce()
     {
         Service.SelectKeyframe(_first, false, false);
         Service.SelectKeyframe(_first, true, false);
 
-        // Pinned as-is: unlike SelectKeyframes, expanding the selection does not check whether the keyframe is already
-        // selected, so it ends up in the selection twice
-        Service.SelectedKeyframes.ShouldBe([_first, _first]);
+        Service.SelectedKeyframes.ShouldBe([_first]);
     }
 
     [Fact]
@@ -153,14 +151,13 @@ public class ProfileEditorTimelineHelperTests : ProfileEditorTestBase
     }
 
     [Fact]
-    public void SnapToTimeline_NeverSnapsToASnapTimeOfZero()
+    public void SnapToTimeline_SnapsToASnapTimeOfZero()
     {
+        // A keyframe at the very start of the timeline
         List<TimeSpan> snapTimes = [TimeSpan.Zero];
 
-        // Pinned as-is: a snap time of zero can't be told apart from "no snap time found" (the default of TimeSpan), so
-        // a keyframe at the very start of the timeline is never snapped to
         Service.SnapToTimeline(TimeSpan.FromMilliseconds(30), TimeSpan.FromMilliseconds(100), false, false, snapTimes)
-            .ShouldBe(TimeSpan.FromMilliseconds(30));
+            .ShouldBe(TimeSpan.Zero);
     }
 
     [Theory]

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 = "Move folder";
-        DisplayName = "Move layer";
+        DisplayName = subject is Folder ? "Move folder" : "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)
-        {
-            _layerProperty.KeyframesEnabled = true;
+        _wasEnabled = _layerProperty.KeyframesEnabled;
+        _addedKeyframe = false;
+        _layerProperty.KeyframesEnabled = _enable;
 
-            // If there weren't any keyframes yet, add one with the current value at the current time
-            if (!_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
+        // If there weren't any keyframes yet, add one with the current value at the current time
+        if (_enable && !_layerProperty.Keyframes.Any())
         {
-            _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 (_keyframe != null)
-                _layerProperty.RemoveKeyframe(_keyframe);
-            _layerProperty.KeyframesEnabled = false;
-        }
-        else
-        {
-            _layerProperty.KeyframesEnabled = true;
-        }
+        if (_addedKeyframe && _keyframe != null)
+            _layerProperty.RemoveKeyframe(_keyframe);
+        _layerProperty.KeyframesEnabled = _wasEnabled;
     }
 
     #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 (!_hasKeyframe && _layerProperty.KeyframesEnabled)
+        // If the layer had no keyframe yet but keyframes are enabled, a new keyframe will be returned. Without a time the
+        // base value is changed instead
+        if (!_hasKeyframe && _time != null && _layerProperty.KeyframesEnabled)
         {
             _newKeyframe = _layerProperty.SetCurrentValue(_newValue, _time);
             DisplayName = $"Add {_layerProperty.PropertyDescription.Name ?? "property"} keyframe";

src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorHistory.cs

@@ -14,8 +14,8 @@ namespace Artemis.UI.Shared.Services.ProfileEditor;
 /// </summary>
 public class ProfileEditorHistory
 {
-    private readonly Subject<bool> _canRedo = new();
-    private readonly Subject<bool> _canUndo = new();
+    private readonly BehaviorSubject<bool> _canRedo = new(false);
+    private readonly BehaviorSubject<bool> _canUndo = new(false);
     private readonly Stack<IProfileEditorCommand> _redoCommands = new();
     private readonly Stack<IProfileEditorCommand> _undoCommands = new();
 
@@ -28,6 +28,9 @@ public class ProfileEditorHistory
         ProfileConfiguration = profileConfiguration;
 
         Execute = ReactiveCommand.Create<IProfileEditorCommand>(ExecuteEditorCommand);
+        // Whoever executes a command reports its failure (the profile editor service shows a dialog), without this
+        // ReactiveUI would also treat it as unhandled
+        Execute.ThrownExceptions.Subscribe(_ => { });
         Undo = ReactiveCommand.Create(ExecuteUndo, CanUndo);
         Redo = ReactiveCommand.Create(ExecuteRedo, CanRedo);
     }

src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorService.cs

@@ -56,7 +56,7 @@ internal class ProfileEditorService : IProfileEditorService
         ProfileConfiguration = _profileConfigurationSubject.AsObservable();
         ProfileElement = _profileElementSubject.AsObservable();
         LayerProperty = _layerPropertySubject.AsObservable();
-        History = Observable.Defer(() => Observable.Return(GetHistory(_profileConfigurationSubject.Value))).Concat(ProfileConfiguration.Select(GetHistory));
+        History = ProfileConfiguration.Select(GetHistory);
         Time = _timeSubject.AsObservable();
         Playing = _playingSubject.AsObservable();
         SuspendedEditing = _suspendedEditingSubject.AsObservable();
@@ -101,9 +101,9 @@ internal class ProfileEditorService : IProfileEditorService
         }
         catch (Exception e)
         {
+            // Don't rethrow, that would end the observable that drives auto-saving
             _windowService.ShowExceptionDialog("Failed to auto-save profile", e);
             _logger.Error(e, "Failed to auto-save profile");
-            throw;
         }
 
         return Unit.Default;
@@ -138,8 +138,12 @@ internal class ProfileEditorService : IProfileEditorService
 
     private void ApplyFocusMode()
     {
+        // While editing is suspended the profile plays normally, without focus
         if (_suspendedEditingSubject.Value)
+        {
             _profileService.FocusProfileElement = null;
+            return;
+        }
 
         _profileService.FocusProfileElement = _focusModeSubject.Value switch
         {
@@ -235,10 +239,12 @@ internal class ProfileEditorService : IProfileEditorService
         {
             if (_profileConfigurationSubject.Value != null)
                 _profileService.UpdateFocusProfile = false;
-            Tick(_timeSubject.Value);
         }
 
         _suspendedEditingSubject.OnNext(suspend);
+        // Apply the editor's time again, this only works once editing is no longer suspended
+        if (!suspend)
+            Tick(_timeSubject.Value);
         ApplyFocusMode();
     }
 
@@ -271,7 +277,10 @@ internal class ProfileEditorService : IProfileEditorService
         else
         {
             if (expand)
-                _selectedKeyframes.Add(keyframe);
+            {
+                if (!_selectedKeyframes.Items.Contains(keyframe))
+                    _selectedKeyframes.Add(keyframe);
+            }
             else
                 _selectedKeyframes.Edit(l =>
                 {
@@ -324,9 +333,11 @@ internal class ProfileEditorService : IProfileEditorService
         if (snapTimes != null)
         {
             // Find the closest keyframe
-            TimeSpan closeSnapTime = snapTimes.FirstOrDefault(s => Math.Abs(time.TotalMilliseconds - s.TotalMilliseconds) < tolerance.TotalMilliseconds)!;
-            if (closeSnapTime != TimeSpan.Zero)
-                return closeSnapTime;
+            TimeSpan? closeSnapTime = snapTimes.Where(s => Math.Abs(time.TotalMilliseconds - s.TotalMilliseconds) < tolerance.TotalMilliseconds)
+                .Cast<TimeSpan?>()
+                .FirstOrDefault();
+            if (closeSnapTime != null)
+                return closeSnapTime.Value;
         }
 
         return time;
@@ -368,7 +379,7 @@ internal class ProfileEditorService : IProfileEditorService
     {
         if (target is Layer targetLayer)
         {
-            Layer layer = new(targetLayer.Parent, targetLayer.GetNewLayerName());
+            Layer layer = new(targetLayer.Parent, targetLayer.Parent.GetNewLayerName());
             _layerBrushService.ApplyDefaultBrush(layer);
 
             layer.AddLeds(_deviceService.EnabledDevices.SelectMany(d => d.Leds));
@@ -463,6 +474,10 @@ internal class ProfileEditorService : IProfileEditorService
         ProfileEditorCommandScope scope = _profileEditorHistoryScope;
         _profileEditorHistoryScope = null;
 
+        // Don't add an undo step that does nothing
+        if (!scope.ProfileEditorCommands.Any())
+            return;
+
         // Executing the composite command won't do anything the first time (see last ctor variable)
         // commands were already executed each time they were added to the scope
         ExecuteCommand(new CompositeCommand(scope.ProfileEditorCommands, scope.Name, true));