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

Add pre-hook callbacks #184

Open
joshka opened this issue Jul 2, 2024 · 3 comments
Open

Add pre-hook callbacks #184

joshka opened this issue Jul 2, 2024 · 3 comments

Comments

@joshka
Copy link
Contributor

joshka commented Jul 2, 2024

#183 adds the ability to add custom pre-hook callbacks to the panic /
error hook. These callbacks will be called before the panic / error hook
is called and can be used to print additional information or perform
other actions before the panic / error hook is called (such as clearing
the terminal, printing a message, etc.)

Often in an interactive TUI application, the terminal state is set to
raw mode (where newlines do not automatically cause the cursor to move
to the start of the next line), and is in the alternate screen buffer
(which is a separate screen buffer that is used for full-screen apps).

This means that when a panic occurs, the terminal will display the panic
message all janky. By adding a pre-hook callback that restores the
terminal state to normal mode, the panic message can be displayed
properly.

HookBuilder::default()
    .add_pre_hook_callback(Box::new(|| eprintln!("pre-hook callback")))
    .install()?

In a crossterm based app, that might look like the following instead of
the more lengthy code in https://ratatui.rs/recipes/apps/color-eyre/

use crossterm::terminal::{disable_raw_mode, LeaveAlternateScreen};

HookBuilder::default()
    .add_pre_hook_callback(Box::new(|| {
        let _ = disable_raw_mode();
        let _ = execute!(stdout(), LeaveAlternateScreen);
    }))
    .install()?

I marked the PR as draft as i'd like to do the error hook side as well, but there's a bit more complexity to that side and I'd like to get some feedback on the idea first.

Some questions:

  1. Would it make sense to have the panic info passed to this callback (this might be good for callbacks that need to sanitize the info in the panic to remove confidential data)
  2. Would it make sense to have pre_panic_hook callbacks separate from pre_error_hook callbacks?

For the benefit of future devs reading this issue, and in case things change, the Ratatui code for configuring custom hooks from the link above generally looks like the following:

/// This replaces the standard color_eyre panic and error hooks with hooks that
/// restore the terminal before printing the panic or error.
pub fn install_hooks() -> color_eyre::Result<()> {
    // add any extra configuration you need to the hook builder
    let hook_builder = color_eyre::config::HookBuilder::default();
    let (panic_hook, eyre_hook) = hook_builder.into_hooks();

    // convert from a color_eyre PanicHook to a standard panic hook
    let panic_hook = panic_hook.into_panic_hook();
    panic::set_hook(Box::new(move |panic_info| {
        tui::restore().unwrap();
        panic_hook(panic_info);
    }));

    // convert from a color_eyre EyreHook to a eyre ErrorHook
    let eyre_hook = eyre_hook.into_eyre_hook();
    eyre::set_hook(Box::new(move |error| {
        tui::restore().unwrap();
        eyre_hook(error)
    }))?;

    Ok(())
}
@LeoniePhiline
Copy link
Contributor

I have definitely had need for that in the past, and I fully support this proposal.

@joshka
Copy link
Contributor Author

joshka commented Jul 18, 2024

ping

@joshka
Copy link
Contributor Author

joshka commented Aug 27, 2024

In #183 (comment) I wrote:


It turns out that my mental model for how the eyre_hook worked was a bit wrong (as noted in ratatui/ratatui#1277). I thought it acted similarly to the panic hook (catch unhandled errors at the main program level and run some code then). Instead, this triggers when running the display / debug methods on Report, and it's the Termination implementation of Result that handles the actual writing to the stderr. As such maybe this is the wrong thing to do at this level (at least for the eyre hook).

The use case mentioned in ratatui/ratatui#1277 was that the user wanted to have recoverable color-eyre reports that are presented to the user. These shouldn't run the pre-hook that would have been setup here. This means the code that I suggested would be inserted in the pre-hook makes more sense to put in the main function.

That leaves the panic hook. There's another approach to handling that hook that is to just replace the panic hook with one that calls the pre-hook first. This is probably a better approach. E.g.:

color_eyre::install()?;
let hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |pi| { pre_hook(); hook(pi); });

I'm inclined to close this on that basis.


I've closed the linked PR based on that.

In Ratatui 0.28.1, we install a panic hook in ratatui::init() that replaces any previously set hook with one that restores the terminal like mentioned. In the standardized main methods for all the examples we now see these as:

fn main() -> Result<()> {
    color_eyre::install()?;
    let terminal = ratatui::init();
    let app_result = run(terminal);
    ratatui::restore();
    app_result
}

This handles everything we needed with a pre-hook callback.

@LeoniePhiline can you perhaps give some more information on your use case if it's different to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants