feat: writer() and writer_mut() on termion/crossterm

[enricozb]

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.

[joshka]

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.

[enricozb]

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 &mut [u8], that is a server-side application, and I want to send it to the clients. So, I want to drain or even just read the writer to see what has been written. I don't want to use channels / streams / sinks / Arc<Mutex<.>> since these have synchronization primitives around the terminal writes.

[EdJoPaTo]

I'm writing bytes to a "fake" terminal

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?

[enricozb]

I can't read the &mut [u8] since the backends take ownership:

let terminal = Terminal::new(CrosstermBackend::new(Vec::<u8>::new()));
let ui = |frame| { ... };

terminal.draw(ui);

let crossterm_backend = terminal.backend();
let buffer = ???;

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.4%. Comparing base (fadc73d) to head (9299567).

❗ Current head 9299567 differs from pull request most recent head c770312

Please upload reports for the commit c770312 to get more accurate results.

Files	Patch %	Lines
src/backend/crossterm.rs	0.0%	4 Missing ⚠️
src/backend/termion.rs	0.0%	4 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

[joshka]

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 &mut W where W: Write + ?Sized, which sounds like it should solve most problems to do with ownership. https://doc.rust-lang.org/std/io/trait.Write.html#impl-Write-for-%26mut+W

That is to say, while this might be useful, it's not strictly necessary.

[enricozb]

The problem with that example is that I can't reuse buffer after reading from writer. Specifically I'm looking to alternate between reading and writing. For example:

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

error[E0502]: cannot borrow `writer` as immutable because it is also borrowed as mutable
  --> tests/widgets_block.rs:10:22
   |
8  |     let mut backend = CrosstermBackend::new(&mut writer);
   |                                             ----------- mutable borrow occurs here
9  |     backend.write(b"ABCDE").unwrap();
10 |     println!("{:?}", writer); // error
   |                      ^^^^^^ immutable borrow occurs here
