feat: writer() and writer_mut() on termion/crossterm #991
Conversation
It is sometimes useful to obtain access to the writer if we want to see what has been written so far. For example, when using &mut [u8] as a writer.
enricozb
requested review from
orhun,
mindoodoo,
sayanarijit,
joshka,
kdheepak,
Valentin271 and
EdJoPaTo
as code owners
|
Would you mind explaining the use case for this a bit more? Exposing a mutable reference to the writer seems like it would be problematic as the terminal implementation assumes a lot about the state of the buffer. |
But isn't this the user's (code-writer's) responsibility to make sure these interactions are safe? I'm writing bytes to a "fake" terminal, a |
Then you should have the access there too I guess? As it's your target it seems to be also a better idea to output it there? Or am I missing something? |
|
I can't read the let terminal = Terminal::new(CrosstermBackend::new(Vec::<u8>::new()));
let ui = |frame| { ... };
terminal.draw(ui);
let crossterm_backend = terminal.backend();
let buffer = ???; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
=======================================
- Coverage 94.3% 89.4% -5.0%
=======================================
Files 61 61
Lines 14768 15472 +704
=======================================
- Hits 13935 13833 -102
- Misses 833 1639 +806 ☔ View full report in Codecov by Sentry. |
|
The following test succeeds without requiring this code change: #[test]
fn new() {
let mut writer = Vec::new();
let mut backend = CrosstermBackend::new(&mut writer);
backend.write(b"ABCDE").unwrap();
writer.flush().unwrap(); // can mutate the writer
assert_eq!(writer, b"ABCDE");
}There's a blanket implementation of Write on That is to say, while this might be useful, it's not strictly necessary. |
|
The problem with that example is that I can't reuse use std::io::Write;
use ratatui::backend::{Backend, CrosstermBackend};
#[test]
fn new() {
let mut writer = Vec::new();
let mut backend = CrosstermBackend::new(&mut writer);
backend.write(b"ABCDE").unwrap();
println!("{:?}", writer);
backend.write(b"HIJKL").unwrap();
println!("{:?}", writer); // error
}fails with the error |
Got it - makes sense. I think you're probably right that this makes this the methods worth adding - if you need a pull approach to getting the info from the writers. One alternative is to write your own I experimented with russh the other day to write a server side pong game that sends info back over an ssh connection. This worked out fairly well, (though there's some easy ways to deadlock). Screen.Recording.2024-03-31.at.6.54.56.AM.movThe write code was lifted verbatim from the ratatui example in russh: #[derive(Clone)]
pub struct TerminalHandle {
handle: Handle,
// The sink collects the data which is finally flushed to the handle.
sink: Vec<u8>,
channel_id: ChannelId,
}
// The crossterm backend writes to the terminal handle.
impl Write for TerminalHandle {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.sink.extend_from_slice(buf);
Ok(buf.len())
}
fn flush(&mut self) -> std::io::Result<()> {
let handle = self.handle.clone();
let channel_id = self.channel_id;
let data = self.sink.clone().into();
futures::executor::block_on(async move {
let result = handle.data(channel_id, data).await;
if result.is_err() {
eprintln!("Failed to send data: {:?}", result);
}
});
self.sink.clear();
Ok(())
}
}This is a push approach rather than a pull approach which you're suggesting. I think that might be a better model for this sort of logic (correct me if I'm wrong on this). How does your server app compare - where does the logic for your write calls sit? I know this push back might seem a bit annoying, but I'm trying to understand whether adding this to the API is something that solves things in the right way. At the very least, I'm happy to merge this PR gated behind an unstable feature flag to get you to a workaround state (search for There are more changes required to robustly support server use cases - particularly the |
I fixed this up in joshka/pong-russh@f565fb7 by having a custom backend wrapper that intercepts those methods. |
|
@EdJoPaTo do you think this is reasonable after the discussion above, or do you still think we should avoid this? |
|
I still think that the cleanest way to do something like this is wrapping in a custom Backend and not expose the inner workings of Terminal as it could create side effects. I'm not sure what the current state is here and whether this is possible for the use case of @enricozb? I don't think this is of interest for most users and as it can introduce side effects it should only be added when really necessary to achieve something otherwise not possible. |
You mean like #1006 that was closed because this PR was about to get merged? I need to be able to enter and exit raw mode during normal app execution, and this PR would have allowed me that. |
#1006 was closed mainly because it covers the same topics we're discussing here and in #1005. It's difficult to have a conversation split over 3 places as tracking which parts of the conversation are resolved is a pain. One of the things we have to do as library creators is make sure that when we add some functionality is generally understanding how the feature will / won't solve the problems and won't be something that we have to remove or change drastically in the future. In effect, designing a library is about seeing around corners that you haven't reached yet. I'm leaning towards including this PR, but in doing so, I'd be going against the advice of someone I trust to have well thought out rationales for this sort of thing (which is the reason we invited @EdJoPaTo to be a maintainer). So before going ahead with that, I want to make sure that I understand the pros/cons/alternatives well enough to know that merging this is the right idea regardless of the downsides, and I want to understand how your specific use case doesn't fit into the current available stuff. Many parts of Ratatui don't need this sort of rigor, but the terminal / backend / frame / buffer are types that it serves us well to be a little more conservative with changing. Something that could help push this into the "this is obviously the right direction" evaluation would be examples of other idiomatic rust libraries / types (particularly if they are in the std library) where a type that accepts a writer (or similar type) allows future mutable access to that type like this. Are you aware of anything like this? Alternatively, showing how the backend wrapper does / doesn't work for your use case might also help. I can't get something that would work somewhat like the following to compile, so there's possibly some merit that what you want to do can't be done with a wrapper. use std::io::Write;
use crate::prelude::*;
struct CrosstermWrapper<'a, W: Write + 'a> {
writer: W,
inner: CrosstermBackend<&'a mut W>,
}
impl<'a, W: Write> CrosstermWrapper<'a, W> {
pub fn new(mut writer: W) -> Self {
Self {
writer,
inner: CrosstermBackend::new(&mut writer),
}
}
}It's difficult to guess how close this gets to the real use case that you're working on however, details likely matter here. |
I'm not aware of anything like this, sorry. My reasoning, however, is that given that Ratatui takes ownership of the stdout/stderr, but I, as an app author need to handle the stdin for the keyboard events, I'd also like to have some way of interacting directly with stdout/stderr, by asking to borrow it from Ratatui. Ratatui is awesome for 99% of the stdout/stderr terminal handling, but for the remaining 1% I think that giving back the stdout/stderr as a borrow to the app developer as an escape hatch would allow to cover everything. |
So, let's pretend that I have an app that uses the alternate screen in raw mode. With the current API in Ratatui this is not possible (at least not with the Termion backend). With this PR, this would become easy. |
I think this might be possible (albeit clunky) using two copies of the RawTerminal in a similar way to the solution for the panic handler approach I recently documented in https://ratatui.rs/how-to/develop-apps/panic-hooks/#termion That said, this a good argument is that this is pretty annoying. Rephrased more strongly:
P.S. @bugnano I mistook your use case for the Ratatui server use case that was the reason for this PR in the first case. Apologies for the confusion there. |
No worries🙂 Your rephrasing is perfectly on point👍 |
To me #1006 sounds about debugging stuff why it’s the way it is. So yeah, a temporary method added locally and linked again with path instead of using the crates.io version is possible for that. I don’t think these methods should exist in production code only to debug possible bugs in the code. I’m still not saying I’m against adding these. But we should make sure we add them for good reason.
Thinking of the XY problem right now… This PR tries to solve X while there is a need of Y. Can we solve Y better?
One of the main ideas of Rust is to always have the valid state represented. To me this sounds like an into thing. Ratatui into plain view back into ratatui. Is an into_writer better suited here? It’s basically an end of the backend without dropping everything but returning the writer. When the backend is needed again a new Backend is created with the given writer. That way the design seems more clear: we are not picking inside of the current writer handled by multiple instances. It’s clear who has the ownership of that. |
|
I suspect @kdheepak might also have some views on this having built a few different apps and templates that handle being able to suspend and run vim / other things. Though they're crossterm based which doesn't have this same problem as the cooked termios are saved globally. |
|
Perhaps I am missing something. I am still unsure I understand the cons of this PR, especially given its size. The example workarounds require more code, synchronization, and/or data duplication. If state corruption is an issue, then having only read access avoids this, which is at least half of this PR. With respect to @joshka's question: in the standard library Also, I still think an unstable feature flag option is a valid one. |
|
Also, why is |
Providing mutable access to the buffer that the backend writes to breaks the assumptions that Ratatui works. The Terminal generally assumes that if it writes something to the buffer, then that stays there for the next draw call and doesn't need to be redrawn.
I'm convinced this is a fine based on that and it gives us some wording guidance: "It is inadvisable to directly write to the underlying writer." We should add a bit more about why ("Ratatui assumes the state of the writer is unchanged between draw calls so that it can efficiently render only changed areas of the screen. See https://ratatui.rs/concepts/rendering/under-the-hood/ for more details") Should
Taking a step back, the reason why we try to be precise with this is that there are a lot of new rust users that tend to hit Ratatui as one of their first projects. We don't want to add footguns and we also want to avoid adding code that will change in the future. The
I think we can probably add this without a flag. @EdJoPaTo wrote:
This sort of dictates the a particular way that an app needs to be constructed though. I think it's reasonable to be able to see switching back and forth from raw mode as something within the scope of the Terminal's lifetime and not something that would require re-initialization. Put another way, forcing the app to have to fully drop the Terminal in order to temporarily run something in the shell seems like it's too much (especially compared to the simplicity of adding this method). There are lots of different ways that people have successfully written apps (component style, Terminal stored in App, stored outside, ...). |
Rust makes moving of variables really easy. The writer is moved out of the struct and not dropped. Both the Termion and Crossterm Backend don't have any other fields in the struct so nothing is dropped here. You will not notice any runtime effects as the writer stays the same and the struct is only a wrapper around which is optimized away. So there is no runtime downside of this. But in my view it seems a lot better when thinking about the responsibility: Either the writer is part of the Ratatui backend then ratatui can make assumptions or there is no backend so ratatui isn't relevant in the first place. That feels a lot cleaner design wise to me as there are only valid states then. And runtime wise there is no drawback. Rust is different here to what I learned from other languages as the concept of moving data around is a lot clearer in Rust and runtime wise its completely optimized away. Maybe I should implement something like that to try it in real code, but it still sounds like the best of the two worlds: Ratatui can make assumptions and access to the raw terminal below is possible with a clear distinction who is in control of the terminal.
Libraries are abstracting code to enforce certain ways of using something in order to simplify its usage. I don't think we should aim for "allow everything". We should aim for "whats a clean and useable API not easy to break".
Should we consider deprecating it? Either people will tell us good use-cases then or we can remove it / have it not public anymore.
It might be a good idea to put a foot gun behind a flag. That way someone actively needs to enable the feature to do something like that. Also, we can take a look for that feature via GitHub code search then, see what people do with that and try to create a better API for their specific use-case with less foot-gun potential. |
|
It sounds like we're in agreement to put this (and perhaps However, surely allowing read access does not affect any assumptions ratatui might make. So at a minimum a method to have read access to the writer should be okay. |
|
Also, I personally do not feel that |
The way I visualize this is that in some main loop, you have a Is there a simpler way that I'm missing here? I understand this is about making a choice of how we want the library to restrict things, but that choice here restricts only termion, but not crossterm due to their different implementations of where they store the termios to restore. |
Yes go for it - let's get this in a state where we can merge it if the discussion resolves that way. Don't worry about adding the flag to backend_mut() |
|
I used one random example in this repo and adapted it. In my case it was the user_input example because there is input to write to the writer (see the key code thingy later). First I migrated the terminal initialisation code into methods: fn enter_ratatui<W>(mut writer: W) -> std::io::Result<Terminal<CrosstermBackend<W>>>
where
W: std::io::Write,
{
enable_raw_mode()?;
execute!(writer, EnterAlternateScreen, EnableMouseCapture)?;
let backend = CrosstermBackend::new(writer);
Terminal::new(backend)
}
fn restore_terminal<W>(terminal: Terminal<CrosstermBackend<W>>) -> std::io::Result<W>
where
W: std::io::Write,
{
disable_raw_mode()?;
let mut backend = terminal.into_backend();
execute!(
backend,
LeaveAlternateScreen,
DisableMouseCapture,
crossterm::cursor::Show,
)?;
Ok(backend.into_writer())
}For the second function I created Sadly I adapted the fn run_app<W>(
mut terminal: Terminal<CrosstermBackend<W>>,
app: &mut App,
) -> io::Result<Terminal<CrosstermBackend<W>>>
where
W: std::io::Write,
{
// …
}Then I added another key handling: KeyCode::Char('p') => { // p for print
let mut writer = restore_terminal(terminal)?;
writeln!(writer, "Current input: {}", app.input)?;
terminal = enter_ratatui(writer)?;
}This works well in this example. Its noticeable by a short flickering of the terminal but that's kind of expected. So I still think it's possible with In the long run I think an approach with into would be cleaner. In the short run it's easier to add |
|
The problem with that approach is that it requires the code that suspends the terminal to take ownership of the terminal variable, whereas a writer_mut approach just requires a mutable ref. In a reasonable size app, the code for this might be be several layers deep of event / action / message / async code, and passing an owned terminal would be difficult. It might be possible to write this and keep to &mut Terminal (using *terminal = ...). I think the right long term solution for this specific problem however is to add methods for suspending raw mode onto the terminal directly so getting at the writer isn't necessary (that said it still may be necessary if you want to write some sort of decorator over one of the backends that does something before the write calls) |
|
I think that it's important to note that even though I also think that the amount of code being written -- and the amount of tradeoffs such as flickering, removing But these are not reasons in my opinion to not add |
This has a completely different scope. It is a generic component.
Personally I think this is a refactoring of the needed design to integrate something like this in a good way. Currently, ratatui applications haven't been written with this in mind and
I don't think any of the listed tradeoffs is a tradeoff:
Comes from changing between the modes.
Personally I think it's a good thing we have found this. It could end up improving the overall situation with backend cleanup. I created #1042 for discussing this as it's unrelated from this PR. Assuming
That's what @joshka suggested 😇 For now
Currently, I don't think I can add more to this than already said. I think the plan is clear: Add this behind a feature flag. Add commonly used features without the need of a feature flag. |
|
I am onboard with adding it under a feature flag (e.g. |
|
Added unstable attribute and merged main to fix a typo lint issue. |
|
ping |
pong :D |
…ratatui-org#991) It is sometimes useful to obtain access to the writer if we want to see what has been written so far. For example, when using &mut [u8] as a writer. Co-authored-by: Josh McKinney <joshka@users.noreply.github.com>
It is sometimes useful to obtain access to the writer if we want to see what has been written so far. For example, when using &mut [u8] as a writer.
I have not found any similar work to this, and this is a small change, so nothing else is linked here.