From a77db5d837bb41f57b533c7dff1b893eaa5148a1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Jan 2024 13:36:50 +0900 Subject: [PATCH 1/8] Add failing test coverage of editor metadata not saving --- .../Editing/TestSceneMetadataSection.cs | 105 ++++++++++++++++-- 1 file changed, 94 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs index a9f8e19e30..f767d9f7a3 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs @@ -3,17 +3,22 @@ #nullable disable +using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; +using osu.Framework.Graphics.UserInterface; +using osu.Framework.Input; +using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Graphics.UserInterfaceV2; using osu.Game.Rulesets.Osu; using osu.Game.Screens.Edit; using osu.Game.Screens.Edit.Setup; +using osuTK.Input; namespace osu.Game.Tests.Visual.Editing { - public partial class TestSceneMetadataSection : OsuTestScene + public partial class TestSceneMetadataSection : OsuManualInputManagerTestScene { [Cached] private EditorBeatmap editorBeatmap = new EditorBeatmap(new Beatmap @@ -26,6 +31,81 @@ namespace osu.Game.Tests.Visual.Editing private TestMetadataSection metadataSection; + [Test] + public void TestUpdateViaTextBoxOnFocusLoss() + { + AddStep("set metadata", () => + { + editorBeatmap.Metadata.Artist = "Example Artist"; + editorBeatmap.Metadata.ArtistUnicode = string.Empty; + }); + + createSection(); + + TextBox textbox = null!; + + AddStep("focus first textbox", () => + { + textbox = metadataSection.ChildrenOfType().First(); + InputManager.MoveMouseTo(textbox); + InputManager.Click(MouseButton.Left); + }); + + AddStep("simulate changing textbox", () => + { + // Can't simulate text input but this should work. + InputManager.Keys(PlatformAction.SelectAll); + InputManager.Keys(PlatformAction.Copy); + InputManager.Keys(PlatformAction.Paste); + InputManager.Keys(PlatformAction.Paste); + }); + + assertArtistMetadata("Example Artist"); + + // It's important values are committed immediately on focus loss so the editor exit sequence detects them. + AddAssert("value immediately changed on focus loss", () => + { + InputManager.TriggerFocusContention(metadataSection); + return editorBeatmap.Metadata.Artist; + }, () => Is.EqualTo("Example ArtistExample Artist")); + } + + [Test] + public void TestUpdateViaTextBoxOnCommit() + { + AddStep("set metadata", () => + { + editorBeatmap.Metadata.Artist = "Example Artist"; + editorBeatmap.Metadata.ArtistUnicode = string.Empty; + }); + + createSection(); + + TextBox textbox = null!; + + AddStep("focus first textbox", () => + { + textbox = metadataSection.ChildrenOfType().First(); + InputManager.MoveMouseTo(textbox); + InputManager.Click(MouseButton.Left); + }); + + AddStep("simulate changing textbox", () => + { + // Can't simulate text input but this should work. + InputManager.Keys(PlatformAction.SelectAll); + InputManager.Keys(PlatformAction.Copy); + InputManager.Keys(PlatformAction.Paste); + InputManager.Keys(PlatformAction.Paste); + }); + + assertArtistMetadata("Example Artist"); + + AddStep("commit", () => InputManager.Key(Key.Enter)); + + assertArtistMetadata("Example ArtistExample Artist"); + } + [Test] public void TestMinimalMetadata() { @@ -40,7 +120,7 @@ namespace osu.Game.Tests.Visual.Editing createSection(); - assertArtist("Example Artist"); + assertArtistTextBox("Example Artist"); assertRomanisedArtist("Example Artist", false); assertTitle("Example Title"); @@ -61,7 +141,7 @@ namespace osu.Game.Tests.Visual.Editing createSection(); - assertArtist("＊なみりん"); + assertArtistTextBox("＊なみりん"); assertRomanisedArtist(string.Empty, true); assertTitle("コイシテイク・プラネット"); @@ -82,7 +162,7 @@ namespace osu.Game.Tests.Visual.Editing createSection(); - assertArtist("＊なみりん"); + assertArtistTextBox("＊なみりん"); assertRomanisedArtist("*namirin", true); assertTitle("コイシテイク・プラネット"); @@ -104,11 +184,11 @@ namespace osu.Game.Tests.Visual.Editing createSection(); AddStep("set romanised artist name", () => metadataSection.ArtistTextBox.Current.Value = "*namirin"); - assertArtist("*namirin"); + assertArtistTextBox("*namirin"); assertRomanisedArtist("*namirin", false); AddStep("set native artist name", () => metadataSection.ArtistTextBox.Current.Value = "＊なみりん"); - assertArtist("＊なみりん"); + assertArtistTextBox("＊なみりん"); assertRomanisedArtist("*namirin", true); AddStep("set romanised title", () => metadataSection.TitleTextBox.Current.Value = "Hitokoto no kyori"); @@ -123,21 +203,24 @@ namespace osu.Game.Tests.Visual.Editing private void createSection() => AddStep("create metadata section", () => Child = metadataSection = new TestMetadataSection()); - private void assertArtist(string expected) - => AddAssert($"artist is {expected}", () => metadataSection.ArtistTextBox.Current.Value == expected); + private void assertArtistMetadata(string expected) + => AddAssert($"artist metadata is {expected}", () => editorBeatmap.Metadata.Artist, () => Is.EqualTo(expected)); + + private void assertArtistTextBox(string expected) + => AddAssert($"artist textbox is {expected}", () => metadataSection.ArtistTextBox.Current.Value, () => Is.EqualTo(expected)); private void assertRomanisedArtist(string expected, bool editable) { - AddAssert($"romanised artist is {expected}", () => metadataSection.RomanisedArtistTextBox.Current.Value == expected); + AddAssert($"romanised artist is {expected}", () => metadataSection.RomanisedArtistTextBox.Current.Value, () => Is.EqualTo(expected)); AddAssert($"romanised artist is {(editable ? "" : "not ")}editable", () => metadataSection.RomanisedArtistTextBox.ReadOnly == !editable); } private void assertTitle(string expected) - => AddAssert($"title is {expected}", () => metadataSection.TitleTextBox.Current.Value == expected); + => AddAssert($"title is {expected}", () => metadataSection.TitleTextBox.Current.Value, () => Is.EqualTo(expected)); private void assertRomanisedTitle(string expected, bool editable) { - AddAssert($"romanised title is {expected}", () => metadataSection.RomanisedTitleTextBox.Current.Value == expected); + AddAssert($"romanised title is {expected}", () => metadataSection.RomanisedTitleTextBox.Current.Value, () => Is.EqualTo(expected)); AddAssert($"romanised title is {(editable ? "" : "not ")}editable", () => metadataSection.RomanisedTitleTextBox.ReadOnly == !editable); } From e54502eef1bd0d99e3a3b5c29476b0addc7a5c13 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Jan 2024 13:59:28 +0900 Subject: [PATCH 2/8] Add full editor save test coverage --- .../TestSceneBeatmapEditorNavigation.cs | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs index 9930349b1b..370c40222e 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs @@ -7,6 +7,7 @@ using osu.Framework.Extensions; using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics.UserInterface; +using osu.Framework.Input; using osu.Framework.Screens; using osu.Framework.Testing; using osu.Game.Beatmaps; @@ -17,6 +18,7 @@ using osu.Game.Rulesets.Mania; using osu.Game.Rulesets.Osu; using osu.Game.Screens.Edit; using osu.Game.Screens.Edit.GameplayTest; +using osu.Game.Screens.Edit.Setup; using osu.Game.Screens.Menu; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Filter; @@ -27,6 +29,59 @@ namespace osu.Game.Tests.Visual.Navigation { public partial class TestSceneBeatmapEditorNavigation : OsuGameTestScene { + [Test] + public void TestChangeMetadataExitWhileTextboxFocusedPromptsSave() + { + BeatmapSetInfo beatmapSet = null!; + + AddStep("import test beatmap", () => Game.BeatmapManager.Import(TestResources.GetTestBeatmapForImport()).WaitSafely()); + AddStep("retrieve beatmap", () => beatmapSet = Game.BeatmapManager.QueryBeatmapSet(set => !set.Protected).AsNonNull().Value.Detach()); + + AddStep("present beatmap", () => Game.PresentBeatmap(beatmapSet)); + AddUntilStep("wait for song select", + () => Game.Beatmap.Value.BeatmapSetInfo.Equals(beatmapSet) + && Game.ScreenStack.CurrentScreen is PlaySongSelect songSelect + && songSelect.IsLoaded); + AddStep("switch ruleset", () => Game.Ruleset.Value = new ManiaRuleset().RulesetInfo); + + AddStep("open editor", () => ((PlaySongSelect)Game.ScreenStack.CurrentScreen).Edit(beatmapSet.Beatmaps.First(beatmap => beatmap.Ruleset.OnlineID == 0))); + AddUntilStep("wait for editor open", () => Game.ScreenStack.CurrentScreen is Editor editor && editor.ReadyForUse); + + AddStep("change to song setup", () => InputManager.Key(Key.F4)); + + TextBox textbox = null!; + + AddUntilStep("wait for metadata section", () => + { + var t = Game.ChildrenOfType().SingleOrDefault().ChildrenOfType().FirstOrDefault(); + + if (t == null) + return false; + + textbox = t; + return true; + }); + + AddStep("focus textbox", () => + { + InputManager.MoveMouseTo(textbox); + InputManager.Click(MouseButton.Left); + }); + + AddStep("simulate changing textbox", () => + { + // Can't simulate text input but this should work. + InputManager.Keys(PlatformAction.SelectAll); + InputManager.Keys(PlatformAction.Copy); + InputManager.Keys(PlatformAction.Paste); + InputManager.Keys(PlatformAction.Paste); + }); + + AddStep("exit", () => Game.ChildrenOfType().Single().Exit()); + + AddAssert("save dialog displayed", () => Game.ChildrenOfType().Single().CurrentDialog is PromptForSaveDialog); + } + [Test] public void TestEditorGameplayTestAlwaysUsesOriginalRuleset() { From 45f2980dc099ba8c421395dd4258cdf3df4a2e55 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Jan 2024 13:23:02 +0900 Subject: [PATCH 3/8] Fix potential editor data loss if exiting while a textbox is focused --- osu.Game/Screens/Edit/Editor.cs | 6 ++++++ osu.Game/Screens/Edit/Setup/MetadataSection.cs | 10 +++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index c1f6c02301..bc376f6165 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -719,6 +719,12 @@ namespace osu.Game.Screens.Edit public override bool OnExiting(ScreenExitEvent e) { + // Before exiting, trigger a focus loss. + // + // This is important to ensure that if the user is still editing a textbox, it will commit + // (and potentially block the exit procedure for save). + GetContainingInputManager().TriggerFocusContention(this); + if (!ExitConfirmed) { // dialog overlay may not be available in visual tests. diff --git a/osu.Game/Screens/Edit/Setup/MetadataSection.cs b/osu.Game/Screens/Edit/Setup/MetadataSection.cs index 752f590308..b51c03b796 100644 --- a/osu.Game/Screens/Edit/Setup/MetadataSection.cs +++ b/osu.Game/Screens/Edit/Setup/MetadataSection.cs @@ -53,9 +53,6 @@ namespace osu.Game.Screens.Edit.Setup sourceTextBox = createTextBox(BeatmapsetsStrings.ShowInfoSource, metadata.Source), tagsTextBox = createTextBox(BeatmapsetsStrings.ShowInfoTags, metadata.Tags) }; - - foreach (var item in Children.OfType()) - item.OnCommit += onCommit; } private TTextBox createTextBox(LocalisableString label, string initialValue) @@ -77,6 +74,10 @@ namespace osu.Game.Screens.Edit.Setup ArtistTextBox.Current.BindValueChanged(artist => transferIfRomanised(artist.NewValue, RomanisedArtistTextBox)); TitleTextBox.Current.BindValueChanged(title => transferIfRomanised(title.NewValue, RomanisedTitleTextBox)); + + foreach (var item in Children.OfType()) + item.OnCommit += onCommit; + updateReadOnlyState(); } @@ -86,7 +87,6 @@ namespace osu.Game.Screens.Edit.Setup target.Current.Value = value; updateReadOnlyState(); - Scheduler.AddOnce(updateMetadata); } private void updateReadOnlyState() @@ -101,7 +101,7 @@ namespace osu.Game.Screens.Edit.Setup // for now, update on commit rather than making BeatmapMetadata bindables. // after switching database engines we can reconsider if switching to bindables is a good direction. - Scheduler.AddOnce(updateMetadata); + updateMetadata(); } private void updateMetadata() From fe0433e6ecd1a69132fee41626562590c4b688ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 2 Feb 2024 13:24:59 +0100 Subject: [PATCH 4/8] Remove redundant default value assignments --- osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs index f767d9f7a3..5930c077a4 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs @@ -42,7 +42,7 @@ namespace osu.Game.Tests.Visual.Editing createSection(); - TextBox textbox = null!; + TextBox textbox; AddStep("focus first textbox", () => { @@ -81,7 +81,7 @@ namespace osu.Game.Tests.Visual.Editing createSection(); - TextBox textbox = null!; + TextBox textbox; AddStep("focus first textbox", () => { From dbd4397bef2419ea4d9b1b8fdc44068358d59159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 2 Feb 2024 13:32:16 +0100 Subject: [PATCH 5/8] Attempt to salvage test by using until step --- .../Visual/Navigation/TestSceneBeatmapEditorNavigation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs index 370c40222e..e3a8e575f8 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneBeatmapEditorNavigation.cs @@ -79,7 +79,7 @@ namespace osu.Game.Tests.Visual.Navigation AddStep("exit", () => Game.ChildrenOfType().Single().Exit()); - AddAssert("save dialog displayed", () => Game.ChildrenOfType().Single().CurrentDialog is PromptForSaveDialog); + AddUntilStep("save dialog displayed", () => Game.ChildrenOfType().SingleOrDefault()?.CurrentDialog is PromptForSaveDialog); } [Test] From a80dbba9d0d4fc836b899b1825692170c1c843d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 24 May 2024 10:21:40 +0200 Subject: [PATCH 6/8] Update to not use obsoleted method --- osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs | 2 +- osu.Game/Screens/Edit/Editor.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs index 5930c077a4..8b6f31d599 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneMetadataSection.cs @@ -65,7 +65,7 @@ namespace osu.Game.Tests.Visual.Editing // It's important values are committed immediately on focus loss so the editor exit sequence detects them. AddAssert("value immediately changed on focus loss", () => { - InputManager.TriggerFocusContention(metadataSection); + ((IFocusManager)InputManager).TriggerFocusContention(metadataSection); return editorBeatmap.Metadata.Artist; }, () => Is.EqualTo("Example ArtistExample Artist")); } diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 47b2a53607..55ab03a590 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -723,7 +723,7 @@ namespace osu.Game.Screens.Edit // // This is important to ensure that if the user is still editing a textbox, it will commit // (and potentially block the exit procedure for save). - GetContainingInputManager().TriggerFocusContention(this); + GetContainingFocusManager().TriggerFocusContention(this); if (!ExitConfirmed) { From 807d982a721ec043990d1871f41316763d3b5b15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 24 May 2024 10:24:50 +0200 Subject: [PATCH 7/8] Move workaround to subscreen --- osu.Game/Screens/Edit/Editor.cs | 6 +----- osu.Game/Screens/Edit/EditorScreen.cs | 5 +++++ osu.Game/Screens/Edit/Setup/SetupScreen.cs | 12 ++++++++++++ 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 55ab03a590..07c32983f5 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -719,11 +719,7 @@ namespace osu.Game.Screens.Edit public override bool OnExiting(ScreenExitEvent e) { - // Before exiting, trigger a focus loss. - // - // This is important to ensure that if the user is still editing a textbox, it will commit - // (and potentially block the exit procedure for save). - GetContainingFocusManager().TriggerFocusContention(this); + currentScreen?.OnExiting(e); if (!ExitConfirmed) { diff --git a/osu.Game/Screens/Edit/EditorScreen.cs b/osu.Game/Screens/Edit/EditorScreen.cs index 3bc870b898..a795b310a2 100644 --- a/osu.Game/Screens/Edit/EditorScreen.cs +++ b/osu.Game/Screens/Edit/EditorScreen.cs @@ -6,6 +6,7 @@ using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Cursor; +using osu.Framework.Screens; namespace osu.Game.Screens.Edit { @@ -37,6 +38,10 @@ namespace osu.Game.Screens.Edit protected override void PopOut() => this.FadeOut(); + public virtual void OnExiting(ScreenExitEvent e) + { + } + #region Clipboard operations public BindableBool CanCut { get; } = new BindableBool(); diff --git a/osu.Game/Screens/Edit/Setup/SetupScreen.cs b/osu.Game/Screens/Edit/Setup/SetupScreen.cs index 266ea1f929..5345db0a4f 100644 --- a/osu.Game/Screens/Edit/Setup/SetupScreen.cs +++ b/osu.Game/Screens/Edit/Setup/SetupScreen.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Shapes; +using osu.Framework.Screens; using osu.Game.Graphics.Containers; using osu.Game.Overlays; @@ -55,6 +56,17 @@ namespace osu.Game.Screens.Edit.Setup })); } + public override void OnExiting(ScreenExitEvent e) + { + base.OnExiting(e); + + // Before exiting, trigger a focus loss. + // + // This is important to ensure that if the user is still editing a textbox, it will commit + // (and potentially block the exit procedure for save). + GetContainingFocusManager().TriggerFocusContention(this); + } + private partial class SetupScreenSectionsContainer : SectionsContainer { protected override UserTrackingScrollContainer CreateScrollContainer() From 7255cc3344e75ec0e0eb1fd99eab94da1f50f784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 24 May 2024 11:25:29 +0200 Subject: [PATCH 8/8] Fix tests dying on a nullref --- osu.Game/Screens/Edit/Setup/SetupScreen.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Setup/SetupScreen.cs b/osu.Game/Screens/Edit/Setup/SetupScreen.cs index 5345db0a4f..7a7907d08a 100644 --- a/osu.Game/Screens/Edit/Setup/SetupScreen.cs +++ b/osu.Game/Screens/Edit/Setup/SetupScreen.cs @@ -64,7 +64,7 @@ namespace osu.Game.Screens.Edit.Setup // // This is important to ensure that if the user is still editing a textbox, it will commit // (and potentially block the exit procedure for save). - GetContainingFocusManager().TriggerFocusContention(this); + GetContainingFocusManager()?.TriggerFocusContention(this); } private partial class SetupScreenSectionsContainer : SectionsContainer