-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
Signed-off-by: tison <[email protected]>
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<()> { |
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.
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> |
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'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.
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.
My current objections.
- place holder docs, mentioned already of course
- 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
- 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.
@fdncred Thanks for your feedback!
Yeah. After a consideration we may directly change the
If we change the impl of ... 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). |
Push a new commit to showcase the reimpl alternative ad9c315 |
Signed-off-by: tison <[email protected]>
@fdncred @sholderbach Any thoughts here? |
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.
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
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.
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.
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)
@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". |
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! |
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. |
August 20th |
Cool! Looking forward to upgrade :D |
This closes #811.
cc @fdncred @sholderbach
I can use this with: