Merge pull request #18093 from peppy/dialog-overlay-thread-woes

Fix dialogs pushed to `DialogOverlay` too early potentially doing cross-thread transforms

osu.Game.Tests/Visual/UserInterface/TestSceneDialogOverlay.cs

@@ -1,10 +1,12 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
+using System.Threading;
 using NUnit.Framework;
+using osu.Framework.Allocation;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Sprites;
-using osu.Framework.Testing;
 using osu.Game.Overlays;
 using osu.Game.Overlays.Dialog;
 
@@ -15,15 +17,11 @@ namespace osu.Game.Tests.Visual.UserInterface
     {
         private DialogOverlay overlay;
 
-        [SetUpSteps]
-        public void SetUpSteps()
-        {
-            AddStep("create dialog overlay", () => Child = overlay = new DialogOverlay());
-        }
-
         [Test]
         public void TestBasic()
         {
+            AddStep("create dialog overlay", () => Child = overlay = new DialogOverlay());
+
             TestPopupDialog firstDialog = null;
             TestPopupDialog secondDialog = null;
 
@@ -37,12 +35,12 @@ namespace osu.Game.Tests.Visual.UserInterface
                     new PopupDialogOkButton
                     {
                         Text = @"I never want to see this again.",
-                        Action = () => System.Console.WriteLine(@"OK"),
+                        Action = () => Console.WriteLine(@"OK"),
                     },
                     new PopupDialogCancelButton
                     {
                         Text = @"Firetruck, I still want quick ranks!",
-                        Action = () => System.Console.WriteLine(@"Cancel"),
+                        Action = () => Console.WriteLine(@"Cancel"),
                     },
                 },
             }));
@@ -87,9 +85,49 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddAssert("first dialog is not part of hierarchy", () => firstDialog.Parent == null);
         }
 
+        [Test]
+        public void TestPushBeforeLoad()
+        {
+            PopupDialog dialog = null;
+
+            AddStep("create dialog overlay", () => overlay = new SlowLoadingDialogOverlay());
+
+            AddStep("start loading overlay", () => LoadComponentAsync(overlay, Add));
+
+            AddStep("push dialog before loaded", () =>
+            {
+                overlay.Push(dialog = new TestPopupDialog
+                {
+                    Buttons = new PopupDialogButton[]
+                    {
+                        new PopupDialogOkButton { Text = @"OK" },
+                    },
+                });
+            });
+
+            AddStep("complete load", () => ((SlowLoadingDialogOverlay)overlay).LoadEvent.Set());
+
+            AddUntilStep("wait for load", () => overlay.IsLoaded);
+
+            AddAssert("dialog displayed", () => overlay.CurrentDialog == dialog);
+        }
+
+        public class SlowLoadingDialogOverlay : DialogOverlay
+        {
+            public ManualResetEventSlim LoadEvent = new ManualResetEventSlim();
+
+            [BackgroundDependencyLoader]
+            private void load()
+            {
+                LoadEvent.Wait(10000);
+            }
+        }
+
         [Test]
         public void TestDismissBeforePush()
         {
+            AddStep("create dialog overlay", () => Child = overlay = new DialogOverlay());
+
             TestPopupDialog testDialog = null;
             AddStep("dismissed dialog push", () =>
             {
@@ -106,6 +144,8 @@ namespace osu.Game.Tests.Visual.UserInterface
         [Test]
         public void TestDismissBeforePushViaButtonPress()
         {
+            AddStep("create dialog overlay", () => Child = overlay = new DialogOverlay());
+
             TestPopupDialog testDialog = null;
             AddStep("dismissed dialog push", () =>
             {

osu.Game/Overlays/DialogOverlay.cs

@@ -49,18 +49,24 @@ namespace osu.Game.Overlays
         {
             if (dialog == CurrentDialog || dialog.State.Value != Visibility.Visible) return;
 
-            // if any existing dialog is being displayed, dismiss it before showing a new one.
+            var lastDialog = CurrentDialog;
-            CurrentDialog?.Hide();
 
+            // Immediately update the externally accessible property as this may be used for checks even before
+            // a DialogOverlay instance has finished loading.
             CurrentDialog = dialog;
-            CurrentDialog.State.ValueChanged += state => onDialogOnStateChanged(dialog, state.NewValue);
 
-            dialogContainer.Add(CurrentDialog);
+            Scheduler.Add(() =>
+            {
+                // if any existing dialog is being displayed, dismiss it before showing a new one.
+                lastDialog?.Hide();
+                dialog.State.ValueChanged += state => onDialogOnStateChanged(dialog, state.NewValue);
+                dialogContainer.Add(dialog);
 
                 Show();
+            }, false);
         }
 
-        public override bool IsPresent => dialogContainer.Children.Count > 0;
+        public override bool IsPresent => Scheduler.HasPendingTasks || dialogContainer.Children.Count > 0;
 
         protected override bool BlockNonPositionalInput => true;
 
@@ -81,23 +87,16 @@ namespace osu.Game.Overlays
         protected override void PopIn()
         {
             base.PopIn();
-            this.FadeIn(PopupDialog.ENTER_DURATION, Easing.OutQuint);
             lowPassFilter.CutoffTo(300, 100, Easing.OutCubic);
         }
 
         protected override void PopOut()
         {
             base.PopOut();
-
             lowPassFilter.CutoffTo(AudioFilter.MAX_LOWPASS_CUTOFF, 100, Easing.InCubic);
 
             if (CurrentDialog?.State.Value == Visibility.Visible)
-            {
                 CurrentDialog.Hide();
-                return;
-            }
-
-            this.FadeOut(PopupDialog.EXIT_DURATION, Easing.InSine);
         }
 
         public override bool OnPressed(KeyBindingPressEvent<GlobalAction> e)