From 16e69b08a161506d191ddf89e782928da79146d0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Sep 2024 19:52:51 +0900 Subject: [PATCH 1/5] Avoid unnecessarily handling two skin changed events when making mutable skin --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index 6f7781ee9c..eca8b7f1d2 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -401,6 +401,10 @@ namespace osu.Game.Overlays.SkinEditor 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)); From 1f2f4a533f8159b986f90538388845820a2c50b9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Sep 2024 19:53:06 +0900 Subject: [PATCH 2/5] Fix initial skin state being stored wrong to undo history --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index eca8b7f1d2..ec9931c673 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -422,17 +422,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); - var targetContainer = getTarget(selectedTarget.Value); + if (targetContainer != null) + changeHandler = new SkinEditorChangeHandler(targetContainer); - if (targetContainer != null) - changeHandler = new SkinEditorChangeHandler(targetContainer); - hasBegunMutating = true; + hasBegunMutating = true; - // Reload sidebar components. - selectedTarget.TriggerChange(); + // Reload sidebar components. + selectedTarget.TriggerChange(); + }); } /// From f84f6b78d9fdd4a1fda1a36c97cb4915981a3a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Oct 2024 13:48:29 +0200 Subject: [PATCH 3/5] Add failing test coverage of skin editor still not undoing correctly to initial state --- .../TestSceneSkinEditorNavigation.cs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs index 5267a57a05..8323aaeaf4 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs +++ b/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; @@ -23,6 +24,7 @@ using osu.Game.Rulesets.Osu; using osu.Game.Rulesets.Osu.Mods; using osu.Game.Screens.Edit.Components; using osu.Game.Screens.Play; +using osu.Game.Screens.Play.HUD; using osu.Game.Screens.Play.HUD.HitErrorMeters; using osu.Game.Skinning; using osu.Game.Tests.Beatmaps.IO; @@ -101,6 +103,77 @@ namespace osu.Game.Tests.Visual.Navigation AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get().CurrentSkin.Value.SkinInfo.Value.Protected); } + [Test] + public void TestMutateProtectedSkinFromMainMenu_UndoToInitialStateIsCorrect() + { + AddStep("set default skin", () => Game.Dependencies.Get().CurrentSkinInfo.SetDefault()); + AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely()); + + openSkinEditor(); + AddUntilStep("current skin is mutable", () => !Game.Dependencies.Get().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().Any(counter => counter.IsLoaded)); + AddStep("dump state of accuracy meter", () => state = JsonConvert.SerializeObject(Game.ChildrenOfType().First().CreateSerialisedInfo())); + AddStep("add any component", () => Game.ChildrenOfType().First().TriggerClick()); + AddStep("undo", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.Z); + InputManager.ReleaseKey(Key.ControlLeft); + }); + AddUntilStep("only one accuracy meter left", + () => Game.ChildrenOfType().Single().ChildrenOfType().Count(), + () => Is.EqualTo(1)); + AddAssert("accuracy meter state unchanged", + () => JsonConvert.SerializeObject(Game.ChildrenOfType().First().CreateSerialisedInfo()), + () => Is.EqualTo(state)); + } + + [Test] + public void TestMutateProtectedSkinFromPlayer_UndoToInitialStateIsCorrect() + { + AddStep("set default skin", () => Game.Dependencies.Get().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().Any(counter => counter.IsLoaded)); + AddStep("dump state of accuracy meter", () => state = JsonConvert.SerializeObject(Game.ChildrenOfType().First().CreateSerialisedInfo())); + AddStep("add any component", () => Game.ChildrenOfType().First().TriggerClick()); + AddStep("undo", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.Z); + InputManager.ReleaseKey(Key.ControlLeft); + }); + AddUntilStep("only one accuracy meter left", + () => Game.ChildrenOfType().Single().ChildrenOfType().Count(), + () => Is.EqualTo(1)); + AddAssert("accuracy meter state unchanged", + () => JsonConvert.SerializeObject(Game.ChildrenOfType().First().CreateSerialisedInfo()), + () => Is.EqualTo(state)); + } + [Test] public void TestComponentsDeselectedOnSkinEditorHide() { From 66ca7448436e7d66072343a1c4af950da3e0d385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Oct 2024 14:23:16 +0200 Subject: [PATCH 4/5] Fix `SkinEditorChangeHandler` not actually storing initial state --- osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs index 673ba873c4..b805e50df6 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorChangeHandler.cs @@ -34,7 +34,7 @@ namespace osu.Game.Overlays.SkinEditor return; components = new BindableList { BindTarget = firstTarget.Components }; - components.BindCollectionChanged((_, _) => SaveState()); + components.BindCollectionChanged((_, _) => SaveState(), true); } protected override void WriteCurrentStateToStream(MemoryStream stream) From 936677f56abd22328fc9450d3b529b87a672f440 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Oct 2024 14:47:29 +0200 Subject: [PATCH 5/5] Fix `SkinEditor` potentially initialising change handler while components are not loaded yet --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index ec9931c673..130684e289 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -353,9 +353,10 @@ namespace osu.Game.Overlays.SkinEditor return; } - changeHandler = new SkinEditorChangeHandler(skinComponentsContainer); - changeHandler.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true); - changeHandler.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true); + if (skinComponentsContainer.IsLoaded) + bindChangeHandler(skinComponentsContainer); + else + skinComponentsContainer.OnLoadComplete += d => Schedule(() => bindChangeHandler((SkinnableContainer)d)); content.Child = new SkinBlueprintContainer(skinComponentsContainer); @@ -397,6 +398,13 @@ 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()