Merge pull request #26717 from peppy/fix-editor-didnt-save

Fix editor not saving when textbox is focused during exit procedure

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;
+
+            AddStep("focus first textbox", () =>
+            {
+                textbox = metadataSection.ChildrenOfType<TextBox>().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", () =>
+            {
+                ((IFocusManager)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;
+
+            AddStep("focus first textbox", () =>
+            {
+                textbox = metadataSection.ChildrenOfType<TextBox>().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)
+        private void assertArtistMetadata(string expected)
-            => AddAssert($"artist is {expected}", () => metadataSection.ArtistTextBox.Current.Value == 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);
         }
 

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<MetadataSection>().SingleOrDefault().ChildrenOfType<TextBox>().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<Editor>().Single().Exit());
+
+            AddUntilStep("save dialog displayed", () => Game.ChildrenOfType<DialogOverlay>().SingleOrDefault()?.CurrentDialog is PromptForSaveDialog);
+        }
+
         [Test]
         public void TestEditorGameplayTestAlwaysUsesOriginalRuleset()
         {

osu.Game/Screens/Edit/Editor.cs

@@ -719,6 +719,8 @@ namespace osu.Game.Screens.Edit
 
         public override bool OnExiting(ScreenExitEvent e)
         {
+            currentScreen?.OnExiting(e);
+
             if (!ExitConfirmed)
             {
                 // dialog overlay may not be available in visual tests.

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();

osu.Game/Screens/Edit/Setup/MetadataSection.cs

@@ -53,9 +53,6 @@ namespace osu.Game.Screens.Edit.Setup
                 sourceTextBox = createTextBox<LabelledTextBox>(BeatmapsetsStrings.ShowInfoSource, metadata.Source),
                 tagsTextBox = createTextBox<LabelledTextBox>(BeatmapsetsStrings.ShowInfoTags, metadata.Tags)
             };
-
-            foreach (var item in Children.OfType<LabelledTextBox>())
-                item.OnCommit += onCommit;
         }
 
         private TTextBox createTextBox<TTextBox>(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<LabelledTextBox>())
+                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()

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<SetupSection>
         {
             protected override UserTrackingScrollContainer CreateScrollContainer()