diff --git a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs index bbe50bdc1..787e03a72 100644 --- a/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs +++ b/src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs @@ -167,15 +167,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 88f605584..00a1f70bb 100644 --- a/src/Artemis.Core/Models/Profile/Layer.cs +++ b/src/Artemis.Core/Models/Profile/Layer.cs @@ -847,6 +847,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/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";