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

Simplify clear implementation #11273

Merged
merged 1 commit into from Dec 9, 2023
Merged

Simplify clear implementation #11273

merged 1 commit into from Dec 9, 2023

Conversation

nibon7
Copy link
Contributor

@nibon7 nibon7 commented Dec 9, 2023

Description

This PR uses the crossterm api to reimplement clear command, since crossterm is cross-platform.
This seems to work on linux and windows.

User-Facing Changes

N/A

Tests + Formatting

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
  • cargo run -- -c "use std testing; testing run-tests --path crates/nu-std" to run the tests for the standard library

After Submitting

N/A

@nibon7 nibon7 changed the title Simplify clear implementation Simplify clear implementation Dec 9, 2023
@fdncred
Copy link
Collaborator

fdncred commented Dec 9, 2023

I have a couple ideas and concerns here. Generally, I like the simplification and hope we can use it. Thanks!

  1. crossterms's ClearCommand calls ansi escape sequences. I wonder if that's a big deal since I assume there are terminals that don't handle ansi escape sequences. We support those terminals now by turning off ansi escape sequences via config. Windows has a special directive, seen below. I wonder if crossterm has a function that checks if the terminal supports ansi escape sequences? If so, we could have a check to see and then clear the old way maybe?
  2. It may be a good idea to implement switches for what crossterm provides as parameters when not on windows. Not sure if that's a good idea or not. FYI - all of these ansi escape sequences are already in nushell in the ansi --list command.
    fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result {
        f.write_str(match self.0 {
            ClearType::All => csi!("2J"),
            ClearType::Purge => csi!("3J"),
            ClearType::FromCursorDown => csi!("J"),
            ClearType::FromCursorUp => csi!("1J"),
            ClearType::CurrentLine => csi!("2K"),
            ClearType::UntilNewLine => csi!("K"),
        })
    }

    #[cfg(windows)]
    fn execute_winapi(&self) -> io::Result<()> {
        sys::clear(self.0)
    }

Thoughts?

@nibon7
Copy link
Contributor Author

nibon7 commented Dec 9, 2023

@fdncred
Copy link
Collaborator

fdncred commented Dec 9, 2023

I'm willing to give this a try and we can add more switches in the future.

@fdncred fdncred merged commit ca05553 into nushell:main Dec 9, 2023
19 checks passed
@nibon7 nibon7 deleted the clear branch December 9, 2023 21:33
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description
This PR uses the `crossterm` api to reimplement `clear` command, since
`crossterm` is cross-platform.
This seems to work on linux and windows.

# User-Facing Changes
N/A

# Tests + Formatting
- [x] `cargo fmt --all -- --check` to check standard code formatting
(`cargo fmt --all` applies these changes)
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
to check that you're using the standard code style
- [x] `cargo test --workspace` to check that all tests pass (on Windows
make sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

# After Submitting
N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
This PR uses the `crossterm` api to reimplement `clear` command, since
`crossterm` is cross-platform.
This seems to work on linux and windows.

# User-Facing Changes
N/A

# Tests + Formatting
- [x] `cargo fmt --all -- --check` to check standard code formatting
(`cargo fmt --all` applies these changes)
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
to check that you're using the standard code style
- [x] `cargo test --workspace` to check that all tests pass (on Windows
make sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

# After Submitting
N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants