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

feat: impl ClearScreen with crossterm's Clear #813

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Aug 5, 2024

This closes #811.

cc @fdncred @sholderbach

I can use this with:

    let mut keybindings = reedline::default_emacs_keybindings();
    keybindings.add_binding(
        reedline::KeyModifiers::CONTROL,
        reedline::KeyCode::Char('l'),
        reedline::ReedlineEvent::ClearDisplay,
    );

    let mut state = Reedline::create()
        .with_validator(Box::new(...))
        .with_highlighter(Box::new(...))
        .with_edit_mode(Box::new(Emacs::new(keybindings)));

src/painting/painter.rs Outdated Show resolved Hide resolved
self.stdout.flush()?;
self.initialize_prompt_position(None)
}

/// Clear the screen by printing enough whitespace to start the prompt or
/// other output back at the first line of the terminal.
pub(crate) fn clear_screen(&mut self) -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lines calculation looks correct @sholderbach. But Warp has a sticky upper block that would hide the first few lines. I don't know how Warp's people implement their terminal and it looks terminal::size() returns the whole window including those hidden by the block.

src/engine.rs Outdated
@@ -666,6 +666,13 @@ impl Reedline {
self.painter.paint_line(msg)
}

/// <placeholder docs>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update these docs once the direction reaches a consensus (that we will implement the feature in this way).

The current patch can fit my requirement, but I can lack some of the design principles or other considerations within this project's practice. So please let me know if there is anything should be considered. And if it's good to proceed, please tell me explicitly also.

@tisonkun tisonkun requested a review from fdncred August 6, 2024 15:27
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

My current objections.

  1. place holder docs, mentioned already of course
  2. i don't really like having clear display and clear screen so i would hope at some point (maybe now) we'd be able to combine them because it's really just confusing why we'd have both
  3. are there ci tests that we could create to validate functionality?

if push comes to shove, i could live with cleardisplay and clearscreen in the short term.

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 7, 2024

@fdncred Thanks for your feedback!

i don't really like having clear display and clear screen so i would hope at some point (maybe now) we'd be able to combine them because it's really just confusing why we'd have both

Yeah. After a consideration we may directly change the clear_screen function to the current clear_display impl. @sholderbach implemented clear_screen directly in the print newlines way and I wonder if the impl in this PR fits the requirement in #28.

are there ci tests that we could create to validate functionality?

If we change the impl of clear_screen, we don't need new tests (covered by regressions). If we have a separated clear_display function, I can add some.

... while my opinion is that we just do some trivial tests (ensure the keybindings cause the event triggered), instead of really check the terminal's manner (and there is no existing tests to check that).

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 7, 2024

Push a new commit to showcase the reimpl alternative ad9c315

@tisonkun
Copy link
Contributor Author

@fdncred @sholderbach Any thoughts here?

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.

Can only verify on Windows Terminal right now but looks good in preserving any old stuff, and the ClearScrollback exists for the full on purge clearing

@tisonkun tisonkun changed the title feat: add ClearDisplay event feat: impl ClearScreen with crossterm's Clear Aug 12, 2024
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

This is probably ok on MacOS but ctrl+l clears some of the back buffer.

Repro with this PR
in nushell do ls as many times as it takes to scroll a couple off the top of the screen
cargo run --example demo
ctrl+l
part of the back buffer is there, some is gone.

it appears to clear the screen and put the cursor on the top line but when i scroll back, i get something like this when i scroll back after ctrl+L where i thought i would have all 5 or 6 ls outputs i only have 2 and a line.
image

Like I said, I can probably live with this behavior, and I doubt it's created by the code change here. I looked at how it worked before these changes and it's a bit different. I wouldn't call either before or after this PR perfect. (On Windows with WT it looks near perfect to me)

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 15, 2024

@sholderbach @fdncred Thanks for your review!

@fdncred is it possible we ship with this patch now? And I wonder if the manner you report is kind of a bug or improvement point in crossterm. That basically we use their ClearScreen but it doesn't work as expected in some term, while they declare it should be "crossterm".

@tisonkun tisonkun requested a review from fdncred August 15, 2024 15:18
@fdncred
Copy link
Collaborator

fdncred commented Aug 15, 2024

I'm fine with landing. FYI - I tried the latest crossterm and it works similarly on macos but eats more scroll back. Since Stefan already approved. Let's go!

@fdncred fdncred merged commit f2b414c into nushell:main Aug 15, 2024
6 checks passed
@tisonkun tisonkun deleted the clear-screen branch August 16, 2024 01:15
@tisonkun
Copy link
Contributor Author

Thank you! The last question is if there is an estimate of the next release I can get this patch in my project ..

Or where I can subscribe the release news.

@fdncred
Copy link
Collaborator

fdncred commented Aug 16, 2024

August 20th

@tisonkun
Copy link
Contributor Author

Cool! Looking forward to upgrade :D

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.

clear_screen to fit in Warp's manner
3 participants