11 |     backend.write(b"HIJKL").unwrap();
   |     ------- mutable borrow later used here
   |
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0502`.
error: could not compile `ratatui` (test "widgets_block") due to 1 previous error

[joshka]

The problem with that example is that I can't reuse buffer after reading from writer. Specifically I'm looking to alternate between reading and writing. For example:

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 Write implementation that handles things with a push instead of pulling the info out of the backend.

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.mov

The 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 stability in the code base for how we do this currently).

There are more changes required to robustly support server use cases - particularly the size, window_size and get_cursor methods can cause problems and would need to be overridden in a proper server backend that wraps an inner backend. (This is probably orthogonal to your specific problem).

[joshka]

There are more changes required to robustly support server use cases - particularly the size, window_size and get_cursor methods can cause problems and would need to be overridden in a proper server backend that wraps an inner backend. (This is probably orthogonal to your specific problem).

I fixed this up in joshka/pong-russh@f565fb7 by having a custom backend wrapper that intercepts those methods.

[joshka]

@EdJoPaTo do you think this is reasonable after the discussion above, or do you still think we should avoid this?

[EdJoPaTo]

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.

[bugnano]

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.

[joshka]

You mean like #1006 that was closed because this PR was about to get merged?

#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.

[bugnano]

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?

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.

[bugnano]

Alternatively, showing how the backend wrapper does / doesn't work for your use case might also help.

So, let's pretend that I have an app that uses the alternate screen in raw mode.
In my app I want to execute a command (a simple ls -l, for example), and write the output of that program to the main screen.
After the command completes, the app returns back to the alternate screen in raw mode.
At program exit, you see on your terminal the output of the ls -l executed from the app.

With the current API in Ratatui this is not possible (at least not with the Termion backend).

With this PR, this would become easy.

[joshka]

With the current API in Ratatui this is not possible (at least not with the Termion backend).

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:

• To disable raw mode in termion, you need to have a RawTerminal that has a saved copy of the Termios to restore and then call suspend_raw_mode
• It's possible to create one RawTerminal instance, immediately call suspend_raw_mode, and then create a second RawTerminal which is owned by Ratatui TermionBackend (that will also save a copy of the cooked Termios). This is useful as an approach for Panic / Error handlers.
• To support switching out of raw mode in an app (i.e. to run a terminal program), it would be necessary to create a third RawTerminal instance which is used only for that purpose
• Three RawTerminals for this scenario seems like overkill, and it would be nicer to be able to call:

  terminal.backend_mut().writer_mut().suspend_raw_mode();

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.

[enricozb]

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 BufWriter takes ownership of a writer when constructed, and has methods for both mutable reference and immutable reference access to the inner writer.

Also, I still think an unstable feature flag option is a valid one.

[enricozb]

Also, why is backend_mut okay but writer_mut not?

[joshka]

Perhaps I am missing something. I am still unsure I understand the cons of this PR, especially given its size.

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.

In the standard library BufWriter takes ownership of a writer when constructed, and has methods for both mutable reference and immutable reference access to the inner writer

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 writer() be writer_ref()? (I'm not 100% sure what the convention is for this)

Also, why is backend_mut okay but writer_mut not?

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 backend_mut method was added without thinking through the implications of this as much.

Also, I still think an unstable feature flag option is a valid one.

I think we can probably add this without a flag.

@EdJoPaTo wrote:

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.

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, ...).

[EdJoPaTo]

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

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.

There are lots of different ways that people have successfully written apps

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".

Also, why is backend_mut okay but writer_mut not?

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 backend_mut method was added without thinking through the implications of this as much.

Should we consider deprecating it? Either people will tell us good use-cases then or we can remove it / have it not public anymore.

I think we can probably add this without a flag.

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.

[enricozb]

It sounds like we're in agreement to put this (and perhaps backend_mut) behind a flag. Can I proceed with this?

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.

[enricozb]

Also, I personally do not feel that backend_mut nor writer_mut are footguns. Searching for backend_mut on the tui-rs repo and on this repo shows no results (except for this issue), so there doesn't seem to be any evidence that I'm aware of that users trip up on this.

[joshka]

Maybe I should implement something like that to try it in real code,

The way I visualize this is that in some main loop, you have a Terminal variable, that owns the Backend, which owns the &mut Write value. In order to get mutable access to the writer (and use it again in a backend / terminal pair after the mutable access to the writer), an app would have to exit the scope where the terminal is defined so that the terminal no longer owns the backend, which no longer owns the &mut writer. Then to restart the application back up, the app would have to go through any Terminal and backend initialization steps - the same as they did on startup taking into account any changes in the application state etc. This seems like a lot of effort for something that both the underlying crossterm and termion make easy to achieve (disable_raw_mode() / RawTerminal::suspend_raw_mode())

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.

[joshka]

It sounds like we're in agreement to put this (and perhaps backend_mut) behind a flag. Can I proceed with this?

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()

[EdJoPaTo]

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 into_backend and into_writer.

Sadly into_backend requires to remove the Drop trait of Terminal. Given that we have Backend specific logic to exit, it probably should be encapsulated into a common ratatui included method (which can also include the hide_cursor like I did here too).
When exiting the ratatui app the terminal should always be restored the same way, even if we overdo some things never set, it can restore it in a general way (for a given Backend implementation). Might also be useful for the panic handler. But that is probably a topic for another issue / PR.

I adapted the run_app function to move the terminal into and return it on finish instead of taking a &mut. (The error also needs to return the terminal in order to cleanly recover from it, I did not integrate that here. That would require a custom error struct I guess.)

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 into_backend and into_writer (which needs to remove Drop → breaking).
The naming of the Terminal<Backend> might be a bit confusing as its the ratatui common backend and not a plain terminal.

---

In the long run I think an approach with into would be cleaner. In the short run it's easier to add writer_mut() and put them behind a feature flag.

[joshka]

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)

[enricozb]

I think that it's important to note that even though BufWriter makes assumptions about its inner writer, it still exposes the inner writer without any feature flag or unsafe.

I also think that the amount of code being written -- and the amount of tradeoffs such as flickering, removing Drop trait, and the cost of creating a new Terminal -- to "avoid" having a writer_mut method is only further evidence for it to be added. If swapping in and out of raw mode is 1) hard to get right and 2) a common enough use-case, then ratatui should offer some facilities to "safely" swap in and out of raw mode. A link to those facilities and additional warnings can be added to the docs for writer_mut.

But these are not reasons in my opinion to not add writer_mut.

[EdJoPaTo]

I think that it's important to note that even though BufWriter makes assumptions about its inner writer, it still exposes the inner writer without any feature flag or unsafe.

This has a completely different scope. It is a generic component.

amount of code being written

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 writer_mut also requires similar design changes, so I don't see a big difference here.

[…] the amount of tradeoffs such as

I don't think any of the listed tradeoffs is a tradeoff:

flickering

Comes from changing between the modes. writer_mut doesn't change that as it would still do that. The performance impact is on the Terminal renderer, gnome-terminal or whatever is used needs to change the whole shown buffer here (which is why there are GPU accelerated terminals like alacritty). This is by far the biggest impact listed here and completely independent of ratatui or a backend like crossterm.

removing Drop trait

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.

the cost of creating a new Terminal

Assuming Viewport::Fullscreen is used: The biggest impact is Buffer::empty due to being O(backend.size) which does not even need a millisecond (worst case on somewhat dated hardware and can be improved, see #1036). The rest is simple if/match logic. Nothing of performance importance.

a common enough use-case, then ratatui should offer some facilities to "safely" swap in and out of raw mode.

That's what @joshka suggested 😇

For now writer_mut could be added behind a feature flag. The common use cases which would require this should be made available as functions not requiring the feature flag.

@EdJoPaTo do you think this is reasonable after the discussion above, or do you still think we should avoid this?

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.

[kdheepak]

I am onboard with adding it under a feature flag (e.g. unstable-backend-writer-mut?) as we explore the design space and figure out what are the best ways to support this use case in the long run.

[joshka]

Added unstable attribute and merged main to fix a typo lint issue.

[bugnano]

ping

[joshka]

ping

pong :D