-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
Handle Ctrl+C
in the terminal properly
#14001
base: main
Are you sure you want to change the base?
Conversation
10276f1
to
3103cea
Compare
f3a017f
to
e5c89a3
Compare
#[cfg(not(target_arch = "wasm32"))] | ||
impl TerminalCtrlCHandlerPlugin { | ||
/// Handler for when the user presses `Ctrl+C` on the terminal. Should be called from the `Ctrl+C` hook. | ||
pub fn on_ctrl_c() { |
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.
Is there a reason this is public? I'd rather hide it if not.
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.
See their migration guide. If I understand it correctly, the idea is that you can still call this manually to notify all apps even when you're doing your own, manual CTRL-C handling.
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 renamed it but the idea was that if a user defines their own ctrlc handler then they will be able to use the plugins exit function to in that handler to exit bevy. They would not have to setup their own system and static flag.
fn build(&self, app: &mut App) { | ||
#[cfg(not(target_arch = "wasm32"))] | ||
{ | ||
ctrlc::set_handler(move || { |
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.
could you use try_set_handler
instead, and handle errors to log if there's already one set instead of panicking?
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.
set_handler
always overwrites the current handler so it should not panic.
But I am changing it to try_set_handle
so that the user can set their own handler. They can then call our TerminalCtrlCHandlerPlugin::gracefully_exit
in their handler to exit bevy gracefully.
should this be behind a feature? |
Imo, no. We already have a lot of features and have issues testing every combination. Users can just disable the plugin if they don't want it. I guess they'd still have a dependency on ctrlc though, but eh. |
0d09884
to
214f06a
Compare
214f06a
to
3b3edf3
Compare
Looks mostly good to me, left some minor notes. I don't like that fact that the graceful shutdown is broadcast globally, but it shouldn't matter in practice. |
e0ea791
to
1ce1958
Compare
One last rephrase, then I'm good :) |
1ce1958
to
a556e2b
Compare
a556e2b
to
95f1877
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.
LGTM now :)
FYI, the PR is squashed anyways, so you don't need to force push your changes during a review to keep the history clean.
Objective
Fixes #13995.
Solution
Override the default
Ctrl+C
handler with one that sendsAppExit
event to every app withTerminalCtrlCHandlerPlugin
.Testing
Tested by running the
3d_scene
example and hittingCtrl+C
in the terminal.Changelog
Handles
Ctrl+C
in the terminal gracefully.Migration Guide
If you are overriding the
Ctrl+C
handler then you should callTerminalCtrlCHandlerPlugin::gracefully_exit
from your handler. It will tell the app to exit.