Merge pull request #30060 from peppy/fix-skin-editor-undo

Fix initial skin state being stored wrong to undo history

osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs

@@ -5,6 +5,7 @@
 
 using System;
 using System.Linq;
+using Newtonsoft.Json;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions;
@@ -102,6 +103,77 @@ namespace osu.Game.Tests.Visual.Navigation
             AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get<SkinManager>().CurrentSkin.Value.SkinInfo.Value.Protected);
         }
 
+        [Test]
+        public void TestMutateProtectedSkinFromMainMenu_UndoToInitialStateIsCorrect()
+        {
+            AddStep("set default skin", () => Game.Dependencies.Get<SkinManager>().CurrentSkinInfo.SetDefault());
+            AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely());
+
+            openSkinEditor();
+            AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get<SkinManager>().CurrentSkin.Value.SkinInfo.Value.Protected);
+
+            AddUntilStep("wait for player", () =>
+            {
+                DismissAnyNotifications();
+                return Game.ScreenStack.CurrentScreen is Player;
+            });
+
+            string state = string.Empty;
+
+            AddUntilStep("wait for accuracy counter", () => Game.ChildrenOfType<ArgonAccuracyCounter>().Any(counter => counter.Position != new Vector2()));
+            AddStep("dump state of accuracy meter", () => state = JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()));
+            AddStep("add any component", () => Game.ChildrenOfType<SkinComponentToolbox.ToolboxComponentButton>().First().TriggerClick());
+            AddStep("undo", () =>
+            {
+                InputManager.PressKey(Key.ControlLeft);
+                InputManager.Key(Key.Z);
+                InputManager.ReleaseKey(Key.ControlLeft);
+            });
+            AddUntilStep("only one accuracy meter left",
+                () => Game.ChildrenOfType<Player>().Single().ChildrenOfType<ArgonAccuracyCounter>().Count(),
+                () => Is.EqualTo(1));
+            AddAssert("accuracy meter state unchanged",
+                () => JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()),
+                () => Is.EqualTo(state));
+        }
+
+        [Test]
+        public void TestMutateProtectedSkinFromPlayer_UndoToInitialStateIsCorrect()
+        {
+            AddStep("set default skin", () => Game.Dependencies.Get<SkinManager>().CurrentSkinInfo.SetDefault());
+            AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely());
+            advanceToSongSelect();
+            AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault);
+
+            AddStep("enable NF", () => Game.SelectedMods.Value = new[] { new OsuModNoFail() });
+            AddStep("enter gameplay", () => InputManager.Key(Key.Enter));
+
+            AddUntilStep("wait for player", () =>
+            {
+                DismissAnyNotifications();
+                return Game.ScreenStack.CurrentScreen is Player;
+            });
+            openSkinEditor();
+
+            string state = string.Empty;
+
+            AddUntilStep("wait for accuracy counter", () => Game.ChildrenOfType<ArgonAccuracyCounter>().Any(counter => counter.Position != new Vector2()));
+            AddStep("dump state of accuracy meter", () => state = JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()));
+            AddStep("add any component", () => Game.ChildrenOfType<SkinComponentToolbox.ToolboxComponentButton>().First().TriggerClick());
+            AddStep("undo", () =>
+            {
+                InputManager.PressKey(Key.ControlLeft);
+                InputManager.Key(Key.Z);
+                InputManager.ReleaseKey(Key.ControlLeft);
+            });
+            AddUntilStep("only one accuracy meter left",
+                () => Game.ChildrenOfType<Player>().Single().ChildrenOfType<ArgonAccuracyCounter>().Count(),
+                () => Is.EqualTo(1));
+            AddAssert("accuracy meter state unchanged",
+                () => JsonConvert.SerializeObject(Game.ChildrenOfType<ArgonAccuracyCounter>().First().CreateSerialisedInfo()),
+                () => Is.EqualTo(state));
+        }
+
         [Test]
         public void TestComponentsDeselectedOnSkinEditorHide()
         {

osu.Game/Overlays/SkinEditor/SkinEditor.cs

@@ -374,9 +374,10 @@ namespace osu.Game.Overlays.SkinEditor
                 return;
             }
 
-            changeHandler = new SkinEditorChangeHandler(skinComponentsContainer);
+            if (skinComponentsContainer.IsLoaded)
-            changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true);
+                bindChangeHandler(skinComponentsContainer);
-            changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true);
+            else
+                skinComponentsContainer.OnLoadComplete += d => Schedule(() => bindChangeHandler((SkinnableContainer)d));
 
             content.Child = new SkinBlueprintContainer(skinComponentsContainer);
 
@@ -418,10 +419,21 @@ namespace osu.Game.Overlays.SkinEditor
                 SelectedComponents.Clear();
                 placeComponent(component);
             }
+
+            void bindChangeHandler(SkinnableContainer skinnableContainer)
+            {
+                changeHandler = new SkinEditorChangeHandler(skinnableContainer);
+                changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true);
+                changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true);
+            }
         }
 
         private void skinChanged()
         {
+            if (skins.EnsureMutableSkin())
+                // Another skin changed event will arrive which will complete the process.
+                return;
+
             headerText.Clear();
 
             headerText.AddParagraph(SkinEditorStrings.SkinEditor, cp => cp.Font = OsuFont.Default.With(size: 16));
@@ -439,17 +451,24 @@ namespace osu.Game.Overlays.SkinEditor
             });
 
             changeHandler?.Dispose();
+            changeHandler = null;
 
-            skins.EnsureMutableSkin();
+            // Schedule is required to ensure that all layout in `LoadComplete` methods has been completed
-
+            // before storing an undo state.
+            //
+            // See https://github.com/ppy/osu/blob/8e6a4559e3ae8c9892866cf9cf8d4e8d1b72afd0/osu.Game/Skinning/SkinReloadableDrawable.cs#L76.
+            Schedule(() =>
+            {
                 var targetContainer = getTarget(selectedTarget.Value);
 
                 if (targetContainer != null)
                     changeHandler = new SkinEditorChangeHandler(targetContainer);
+
                 hasBegunMutating = true;
 
                 // Reload sidebar components.
                 selectedTarget.TriggerChange();
+            });
         }
 
         /// <summary>

osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs

@@ -34,7 +34,7 @@ namespace osu.Game.Overlays.SkinEditor
                 return;
 
             components = new BindableList<ISerialisableDrawable> { BindTarget = firstTarget.Components };
-            components.BindCollectionChanged((_, _) => SaveState());
+            components.BindCollectionChanged((_, _) => SaveState(), true);
         }
 
         protected override void WriteCurrentStateToStream(MemoryStream stream)