From 295038f3381c32feb8271f753321f690d8e078c5 Mon Sep 17 00:00:00 2001 From: RobertBeekman Date: Sat, 24 Oct 2026 16:17:05 +0200 Subject: [PATCH] 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 --- .../Profile/Conditions/EventCondition.cs | 12 +++--- src/Artemis.Core/Models/Profile/Layer.cs | 18 +++++++++ .../Commands/ConditionCommandTests.cs | 10 ++--- .../Commands/KeyframeCommandTests.cs | 8 ++-- .../Commands/LayerBrushAndLedCommandTests.cs | 6 +-- .../Commands/LayerEffectCommandTests.cs | 11 ++---- .../Commands/LayerPropertyCommandTests.cs | 16 +++----- .../Commands/ProfileElementCommandTests.cs | 12 +++--- .../ProfileEditorCreateElementTests.cs | 9 ++--- .../ProfileEditorFocusModeTests.cs | 10 ++--- .../ProfileEditorHistoryTests.cs | 23 +++++------- .../ProfileEditorServiceTests.cs | 21 +++++------ .../ProfileEditor/ProfileEditorTimeTests.cs | 8 +--- .../ProfileEditorTimelineHelperTests.cs | 13 +++---- .../Commands/AddProfileElement.cs | 1 + .../Commands/ChangeLayerBrush.cs | 2 + .../Commands/MoveProfileElement.cs | 4 +- .../Commands/RemoveLayerEffect.cs | 8 ++++ .../Commands/ToggleLayerPropertyKeyframes.cs | 37 +++++++------------ .../Commands/UpdateLayerProperty.cs | 6 ++- .../ProfileEditor/ProfileEditorHistory.cs | 7 +++- .../ProfileEditor/ProfileEditorService.cs | 31 ++++++++++++---- 22 files changed, 140 insertions(+), 133 deletions(-) diff --git a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs index acf68a02d..5b38d921a 100644 --- a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs +++ b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs @@ -173,15 +173,13 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition /// 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 diff --git a/src/Artemis.Core/Models/Profile/Layer.cs b/src/Artemis.Core/Models/Profile/Layer.cs index 617035b5a..b68cc08e6 100644 --- a/src/Artemis.Core/Models/Profile/Layer.cs +++ b/src/Artemis.Core/Models/Profile/Layer.cs @@ -857,6 +857,24 @@ public sealed class Layer : RenderProfileElement ActivateLayerBrush(); } + /// + /// Removes the current layer brush, leaving the layer without a brush + /// + 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 diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ConditionCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ConditionCommandTests.cs index 386fb8c35..220818fce 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ConditionCommandTests.cs +++ b/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] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/KeyframeCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/KeyframeCommandTests.cs index 253ca8983..b05c6b06d 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/KeyframeCommandTests.cs +++ b/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 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]); } } diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerBrushAndLedCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerBrushAndLedCommandTests.cs index 5d1ef376c..dcc3bccec 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerBrushAndLedCommandTests.cs +++ b/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(); + layer.LayerBrush.ShouldBeNull(); + layer.General.BrushReference.CurrentValue.ShouldBeNull(); } [Fact] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerEffectCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerEffectCommandTests.cs index 9a5c8acfb..bae92fc79 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerEffectCommandTests.cs +++ b/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(_layer); Harness.AddEffect(_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] diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerPropertyCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerPropertyCommandTests.cs index ee6e3ee4d..5a233d5de 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/LayerPropertyCommandTests.cs +++ b/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 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); diff --git a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ProfileElementCommandTests.cs b/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ProfileElementCommandTests.cs index 7c76517b2..75637083d 100644 --- a/src/Artemis.UI.Shared.Tests/ProfileEditor/Commands/ProfileElementCommandTests.cs +++ b/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"); } 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/Commands/AddProfileElement.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/AddProfileElement.cs index 124a5bcdc..11f8424ed 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/AddProfileElement.cs +++ b/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); } diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ChangeLayerBrush.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ChangeLayerBrush.cs index ce92133d6..de50e81a7 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ChangeLayerBrush.cs +++ b/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; } diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/MoveProfileElement.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/MoveProfileElement.cs index 69d67bd08..d0e223d8d 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/MoveProfileElement.cs +++ b/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 diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/RemoveLayerEffect.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/RemoveLayerEffect.cs index 457fefea5..e11e1aa3d 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/RemoveLayerEffect.cs +++ b/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; /// /// Creates a new instance of the class. @@ -35,6 +37,7 @@ public class RemoveLayerEffect : IProfileEditorCommand, IDisposable /// public void Execute() { + _order = _layerEffect.Order; _renderProfileElement.RemoveLayerEffect(_layerEffect); _executed = true; } @@ -42,6 +45,11 @@ public class RemoveLayerEffect : IProfileEditorCommand, IDisposable /// 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; } diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ToggleLayerPropertyKeyframes.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ToggleLayerPropertyKeyframes.cs index 0d05b0bc8..2d920adf8 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ToggleLayerPropertyKeyframes.cs +++ b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/ToggleLayerPropertyKeyframes.cs @@ -14,6 +14,8 @@ public class ToggleLayerPropertyKeyframes : IProfileEditorCommand private readonly LayerProperty _layerProperty; private readonly TimeSpan _time; private LayerPropertyKeyframe? _keyframe; + private bool _addedKeyframe; + private bool _wasEnabled; /// /// Creates a new instance of the class. @@ -33,37 +35,26 @@ public class ToggleLayerPropertyKeyframes : IProfileEditorCommand /// 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(_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(_layerProperty.CurrentValue, _time, Easings.Functions.Linear, _layerProperty); + _layerProperty.AddKeyframe(_keyframe); + _addedKeyframe = true; } } /// 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 diff --git a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/UpdateLayerProperty.cs b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/UpdateLayerProperty.cs index 77e4d8fe9..ea89cc41e 100644 --- a/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/UpdateLayerProperty.cs +++ b/src/Artemis.UI.Shared/Services/ProfileEditor/Commands/UpdateLayerProperty.cs @@ -39,6 +39,7 @@ public class UpdateLayerProperty : 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 : 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"; 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));