Merge pull request #22506 from peppy/skin-editor-undo-support

Add very basic skin editor undo / redo support

osu.Game.Tests/Editing/EditorChangeHandlerTest.cs → osu.Game.Tests/Editing/BeatmapEditorChangeHandlerTest.cs

@@ -1,8 +1,6 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using NUnit.Framework;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Beatmaps;
@@ -12,7 +10,7 @@
 namespace osu.Game.Tests.Editing
 {
     [TestFixture]
-    public class EditorChangeHandlerTest
+    public class BeatmapEditorChangeHandlerTest
     {
         private int stateChangedFired;
 
@@ -22,6 +20,35 @@ public void SetUp()
             stateChangedFired = 0;
         }
 
+        [Test]
+        public void TestSaveRestoreStateUsingTransaction()
+        {
+            var (handler, beatmap) = createChangeHandler();
+
+            Assert.That(handler.CanUndo.Value, Is.False);
+            Assert.That(handler.CanRedo.Value, Is.False);
+
+            handler.BeginChange();
+
+            // Initial state will be saved on BeginChange
+            Assert.That(stateChangedFired, Is.EqualTo(1));
+
+            addArbitraryChange(beatmap);
+            handler.EndChange();
+
+            Assert.That(stateChangedFired, Is.EqualTo(2));
+
+            Assert.That(handler.CanUndo.Value, Is.True);
+            Assert.That(handler.CanRedo.Value, Is.False);
+
+            handler.RestoreState(-1);
+
+            Assert.That(handler.CanUndo.Value, Is.False);
+            Assert.That(handler.CanRedo.Value, Is.True);
+
+            Assert.That(stateChangedFired, Is.EqualTo(3));
+        }
+
         [Test]
         public void TestSaveRestoreState()
         {
@@ -30,10 +57,14 @@ public void TestSaveRestoreState()
             Assert.That(handler.CanUndo.Value, Is.False);
             Assert.That(handler.CanRedo.Value, Is.False);
 
+            // Save initial state
+            handler.SaveState();
+            Assert.That(stateChangedFired, Is.EqualTo(1));
+
             addArbitraryChange(beatmap);
             handler.SaveState();
 
-            Assert.That(stateChangedFired, Is.EqualTo(1));
+            Assert.That(stateChangedFired, Is.EqualTo(2));
 
             Assert.That(handler.CanUndo.Value, Is.True);
             Assert.That(handler.CanRedo.Value, Is.False);
@@ -43,7 +74,7 @@ public void TestSaveRestoreState()
             Assert.That(handler.CanUndo.Value, Is.False);
             Assert.That(handler.CanRedo.Value, Is.True);
 
-            Assert.That(stateChangedFired, Is.EqualTo(2));
+            Assert.That(stateChangedFired, Is.EqualTo(3));
         }
 
         [Test]
@@ -54,22 +85,26 @@ public void TestApplyThenUndoThenApplySameChange()
             Assert.That(handler.CanUndo.Value, Is.False);
             Assert.That(handler.CanRedo.Value, Is.False);
 
+            // Save initial state
+            handler.SaveState();
+            Assert.That(stateChangedFired, Is.EqualTo(1));
+
             string originalHash = handler.CurrentStateHash;
 
             addArbitraryChange(beatmap);
             handler.SaveState();
 
             Assert.That(handler.CanUndo.Value, Is.True);
             Assert.That(handler.CanRedo.Value, Is.False);
-            Assert.That(stateChangedFired, Is.EqualTo(1));
+            Assert.That(stateChangedFired, Is.EqualTo(2));
 
             string hash = handler.CurrentStateHash;
 
             // undo a change without saving
             handler.RestoreState(-1);
 
             Assert.That(originalHash, Is.EqualTo(handler.CurrentStateHash));
-            Assert.That(stateChangedFired, Is.EqualTo(2));
+            Assert.That(stateChangedFired, Is.EqualTo(3));
 
             addArbitraryChange(beatmap);
             handler.SaveState();
@@ -84,20 +119,24 @@ public void TestSaveSameStateDoesNotSave()
             Assert.That(handler.CanUndo.Value, Is.False);
             Assert.That(handler.CanRedo.Value, Is.False);
 
+            // Save initial state
+            handler.SaveState();
+            Assert.That(stateChangedFired, Is.EqualTo(1));
+
             addArbitraryChange(beatmap);
             handler.SaveState();
 
             Assert.That(handler.CanUndo.Value, Is.True);
             Assert.That(handler.CanRedo.Value, Is.False);
-            Assert.That(stateChangedFired, Is.EqualTo(1));
+            Assert.That(stateChangedFired, Is.EqualTo(2));
 
             string hash = handler.CurrentStateHash;
 
             // save a save without making any changes
             handler.SaveState();
 
             Assert.That(hash, Is.EqualTo(handler.CurrentStateHash));
-            Assert.That(stateChangedFired, Is.EqualTo(1));
+            Assert.That(stateChangedFired, Is.EqualTo(2));
 
             handler.RestoreState(-1);
 
@@ -106,19 +145,23 @@ public void TestSaveSameStateDoesNotSave()
             // we should only be able to restore once even though we saved twice.
             Assert.That(handler.CanUndo.Value, Is.False);
             Assert.That(handler.CanRedo.Value, Is.True);
-            Assert.That(stateChangedFired, Is.EqualTo(2));
+            Assert.That(stateChangedFired, Is.EqualTo(3));
         }
 
         [Test]
         public void TestMaxStatesSaved()
         {
             var (handler, beatmap) = createChangeHandler();
 
+            // Save initial state
+            handler.SaveState();
+            Assert.That(stateChangedFired, Is.EqualTo(1));
+
             Assert.That(handler.CanUndo.Value, Is.False);
 
             for (int i = 0; i < EditorChangeHandler.MAX_SAVED_STATES; i++)
             {
-                Assert.That(stateChangedFired, Is.EqualTo(i));
+                Assert.That(stateChangedFired, Is.EqualTo(i + 1));
 
                 addArbitraryChange(beatmap);
                 handler.SaveState();
@@ -169,7 +212,7 @@ public void TestMaxStatesExceeded()
                 },
             });
 
