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

The documented panic handler does not exit raw mode correctly in the Termion backend #1005

Open
bugnano opened this issue Mar 30, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@bugnano
Copy link

bugnano commented Mar 30, 2024

Description

Following the code on : https://ratatui.rs/how-to/develop-apps/panic-hooks/

I'm using the Termion backend.

The line in the docs

output.into_raw_mode()?.suspend_raw_mode()?;

should in theory disable raw mode from the terminal, but in reality it does not.
The '\n' characters on strdout/stderrr go to a new line, but they don't place the cursor at the beginning of the line, thus resulting in a weird output.

To Reproduce

painc!() anywhere in the app and see the stdout/stderr with weird newline behaviour.

Expected behavior

Screenshots

Environment

  • OS: Fedora 39
  • Terminal Emulator: GNOME Terminal
  • Font: Liberation Mono
  • Crate version: 0.26.1
  • Backend: Termion

Additional context

I don't know if this should be a bug in Termion itself.

@orhun
Copy link
Sponsor Member

orhun commented Mar 30, 2024

I pointed out this issue while adding this code to the website: ratatui-org/ratatui-website#69

It might be an issue with termion and I'm thinking if we should move this issue to the website repository or not...

@joshka
Copy link
Member

joshka commented Mar 30, 2024

output.into_raw_mode()?.suspend_raw_mode()?; does suspend raw mode but only until the end of the block, where the RawTerminal created by into_raw_mode is dropped, and the previous state is restored (which is likely still Raw mode I'd guess). See:

https://gitlab.redox-os.org/redox-os/termion/-/blob/master/src/raw.rs?ref_type=heads#L44-48

Also note that panic_cleanup().expect("failed to clean up for panic"); will prevent the panic hook running on the next line if the cleanup fails. This is likely not what you want.

That is to say, the website doc is wrong and untested and should be fixed. Would you want to submit a PR for this?

BTW, I'm curious about your rationale for choosing termion over crossterm for your app. Do you have any thoughts you can share on this choice?

@bugnano
Copy link
Author

bugnano commented Mar 30, 2024

From what I understand from the source that you linked ( https://gitlab.redox-os.org/redox-os/termion/-/blob/master/src/raw.rs?ref_type=heads#L44-48 ), creating a new io::stdout() / io::stderr() sets the prev_ios to the mode of the terminal when calling into_raw_mode(), so if the terminal is already in raw mode, prev_ios is already raw mode, so suspend_raw_mode() doesn't really exit raw mode.

One more reason to implement what I requested in #1006 , so that I can switch back and forth from raw mode at least during normal program execution, if not in the panic handler😉

BTW, I'm curious about your rationale for choosing termion over crossterm for your app. Do you have any thoughts you can share on this choice?

I chose Termion for 3 main reasons:

  1. What I'm developing is Linux-only
  2. Termion is lighter and has fewer dependencies than Crossterm
  3. I prefer Termion's Input/Event handling APIs compared to Crossterm

@joshka
Copy link
Member

joshka commented Mar 30, 2024

From what I understand

yep, that's basically how the underlying libc call works:

man cfmakeraw:

The cfmakeraw() function modifies the flags stored in the termios structure to a state disabling all input and output processing, giving a “raw I/O path”. It should be noted that there is no function to reverse this effect. Because a variety of processing options could be re-enabled, the correct method is for an application to snapshot the current terminal state using the function tcgetattr(), setting raw mode with cfmakeraw() and the subsequent tcsetattr(), and then using another tcsetattr() with the saved state to revert to the previous terminal state.

...

One more reason to implement what I requested in #1006 , so that I can switch back and forth from raw mode at least during normal program execution, if not in the panic handler😉

yep agreed

I chose Termion for 3 main reasons:

Makes sense :)

I think there's still likely a problem with having a panic handler and using raw mode with termion, and that can probably only really be fixed by termion (or by providing your own code to do basically the same thing as termion does earlier on in the calls (save the termios value and restore it when panicking)..

@joshka
Copy link
Member

joshka commented Apr 1, 2024

I think there's still likely a problem with having a panic handler and using raw mode with termion, and that can probably only really be fixed by termion (or by providing your own code to do basically the same thing as termion does earlier on in the calls (save the termios value and restore it when panicking)..

See https://gitlab.redox-os.org/redox-os/termion/-/issues/176 for more discussion on this.

@bugnano
Copy link
Author

bugnano commented Apr 2, 2024

I managed to write a panic handler that works:

use std::{
    io::{self, Write},
    panic,
};

use anyhow::{Context, Result};
use ratatui::prelude::*;
use termion::{input::MouseTerminal, raw::IntoRawMode, screen::IntoAlternateScreen};

fn initialize_panic_handler() -> Result<()> {
    let raw_output = io::stdout().into_raw_mode()?;

    raw_output.suspend_raw_mode()?;

    let panic_hook = panic::take_hook();
    panic::set_hook(Box::new(move |panic| {
        let panic_cleanup = || -> Result<()> {
            let mut output = io::stdout();
            write!(
                output,
                "{}{}{}",
                termion::clear::All,
                termion::screen::ToMainScreen,
                termion::cursor::Show
            )?;
            raw_output.suspend_raw_mode()?;
            output.flush()?;
            Ok(())
        };
        panic_cleanup().expect("failed to clean up for panic");
        panic_hook(panic);
    }));

    Ok(())
}

fn main() -> Result<()> {
    initialize_panic_handler().context("failed to initialize panic handler")?;

    let stdout = MouseTerminal::from(
        io::stdout()
            .into_raw_mode()
            .context("failed to enable raw mode")?
            .into_alternate_screen()
            .context("unable to enter alternate screen")?,
    );

    let mut terminal =
        Terminal::new(TermionBackend::new(stdout)).context("creating terminal failed")?;

In the initialize_panic_handler function I create a raw terminal, and immediately exit raw mode, so that the raw terminal has the correct prev_ios field set.
In the panic handler I reference that raw terminal to exit raw mode, and it works as long as initialize_panic_handler is called before entering raw mode in the main function.

So if you want to update the docs with a working implementation, there you have it😉

Note that this doesn't solve the need for #1006 and #991 .

joshka added a commit to ratatui-org/ratatui-website that referenced this issue Apr 4, 2024
see ratatui-org/ratatui#1005

move how-to-panic code to rust project and rewrite article

---------

Co-authored-by: Josh McKinney <joshka@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants