osu icon indicating copy to clipboard operation
osu copied to clipboard

Fix online play screen only accounting for current sub screen onexiting blocks

Open Joehuu opened this issue 2 years ago • 5 comments

The recently implemented exit block when playlist items are added only works when in the settings screen. This PR makes the home and window exit also work on advancing screens (e.g. song select).

Joehuu avatar Aug 01 '22 15:08 Joehuu

I was aware of this at the point of having this feature but it's already tracked in https://github.com/ppy/osu/issues/13613 as a general issue, and I honestly think the PerformFromScreen approach mentioned there would be much better than adding local workarounds to solve it.

frenzibyte avatar Aug 01 '22 15:08 frenzibyte

I took that issue as a different one altogether because this is dealing with sub screens. I'll see if PerformFromScreen can handle this case too.

Joehuu avatar Aug 01 '22 16:08 Joehuu

It was updated to handle this case, see https://github.com/ppy/osu/pull/17817.

frenzibyte avatar Aug 01 '22 16:08 frenzibyte

Agree with using PerformFromScreen if anything. I don't like this kind of logic being locally implemented.

peppy avatar Aug 02 '22 05:08 peppy

@Joehuu any interest in still trying to make this use PerformFromScreen?

peppy avatar Sep 09 '22 13:09 peppy

I'm still interested in fixing this. Sorry for ignoring this PR for so long.

There are two behavioral differences when using PerformFromScreen:

  • there is a 200 ms delay when it is exiting the subscreens https://github.com/ppy/osu/blob/209d44746ad0386029a375a2c87fe8a217c4184d/osu.Game/PerformFromMenuRunner.cs#L57
  • when there is any dialog open (blocking or otherwise), you can't alt-f4 out

I'll push the changes, but I'm not sure if those are issues or not.

Joehuu avatar Dec 25 '22 04:12 Joehuu

I was initially referring to updating the home button to use PerformFromScreen as I mentioned in https://github.com/ppy/osu/issues/13613#issuecomment-1101141404, but it turns out that still bypasses any blocking dialog inside the online play screen stack.

Doing a PerformFromScreen in the exit event of OnlinePlaySubScreen is hard to mentally parse considering that this exit event may have been caused by another PerformFromScreen that's also trying to make the destination screen as current (so now two tasks are trying to make the same screen current).

I'm hoping OnlinePlayScreen.OnExiting would make sure it exits each screen until the stack is empty, and block if one of the subscreens block exiting:

            while (screenStack.CurrentScreen != null)
            {
                var subScreen = (Screen)screenStack.CurrentScreen;
                if (subScreen.IsLoaded && subScreen.OnExiting(e))
                    return true;

                subScreen.Exit();
            }

That should be friendly enough with the PerformFromScreen logic (and work with alt+f4). And also I suppose is more or less similar to what you had initially, but I think using OnExiting instead of checking current screen reads better as far as I understand how screens block exiting.

frenzibyte avatar Dec 26 '22 00:12 frenzibyte

The code makes sense and is similar to master but looping subScreen.Exit(). When I did my implementation, I probably got lost in what OnExiting does. It only does the exit visuals / blocking and not the actual exit, so I thought I was exiting twice.

The while condition still checks for the first sub screen as there is a null ref in OnlinePlaySubScreenStack.

Joehuu avatar Dec 26 '22 05:12 Joehuu

That nullref looks like the implementation doesn't handle the case of exiting from the final screen in the stack, should probably early-return before doing its logic.

frenzibyte avatar Dec 26 '22 09:12 frenzibyte

OnBackButton also checks for LoungeSubScreen: https://github.com/ppy/osu/blob/2470991aaa4fc51c138977886f7dc5d7f7984d45/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs#L173-L177

and bypasses its OnExiting, so I think doing the same here is fine.

Joehuu avatar Dec 26 '22 19:12 Joehuu