Skip to content
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

Fix/input suppress output on windows #9459

Merged
merged 2 commits into from Jun 19, 2023

Conversation

Sygmei
Copy link
Contributor

@Sygmei Sygmei commented Jun 17, 2023

this PR should close #9018

Description

This PR aims to fix the input command with --suppress-output on Windows
This fixes two separates issues :

  • Keypresses being duplicated in output due to "Release" event not being ignored
  • "Return" event from entering the input -s "blah :" still being in the event buffer (need to be cleared before reading keypresses)

Tests + Formatting

  • cargo fmt --all -- --check ✔️
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect -A clippy::result_large_err ✔️
  • cargo test --workspace ✔️
  • cargo run -- crates/nu-std/tests/run.nu ✔️

@amtoine amtoine added platform-specific platform-specific issues windows A Windows specific issue labels Jun 17, 2023
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me for interactive use.

Not sure if it can become problematic if you were to run a script containing such an input in a scripted way.

Comment on lines +136 to +140
// clear terminal events
while crossterm::event::poll(Duration::from_secs(0))? {
// If there's an event, read it to remove it from the queue
let _ = crossterm::event::read()?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this might become problematic if you try to run the whole thing in a "scripted" way (e.g. through pexpect or similar tools) if the buffer gets filled quicker than a human would typically type it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll make some tests !

crossterm::terminal::disable_raw_mode()?;
return Err(ShellError::IOError("SIGINT".to_string()));
Ok(Event::Key(k)) => match k.kind {
KeyEventKind::Press | KeyEventKind::Repeat => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@fdncred
Copy link
Collaborator

fdncred commented Jun 19, 2023

fyi - i tested this on Windows 10 and it fixes the issue described in #9018. Thanks!

@sholderbach
Copy link
Member

Let's land it to benefit from the obvious fix and revisit any potential edge cases.

@sholderbach sholderbach merged commit d25fb3a into nushell:main Jun 19, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-specific platform-specific issues windows A Windows specific issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants