Confirm exit dialogs after pressing home only go one screen back

[Joehuu]

Describe the bug: As noticed on stream and while fixing https://github.com/ppy/osu/issues/13609.

There is already a case in the editor. Making an issue instead because I'm not sure how to tackle it.

Screenshots or videos showing encountered issue:

https://user-images.githubusercontent.com/35318437/122879378-aeff9680-d2ed-11eb-99ff-89fdf64b68af.mp4

osu!lazer version: 2021.619.1

[bdach]

The prompt-for-save dialog logic from the editor is suppressing the exit sequence as initiated by the home button, and then only ever exiting one level up when the dialog is dismissed. For this to be resolved, that editor dialog logic would need to somehow be aware of whether it was being called in normal "go back a screen" scenario, rather than a "go to main menu directly" one - in other words, what the target screen is when the screen stack is being unwound. Seems like something that may require new ScreenStack/IScreen API.

[Joehuu]

https://github.com/Joehuu/osu-framework/commit/2ee0465c187175bb963cc3e44b02bd4b9fb0a6fc

Linking a commit to see if that is the correct path (adding a param to OnExiting). It'll require changes to all existing implementations though and might not even be used that much. Even the next param doesn't have any uses apart from the framework test scene.

As for the osu!-side changes, replace all the this.Exit() with destination.MakeCurrent() for the dialog overlay implementations.

[peppy]

Seems like it's a direction that could work, but not yet sure if we want to add that extra complexity.

If we are going to break the override methods, it's probably a good point to use EventArgs so we don't do so again.

[frenzibyte]

While I was reviewing https://github.com/ppy/osu/pull/17817, I noticed this issue could be alternatively solved by a simple PerformFromScreen call:

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index 73121f6e7d..080e92945d 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -762,11 +762,7 @@ protected override void LoadComplete()

             loadComponentSingleFile(Toolbar = new Toolbar
             {
-                OnHome = delegate
-                {
-                    CloseAllOverlays(false);
-                    menuScreen?.MakeCurrent();
-                },
+                OnHome = () => PerformFromScreen(_ => { }),
             }, topMostOverlayContent.Add);

             loadComponentSingleFile(volume = new VolumeOverlay(), leftFloatingOverlayContent.Add, true);

Which can be better cleaned up to a ReturnToScreenRunner, with PerformFromScreen only being an extension which invokes an action after reaching the target screen.

[peppy]

PerformFromScreen

That's precisely why I blocked the framework merging for the moment. I want to check whether that is a better path forward.

[Joehuu]

The alternative works as I've expected. Haven't thought of that.

There's another area I had in mind that this issue didn't cover which is using windows exit (cmd-q, alt-f4, X on window), and doing the alternative also works (can be written better I think, but just showing the line):

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index fb81e4fd14..3198e35743 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -1111,7 +1111,7 @@ protected override bool OnExiting()

             if (introScreen?.DidLoadMenu == true && !(ScreenStack.CurrentScreen is IntroScreen))
             {
-                Scheduler.Add(introScreen.MakeCurrent);
+                PerformFromScreen(_ => Scheduler.Add(introScreen.MakeCurrent));
                 return true;
             }

But, the exit flow regressed recently and I don't know the cause so I tested this on an outdated branch.

You can take credit on fixing.

Edit: the diff above needs a bit more thought as it doesn't work when spamming windows exit

[bdach]

But, the exit flow regressed recently and I don't know the cause so I tested this on an outdated branch.

@Joehuu what does this mean? is this tracked somewhere else?

[Joehuu]

No, but it's currently being fixed in https://github.com/ppy/osu/pull/17870.

[peppy]

As a follow-up, I do believe using the PerformFromScreen method is a better solution to this issue, as it is better tested and handles edge cases (ie. the case where the user aborts the operation at another screen).

[Joehuu]

Going back to this. With these changes using PerformFromScreen, https://github.com/ppy/osu/compare/master...Joehuu:osu:fix-dialog-one-screen-exit:

I get behavioral differences as noted in https://github.com/ppy/osu/pull/19528#issuecomment-1364624149 + the inability to spam alt-f4 twice to exit the game because of the 200ms.

I still believe my solution in using e.Destination.MakeCurrent() on every confirm exit dialog is better than using PerformFromScreen.

[peppy]

I think you should PR your solution and we can see how it feels.