-            var changeHandler = new EditorChangeHandler(beatmap);
+            var changeHandler = new BeatmapEditorChangeHandler(beatmap);
 
             changeHandler.OnStateChange += () => stateChangedFired++;
             return (changeHandler, beatmap);

osu.Game/Extensions/DrawableExtensions.cs

@@ -66,10 +66,16 @@ public static void ApplySkinnableInfo(this Drawable component, SkinnableInfo inf
 
                 foreach (var (_, property) in component.GetSettingsSourceProperties())
                 {
+                    var bindable = ((IBindable)property.GetValue(component)!);
+
                     if (!info.Settings.TryGetValue(property.Name.ToSnakeCase(), out object? settingValue))
+                    {
+                        // TODO: We probably want to restore default if not included in serialisation information.
+                        // This is not simple to do as SetDefault() is only found in the typed Bindable<T> interface right now.
                         continue;
+                    }
 
-                    skinnable.CopyAdjustedSetting(((IBindable)property.GetValue(component)!), settingValue);
+                    skinnable.CopyAdjustedSetting(bindable, settingValue);
                 }
             }
 

osu.Game/Overlays/SkinEditor/SkinEditor.cs

@@ -24,14 +24,15 @@
 using osu.Game.Graphics.UserInterface;
 using osu.Game.Localisation;
 using osu.Game.Overlays.OSD;
+using osu.Game.Screens.Edit;
 using osu.Game.Screens.Edit.Components;
 using osu.Game.Screens.Edit.Components.Menus;
 using osu.Game.Skinning;
 
 namespace osu.Game.Overlays.SkinEditor
 {
     [Cached(typeof(SkinEditor))]
-    public partial class SkinEditor : VisibilityContainer, ICanAcceptFiles, IKeyBindingHandler<PlatformAction>
+    public partial class SkinEditor : VisibilityContainer, ICanAcceptFiles, IKeyBindingHandler<PlatformAction>, IEditorChangeHandler
     {
         public const double TRANSITION_DURATION = 300;
 
@@ -72,6 +73,11 @@ public partial class SkinEditor : VisibilityContainer, ICanAcceptFiles, IKeyBind
         private EditorSidebar componentsSidebar = null!;
         private EditorSidebar settingsSidebar = null!;
 
+        private SkinEditorChangeHandler? changeHandler;
+
+        private EditorMenuItem undoMenuItem = null!;
+        private EditorMenuItem redoMenuItem = null!;
+
         [Resolved]
         private OnScreenDisplay? onScreenDisplay { get; set; }
 
@@ -131,6 +137,14 @@ private void load()
                                                     new EditorMenuItem(CommonStrings.Exit, MenuItemType.Standard, () => skinEditorOverlay?.Hide()),
                                                 },
                                             },
+                                            new MenuItem(CommonStrings.MenuBarEdit)
+                                            {
+                                                Items = new[]
+                                                {
+                                                    undoMenuItem = new EditorMenuItem(CommonStrings.Undo, MenuItemType.Standard, Undo),
+                                                    redoMenuItem = new EditorMenuItem(CommonStrings.Redo, MenuItemType.Standard, Redo),
+                                                }
+                                            },
                                         }
                                     },
                                     headerText = new OsuTextFlowContainer
