-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show results immediately if user hits "back" key after finishing gameplay #25097
Conversation
…play I've gone ahead and matched the osu!stable behaviour for this, as it seems like it's what people are used to and they will settle for no less. Closes ppy#18089.
Two things: First one, minor. With this change, do we still need the following code? It's not being removed in this pull. osu/osu.Game/Screens/Play/Player.cs Lines 570 to 580 in d2044fc
I can't actually hit this path but it seems like it could be removable (just do nothing if screen isn't current). Second, probably more important. If a storyboard has an outro, exiting during outro will not progress to results due to: osu/osu.Game/Screens/Play/Player.cs Lines 737 to 743 in d2044fc
I haven't checked what stable does but the fact that skipping outro will get you to results but exiting kicks to song select feels quite bad regardless. |
Likely not. I noticed this but forgot to check whether it can be removed.
Should definitely use the new behaviour instead. |
@@ -660,10 +671,10 @@ protected void SetGameplayStartTime(double time) | |||
/// <remarks>This can be called from a child screen in order to trigger the restart process.</remarks> | |||
/// </summary> | |||
/// <param name="quickRestart">Whether a quick restart was requested (skipping intro etc.).</param> | |||
public void Restart(bool quickRestart = false) | |||
public bool Restart(bool quickRestart = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for a nitpick:
also need to add /// <returns>
doc like for PerformExit
?
It can be removed, but requires some (probably fine) changes to the restart process, which may add a bit of mental burden to the review process (see d174a6c).
Have fixed this, a very easy fix luckily. It even had test coverage. |
e9db2c1
to
fa47309
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is okay, but leaning heavily on tests passing here...
Closes #18089.
I've gone ahead and matched the osu!stable behaviour for this, as it seems like it's what people are used to and they will settle for no less.