mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(tui2): fix screen corruption (#8463)
Summary
Fixes intermittent screen corruption in tui2 (random stale characters)
by
addressing two terminal state desyncs: nested alt-screen transitions and
the
first-draw viewport clear.
- Make alt-screen enter/leave re-entrant via a small nesting guard so
closing
- Ensure the first viewport draw clears after the viewport is sized,
preventing
old terminal contents from leaking through when diff-based rendering
skips
space cells.
- Add docs + a small unit test for the alt-screen nesting behavior.
Testing
- cargo test -p codex-tui2
- cargo clippy -p codex-tui2 --all-features --tests
- Manual:
- Opened the transcript overlay and dismissed it repeatedly; verified
the
normal view redraws cleanly with no leftover characters.
- Ran tui2 in a new folder with no trust settings (and also cleared the
trust setting from config to re-trigger the prompt); verified the
initial
trust/onboarding screen renders without artifacts.
This commit is contained in:
committed by
GitHub
Unverified
parent
e27d9bd88f
commit
310f2114ae
@@ -46,6 +46,7 @@ use crate::tui::job_control::SUSPEND_KEY;
|
||||
#[cfg(unix)]
|
||||
use crate::tui::job_control::SuspendContext;
|
||||
|
||||
mod alt_screen_nesting;
|
||||
mod frame_requester;
|
||||
#[cfg(unix)]
|
||||
mod job_control;
|
||||
@@ -131,6 +132,7 @@ pub struct Tui {
|
||||
draw_tx: broadcast::Sender<()>,
|
||||
pub(crate) terminal: Terminal,
|
||||
pending_history_lines: Vec<Line<'static>>,
|
||||
alt_screen_nesting: alt_screen_nesting::AltScreenNesting,
|
||||
alt_saved_viewport: Option<ratatui::layout::Rect>,
|
||||
#[cfg(unix)]
|
||||
suspend_context: SuspendContext,
|
||||
@@ -159,6 +161,7 @@ impl Tui {
|
||||
draw_tx,
|
||||
terminal,
|
||||
pending_history_lines: vec![],
|
||||
alt_screen_nesting: alt_screen_nesting::AltScreenNesting::default(),
|
||||
alt_saved_viewport: None,
|
||||
#[cfg(unix)]
|
||||
suspend_context: SuspendContext::new(),
|
||||
@@ -305,6 +308,10 @@ impl Tui {
|
||||
/// Enter alternate screen and expand the viewport to full terminal size, saving the current
|
||||
/// inline viewport for restoration when leaving.
|
||||
pub fn enter_alt_screen(&mut self) -> Result<()> {
|
||||
if !self.alt_screen_nesting.enter() {
|
||||
self.alt_screen_active.store(true, Ordering::Relaxed);
|
||||
return Ok(());
|
||||
}
|
||||
let _ = execute!(self.terminal.backend_mut(), EnterAlternateScreen);
|
||||
if let Ok(size) = self.terminal.size() {
|
||||
self.alt_saved_viewport = Some(self.terminal.viewport_area);
|
||||
@@ -322,6 +329,11 @@ impl Tui {
|
||||
|
||||
/// Leave alternate screen and restore the previously saved inline viewport, if any.
|
||||
pub fn leave_alt_screen(&mut self) -> Result<()> {
|
||||
if !self.alt_screen_nesting.leave() {
|
||||
self.alt_screen_active
|
||||
.store(self.alt_screen_nesting.is_active(), Ordering::Relaxed);
|
||||
return Ok(());
|
||||
}
|
||||
let _ = execute!(self.terminal.backend_mut(), LeaveAlternateScreen);
|
||||
if let Some(saved) = self.alt_saved_viewport.take() {
|
||||
self.terminal.set_viewport_area(saved);
|
||||
@@ -368,8 +380,17 @@ impl Tui {
|
||||
let area = Rect::new(0, 0, size.width, height.min(size.height));
|
||||
if area != terminal.viewport_area {
|
||||
// TODO(nornagon): probably this could be collapsed with the clear + set_viewport_area above.
|
||||
terminal.clear()?;
|
||||
terminal.set_viewport_area(area);
|
||||
if terminal.viewport_area.is_empty() {
|
||||
// On the first draw the viewport is empty, so `Terminal::clear()` is a no-op.
|
||||
// If we don't clear after sizing the viewport, diff-based rendering may skip
|
||||
// writing spaces (because "space" == "space" in the buffers) and stale terminal
|
||||
// contents can leak through as random characters between words.
|
||||
terminal.set_viewport_area(area);
|
||||
terminal.clear()?;
|
||||
} else {
|
||||
terminal.clear()?;
|
||||
terminal.set_viewport_area(area);
|
||||
}
|
||||
}
|
||||
|
||||
// Update the y position for suspending so Ctrl-Z can place the cursor correctly.
|
||||
|
||||
@@ -0,0 +1,81 @@
|
||||
//! Alternate-screen nesting guard.
|
||||
//!
|
||||
//! The main `codex-tui2` UI typically runs inside the terminal’s alternate screen buffer so the
|
||||
//! full viewport can be used without polluting normal scrollback. Some sub-flows (e.g. pager-style
|
||||
//! overlays) also call `enter_alt_screen()`/`leave_alt_screen()` for historical reasons.
|
||||
//!
|
||||
//! Those calls are conceptually “idempotent” (the UI is already on the alt screen), but the
|
||||
//! underlying terminal commands are *not*: issuing a real `LeaveAlternateScreen` while the rest of
|
||||
//! the app still thinks it is drawing on the alternate buffer desynchronizes rendering and can
|
||||
//! leave stale characters behind when returning to the normal view.
|
||||
//!
|
||||
//! `AltScreenNesting` tracks a small nesting depth so only the outermost enter/leave actually
|
||||
//! toggles the terminal mode.
|
||||
|
||||
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) struct AltScreenNesting {
|
||||
depth: u16,
|
||||
}
|
||||
|
||||
impl AltScreenNesting {
|
||||
pub(crate) fn is_active(self) -> bool {
|
||||
self.depth > 0
|
||||
}
|
||||
|
||||
/// Record an enter-alt-screen request.
|
||||
///
|
||||
/// Returns `true` when the caller should actually enter the alternate screen.
|
||||
pub(crate) fn enter(&mut self) -> bool {
|
||||
if self.depth == 0 {
|
||||
self.depth = 1;
|
||||
true
|
||||
} else {
|
||||
self.depth = self.depth.saturating_add(1);
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
/// Record a leave-alt-screen request.
|
||||
///
|
||||
/// Returns `true` when the caller should actually leave the alternate screen.
|
||||
pub(crate) fn leave(&mut self) -> bool {
|
||||
match self.depth {
|
||||
0 => false,
|
||||
1 => {
|
||||
self.depth = 0;
|
||||
true
|
||||
}
|
||||
_ => {
|
||||
self.depth = self.depth.saturating_sub(1);
|
||||
false
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::AltScreenNesting;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn alt_screen_nesting_tracks_outermost_transitions() {
|
||||
let mut nesting = AltScreenNesting::default();
|
||||
assert_eq!(false, nesting.is_active());
|
||||
|
||||
assert_eq!(true, nesting.enter());
|
||||
assert_eq!(true, nesting.is_active());
|
||||
|
||||
assert_eq!(false, nesting.enter());
|
||||
assert_eq!(true, nesting.is_active());
|
||||
|
||||
assert_eq!(false, nesting.leave());
|
||||
assert_eq!(true, nesting.is_active());
|
||||
|
||||
assert_eq!(true, nesting.leave());
|
||||
assert_eq!(false, nesting.is_active());
|
||||
|
||||
assert_eq!(false, nesting.leave());
|
||||
assert_eq!(false, nesting.is_active());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user