@@ -210,6 +224,14 @@ public bool OnPressed(KeyBindingPressEvent<PlatformAction> e)
         {
             switch (e.Action)
             {
+                case PlatformAction.Undo:
+                    Undo();
+                    return true;
+
+                case PlatformAction.Redo:
+                    Redo();
+                    return true;
+
                 case PlatformAction.Save:
                     if (e.Repeat)
                         return false;
@@ -229,6 +251,8 @@ public void UpdateTargetScreen(Drawable targetScreen)
         {
             this.targetScreen = targetScreen;
 
+            changeHandler?.Dispose();
+
             SelectedComponents.Clear();
 
             // Immediately clear the previous blueprint container to ensure it doesn't try to interact with the old target.
@@ -241,6 +265,10 @@ void loadBlueprintContainer()
             {
                 Debug.Assert(content != null);
 
+                changeHandler = new SkinEditorChangeHandler(targetScreen);
+                changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true);
+                changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true);
+
                 content.Child = new SkinBlueprintContainer(targetScreen);
 
                 componentsSidebar.Child = new SkinComponentToolbox(getFirstTarget() as CompositeDrawable)
@@ -333,6 +361,10 @@ private void revert()
             }
         }
 
+        protected void Undo() => changeHandler?.RestoreState(-1);
+
+        protected void Redo() => changeHandler?.RestoreState(1);
+
         public void Save(bool userTriggered = true)
         {
             if (!hasBegunMutating)
@@ -436,5 +468,27 @@ public SkinEditorToast(LocalisableString value, string skinDisplayName)
             {
             }
         }
+
+        #region Delegation of IEditorChangeHandler
+
+        public event Action? OnStateChange
+        {
+            add => throw new NotImplementedException();
+            remove => throw new NotImplementedException();
+        }
+
+        private IEditorChangeHandler? beginChangeHandler;
+
+        public void BeginChange()
+        {
+            // Change handler may change between begin and end, which can cause unbalanced operations.
+            // Let's track the one that was used when beginning the change so we can call EndChange on it specifically.
+            (beginChangeHandler = changeHandler)?.BeginChange();
+        }
+
+        public void EndChange() => beginChangeHandler?.EndChange();
+        public void SaveState() => changeHandler?.SaveState();
+
+        #endregion
     }
 }

osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs

@@ -0,0 +1,78 @@
+// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using Newtonsoft.Json;
+using osu.Framework.Bindables;
+using osu.Framework.Graphics;
+using osu.Framework.Testing;
+using osu.Game.Extensions;
+using osu.Game.Screens.Edit;
+using osu.Game.Screens.Play.HUD;
+using osu.Game.Skinning;
+
+namespace osu.Game.Overlays.SkinEditor
+{
+    public partial class SkinEditorChangeHandler : EditorChangeHandler
+    {
+        private readonly ISkinnableTarget? firstTarget;
+
+        // ReSharper disable once PrivateFieldCanBeConvertedToLocalVariable
+        private readonly BindableList<ISkinnableDrawable>? components;
+
+        public SkinEditorChangeHandler(Drawable targetScreen)
+        {
+            // To keep things simple, we are currently only handling the current target screen for undo / redo.
+            // In the future we'll want this to cover all changes, even to skin's `InstantiationInfo`.
+            // We'll also need to consider cases where multiple targets are on screen at the same time.
+
+            firstTarget = targetScreen.ChildrenOfType<ISkinnableTarget>().FirstOrDefault();
+
+            if (firstTarget == null)
+                return;
+
+            components = new BindableList<ISkinnableDrawable> { BindTarget = firstTarget.Components };
+            components.BindCollectionChanged((_, _) => SaveState());
+        }
+
+        protected override void WriteCurrentStateToStream(MemoryStream stream)
+        {
+            if (firstTarget == null)
+                return;
+
+            var skinnableInfos = firstTarget.CreateSkinnableInfo().ToArray();
+            string json = JsonConvert.SerializeObject(skinnableInfos, new JsonSerializerSettings { Formatting = Formatting.Indented });
+            stream.Write(Encoding.UTF8.GetBytes(json));
+        }
+
+        protected override void ApplyStateChange(byte[] previousState, byte[] newState)
+        {
+            if (firstTarget == null)
+                return;
+
+            var deserializedContent = JsonConvert.DeserializeObject<IEnumerable<SkinnableInfo>>(Encoding.UTF8.GetString(newState));
+
+            if (deserializedContent == null)
+                return;
+
+            SkinnableInfo[] skinnableInfo = deserializedContent.ToArray();
+            Drawable[] targetComponents = firstTarget.Components.OfType<Drawable>().ToArray();
+
+            if (!skinnableInfo.Select(s => s.Type).SequenceEqual(targetComponents.Select(d => d.GetType())))
+            {
+                // Perform a naive full reload for now.
+                firstTarget.Reload(skinnableInfo);
+            }
+            else
+            {
+                int i = 0;
+
+                foreach (var drawable in targetComponents)
+                    drawable.ApplySkinnableInfo(skinnableInfo[i++]);
+            }
+        }
+    }
+}