osu
osu copied to clipboard
Fix online play screen only accounting for current sub screen onexiting blocks
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).
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.
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.
It was updated to handle this case, see https://github.com/ppy/osu/pull/17817.
Agree with using PerformFromScreen
if anything. I don't like this kind of logic being locally implemented.
@Joehuu any interest in still trying to make this use PerformFromScreen
?
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.
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.
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
.
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.
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.