-
Notifications
You must be signed in to change notification settings - Fork 824
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
Restore cursor after SIGINT during dialogue #4841
Conversation
Note: this PR is trying to fix #4837 |
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 find surprising that we were not using ctrlc
before. Because I know for journaling we listen on Ctrl^C.
We need to make sure that handler for WASIX and this one are not colliding before merging the PR (please check with @john-sharratt )
(I force-pushed to remove a commit that was not related to this PR). The signal handler is installed in those commands that implement the |
Just more context: the current signal handler hops in when saving journaling upon doing ctrl-c |
lib/cli/src/commands/mod.rs
Outdated
@@ -70,12 +71,21 @@ pub(crate) trait AsyncCliCommand: Send + Sync { | |||
type Output: Send + Sync; | |||
|
|||
async fn run_async(self) -> Result<Self::Output, anyhow::Error>; | |||
|
|||
fn setup(&self) -> Result<(), anyhow::Error> { | |||
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.
nit: no need for the separate crtlc
dependency, we can use tokio::signal::ctrl_c
.
With the current structure it'll be easiest to just run it in a spawned task.
lib/cli/src/commands/mod.rs
Outdated
@@ -70,12 +71,21 @@ pub(crate) trait AsyncCliCommand: Send + Sync { | |||
type Output: Send + Sync; | |||
|
|||
async fn run_async(self) -> Result<Self::Output, anyhow::Error>; | |||
|
|||
fn setup(&self) -> Result<(), anyhow::Error> { | |||
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.
Note: we should probably restrict this to TTYs.
stdin.is_terminal()
.
See https://doc.rust-lang.org/stable/std/io/trait.IsTerminal.html#tymethod.is_terminal
@syrusakbary this will not be invoked for the |
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
As per title. This PR adds a
setup
function toAsyncCliCommand
with a default implementation that installs a signal handler that restores the cursor (see console-rs/dialoguer#77). Solves #4837