From 3e4357d09284afd46d0da2cd1b165d9a79c6ab42 Mon Sep 17 00:00:00 2001 From: Victor Garcia Date: Thu, 7 May 2026 02:18:28 +0200 Subject: [PATCH] fix(FIX-015): render desync on scroll / window-resize / pane-resize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user wheel-scrolls, drags the window edge, or drags a pane divider, the rendered cell grid was painting stale cells inside the new viewport geometry — visible to the user as duplicate rows at multiple y-offsets, displaced chart-art in TUIs like htop, or content that required a keystroke to "kick" the screen back into a coherent state. Root cause: a cache coherence gap between libghostty-vt and our Rust mirror. `Terminal::resize` and the scroll_viewport_* functions correctly set `cache.screen_dirty = true`, but `sync_screen` only consulted libghostty's `GHOSTTY_RENDER_STATE_DATA_DIRTY` flag — never the Rust flag. libghostty doesn't always set its own dirty flag for viewport mutations (resize, scroll), so sync_screen would short-circuit and return the previous grid contents inside the new geometry. The bug had three triggers (wheel scroll, window resize, pane divider) all rooted in this one cache-coherence hole, with a coordination-bug aggravation between connect_resize and the 16 ms scrollbar poll. Fix is three coordinated landings entirely client-side, satisfying AD-007 (no daemon involvement), AD-008 (clients own VT semantics), AD-009 (no new polling on the data path), and AD-014 (palette resolution location unchanged): 1. crates/forgetty-vt/src/terminal.rs: sync_screen's early-return guard now honors `cache.screen_dirty` alongside libghostty's render-state dirty flag, so resize / scroll_viewport_delta / scroll_viewport_bottom force a re-extract on the next paint. 2. crates/forgetty-gtk/src/terminal.rs (connect_resize): the resize handler becomes the canonical writer of the scrollbar adjustment under `s.updating_scrollbar = true`, eliminating the race window where the 16 ms poll could read a different (total, offset, len) triple and trigger a divergent viewport scroll via the value-changed path. Scrollbar widget construction moves earlier so the resize handler can capture the adjustment. 3. crates/forgetty-gtk/src/terminal.rs (draw_terminal): top-of-frame re-read of `scrollbar_state()` updates `s.viewport_offset` from libghostty's truth, so the cells we draw and the viewport_offset we use to project absolute selection rows both come from one snapshot. Single FFI struct read, sub-µs. ADs: AD-007 / AD-008 / AD-009 / AD-014 all preserved (verified by audit pass). No new RPC, no new file write, no new parsing, no new FFI surface. crates/forgetty-vt/src/ffi.rs untouched. Perf: cargo bench --bench daemon_hotpath all benches comfortably under SPEC ceilings — pty_to_subscriber_idle 21.86 µs (23× under 500 µs), daemon_cold_start 27.23 ms (7.4× under 200 ms). FIX-015 doesn't touch daemon-side code (libghostty-vt is not linked by the daemon binary per V2-008/AD-007), so bench movement is host noise. Net: 3 files, +41 / -23 / +18 LoC. The forgetty-session/src/manager.rs change is an incidental cargo-fmt reformat (multi-line call → single-line, fits within rustfmt's 100-char limit) that is rustfmt-preferred and semantically identical. Filed retroactively in BACKLOG as FIX-015 (originally drafted as FIX-014 by a parallel session that never wrote the BACKLOG entry; the FIX-014 slot was claimed by the bundled command-palette removal in PR #35). --- crates/forgetty-gtk/src/terminal.rs | 42 ++++++++++++++++++-------- crates/forgetty-session/src/manager.rs | 9 +----- crates/forgetty-vt/src/terminal.rs | 13 ++++++-- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/crates/forgetty-gtk/src/terminal.rs b/crates/forgetty-gtk/src/terminal.rs index 77bcd75..e670d65 100644 --- a/crates/forgetty-gtk/src/terminal.rs +++ b/crates/forgetty-gtk/src/terminal.rs @@ -1194,10 +1194,19 @@ pub fn create_terminal( drawing_area.add_controller(scroll_controller); } + // --- Scrollbar (declared before connect_resize so the resize handler can update + // the adjustment directly under `updating_scrollbar` to coordinate with the + // 16 ms scrollbar poll; see FIX-015). + let adjustment = gtk4::Adjustment::new(0.0, 0.0, 0.0, 1.0, 10.0, 0.0); + let scrollbar = gtk4::Scrollbar::new(gtk4::Orientation::Vertical, Some(&adjustment)); + scrollbar.set_vexpand(true); + scrollbar.set_visible(false); + // --- Resize handler --- { let state = Rc::clone(&state); let cell_measured_resize = Rc::clone(&cell_measured); + let adj_resize = adjustment.clone(); drawing_area.connect_resize(move |da, width, height| { if !*cell_measured_resize.borrow() { return; @@ -1223,8 +1232,18 @@ pub fn create_terminal( s.cols = new_cols; s.rows = new_rows; s.terminal.resize(new_rows, new_cols); - let (_, off, _) = s.terminal.scrollbar_state(); + // Snapshot scrollbar state once after the resize and become the canonical + // writer of the adjustment for this event — closes the FIX-015 race where + // the 16 ms scrollbar poll could race with the resize handler and trigger + // a divergent viewport scroll via connect_value_changed. + let (total, off, len) = s.terminal.scrollbar_state(); s.viewport_offset = off; + s.updating_scrollbar = true; + adj_resize.set_lower(0.0); + adj_resize.set_upper(total as f64); + adj_resize.set_page_size(len as f64); + adj_resize.set_value(off as f64); + s.updating_scrollbar = false; if let (Some(ref dc), Some(pane_id)) = (s.daemon_client.clone(), s.daemon_pane_id) { let _ = dc.resize_pane(pane_id, new_rows as u16, new_cols as u16); } @@ -1234,12 +1253,7 @@ pub fn create_terminal( }); } - // --- Scrollbar --- - let adjustment = gtk4::Adjustment::new(0.0, 0.0, 0.0, 1.0, 10.0, 0.0); - let scrollbar = gtk4::Scrollbar::new(gtk4::Orientation::Vertical, Some(&adjustment)); - scrollbar.set_vexpand(true); - scrollbar.set_visible(false); - + // --- Scrollbar value-changed (user scroll via scrollbar widget) --- { let state = Rc::clone(&state); let da_scroll = drawing_area.clone(); @@ -2326,11 +2340,6 @@ fn draw_terminal( // Clone search state for rendering let search = s.search.clone(); - // Query viewport offset for converting absolute selection rows to screen rows. - // Selection coordinates are stored as absolute scrollback positions; we need - // the viewport offset to map them back to screen-space for drawing. - let viewport_offset = s.viewport_offset as usize; - // Build font description using the current zoom level let font_desc = font_description_with_size(&s.config, s.font_size); @@ -2360,6 +2369,15 @@ fn draw_terminal( } } + // FIX-015 step 3: re-read libghostty's scrollbar state at the top of every paint + // so the cells we draw and the viewport offset we use for absolute→screen row + // projection (selection, search highlight) are consistent within one frame. + // `scrollbar_state()` is a single FFI struct read — sub-µs — far under the + // per-frame paint budget. + let (_sb_total, sb_offset, _sb_len) = s.terminal.scrollbar_state(); + s.viewport_offset = sb_offset; + let viewport_offset = sb_offset as usize; + let cell_w = s.cell_width; let cell_h = s.cell_height; diff --git a/crates/forgetty-session/src/manager.rs b/crates/forgetty-session/src/manager.rs index f07bf43..056c396 100644 --- a/crates/forgetty-session/src/manager.rs +++ b/crates/forgetty-session/src/manager.rs @@ -889,14 +889,7 @@ impl SessionManager { size: PtySize, cwd: Option, ) -> Result { - self.split_pane_with_ratio_and_pane_id( - pane_id, - direction, - ratio, - size, - cwd, - PaneId::new(), - ) + self.split_pane_with_ratio_and_pane_id(pane_id, direction, ratio, size, cwd, PaneId::new()) } /// Like `split_pane_with_ratio`, but uses the supplied `new_pane_id` instead of diff --git a/crates/forgetty-vt/src/terminal.rs b/crates/forgetty-vt/src/terminal.rs index 8a3734a..3715a10 100644 --- a/crates/forgetty-vt/src/terminal.rs +++ b/crates/forgetty-vt/src/terminal.rs @@ -609,9 +609,16 @@ impl Terminal { } let first_sync = cache.screen.generation() == 0; - tracing::trace!("sync_screen: dirty={dirty}, first_sync={first_sync}"); - - if dirty == ffi::GHOSTTY_RENDER_STATE_DIRTY_FALSE && !first_sync { + tracing::trace!( + "sync_screen: dirty={dirty}, first_sync={first_sync}, screen_dirty={}", + cache.screen_dirty + ); + + // `cache.screen_dirty` forces re-extraction even when libghostty's render-state + // dirty flag is clear: resize and viewport scroll mutate the libghostty viewport + // but don't always set the render-state dirty flag, and a stale cache in the new + // geometry/offset is the FIX-015 root cause. + if dirty == ffi::GHOSTTY_RENDER_STATE_DIRTY_FALSE && !first_sync && !cache.screen_dirty { return; }