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.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
+        layer.Parent.ShouldBe(Root);
-        // parent, so the new layer gets the same name as its sibling. CreateAndAddFolder uses the parent.
+        layer.Name.ShouldNotBe("New layer");
-        layer.Name.ShouldBe("New layer");
+        Root.Children.Count(c => c.Name == "New layer").ShouldBe(1);
-        Root.Children.Count(c => c.Name == "New layer").ShouldBe(2);
     }
 
     [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
+        // The profile plays normally while editing is suspended
-        // from the focus mode, so the clear has no effect. It looks like the intention was to not focus anything while
+        ProfileService.FocusProfileElementChanges.ShouldBe([null]);
-        // the profile plays normally.
+        ProfileService.FocusProfileElement.ShouldBeNull();
-        ProfileService.FocusProfileElementChanges.ShouldBe([null, _layer]);
-        ProfileService.FocusProfileElement.ShouldBe(_layer);
     }
 }

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
+        canUndo.ShouldBe([false]);
-        // the next change. Bindings made after a change show the wrong state until the next one
+        canRedo.ShouldBe([false]);
-        canUndo.ShouldBeEmpty();
-        canRedo.ShouldBeEmpty();
     }
 
     [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 dialog is enough, ReactiveUI doesn't also treat the failure as unhandled (a crash in the app)
-        // the ThrownExceptions of the history's Execute command. In the app that means a failing command is reported as
+        unhandled.ShouldBeEmpty();
-        // a crash on top of the dialog.
-        unhandled.ShouldHaveSingleItem().ShouldBeOfType<InvalidOperationException>();
         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().ShouldBeNull();
-        Undo().ShouldBeOfType<CompositeCommand>().DisplayName.ShouldBe("Empty");
     }
 
     [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
+        received.ShouldHaveSingleItem()!.ProfileConfiguration.ShouldBe(Configuration);
-        // 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);
     }
 
     [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
+        await WaitUntil(() =>
-        // drives auto-saving. Nothing is auto-saved anymore for as long as the app runs, without telling the user
+        {
-        lock (ProfileService.SavedProfiles)
+            lock (ProfileService.SavedProfiles)
-            ProfileService.SavedProfiles.Count.ShouldBe(1);
+                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
+        Service.SelectedKeyframes.ShouldBe([_first]);
-        // selected, so it ends up in the selection twice
-        Service.SelectedKeyframes.ShouldBe([_first, _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/ProfileEditorHistory.cs

@@ -14,8 +14,8 @@ namespace Artemis.UI.Shared.Services.ProfileEditor;
 /// </summary>
 public class ProfileEditorHistory
 {
-    private readonly Subject<bool> _canRedo = new();
+    private readonly BehaviorSubject<bool> _canRedo = new(false);
-    private readonly Subject<bool> _canUndo = new();
+    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)!;
+            TimeSpan? closeSnapTime = snapTimes.Where(s => Math.Abs(time.TotalMilliseconds - s.TotalMilliseconds) < tolerance.TotalMilliseconds)
-            if (closeSnapTime != TimeSpan.Zero)
+                .Cast<TimeSpan?>()
-                return closeSnapTime;
+                .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));