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 a panic hook to reset terminal upon panic #83

Closed
orhun opened this issue Aug 9, 2023 · 10 comments
Closed

Add a panic hook to reset terminal upon panic #83

orhun opened this issue Aug 9, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@orhun
Copy link
Owner

orhun commented Aug 9, 2023

Is your feature request related to a problem? Please describe.

When an unexpected error happens, the terminal is messed up.

Describe the solution you'd like

Add a custom panic hook. See: ratatui/templates#13

The code changes on this PR will add a panic hook, so if a problem happens and the program panics, before the program closes it will run the code inside the panic hook that will leave alternate screen and disable raw mode. This way, you won't have your terminal messed up if an unexpected error happens.

Describe alternatives you've considered

None.

Additional context

The implementation might be the same as the linked pull request above.

@orhun orhun added enhancement New feature or request good first issue Good for newcomers labels Aug 9, 2023
@orhun orhun self-assigned this Aug 9, 2023
@eld4niz
Copy link
Contributor

eld4niz commented Apr 1, 2024

hey orhun, how can I produce unexpected error in kmon? If you can mention some keybindings or any tips, I'm happy to contribute for this issue.

@orhun
Copy link
Owner Author

orhun commented Apr 1, 2024

Hey @eld4niz, you can simply trigger a panic by adding panic! to one of the key handlers like so:

diff --git a/src/lib.rs b/src/lib.rs
index 4edb06a..810cbce 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -118,6 +118,7 @@ where
 						| Key::Ctrl('c')
 						| Key::Ctrl('d')
 						| Key::Esc => {
+							panic!("!");
 							if app.show_options {
 								app.show_options = false;
 							} else {

So when you press q the application will panic. What we are trying to achieve here is handling this panic with a custom panic hook to not mess up the terminal.

By the way, there is an ongoing discussion about panic handler and termion here: ratatui/ratatui#1005

@eld4niz
Copy link
Contributor

eld4niz commented Apr 1, 2024

@orhun I'm trying to do this within kmon: save the termios value and restore it when panicking

Specifically I want to implement this functionality: termion raw.rs

Just with basic search I found they used this libc:

pub use self::libc::termios as Termios;

should I try to implement same functionality in raw.rs in our util.rs file? Or is it more deep problem that we shouldn't implement on kmon? If you have any suggestion, I'm open to discuss about it.

@orhun
Copy link
Owner Author

orhun commented Apr 1, 2024

Not sure if we should go that deep. But I'm happy to see how using that functionality looks before commenting too early.

I was thinking we can simply go with this: https://ratatui.rs/how-to/develop-apps/panic-hooks/#termion

@eld4niz
Copy link
Contributor

eld4niz commented Apr 2, 2024

I implemented the same as documentation in our main.rs file, but as we mentioned in the issue earlier, it does not work correctly.

let panic_hook = panic::take_hook();

panic::set_hook(Box::new(move |panic| {

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

@orhun
Copy link
Owner Author

orhun commented Apr 2, 2024

There is a new comment here: ratatui/ratatui#1005 (comment)

@eld4niz
Copy link
Contributor

eld4niz commented Apr 2, 2024

hey @orhun, I implemented the function in that issue within our main.rs file. I was thinking we can create function in utils.rs file and call it before kernel and events in main.rs. What do you think?

main.rs

Screenshot from 2024-04-02 20-36-52

output

Screenshot from 2024-04-02 20-31-26

There's still problems with mouse(both in kitty/gnome terminal) as you can see from the output picture's last line. When you click left/right it returns ANSI characters(as I suppose). If you have suggestions, let me know.

PS: I'm still new with TUIs, I can make some basic mistakes :)

@orhun
Copy link
Owner Author

orhun commented Apr 2, 2024

This looks good! 🚀 Can you send a PR?

@eld4niz
Copy link
Contributor

eld4niz commented Apr 2, 2024

Yes, 5 dk'ya hazir :)

@eld4niz
Copy link
Contributor

eld4niz commented Apr 2, 2024

@orhun first I created pr that's fully messed up lol, then opened new one with correction

@orhun orhun closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants