-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove confirmation when game is asked to be restarted by the user #26558
base: master
Are you sure you want to change the base?
Conversation
{ | ||
game.AttemptExit(); | ||
} | ||
else |
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.
The flow here makes me wonder whether Restart
should handle the confirmation for platforms which can't restart themselves, rather than having this handled on each usage.
But then again, the usages may be so few and far between that it's fine to leave it as-is?
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'd say per-usage is better because places like tournament client use Restart()
and I don't really wanna think about how to make that work well across such varied usages.
fbd1b84
to
3649876
Compare
I guess bump to @ppy/team-client on this one, I think it's still good to go ahead if the direction of using |
@@ -125,7 +126,10 @@ public override bool RestartAppWhenExited() | |||
return true; | |||
} | |||
|
|||
return base.RestartAppWhenExited(); | |||
SessionStatics.SetValue(Static.RestartRequested, true); | |||
AttemptExit(); |
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.
This is never reached in Windows because of the early return above.
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.
This seems valid to me, it looks like the game would not actually exit itself.
game.AttemptExit(); | ||
} | ||
else | ||
if (game?.Restart() != true) |
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.
If I thinking this right, on Windows, changing the renderer will just outright restart. Would still expect a confirmation dialog here, but different wording that it will restart automatically.
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.
Don't really think its necessary.
The |
{ | ||
SessionStatics.SetValue(Static.RestartRequested, true); |
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.
This is never set back to false if the user cancels the dialog on unsupported platforms.
With hold-to-confirm delay set to 0:
Opening this as a draft to figure out direction with this. Arguably
SessionStatics
is not the correct place for the flag, and it should live inOsuGame
. I'd keep it in the parameters alone but it's hard to get the message toMainMenu
as part of theOnExiting
flow.Will add tests once a direction is agreed upon.
This is low priority.
All game restart requests are triggered via user intent, so they shouldn't require confirmation.
Addresses #25826 (reply in thread).