-
Notifications
You must be signed in to change notification settings - Fork 92
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
Leave alt screen on panic #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall! Just a couple of thoughts but nothing blocking.
impl Drop for TerminalManager { | ||
fn drop(&mut self) { | ||
disable_raw_mode().unwrap_or_else(|e| eprintln!("Error disabling raw mode: {:?}", e)); | ||
execute!(self.terminal.backend_mut(), LeaveAlternateScreen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should swap this with the previous line, I think. Need to send the ANSI resource sequence before disabling raw mode - or at least before bailing if raw mode fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped
execute!(stdout, EnterAlternateScreen)?; | ||
let backend = CrosstermBackend::new(stdout); | ||
let mut terminal = Terminal::new(backend)?; | ||
terminal.clear()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you still need to do this? Wasn't sure why this was here anyway since you're switching to the alt buffer, so maybe it was just unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't. I also removed the clear()
from the Drop impl. It was incorrectly clearing the primary screen.
Move the terminal setup and cleanup logic to a new `TaskManager` struct. This struct is responsible for setting up the terminal and cleanup up when the program exits. Terminal cleanup will now happen on panic.
1ab8e9f
to
7ebd674
Compare
Move the terminal setup and cleanup logic to a new
TaskManager
struct. This struct is responsible for setting up the terminal and cleanup up when the program exits. Terminal cleanup will now happen on panic.Closes #41