-
Notifications
You must be signed in to change notification settings - Fork 260
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(backend): Add convenience methods for setting up terminals #1180
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
=======================================
- Coverage 94.4% 93.9% -0.5%
=======================================
Files 62 62
Lines 14941 15127 +186
=======================================
+ Hits 14110 14217 +107
- Misses 831 910 +79 ☔ View full report in Codecov by Sentry. |
aacf548
to
1d176d9
Compare
Note - this is intentionally only implemented for crossterm right now, and it's not carried through to the examples as yet to allow for review of the concept without the bulk. Note also the comments / previous effort on #280 |
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 like seeing the with_*
methods and the internal state to allow for a simpler reset. This will simplify a lot of code.
With other stuff I have my problems, noted on them directly for easier, threaded discussions.
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 really like the simplicity that will come with this and a built-in panic handler. I still want to provoke some thoughts about the current state here. (Also, I want to test this branch to get a feel for its real world usage, which I haven't done yet.)
Adds: - `CrosstermBackend::stdout` and `CrosstermBackend::stderr` for creating backends with `std::io::stdout` and `std::io::stderr` respectively. - `CrosstermBackend::into_terminal` for converting a backend into a terminal. - `CrosstermBackend::into_terminal_with_options` for converting a backend into a terminal with options. - `CrosstermBackend::into_terminal_with_defaults` for converting a backend into a terminal with default settings (raw mode and alternate screen). - `CrosstermBackend::with_raw_mode` for enabling raw mode. - `CrosstermBackend::with_alternate_screen` for switching to the alternate screen. - `CrosstermBackend::with_mouse_support` for enabling mouse support. - `CrosstermBackend::reset` for resetting the terminal to its default state. - `Drop` implementation for restoring raw mode, mouse capture, and alternate screen on drop. Most applications can now just use the `into_terminal_with_defaults` method to set up the terminal with raw mode and the alternate screen, and now longer need to worry about restoring the terminal to its default state when the application exits. The reset method can be used to restore the terminal to its default state if needed (e.g. in a panic handler). ```rust use ratatui::backend::CrosstermBackend; let terminal = CrosstermBackend::stdout() .into_terminal_with_defaults()?; ```
3e8bab2
to
0b75ddc
Compare
0b75ddc
to
48d6c5a
Compare
I am curious why crossterm itself doesn't have something like this. I think everyone using crossterm would benefit from something like this. Should this be proposed for crossterm? |
Define "this". Regardless it doesn't change whether we'd want this PR to land in the meantime as the development cycle on that side of the fence is pretty slow. @EdJoPaTo do you have any hard blockers for this PR getting merged? This PR underpins a massive simplification of all the example code in Ratatui as well as customer apps, tutorials, recipes, etc. I'm blocked on completing all of that by this not being merged. |
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 still like the simplification that will come with this a lot. Some curiosity / minor nitpicks remain, some docs should improve, then I think this can be merged.
@joshka dismissed my review, I think this should be enabled automatically for this repo: dismiss reviews on new commits. Either it's quite fast to review them again or it might have changed something that should be reviewed again. (Maybe also a review of multiple and not just one person might be ideal especially for bigger things.)
That approach causes a long turn around time for things which don't matter. I prefer to work based on trust that we'll do the right thing once given the go ahead to merge. |
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've noted that a most of these points are nitpicks and my tolerance for handling this amount of not relevant feedback on this PR is rapidly approaching zero. There's nothing that would block this from being merged or that would cause a user not to understand how these methods work. As such resolving them.
@EdJoPaTo see my email about nits etc. |
Most of the PR looks good to me (but I haven't read all the comments on this thread), will do so over the weekend. |
pub fn stdout() -> io::Result<Self> { | ||
Self::new(io::stdout()) | ||
.with_raw_mode()? | ||
.with_alternate_screen() | ||
} |
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 PR description says:
CrosstermBackend::stdout
andCrosstermBackend::stderr
for creating backends withstd::io::stdout
andstd::io::stderr
respectively.CrosstermBackend::stdout_with_defaults
andCrosstermBackend::stderr_with_defaults
for creating backends with that use the alternate screen and raw mode.
which doesn't match this implementation.
I prefer it to work the way the PR description mentions. It makes it much more clearer imo.
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.
Alternatively, CrosstermBackend::stdout_tui
and CrosstermBackend::stderr_tui
.
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 idea behind doing some reasonable defaults (raw/alternate screen) on these is that this is necessary for 90% of all apps. Anything else can do CrosstermBackend::new(stdout()).with_xxx()
...
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.
It'll be a little confusing imo to find in a Ratatui app's user code two things that are very similar but do different things
CrosstermBackend::new(stdout())
for inlineCrosstermBackend::stdout()
for a TUI
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 like the reasonable defaults in place here, I'm just not convinced that the name conveys those defaults clearly.
Adds:
CrosstermBackend::stdout
andCrosstermBackend::stderr
for creatingbackends with
std::io::stdout
andstd::io::stderr
respectively.CrosstermBackend::stdout_with_defaults
andCrosstermBackend::stderr_with_defaults
for creatingbackends with that use the alternate screen and raw mode.
CrosstermBackend::with_raw_mode
for enabling raw mode.CrosstermBackend::with_alternate_screen
for switching to thealternate screen.
CrosstermBackend::with_mouse_capture
for enabling mouse support.CrosstermBackend::reset
for resetting the terminal to its defaultstate.
Drop
implementation for restoring raw mode, mouse capture, andalternate screen on drop.
Most applications should generally use
stdout_with_defaults
to createa backend with raw mode and the alternate screen, and now longer need
to worry about restoring the terminal to its default state when the
application exits. The reset method can be used to restore the terminal to
its default state if needed (e.g. in a panic handler).