From 55dc04163c46a478a4350d2b5f8534a3307775e6 Mon Sep 17 00:00:00 2001 From: RobertBeekman Date: Tue, 20 Oct 2026 09:47:43 +0200 Subject: [PATCH] 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 --- .../ProfileEditorCreateElementTests.cs | 9 +++--- .../ProfileEditorFocusModeTests.cs | 10 +++--- .../ProfileEditorHistoryTests.cs | 23 ++++++-------- .../ProfileEditorServiceTests.cs | 21 ++++++------- .../ProfileEditor/ProfileEditorTimeTests.cs | 8 +---- .../ProfileEditorTimelineHelperTests.cs | 13 +++----- .../ProfileEditor/ProfileEditorHistory.cs | 7 +++-- .../ProfileEditor/ProfileEditorService.cs | 31 ++++++++++++++----- 8 files changed, 60 insertions(+), 62 deletions(-) diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorCreateElementTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorCreateElementTests.cs index 7c60ea5e0..be5aa7ae3 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorCreateElementTests.cs +++ b/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] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorFocusModeTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorFocusModeTests.cs index d396a3ef0..4fec7b355 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorFocusModeTests.cs +++ b/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(); } } diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorHistoryTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorHistoryTests.cs index baf17ec2c..086d955b7 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorHistoryTests.cs +++ b/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 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(); + // 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().DisplayName.ShouldBe("Empty"); + Undo().ShouldBeNull(); } [Fact] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorServiceTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorServiceTests.cs index a6aa24504..6320d72f1 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorServiceTests.cs +++ b/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 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] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimeTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimeTests.cs index 36370f54b..2080f8fcc 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimeTests.cs +++ b/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)); } diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimelineHelperTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimelineHelperTests.cs index f12fea478..1988ab3bc 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/ProfileEditorTimelineHelperTests.cs +++ b/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 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] diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorHistory.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorHistory.cs index 5cb9d3891..4d2ce374f 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorHistory.cs +++ b/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorHistory.cs @@ -14,8 +14,8 @@ namespace Artemis.UI.Shared.Services.ProfileEditor; /// public class ProfileEditorHistory { - private readonly Subject _canRedo = new(); - private readonly Subject _canUndo = new(); + private readonly BehaviorSubject _canRedo = new(false); + private readonly BehaviorSubject _canUndo = new(false); private readonly Stack _redoCommands = new(); private readonly Stack _undoCommands = new(); @@ -28,6 +28,9 @@ public class ProfileEditorHistory ProfileConfiguration = profileConfiguration; Execute = ReactiveCommand.Create(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); } diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorService.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorService.cs index 421aad406..9c70ceb43 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/ProfileEditorService.cs +++ b/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() + .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));