diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee789cf4..a0a586e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1104,10 +1104,19 @@ jobs: run: | results="${{ needs.rust-lint.result }} ${{ needs.harness-unit.result }} ${{ needs.themes-parity.result }} ${{ needs.rust-build.result }} ${{ needs.swift-mac.result }} ${{ needs.gtk-build.result }} ${{ needs.iced-build-e2e.result }} ${{ needs.e2e-gtk.result }} ${{ needs.e2e-mac.result }}" echo "job results: ${results}" + # Allowlist, not a denylist. A denylist of failure/cancelled let + # `abandoned` through — the status GitHub assigns when its own + # infrastructure kills a job — so a run where 7 of 9 jobs never + # executed reported this gate GREEN. Observed on PR #306 during a + # GitHub "Failed to resolve action download info" incident. + # Anything that is not an actual pass now fails the gate. for r in ${results}; do - if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then - echo "::error::a CI job failed (${results})" - exit 1 - fi + case "${r}" in + success|skipped) ;; + *) + echo "::error::a required CI job did not pass: '${r}' (${results})" + exit 1 + ;; + esac done echo "all good" diff --git a/Makefile b/Makefile index 60e802fa..b5d03267 100644 --- a/Makefile +++ b/Makefile @@ -67,14 +67,19 @@ run-mac: bundle ## Launch the bundled Mac app # ---- test ------------------------------------------------------------- -.PHONY: test test-rust test-iced test-mac test-harness e2e e2e-gtk e2e-iced e2e-iced-clipboard e2e-mac e2e-gtk-ci e2e-iced-ci e2e-mac-ci smoke-gtk smoke-iced smoke-mac visual-parity smoke-mac-launch test-real-input test-iced-real-input test-iced-wayland-input check-iced +.PHONY: test test-rust test-iced test-mac test-harness e2e e2e-gtk e2e-iced e2e-iced-clipboard e2e-mac e2e-gtk-ci e2e-iced-ci e2e-mac-ci smoke-gtk smoke-iced smoke-mac visual-parity smoke-mac-launch test-real-input test-iced-real-input test-iced-wayland-input check-iced perf-refresh perf-render-stats ICED_E2E_TESTS := tools/roosttest/test_smoke.py tools/roosttest/test_iced_walking_skeleton.py tools/roosttest/test_notifications.py tools/roosttest/test_provider.py tools/roosttest/test_sidebar_pixels.py tools/roosttest/test_tab_strip_pixels.py tools/roosttest/test_focus.py tools/roosttest/test_palette.py tools/roosttest/test_z_typography.py tools/roosttest/test_project_lifecycle.py tools/roosttest/test_sidebar_resize.py ICED_CLIPBOARD_TESTS := tools/roosttest/test_selection.py tools/roosttest/test_osc52.py test: test-rust test-mac test-harness ## All unit/integration tests (Rust + Swift + harness) -test-rust: ## cargo test --workspace +# roost-vt's tests/*.rs all start with `#![cfg(feature = "ffi")]`, so the +# `--workspace` run compiles and then silently skips every one of them. The +# second line mirrors CI's separate `cargo test -p roost-vt --features ffi` +# step (.github/workflows/ci.yml, rust job) so `make test` runs them too. +test-rust: ## cargo test --workspace (+ roost-vt ffi tests, cfg-gated out of the default run) cargo test --workspace + cargo test -p roost-vt --features ffi test-iced: ## Iced unit tests (renderer + input + adapter) cargo test -p roost-iced @@ -140,6 +145,12 @@ test-iced-real-input: build-iced ## Iced real clipboard input (self-contained L test-iced-wayland-input: build-iced ## Iced system clipboard with cage + a real uinput seat ROOST_REQUIRE_REAL_INPUT=1 uv run --group test python tools/input/linux/iced_wayland_clipboard_check.py +perf-refresh: ## In-crate refresh_snapshot perf harness (release, --ignored; see tools/perf/README.md) + cargo test -p roost-iced --release -- --ignored --nocapture + +perf-render-stats: ## Render-path counters for a running Iced UI (tools/perf/render-stats.sh iced ; args for other targets) + tools/perf/render-stats.sh iced + # ---- code quality ----------------------------------------------------- .PHONY: fmt fmt-check clippy themes-check check diff --git a/crates/roost-cli/src/main.rs b/crates/roost-cli/src/main.rs index a98f313d..afc127e8 100644 --- a/crates/roost-cli/src/main.rs +++ b/crates/roost-cli/src/main.rs @@ -21,6 +21,7 @@ //! roostctl project {list,create,rename,delete,reorder} //! roostctl palette {open,state,query,activate,dismiss} //! roostctl screenshot [--out PATH] [--scale 1|2] +//! roostctl render-stats [--reset] //! roostctl claude-hook EVENT //! roostctl claude install [--force] //! @@ -45,13 +46,14 @@ use clap::{Parser, Subcommand, ValueEnum}; use roost_agent::claude::{canonical_hook_event, claude_event_to_reports, CLAUDE_HOOK_EVENTS}; use roost_ipc::messages::ops; use roost_ipc::messages::{ - IdentifyParams, IdentifyResult, NotificationCreateParams, PaletteActivateParams, - PaletteItemView, PaletteOpenParams, PalettePresentParams, PalettePresentResult, - PaletteQueryParams, PaletteStateResult, ProjectCreateParams, ProjectCreateResult, - ProjectDeleteParams, ProjectRenameParams, ProjectReorderParams, ScreenshotParams, - ScreenshotResult, TabClearNotificationParams, TabCloseParams, TabDumpParams, TabDumpResult, - TabFocusParams, TabListResult, TabOpenParams, TabOpenResult, TabReorderParams, TabResizeParams, - TabSetStateParams, TabSetTitleParams, TabState, TabWriteParams, + AppRenderStatsParams, AppRenderStatsResult, IdentifyParams, IdentifyResult, + NotificationCreateParams, PaletteActivateParams, PaletteItemView, PaletteOpenParams, + PalettePresentParams, PalettePresentResult, PaletteQueryParams, PaletteStateResult, + ProjectCreateParams, ProjectCreateResult, ProjectDeleteParams, ProjectRenameParams, + ProjectReorderParams, ScreenshotParams, ScreenshotResult, TabClearNotificationParams, + TabCloseParams, TabDumpParams, TabDumpResult, TabFocusParams, TabListResult, TabOpenParams, + TabOpenResult, TabReorderParams, TabResizeParams, TabSetStateParams, TabSetTitleParams, + TabState, TabWriteParams, }; use roost_ipc::paths::BundleProfileKind; use roost_ipc::target::{ResolvedTarget, TargetError, TargetSelector}; @@ -169,6 +171,16 @@ enum Cmd { #[arg(long, default_value_t = 1, value_parser = clap::value_parser!(u32).range(1..=2))] scale: u32, }, + /// Read the running UI's render-path counters — refresh/draw call + /// counts, elapsed nanos, rows and cells walked, `fill_text` calls. + /// The only way to measure the real draw path: it needs a live + /// renderer no unit test can construct. + RenderStats { + /// Zero the counters after reading them, so the next read is a + /// clean delta over whatever ran in between. + #[arg(long)] + reset: bool, + }, /// Claude Code hook entry point. Reads the JSON event payload /// from stdin (Claude's contract), dispatches state + /// notification ops to the running UI, and ALWAYS exits 0 with @@ -881,6 +893,33 @@ async fn main() -> Result<()> { } } } + Cmd::RenderStats { reset } => { + let stats: AppRenderStatsResult = client + .call(ops::APP_RENDER_STATS, AppRenderStatsParams { reset }) + .await?; + let per = |total: i64, calls: i64| { + if calls > 0 { + (total / calls).to_string() + } else { + "-".to_string() + } + }; + println!("refresh_calls {}", stats.refresh_calls); + println!("refresh_nanos {}", stats.refresh_nanos); + println!("rows_rebuilt {}", stats.rows_rebuilt); + println!("cells_walked {}", stats.cells_walked); + println!("draw_calls {}", stats.draw_calls); + println!("draw_nanos {}", stats.draw_nanos); + println!("fill_text_calls {}", stats.fill_text_calls); + println!( + "ns_per_refresh {}", + per(stats.refresh_nanos, stats.refresh_calls) + ); + println!( + "ns_per_draw {}", + per(stats.draw_nanos, stats.draw_calls) + ); + } Cmd::Palette(PaletteCmd::Open { kind, json }) => { let state: PaletteStateResult = client .call(ops::PALETTE_OPEN, PaletteOpenParams { kind }) diff --git a/crates/roost-engine/src/ipc.rs b/crates/roost-engine/src/ipc.rs index 336e09d8..db82f9f2 100644 --- a/crates/roost-engine/src/ipc.rs +++ b/crates/roost-engine/src/ipc.rs @@ -21,16 +21,16 @@ use std::sync::Arc; use roost_ipc::agent::{self, TabAgentReportParams}; use roost_ipc::messages::{ ops, AppActivateParams, AppActiveTerminalFocusedParams, AppActiveTerminalFocusedResult, - AppCursorShapeParams, AppCursorShapeResult, AppSelectedTabIdParams, AppSelectedTabIdResult, - AppSetWindowFocusParams, ClipboardDumpParams, ClipboardDumpResult, ClipboardWriteParams, - IdentifyParams, IdentifyResult, NotificationCreateParams, PaletteActivateParams, - PaletteDismissParams, PaletteOpenParams, PalettePresentParams, PalettePresentResult, - PaletteQueryParams, PaletteStateParams, PaletteStateResult, ProjectCreateParams, - ProjectCreateResult, ProjectDeleteParams, ProjectRenameParams, ProjectReorderParams, - ResolvedCell, ScreenshotParams, ScreenshotResult, SelectionClearParams, SelectionDumpParams, - SelectionDumpResult, SelectionSetParams, SidebarDumpParams, SidebarDumpResult, - SidebarSetWidthParams, TabAgentReportResult, TabCapturePtyInputParams, - TabCapturePtyInputResult, TabClearNotificationParams, TabCloseParams, + AppCursorShapeParams, AppCursorShapeResult, AppRenderStatsParams, AppRenderStatsResult, + AppSelectedTabIdParams, AppSelectedTabIdResult, AppSetWindowFocusParams, ClipboardDumpParams, + ClipboardDumpResult, ClipboardWriteParams, IdentifyParams, IdentifyResult, + NotificationCreateParams, PaletteActivateParams, PaletteDismissParams, PaletteOpenParams, + PalettePresentParams, PalettePresentResult, PaletteQueryParams, PaletteStateParams, + PaletteStateResult, ProjectCreateParams, ProjectCreateResult, ProjectDeleteParams, + ProjectRenameParams, ProjectReorderParams, ResolvedCell, ScreenshotParams, ScreenshotResult, + SelectionClearParams, SelectionDumpParams, SelectionDumpResult, SelectionSetParams, + SidebarDumpParams, SidebarDumpResult, SidebarSetWidthParams, TabAgentReportResult, + TabCapturePtyInputParams, TabCapturePtyInputResult, TabClearNotificationParams, TabCloseParams, TabDispatchMouseEventParams, TabDumpCursor, TabDumpParams, TabDumpResolvedParams, TabDumpResolvedResult, TabDumpResult, TabExpandSelectionAtParams, TabExpandSelectionAtResult, TabFeedPtyBytesParams, TabFocusParams, TabFocusResult, TabListResult, TabOpenParams, @@ -66,6 +66,12 @@ type WindowMetricsReply = tokio::sync::oneshot::Sender>; +/// Reply for [`UiRequest::AppRenderStats`]: the UI's render-path +/// counters. Read-only; always answers `Ok`, matching +/// `WindowMetricsReply`. A UI with no instrumentation answers with a +/// zeroed struct rather than an error — see the GTK arm. +type RenderStatsReply = tokio::sync::oneshot::Sender>; + /// Reply for a [`UiRequest::Dump`]: the viewport text on success, an /// error message (e.g. tab not found / no live terminal) on failure. type DumpReply = tokio::sync::oneshot::Sender>; @@ -268,6 +274,15 @@ pub enum UiRequest { /// collapsed flag (logical points). Backs the sidebar-holds-width /// regression suite. Ungated (read-only). WindowMetrics { reply: WindowMetricsReply }, + /// `app.render_stats` — read the UI's render-path counters, and + /// zero them afterward when `reset`. Ungated. Not read-only: with + /// `reset` it reads and then clears. The counters are the only way + /// to measure the real draw path, which needs a live renderer no + /// unit test can construct. + AppRenderStats { + reset: bool, + reply: RenderStatsReply, + }, /// `app.sidebar_dump` — read the sidebar's last-rendered agent rows /// per project, plus the agents-visible toggle. Ungated (read-only); /// reads `ProjectUi::rendered_agents`, the same cache the sidebar @@ -681,6 +696,17 @@ async fn dispatch( .map_err(|m| HandlerError::new("internal", m))?; encode(&result) } + ops::APP_RENDER_STATS => { + let p: AppRenderStatsParams = decode(params)?; + let result = h + .ui_call(|reply| UiRequest::AppRenderStats { + reset: p.reset, + reply, + }) + .await? + .map_err(|m| HandlerError::new("internal", m))?; + encode(&result) + } ops::SIDEBAR_DUMP => { let _p: SidebarDumpParams = decode(params)?; let result = h diff --git a/crates/roost-iced/src/app.rs b/crates/roost-iced/src/app.rs index df1574dd..66b14b2c 100644 --- a/crates/roost-iced/src/app.rs +++ b/crates/roost-iced/src/app.rs @@ -29,8 +29,8 @@ use roost_engine::{ }; use roost_ipc::agent; use roost_ipc::messages::{ - PaletteItemView, PalettePresentResult, PaletteStateResult, Project, SidebarDumpAgentRow, - SidebarDumpProject, SidebarDumpResult, WindowMetricsResult, + AppRenderStatsResult, PaletteItemView, PalettePresentResult, PaletteStateResult, Project, + SidebarDumpAgentRow, SidebarDumpProject, SidebarDumpResult, WindowMetricsResult, }; use roost_ipc::paths::BundleProfile; use roost_ipc::IpcServer; @@ -47,9 +47,9 @@ use roost_ui_model::{ }; use roost_url::HoverUrl; use roost_vt::{ - key_action, mouse_action, mouse_button, KeyEncoder, KeyEvent, MouseEncoder, MouseEvent, - PageDirection, PageRoute, RenderState, ScrollDirection, ScrollRoute, Terminal, TerminalOptions, - TerminalScroll, TerminalSelection, + key_action, mouse_action, mouse_button, ColorRgb, KeyEncoder, KeyEvent, MouseEncoder, + MouseEvent, PageDirection, PageRoute, RenderState, ScrollDirection, ScrollRoute, Terminal, + TerminalOptions, TerminalScroll, TerminalSelection, }; use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; @@ -60,7 +60,7 @@ use crate::palette_scroll::Visibility; use crate::sidebar_resize::SidebarResizeGrip; use crate::strip_reorder::{ReorderStrip, StripEvent}; use crate::terminal_widget::{ - resolve_colors, DrawCell, TerminalMetrics, TerminalPointerEvent, TerminalSnapshot, + DrawCell, RenderedRow, TerminalMetrics, TerminalPointerEvent, TerminalSnapshot, TerminalWheelEvent, TerminalWidget, TERMINAL_PADDING, }; use crate::Message; @@ -73,6 +73,12 @@ mod interactions; mod palettes; mod servicing; mod terminal_tab; +// The in-crate `#[ignore]`d perf harness — see `tools/perf/README.md` for +// how to run it. Gated on `cfg(test)` like `terminal_tab`'s test-only +// `attach_test_terminal` fixture it depends on; it carries no production +// code, so the whole module (not just an inner `mod tests`) is test-only. +#[cfg(test)] +mod perf_bench; pub(crate) use self::interactions::RenameTarget; use self::interactions::{ diff --git a/crates/roost-iced/src/app/perf_bench.rs b/crates/roost-iced/src/app/perf_bench.rs new file mode 100644 index 00000000..73b2dd9a --- /dev/null +++ b/crates/roost-iced/src/app/perf_bench.rs @@ -0,0 +1,154 @@ +//! In-crate performance harness for `TerminalTab::refresh_snapshot`. +//! +//! `roost-iced` has no `[lib]` target (`crates/roost-iced/Cargo.toml`), so +//! nothing outside the crate can import `TerminalTab` — an external +//! Criterion-style bench binary is not an option here. This lives +//! in-crate instead, `#[ignore]`d so it never runs under a normal `cargo +//! test -p roost-iced` or in CI. Invoke it explicitly: +//! +//! cargo test -p roost-iced --release -- --ignored --nocapture +//! +//! It reuses the `attach_test_terminal` fixture the rest of the app's +//! unit tests use and reads its numbers off each tab's own +//! `TabRenderStats`, not the process-global aggregate in `crate::perf` — +//! `cargo test` runs tests concurrently and a shared counter would be +//! polluted by whatever other test is refreshing its own tab at the same +//! moment (see `crate::perf`'s module doc for the same reasoning). +//! +//! This measures `refresh_snapshot` only. The `draw_*` / `fill_text_calls` +//! counters in `crate::perf::RenderStats` need a live iced `Renderer` that +//! only the windowing system hands `TerminalWidget::draw` — no unit test +//! constructs one, and there is no per-tab equivalent of them for the same +//! reason (see `TabRenderStats`'s doc comment). Read those with `roostctl +//! render-stats` against a running app instead — see `tools/perf/`. +use super::*; + +const ITERATIONS: usize = 200; + +struct WorkloadResult { + name: &'static str, + iterations: usize, + wall: Duration, + stats: crate::perf::TabRenderStats, +} + +impl WorkloadResult { + /// Stable, greppable `key value` lines — commit C7 diffs two runs of + /// this output (this commit vs. HEAD, via a `git worktree`) to build + /// the before/after table, so the format here must not drift. + fn print(&self) { + let ns_per_refresh = self + .stats + .refresh_nanos + .checked_div(self.stats.refresh_calls) + .map_or_else(|| "-".to_string(), |ns| ns.to_string()); + let rows_per_refresh = if self.stats.refresh_calls > 0 { + self.stats.rows_rebuilt as f64 / self.stats.refresh_calls as f64 + } else { + 0.0 + }; + println!("=== {} ===", self.name); + println!("iterations {}", self.iterations); + println!("wall_ns {}", self.wall.as_nanos()); + println!("refresh_calls {}", self.stats.refresh_calls); + println!("refresh_nanos {}", self.stats.refresh_nanos); + println!("ns_per_refresh {ns_per_refresh}"); + println!("rows_rebuilt {}", self.stats.rows_rebuilt); + println!("cells_walked {}", self.stats.cells_walked); + println!("rows_per_refresh {rows_per_refresh:.2}"); + println!(); + } +} + +/// Attach a fresh test terminal, run `iterations` steps of `mutate` + +/// `refresh_snapshot`, and return the per-tab counter delta. A fresh tab +/// per workload (rather than one shared tab) keeps each workload's +/// counters at exactly its own numbers with no baseline subtraction to +/// get wrong. +fn run_workload( + tab_id: i64, + name: &'static str, + iterations: usize, + mut mutate: impl FnMut(&mut TerminalTab, usize), +) -> WorkloadResult { + let (feed_tx, _feed_rx) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(tab_id, feed_tx); + assert_eq!( + tab.render_stats, + crate::perf::TabRenderStats::default(), + "a freshly attached tab has untouched counters" + ); + + let wall_started = Instant::now(); + for i in 0..iterations { + mutate(&mut tab, i); + tab.refresh_snapshot().expect("refresh_snapshot"); + } + let wall = wall_started.elapsed(); + + let stats = tab.render_stats; + supervisor.close(tab_id); + + WorkloadResult { + name, + iterations, + wall, + stats, + } +} + +/// Runs three workloads back to back and prints each one's per-tab +/// `refresh_snapshot` counters. No numbers are asserted on or persisted +/// here — this commit only builds the harness; C7 runs it against this +/// commit and against `HEAD` back to back via a `git worktree` (a +/// same-machine, same-moment A/B) and diffs the two printed tables. +/// +/// The three workloads are deliberately distinct shapes, not three sizes +/// of the same thing: +/// +/// - **W1 — pointer-motion storm.** No terminal mutation between +/// refreshes at all. This is what `interactions.rs`'s pointer-motion +/// handler produces: `refresh_or_warn` (wrapping `refresh_snapshot`) +/// runs on *every* mouse-motion event over a terminal +/// (`interactions.rs:1341`), even though nothing in the grid changed. +/// This is the headline case for dirty tracking — today it rebuilds +/// the entire grid for zero-content-change motion. +/// - **W2 — in-place TUI redraw.** Each iteration writes to a couple of +/// fixed rows via absolute cursor positioning (`\x1b[{row};1H...`), so +/// the viewport never scrolls. This is the vim/htop shape: a full +/// redraw of a bounded region, not a scroll. +/// - **W3 — scrolling stream (CONTROL).** Plain `line\r\n` output that +/// scrolls the viewport, same as a chatty build log. **This workload is +/// expected to show NO improvement, ever**: libghostty full-rebuilds +/// the render state whenever the viewport's scroll pin changes +/// (`third_party/ghostty/src/src/terminal/render.zig:299-302`). It is +/// in this harness precisely so a before/after table has a control +/// that stays flat — a future reader seeing W3 unchanged should read +/// that as the harness working correctly, not as something to "fix". +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "perf harness — run explicitly: cargo test -p roost-iced --release -- --ignored --nocapture"] +async fn refresh_snapshot_perf_harness() { + let w1 = run_workload(9_101, "W1 pointer-motion-storm", ITERATIONS, |_tab, _i| { + // Deliberately empty: the whole point of W1 is a refresh with no + // preceding terminal mutation. + }); + + let w2 = run_workload(9_102, "W2 in-place-tui-redraw", ITERATIONS, |tab, i| { + let row = 3 + (i % 2); // alternate two fixed rows; never scrolls + tab.write_vt(format!("\x1b[{row};1Hframe {i:04}").as_bytes()); + }); + + let w3 = run_workload( + 9_103, + "W3 scrolling-stream-CONTROL", + ITERATIONS, + |tab, i| { + tab.write_vt(format!("line-{i:04}\r\n").as_bytes()); + }, + ); + + println!(); + w1.print(); + w2.print(); + w3.print(); +} diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index ab4a0a81..f7c2ffd2 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -759,6 +759,21 @@ impl App { .ok_or_else(|| format!("tab {tab_id} has no live terminal")); let _ = reply.send(result); } + UiRequest::AppRenderStats { reset, reply } => { + let stats = crate::perf::snapshot(); + if reset { + crate::perf::reset(); + } + let _ = reply.send(Ok(AppRenderStatsResult { + refresh_calls: stats.refresh_calls as i64, + refresh_nanos: stats.refresh_nanos as i64, + rows_rebuilt: stats.rows_rebuilt as i64, + cells_walked: stats.cells_walked as i64, + draw_calls: stats.draw_calls as i64, + draw_nanos: stats.draw_nanos as i64, + fill_text_calls: stats.fill_text_calls as i64, + })); + } UiRequest::WindowMetrics { reply } => { let collapsed = self.workspace.sidebar_collapsed(); let resolved_family = self @@ -1304,7 +1319,7 @@ mod tests { assert!(collected.error.is_none()); let tab = tabs.get_mut(&70).expect("the tab is still attached"); tab.refresh_snapshot().expect("refresh the touched tab"); - assert_eq!(tab.snapshot.rows_text[0], "hello"); + assert_eq!(tab.snapshot.grid[0].text, "hello"); supervisor.close(70); } @@ -1509,4 +1524,433 @@ mod tests { ); assert_eq!(workspace.active().1, other.id, "and moves nothing"); } + + /// Pins the per-tab counters `refresh_snapshot` maintains. These are + /// asserted on the tab's own `TabRenderStats`, not the process-global + /// aggregate in `perf` — `cargo test -p roost-iced` runs concurrently + /// with other tests that spawn their own PTY and refresh their own + /// tab, and a global counter would pick up their activity too. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn refresh_snapshot_updates_the_tabs_own_render_stats() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(75, feed_tx); + assert_eq!(tab.render_stats, crate::perf::TabRenderStats::default()); + + tab.refresh_snapshot().expect("refresh"); + + assert_eq!(tab.render_stats.refresh_calls, 1); + assert_eq!( + tab.render_stats.rows_rebuilt, + u64::from(DEFAULT_ROWS), + "the first refresh has no cached grid, so it rebuilds every row" + ); + assert_eq!( + tab.render_stats.cells_walked, + u64::from(DEFAULT_COLS) * u64::from(DEFAULT_ROWS) + ); + assert!( + tab.render_stats.refresh_nanos > 0, + "refresh does real work, so elapsed time should be nonzero" + ); + + tab.refresh_snapshot().expect("second refresh"); + assert_eq!( + tab.render_stats.refresh_calls, 2, + "counters accumulate across calls rather than resetting" + ); + assert_eq!( + tab.render_stats.rows_rebuilt, + u64::from(DEFAULT_ROWS), + "nothing touched the terminal, so the second refresh rebuilds \ + zero rows and the total does not move" + ); + assert_eq!( + tab.render_stats.cells_walked, + u64::from(DEFAULT_COLS) * u64::from(DEFAULT_ROWS), + "and walks no cells either" + ); + + supervisor.close(75); + } + + /// The failure a per-row cache can silently produce is "right cells, + /// wrong row" — content landing one row off, or a stale row surviving + /// a rebuild. A substring search over the joined dump would not catch + /// either, so this writes a distinct marker to one row at a time and + /// checks the WHOLE row vector element-for-element after every write. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn incremental_rebuild_keeps_every_row_at_its_own_index() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(76, feed_tx); + // Absolute positioning only: a scroll would move every row and + // turn this into a full-rebuild test by accident. + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("refresh the cleared grid"); + + let rows = usize::from(DEFAULT_ROWS); + let mut expected = vec![String::new(); rows]; + // Out of order on purpose, and not every row: a cache that keys on + // walk order rather than the reported row index passes an + // in-order fill. + for (step, row) in [4usize, 1, rows - 1, 0, 9, 4].into_iter().enumerate() { + let marker = format!("marker-{step}-row-{row}"); + tab.write_vt(format!("\x1b[{};1H{marker}", row + 1).as_bytes()); + tab.refresh_snapshot().expect("refresh after the write"); + expected[row] = marker; + assert_eq!( + tab.dump().rows_text, + expected, + "after step {step} (row {row}) every row must hold exactly its own content" + ); + } + + supervisor.close(76); + } + + /// `TerminalSnapshot::blank` fills its rows with an empty string while + /// `refresh_snapshot` builds `" "`-filled rows and trims them. Both + /// must land on `""`, because `tab.dump` — and the whole e2e suite + /// through it — reads one before the first refresh and the other + /// after. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_blank_snapshot_and_a_refreshed_empty_grid_dump_the_same_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(77, feed_tx); + let blank = tab.dump().rows_text; + assert_eq!(blank, vec![String::new(); usize::from(DEFAULT_ROWS)]); + + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("refresh the cleared grid"); + assert_eq!( + tab.dump().rows_text, + blank, + "a refreshed empty grid trims down to the same rows blank starts at" + ); + + supervisor.close(77); + } + + /// `OSC 11` changes the terminal's default background with libghostty + /// reporting nothing dirty, so only `refresh_snapshot`'s cached-default + /// guard keeps cached rows from freezing at the old color. Without it + /// the untouched row below would keep rendering the old background. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn changing_the_default_background_rebuilds_cached_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(78, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[1;1Hcolored"); + tab.refresh_snapshot() + .expect("refresh with the row written"); + let before = tab.snapshot.background; + + let rebuilt_before = tab.render_stats.rows_rebuilt; + tab.write_vt(b"\x1b]11;rgb:00/00/ff\x07"); + tab.refresh_snapshot().expect("refresh after OSC 11"); + + assert_ne!( + tab.snapshot.background, before, + "OSC 11 must reach the render state's default background" + ); + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a default-color change invalidates every cached row" + ); + let resolved = tab.resolved_cells(); + let cell = resolved + .cells + .iter() + .find(|cell| cell.row == 0 && cell.col == 0) + .expect("row 0 col 0 is in the resolved grid"); + assert_eq!( + cell.bg, + ( + tab.snapshot.background.r, + tab.snapshot.background.g, + tab.snapshot.background.b + ), + "the rebuilt row resolves against the new default, not the cached one" + ); + + supervisor.close(78); + } + + /// `tab.dump_resolved` densifies the sparse per-row cells back into a + /// full grid. It is the one consumer that has to re-derive a cell's row + /// from its grid position now that `DrawCell` no longer carries one, so + /// it gets its own coverage: dense, row-major, and each cell resolved + /// against the row it actually came from. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn resolved_cells_densifies_the_grid_row_major_from_the_row_index() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(79, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[H"); + // Row 3 (1-based) col 2 (1-based) — off both axes' origin, so a + // transposed or off-by-one index cannot coincide with the truth. + tab.write_vt(b"\x1b[3;2H\x1b[1;41mX\x1b[0m"); + tab.refresh_snapshot().expect("refresh"); + + let resolved = tab.resolved_cells(); + assert_eq!(resolved.cols, DEFAULT_COLS); + assert_eq!(resolved.rows, DEFAULT_ROWS); + assert_eq!( + resolved.cells.len(), + usize::from(DEFAULT_COLS) * usize::from(DEFAULT_ROWS), + "the resolved grid is dense" + ); + for (index, cell) in resolved.cells.iter().enumerate() { + assert_eq!(cell.row, (index / usize::from(DEFAULT_COLS)) as u32); + assert_eq!(cell.col, (index % usize::from(DEFAULT_COLS)) as u16); + } + + let marked = &resolved.cells[2 * usize::from(DEFAULT_COLS) + 1]; + assert_eq!(marked.text, "X"); + assert!(marked.bold); + assert!(marked.has_explicit_bg); + let red = tab.theme.palette[1]; + assert_eq!( + marked.bg, + (red.r, red.g, red.b), + "SGR 41 resolves through the theme palette's red" + ); + + let neighbor = &resolved.cells[2 * usize::from(DEFAULT_COLS)]; + assert_eq!(neighbor.text, " "); + assert!(!neighbor.has_explicit_bg); + assert!(!neighbor.bold); + assert_eq!( + neighbor.bg, + ( + tab.snapshot.background.r, + tab.snapshot.background.g, + tab.snapshot.background.b + ), + "an untouched cell falls back to the terminal default" + ); + + supervisor.close(79); + } + + /// A single-row write with the cursor already parked on that row must + /// rebuild exactly one row — the headline claim `refresh_snapshot`'s + /// per-row cache makes. The cursor is parked and settled *before* the + /// write under test because libghostty dirties both the row the cursor + /// leaves and the row it lands on (pinned by + /// `crates/roost-vt/tests/render_dirty_test.rs`'s + /// `row_flags_are_cleared_alongside_the_global_layer`); moving and + /// writing in the same step would fold that cursor-motion row into the + /// count this test is trying to isolate. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_single_row_write_with_the_cursor_already_parked_rebuilds_exactly_that_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(80, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[H"); + tab.refresh_snapshot().expect("settle the cleared grid"); + + tab.write_vt(b"\x1b[3;1H"); + tab.refresh_snapshot() + .expect("settle with the cursor parked on row 2"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let cells_before = tab.render_stats.cells_walked; + tab.write_vt(b"X"); + tab.refresh_snapshot() + .expect("refresh after the single-row write"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + 1, + "the cursor was already on row 2, so writing to it dirties only that row" + ); + assert_eq!( + tab.render_stats.cells_walked - cells_before, + u64::from(DEFAULT_COLS), + "walk_dirty hands the whole row's cells to the one rebuilt row" + ); + + supervisor.close(80); + } + + /// `set_theme` bumps `theme_generation`, and `refresh_snapshot`'s + /// `cached_theme_generation` guard exists precisely to force a full + /// rebuild off that bump — today nothing but the default fg/bg pair + /// (already covered by the default-color guard) is theme-derived, but + /// the guard is there so a future theme-derived input (e.g. GTK's + /// `bold_color` override) fails safe toward over-rebuilding rather than + /// silently keeping stale rows. + /// + /// Measured while writing this test: `apply_theme_candidate`'s color + /// FFI calls (`set_color_foreground`/`background`/`cursor`/`palette`) + /// already report `Dirty::Full` on their own at our pinned Ghostty SHA + /// — pinned separately by `theme_color_changes_report_full` in + /// `crates/roost-vt/tests/render_dirty_test.rs` — so with a real theme + /// apply neither `cached_defaults` nor `cached_theme_generation` is + /// individually load-bearing for this test (confirmed: disabling both + /// at once still left it passing). What *does* make it fail is the + /// same class of bug as the resize guard above — the FFI calls + /// silently not reaching libghostty while `theme_generation` still + /// bumps: stubbing those calls out with the generation guard in place + /// still passed (`DEFAULT_ROWS`), and disabling the guard on top of + /// that stub dropped the rebuild to 0. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn applying_a_theme_rebuilds_every_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(81, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let dracula = Theme::load_bundled("Dracula"); + tab.set_theme(&dracula).expect("theme applies"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "set_theme's own refresh must rebuild every row, not just the changed ones" + ); + + supervisor.close(81); + } + + /// Pins the `(cols, rows)` cache-size guard against a narrower + /// row-count-only guard: a width-only resize leaves `self.rows` + /// unchanged, so a guard keyed on row count alone would miss it and + /// every cached row would keep rendering at the old column width. + /// + /// Measured while writing this test: at our pinned Ghostty SHA, + /// `Terminal::resize` itself always reports `Dirty::Full` regardless of + /// which axis moved (pinned separately by + /// `resize_reports_full_over_the_new_row_count` in + /// `crates/roost-vt/tests/render_dirty_test.rs`), so on the real + /// `apply_geometry` path this guard's own `mark_full` is currently a + /// redundant second line of defense, not the sole reason this test + /// passes. It stops being redundant, and this test starts actually + /// depending on it, the moment `apply_geometry`'s call into libghostty + /// silently no-ops while `self.cols`/`self.rows` still move — verified + /// by temporarily stubbing that call out during review: with the + /// `(cols, rows)` guard intact the rebuild count held at + /// `DEFAULT_ROWS`, and narrowing the guard to rows-only on top of that + /// stub dropped it to 0. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_width_only_resize_rebuilds_every_row() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(82, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let metrics = tab.applied_metrics.expect("installed metrics"); + let rebuilt_before = tab.render_stats.rows_rebuilt; + let change = tab + .apply_geometry( + DEFAULT_COLS + 10, + DEFAULT_ROWS, + metrics, + tab.metric_generation + 1, + ) + .expect("apply a width-only geometry change") + .expect("cols moved, so this is a real geometry change"); + assert!( + change.grid_changed, + "cols moved, so the grid-changed flag must fire even though rows did not" + ); + tab.commit_geometry(change); + tab.refresh_snapshot() + .expect("refresh after the width-only resize"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a width-only resize invalidates every cached row even though the row count is unchanged" + ); + + supervisor.close(82); + } + + /// Pins that a scrolled-back viewport is never served from the stale + /// row cache. None of `refresh_snapshot`'s three cache-key guards fire + /// here — grid size, defaults, and theme generation are all unchanged + /// by a page up — so this pins libghostty's own dirty reporting for a + /// viewport move (it reports every row dirty) rather than one of this + /// module's guards. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn scrolling_back_into_history_rebuilds_every_row_and_changes_the_text() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(83, feed_tx); + for line in 0..(usize::from(DEFAULT_ROWS) * 3) { + tab.write_vt(format!("history-{line:04}\r\n").as_bytes()); + } + tab.refresh_snapshot().expect("settle at the live bottom"); + let before_text = tab.dump().rows_text; + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let route = tab + .handle_page(PageDirection::Up) + .expect("page up into history"); + assert!( + matches!( + route, + PageRoute::LocalViewport { + scrolled_back: true + } + ), + "enough history exists that page up must move the local viewport: {route:?}" + ); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + u64::from(DEFAULT_ROWS), + "a viewport move rebuilds every row rather than reusing the live-bottom cache" + ); + assert_ne!( + tab.dump().rows_text, + before_text, + "the scrolled-back viewport must show different rows than the live bottom" + ); + + supervisor.close(83); + } + + /// The headline win of the dirty-tracking change: a hover-only motion + /// event — no button, no terminal mouse tracking — never writes to the + /// terminal, so `refresh_snapshot` must rebuild nothing even though it + /// still republishes the snapshot (pointer shape / hover overlay can + /// change independently of content). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_pointer_motion_refresh_with_no_terminal_change_rebuilds_zero_rows() { + let (feed_tx, _) = engine_feed::channel(); + let (mut tab, supervisor) = attach_test_terminal(84, feed_tx); + tab.write_vt(b"\x1b[2J\x1b[Hhello"); + tab.refresh_snapshot().expect("settle the written grid"); + + let rebuilt_before = tab.render_stats.rows_rebuilt; + let cells_before = tab.render_stats.cells_walked; + // What `App::pointer` does for a hover-only motion: dispatch through + // `handle_native_pointer`, then refresh. + tab.handle_native_pointer(NativePointerDispatch { + action: PointerAction::Motion, + button: None, + col: 2, + row: 0, + mods: 0, + click_count: 0, + inside: true, + link_modifier_held: false, + }) + .expect("hover motion dispatch"); + tab.refresh_snapshot() + .expect("refresh after the motion event"); + + assert_eq!( + tab.render_stats.rows_rebuilt - rebuilt_before, + 0, + "a motion event with no mouse tracking touches only overlay state, not content" + ); + assert_eq!( + tab.render_stats.cells_walked - cells_before, + 0, + "and walks no cells either" + ); + + supervisor.close(84); + } } diff --git a/crates/roost-iced/src/app/terminal_tab.rs b/crates/roost-iced/src/app/terminal_tab.rs index c239e0bd..48cfe9c8 100644 --- a/crates/roost-iced/src/app/terminal_tab.rs +++ b/crates/roost-iced/src/app/terminal_tab.rs @@ -202,10 +202,25 @@ pub(super) struct TerminalTab { pub(super) pointer_shape: String, pub(super) theme: Theme, pub(super) snapshot: TerminalSnapshot, + /// The per-row render cache `refresh_snapshot` maintains; the snapshot + /// gets a clone of it (O(rows) refcount bumps). The three `cached_*` + /// fields are the keys this cache is valid under — see + /// `refresh_snapshot`'s caching invariant. + grid: Vec>, + cached_grid_size: Option<(u16, u16)>, + cached_defaults: Option<(ColorRgb, ColorRgb)>, + cached_theme_generation: Option, + /// Bumped whenever a theme lands on this tab. Nothing theme-derived + /// besides the default fg/bg pair enters `RenderedRow::build` today, + /// but GTK's twin resolver already pulls the theme's `bold_color`, so + /// this fails the cache safe toward over-rebuilding if that override + /// ever lands here. + theme_generation: u64, cols: u16, rows: u16, pub(super) applied_metrics: Option, pub(super) metric_generation: u64, + pub(super) render_stats: crate::perf::TabRenderStats, } impl Drop for TerminalTab { @@ -282,10 +297,18 @@ impl TerminalTab { pointer_shape: "default".into(), theme, snapshot: TerminalSnapshot::blank(DEFAULT_COLS, DEFAULT_ROWS), + // Left empty on purpose: the first `refresh_snapshot` finds no + // cached grid size, sizes the grid and forces a full rebuild. + grid: Vec::new(), + cached_grid_size: None, + cached_defaults: None, + cached_theme_generation: None, + theme_generation: 0, cols: DEFAULT_COLS, rows: DEFAULT_ROWS, applied_metrics: None, metric_generation: 0, + render_stats: crate::perf::TabRenderStats::default(), }) } @@ -869,6 +892,9 @@ impl TerminalTab { fn apply_theme_candidate(&mut self, theme: &Theme) -> Result<()> { self.theme = theme.clone(); + // Every theme application — including `set_theme`'s rollback — + // lands here, so this is the one place the generation must move. + self.theme_generation = self.theme_generation.wrapping_add(1); self.terminal.set_color_foreground(theme.foreground)?; self.terminal.set_color_background(theme.background)?; self.terminal.set_color_cursor(theme.cursor)?; @@ -876,55 +902,79 @@ impl TerminalTab { self.refresh_snapshot() } + /// Rebuild the snapshot from the terminal, reusing every cached row + /// libghostty reports as unchanged. + /// + /// **Caching invariant.** A cached `RenderedRow` is valid exactly + /// while (a) libghostty reports its row undirty and (b) the inputs + /// `RenderedRow::build` reads besides the row's own vt cells — the + /// default fg/bg pair and the grid width — are unchanged. Everything + /// that alters what a row should render must therefore either mark + /// that row dirty inside libghostty or move one of the cache keys + /// guarded below. Anyone adding a third input to `RenderedRow::build` + /// must add a guard for it here. + /// + /// The default-color guard is not belt-and-braces: `OSC 10`/`OSC 11` + /// and DECSCNM (`CSI ?5h`) change the terminal's default fg/bg with + /// libghostty reporting `Clean` and no row flagged (measured; pinned + /// by `crates/roost-vt/tests/render_dirty_test.rs`). Since + /// `resolve_colors` folds those defaults into every cell that does not + /// set its own, a cached row would otherwise freeze at the old color. pub(super) fn refresh_snapshot(&mut self) -> Result<()> { + let refresh_started_at = Instant::now(); self.recompute_hover()?; self.render_state.update(&self.terminal)?; let colors = self.render_state.colors()?; let cursor = self.render_state.cursor(); - let mut cells = Vec::new(); - let mut rows = vec![vec![String::new(); usize::from(self.cols)]; usize::from(self.rows)]; - self.render_state.walk(&self.terminal, |row, cell| { - if row >= u32::from(self.rows) || cell.col >= self.cols { + + // Each guard raises the dirty state BEFORE recording its new key, + // so a failed `mark_full` leaves the key stale and the next + // refresh retries the invalidation rather than skipping it. + let size = (self.cols, self.rows); + if self.cached_grid_size != Some(size) { + // Both axes: a width-only resize leaves the row count alone + // while invalidating every cached row's column content. + self.render_state.mark_full()?; + // Every slot shares one empty row — rows are replaced + // wholesale by the walk below, never mutated in place. + let blank_row = Arc::new(RenderedRow::default()); + self.grid = vec![blank_row; usize::from(self.rows)]; + self.cached_grid_size = Some(size); + } + let defaults = (colors.foreground, colors.background); + if self.cached_defaults != Some(defaults) { + self.render_state.mark_full()?; + self.cached_defaults = Some(defaults); + } + if self.cached_theme_generation != Some(self.theme_generation) { + self.render_state.mark_full()?; + self.cached_theme_generation = Some(self.theme_generation); + } + + let cols = self.cols; + let grid = &mut self.grid; + let mut rows_rebuilt: u64 = 0; + let mut cells_walked: u64 = 0; + self.render_state.walk_dirty(&self.terminal, |row, cells| { + cells_walked += cells.len() as u64; + // Clamped against the cache's own length, not `self.rows`: + // the guard above keeps the two equal, and reading the length + // here means a row index past the end can never index out of + // bounds even if that ever stopped holding. + if row as usize >= grid.len() { return; } - let text = if cell.text.is_empty() { - " ".to_string() - } else { - cell.text - }; - rows[row as usize][usize::from(cell.col)] = text.clone(); - let (foreground, background) = resolve_colors( - cell.fg, - cell.bg, - (colors.foreground, colors.background), - cell.style.inverse, - ); - if text != " " || cell.bg.is_some() || cell.style.inverse { - cells.push(DrawCell { - row, - col: cell.col, - text, - foreground, - background, - explicit_background: cell.bg.is_some() || cell.style.inverse, - bold: cell.style.bold, - italic: cell.style.italic, - inverse: cell.style.inverse, - }); - } + grid[row as usize] = Arc::new(RenderedRow::build(cells, defaults, cols)); + rows_rebuilt += 1; })?; - let rows_text = rows - .into_iter() - .map(|row| row.concat().trim_end().to_string()) - .collect(); + self.snapshot = TerminalSnapshot { cols: self.cols, rows: self.rows, foreground: colors.foreground, background: colors.background, cursor, - cells, - rows_text, + grid: self.grid.clone(), selection_background: self.theme.selection_background, selection_spans: self .selection @@ -939,6 +989,10 @@ impl TerminalTab { }), pointer_shape: self.effective_pointer_shape().into(), }; + let elapsed = refresh_started_at.elapsed(); + self.render_stats + .record_refresh(elapsed, rows_rebuilt, cells_walked); + crate::perf::record_refresh(elapsed, rows_rebuilt, cells_walked); Ok(()) } @@ -951,21 +1005,26 @@ impl TerminalTab { .cursor .filter(|cursor| cursor.visible) .map(|cursor| (cursor.row, cursor.col, cursor.visible)), - rows_text: self.snapshot.rows_text.clone(), + rows_text: self + .snapshot + .grid + .iter() + .map(|row| row.text.clone()) + .collect(), } } pub(super) fn resolved_cells(&self) -> ResolvedCellsData { - let mut by_position: HashMap<(u32, u16), &DrawCell> = self - .snapshot - .cells - .iter() - .map(|cell| ((cell.row, cell.col), cell)) - .collect(); let mut cells = Vec::with_capacity(usize::from(self.cols) * usize::from(self.rows)); for row in 0..u32::from(self.rows) { + let mut by_col: HashMap = self + .snapshot + .grid + .get(row as usize) + .map(|rendered| rendered.cells.iter().map(|cell| (cell.col, cell)).collect()) + .unwrap_or_default(); for col in 0..self.cols { - let cell = by_position.remove(&(row, col)); + let cell = by_col.remove(&col); let foreground = cell.map_or(self.snapshot.foreground, |cell| cell.foreground); let background = cell.map_or(self.snapshot.background, |cell| cell.background); cells.push(ResolvedCellData { diff --git a/crates/roost-iced/src/main.rs b/crates/roost-iced/src/main.rs index 1e9d25a8..8cd49f4a 100644 --- a/crates/roost-iced/src/main.rs +++ b/crates/roost-iced/src/main.rs @@ -6,6 +6,7 @@ mod input; mod notifications; mod palette_scroll; mod paste_image; +mod perf; mod png_encode; mod screenshot; mod sidebar_resize; diff --git a/crates/roost-iced/src/perf.rs b/crates/roost-iced/src/perf.rs new file mode 100644 index 00000000..35fa3abd --- /dev/null +++ b/crates/roost-iced/src/perf.rs @@ -0,0 +1,120 @@ +//! Render performance instrumentation. Counters and timings only — this +//! commit changes no rendering behavior, it just measures the current one +//! so a later commit has a baseline to optimize against. +//! +//! Two scopes, deliberately: +//! +//! - [`TabRenderStats`] is a plain (non-atomic) per-`TerminalTab` struct. +//! `cargo test -p roost-iced` runs tests in parallel and the test fixture +//! spawns a real PTY per test, so several tests refresh concurrently; a +//! `after - before` delta read off a shared global counter would be +//! polluted by whatever other tests are doing at the same moment. Per-tab +//! counters are what unit tests assert on, and they sidestep that flake +//! class entirely by construction. +//! - [`snapshot`] reads a process-global `AtomicU64` aggregate that every +//! tab folds its work into. This is what the `app.render_stats` IPC op +//! reads out of a running app; no test asserts on it. +//! +//! Two traps to know about before trusting a number out of this module: +//! +//! - `iced::window::screenshot` re-renders the window, so `roostctl +//! screenshot` (and everything in `tools/screenshot/`) inflates +//! `draw_calls` / `draw_nanos` / `fill_text_calls` just by having run. +//! Read the counters before taking a screenshot, or [`reset`] afterward. +//! - The `draw_*` and `fill_text_calls` counters only exist in a running +//! app: `TerminalWidget::draw` needs a live iced `Renderer`, which unit +//! tests don't construct. There is no per-tab equivalent of them for the +//! same reason — see [`TabRenderStats`]. + +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Duration; + +static REFRESH_CALLS: AtomicU64 = AtomicU64::new(0); +static REFRESH_NANOS: AtomicU64 = AtomicU64::new(0); +static ROWS_REBUILT: AtomicU64 = AtomicU64::new(0); +static CELLS_WALKED: AtomicU64 = AtomicU64::new(0); +static DRAW_CALLS: AtomicU64 = AtomicU64::new(0); +static DRAW_NANOS: AtomicU64 = AtomicU64::new(0); +static FILL_TEXT_CALLS: AtomicU64 = AtomicU64::new(0); + +/// A read of the process-global aggregate at one instant. Every field is a +/// running total since process start (or the last [`reset`]). +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct RenderStats { + pub refresh_calls: u64, + pub refresh_nanos: u64, + pub rows_rebuilt: u64, + pub cells_walked: u64, + pub draw_calls: u64, + pub draw_nanos: u64, + pub fill_text_calls: u64, +} + +/// Per-tab counters, folded into the global aggregate on every refresh. +/// There is deliberately no per-tab equivalent of `draw_calls` / +/// `draw_nanos` / `fill_text_calls`: `TerminalWidget::draw` renders from a +/// `TerminalSnapshot` clone handed to it by iced and has no way back to the +/// `TerminalTab` that produced it, so those three counters only exist in +/// the global aggregate. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct TabRenderStats { + pub refresh_calls: u64, + pub refresh_nanos: u64, + pub rows_rebuilt: u64, + pub cells_walked: u64, +} + +impl TabRenderStats { + pub(crate) fn record_refresh( + &mut self, + elapsed: Duration, + rows_rebuilt: u64, + cells_walked: u64, + ) { + self.refresh_calls += 1; + self.refresh_nanos += elapsed.as_nanos() as u64; + self.rows_rebuilt += rows_rebuilt; + self.cells_walked += cells_walked; + } +} + +/// Read the global aggregate. Backs the `app.render_stats` IPC op. +pub fn snapshot() -> RenderStats { + RenderStats { + refresh_calls: REFRESH_CALLS.load(Ordering::Relaxed), + refresh_nanos: REFRESH_NANOS.load(Ordering::Relaxed), + rows_rebuilt: ROWS_REBUILT.load(Ordering::Relaxed), + cells_walked: CELLS_WALKED.load(Ordering::Relaxed), + draw_calls: DRAW_CALLS.load(Ordering::Relaxed), + draw_nanos: DRAW_NANOS.load(Ordering::Relaxed), + fill_text_calls: FILL_TEXT_CALLS.load(Ordering::Relaxed), + } +} + +/// Zero the global aggregate. Useful before an operation known to skew it +/// (see the screenshot trap above) so the next read is uncontaminated. +/// Exposed over IPC as `app.render_stats` with `reset: true`. +pub fn reset() { + REFRESH_CALLS.store(0, Ordering::Relaxed); + REFRESH_NANOS.store(0, Ordering::Relaxed); + ROWS_REBUILT.store(0, Ordering::Relaxed); + CELLS_WALKED.store(0, Ordering::Relaxed); + DRAW_CALLS.store(0, Ordering::Relaxed); + DRAW_NANOS.store(0, Ordering::Relaxed); + FILL_TEXT_CALLS.store(0, Ordering::Relaxed); +} + +/// Fold one `refresh_snapshot` call into the global aggregate. +pub(crate) fn record_refresh(elapsed: Duration, rows_rebuilt: u64, cells_walked: u64) { + REFRESH_CALLS.fetch_add(1, Ordering::Relaxed); + REFRESH_NANOS.fetch_add(elapsed.as_nanos() as u64, Ordering::Relaxed); + ROWS_REBUILT.fetch_add(rows_rebuilt, Ordering::Relaxed); + CELLS_WALKED.fetch_add(cells_walked, Ordering::Relaxed); +} + +/// Fold one `TerminalWidget::draw` call into the global aggregate. +pub(crate) fn record_draw(elapsed: Duration, fill_text_calls: u64) { + DRAW_CALLS.fetch_add(1, Ordering::Relaxed); + DRAW_NANOS.fetch_add(elapsed.as_nanos() as u64, Ordering::Relaxed); + FILL_TEXT_CALLS.fetch_add(fill_text_calls, Ordering::Relaxed); +} diff --git a/crates/roost-iced/src/terminal_widget.rs b/crates/roost-iced/src/terminal_widget.rs index a7fc8b12..f198bb73 100644 --- a/crates/roost-iced/src/terminal_widget.rs +++ b/crates/roost-iced/src/terminal_widget.rs @@ -11,6 +11,7 @@ use iced::{ }; use roost_engine::pointer::{PointerAction, PointerButton}; use roost_vt::{ColorRgb, CursorInfo, CursorVisualStyle, SelectionSpan}; +use std::sync::Arc; use std::time::{Duration, Instant}; use unicode_width::UnicodeWidthStr; @@ -124,9 +125,12 @@ fn draw_font(base: Font, text: &str, bold: bool, italic: bool) -> Font { } } +/// One resolved cell. Deliberately carries no row index: its row is the +/// index of the [`RenderedRow`] that owns it. Storing the row here as well +/// would let the two disagree, which is exactly the "right cells, wrong +/// row" failure the per-row cache could otherwise hide. #[derive(Debug, Clone)] pub struct DrawCell { - pub row: u32, pub col: u16, pub text: String, pub foreground: ColorRgb, @@ -137,6 +141,74 @@ pub struct DrawCell { pub inverse: bool, } +/// One viewport row's render output, shared behind an `Arc` so cloning a +/// snapshot (which `App::view` does every frame) is O(rows) refcount bumps +/// rather than O(cells) `String` clones, and so a row libghostty reports +/// undirty survives a refresh without being rebuilt. +/// +/// **Overlay invariant — a `RenderedRow` holds terminal content ONLY.** +/// Selection tint, link-hover underline and the cursor are snapshot-level +/// fields drawn in separate passes after the cell loop in +/// [`TerminalWidget::draw`], and must NEVER be baked into a [`DrawCell`]'s +/// colors. A row is cached across refreshes; folding selection into a +/// cell's background would freeze the tint in the cache, which surfaces as +/// "the selection sometimes doesn't clear". +#[derive(Debug, Default)] +pub struct RenderedRow { + /// Sparse: only the cells that draw something, ascending by column. + pub cells: Vec, + /// The row's text, joined and `trim_end`ed — what `tab.dump` returns. + pub text: String, +} + +impl RenderedRow { + /// Resolve one viewport row from libghostty's cells for that row. + /// + /// Everything this reads is a parameter — the row's vt cells, the + /// terminal's default fg/bg pair, and the grid width. That list IS the + /// cache key `TerminalTab::refresh_snapshot` guards on; adding a + /// fourth input here means extending those guards (see that function's + /// caching invariant). + pub fn build(cells: &[roost_vt::Cell], defaults: (ColorRgb, ColorRgb), cols: u16) -> Self { + let mut row = RenderedRow { + cells: Vec::new(), + text: String::with_capacity(usize::from(cols)), + }; + for cell in cells { + // libghostty yields a row's cells in ascending, gapless column + // order, so appending is the same string the old + // index-into-a-dense-`Vec`-then-`concat` build produced + // — including for a short row, whose missing tail contributed + // empty strings there and contributes nothing here. + if cell.col >= cols { + continue; + } + let text = if cell.text.is_empty() { + " " + } else { + cell.text.as_str() + }; + row.text.push_str(text); + let (foreground, background) = + resolve_colors(cell.fg, cell.bg, defaults, cell.style.inverse); + if text != " " || cell.bg.is_some() || cell.style.inverse { + row.cells.push(DrawCell { + col: cell.col, + text: text.to_string(), + foreground, + background, + explicit_background: cell.bg.is_some() || cell.style.inverse, + bold: cell.style.bold, + italic: cell.style.italic, + inverse: cell.style.inverse, + }); + } + } + row.text.truncate(row.text.trim_end().len()); + row + } +} + #[derive(Debug, Clone)] pub struct TerminalSnapshot { pub cols: u16, @@ -144,8 +216,8 @@ pub struct TerminalSnapshot { pub foreground: ColorRgb, pub background: ColorRgb, pub cursor: Option, - pub cells: Vec, - pub rows_text: Vec, + /// Indexed by viewport row; `grid.len() == rows`. + pub grid: Vec>, pub selection_background: ColorRgb, pub selection_spans: Vec, pub link_hover: Option, @@ -154,6 +226,11 @@ pub struct TerminalSnapshot { impl TerminalSnapshot { pub fn blank(cols: u16, rows: u16) -> Self { + // Every row shares one empty `RenderedRow`: rows are replaced + // wholesale, never mutated in place. `RenderedRow::text` is `""` + // here, which is what a `refresh_snapshot`-built blank row also + // trims down to — `tab.dump` depends on the two agreeing. + let blank_row = Arc::new(RenderedRow::default()); Self { cols, rows, @@ -168,8 +245,7 @@ impl TerminalSnapshot { b: 24, }, cursor: None, - cells: Vec::new(), - rows_text: vec![String::new(); usize::from(rows)], + grid: vec![blank_row; usize::from(rows)], selection_background: ColorRgb { r: 72, g: 83, @@ -569,48 +645,58 @@ impl Widget for TerminalWidget { _cursor: mouse::Cursor, viewport: &Rectangle, ) { + let draw_started_at = Instant::now(); let bounds = layout.bounds(); let Some(clip) = bounds.intersection(viewport) else { + crate::perf::record_draw(draw_started_at.elapsed(), 0); return; }; + let mut fill_text_calls: u64 = 0; renderer.with_layer(clip, |renderer| { fill_quad(renderer, bounds, color(self.snapshot.background)); let metrics = self.metrics; - for cell in &self.snapshot.cells { - let position = cell_position(bounds.position(), cell.col, cell.row, metrics); - if cell.explicit_background { - fill_quad( - renderer, - // Preserve the Canvas path's overdraw policy. Adjacent - // antialiased quads can otherwise expose hairlines of - // the default background under tiny-skia. - Rectangle::new( - position, - Size::new(metrics.cell_width, metrics.cell_height), - ), - color(cell.background), - ); - } - if !cell.text.is_empty() && cell.text != " " { - let font = draw_font(metrics.font, cell.text.as_str(), cell.bold, cell.italic); - renderer.fill_text( - text::Text { - content: cell.text.clone(), - bounds: Size::new(f32::INFINITY, metrics.cell_height), - size: Pixels(metrics.font_pixels), - line_height: text::LineHeight::Relative(TERMINAL_LINE_HEIGHT), - font, - align_x: text::Alignment::Default, - align_y: iced::alignment::Vertical::Top, - shaping: text::Shaping::Auto, - wrapping: text::Wrapping::None, - }, - Point::new(position.x, position.y + 1.0), - color(cell.foreground), - clip, - ); + for (row_idx, row) in self.snapshot.grid.iter().enumerate() { + // The row index comes from the grid position, never from + // the cell — see `DrawCell`. + let row_y = row_idx as u32; + for cell in &row.cells { + let position = cell_position(bounds.position(), cell.col, row_y, metrics); + if cell.explicit_background { + fill_quad( + renderer, + // Preserve the Canvas path's overdraw policy. Adjacent + // antialiased quads can otherwise expose hairlines of + // the default background under tiny-skia. + Rectangle::new( + position, + Size::new(metrics.cell_width, metrics.cell_height), + ), + color(cell.background), + ); + } + if !cell.text.is_empty() && cell.text != " " { + let font = + draw_font(metrics.font, cell.text.as_str(), cell.bold, cell.italic); + renderer.fill_text( + text::Text { + content: cell.text.clone(), + bounds: Size::new(f32::INFINITY, metrics.cell_height), + size: Pixels(metrics.font_pixels), + line_height: text::LineHeight::Relative(TERMINAL_LINE_HEIGHT), + font, + align_x: text::Alignment::Default, + align_y: iced::alignment::Vertical::Top, + shaping: text::Shaping::Auto, + wrapping: text::Wrapping::None, + }, + Point::new(position.x, position.y + 1.0), + color(cell.foreground), + clip, + ); + fill_text_calls += 1; + } } } @@ -692,6 +778,7 @@ impl Widget for TerminalWidget { } } }); + crate::perf::record_draw(draw_started_at.elapsed(), fill_text_calls); } } diff --git a/crates/roost-ipc/src/messages.rs b/crates/roost-ipc/src/messages.rs index 1157338f..2f72574d 100644 --- a/crates/roost-ipc/src/messages.rs +++ b/crates/roost-ipc/src/messages.rs @@ -1028,6 +1028,46 @@ pub struct SidebarDumpResult { pub projects: Vec, } +/// `app.render_stats` request — read the running UI's render-path +/// counters. `reset` zeroes them *after* the read, so a caller can +/// read-reset, run a workload, then read the delta directly. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct AppRenderStatsParams { + #[serde(default)] + pub reset: bool, +} + +/// `app.render_stats` response — running totals since process start +/// (or the last `reset: true`). +/// +/// Every counter is string-wrapped: the nanosecond accumulators pass +/// 2^53 after roughly 104 days of measured render time, and a JSON +/// number would silently lose precision in any JS client. The +/// remaining counters ride the same convention so the shape is +/// uniform rather than half-and-half. +/// +/// Permissive (no `deny_unknown_fields`) like every other result +/// struct, so a newer UI can add counters without breaking older +/// clients. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct AppRenderStatsResult { + #[serde(with = "string_int64")] + pub refresh_calls: i64, + #[serde(with = "string_int64")] + pub refresh_nanos: i64, + #[serde(with = "string_int64")] + pub rows_rebuilt: i64, + #[serde(with = "string_int64")] + pub cells_walked: i64, + #[serde(with = "string_int64")] + pub draw_calls: i64, + #[serde(with = "string_int64")] + pub draw_nanos: i64, + #[serde(with = "string_int64")] + pub fill_text_calls: i64, +} + /// `window.resize` request — programmatically set the window's logical /// size. Test-mode only (gated by `ROOST_TEST_MODE=1`); see the op /// const comment block below for the rationale. @@ -1204,6 +1244,15 @@ pub mod ops { /// is a wire-visible test failure rather than an invisible one /// (plan 007 §3.8). pub const SIDEBAR_DUMP: &str = "app.sidebar_dump"; + /// Read the running UI's render-path counters (refresh + draw call + /// counts, elapsed nanos, rows/cells walked, `fill_text` calls), + /// optionally zeroing them after the read. Ungated for the same + /// reason as `tab.dump_resolved`: reading counters mutates nothing + /// a user can see. The draw-side numbers only exist in a running + /// app — `TerminalWidget::draw` needs a live renderer, which unit + /// tests can't construct — so this op is the only way to measure + /// the real draw path. + pub const APP_RENDER_STATS: &str = "app.render_stats"; /// Command-palette overlay: open a root frame, read the current /// frame's rows, set the filter, activate a row (same dispatch as its @@ -1901,6 +1950,60 @@ mod tests { assert!(serde_json::from_str::(bad).is_err()); } + /// `reset` defaults to false so `{"op":"app.render_stats"}` with no + /// params at all is a plain read — the common case. Same shape as + /// `TabCapturePtyInputParams::drain`. + #[test] + fn app_render_stats_params_default_reset_is_false() { + let p: AppRenderStatsParams = serde_json::from_str("{}").unwrap(); + assert!(!p.reset); + assert!(!AppRenderStatsParams::default().reset); + round_trip(&AppRenderStatsParams { reset: true }); + let bad = r#"{"reset":true,"extra":"x"}"#; + assert!(serde_json::from_str::(bad).is_err()); + } + + /// Every counter is string-wrapped int64: nanosecond accumulators + /// exceed JS's 2^53 safe range, and a mixed number/string shape + /// would be the worst of both. Assert the *encoding*, not just the + /// round-trip, so dropping `string_int64` from a field fails here. + #[test] + fn app_render_stats_result_counters_are_string_wrapped() { + let r = AppRenderStatsResult { + refresh_calls: 12, + refresh_nanos: 9_007_199_254_740_993, + rows_rebuilt: 288, + cells_walked: 23_040, + draw_calls: 30, + draw_nanos: 4_500_000, + fill_text_calls: 720, + }; + let json = serde_json::to_string(&r).unwrap(); + assert!(json.contains(r#""refresh_calls":"12""#), "got: {json}"); + assert!( + json.contains(r#""refresh_nanos":"9007199254740993""#), + "got: {json}" + ); + assert!(json.contains(r#""rows_rebuilt":"288""#), "got: {json}"); + assert!(json.contains(r#""cells_walked":"23040""#), "got: {json}"); + assert!(json.contains(r#""draw_calls":"30""#), "got: {json}"); + assert!(json.contains(r#""draw_nanos":"4500000""#), "got: {json}"); + assert!(json.contains(r#""fill_text_calls":"720""#), "got: {json}"); + round_trip(&r); + } + + /// Result structs stay permissive so a newer UI can add a counter + /// without breaking older clients — the same contract + /// `TabDumpResolvedResult` documents. + #[test] + fn app_render_stats_result_accepts_extra_fields() { + let json = r#"{"refresh_calls":"1","refresh_nanos":"2","rows_rebuilt":"3", + "cells_walked":"4","draw_calls":"5","draw_nanos":"6", + "fill_text_calls":"7","gpu_nanos":"8"}"#; + let r: AppRenderStatsResult = serde_json::from_str(json).expect("permissive decode"); + assert_eq!(r.fill_text_calls, 7); + } + #[test] fn window_resize_params_reject_unknown_field() { round_trip(&WindowResizeParams { diff --git a/crates/roost-ipc/tests/vectors.rs b/crates/roost-ipc/tests/vectors.rs index 7a513d50..2fa0e751 100644 --- a/crates/roost-ipc/tests/vectors.rs +++ b/crates/roost-ipc/tests/vectors.rs @@ -210,6 +210,40 @@ fn sidebar_dump_vector_decodes_into_its_typed_params() { assert!(result.projects[1].agents.is_empty()); } +/// `app.render_stats` is the one op whose *every* result field is a +/// string-wrapped int64. Generic round-tripping would happily accept a +/// vector that wrote them as JSON numbers, which is exactly the drift +/// this convention exists to prevent — so decode it into the typed +/// struct. +#[test] +fn render_stats_vector_decodes_into_its_typed_params() { + use roost_ipc::messages::{ops, AppRenderStatsParams, AppRenderStatsResult, RawRequest}; + + let mut path = vectors_dir(); + path.push("app.render_stats.request.json"); + let raw = fs::read_to_string(&path).expect("read request vector"); + let req: RawRequest = serde_json::from_str(&raw).expect("decode envelope"); + assert_eq!(req.op, ops::APP_RENDER_STATS); + let params: AppRenderStatsParams = + serde_json::from_value(req.params).expect("decode render_stats params"); + assert!(!params.reset); + + let mut path = vectors_dir(); + path.push("app.render_stats.response.json"); + let raw = fs::read_to_string(&path).expect("read response vector"); + let resp: roost_ipc::messages::Response = + serde_json::from_str(&raw).expect("decode response envelope"); + let result: AppRenderStatsResult = + serde_json::from_value(resp.result.expect("result body")).expect("decode result"); + assert_eq!(result.refresh_calls, 412); + assert_eq!(result.refresh_nanos, 51_500_000); + assert_eq!(result.rows_rebuilt, 9_888); + assert_eq!(result.cells_walked, 790_400); + assert_eq!(result.draw_calls, 377); + assert_eq!(result.draw_nanos, 94_250_000); + assert_eq!(result.fill_text_calls, 9_048); +} + #[test] fn event_vectors_have_required_envelope_shape() { let dir = vectors_dir(); diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 80df539b..7359f5c5 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -26,8 +26,8 @@ use libadwaita::prelude::*; use libadwaita::{ApplicationWindow, TabView, WindowTitle}; use roost_ipc::agent::{self, AgentLifecycle, AgentTabState}; use roost_ipc::messages::{ - PaletteItemView, PaletteStateResult, Project, SidebarDumpAgentRow, SidebarDumpProject, - SidebarDumpResult, Tab, WindowMetricsResult, + AppRenderStatsResult, PaletteItemView, PaletteStateResult, Project, SidebarDumpAgentRow, + SidebarDumpProject, SidebarDumpResult, Tab, WindowMetricsResult, }; use tokio::runtime::Handle; @@ -1099,6 +1099,16 @@ impl App { UiRequest::WindowMetrics { reply } => { let _ = reply.send(app.ipc_window_metrics()); } + UiRequest::AppRenderStats { reset: _, reply } => { + // Deliberate placeholder: the GTK renderer has no + // perf instrumentation yet (roadmap slice E3b). + // Zeros keep the op's contract identical on both + // Rust UIs — refusing here would make this the + // first op one Rust UI answers and the other + // doesn't, and there is no `unsupported` error + // code to refuse with. + let _ = reply.send(Ok(AppRenderStatsResult::default())); + } UiRequest::SidebarDump { reply } => { let _ = reply.send(app.ipc_sidebar_dump()); } diff --git a/crates/roost-osc/src/lib.rs b/crates/roost-osc/src/lib.rs index 9664afbf..b7335f21 100644 --- a/crates/roost-osc/src/lib.rs +++ b/crates/roost-osc/src/lib.rs @@ -1,18 +1,26 @@ //! Streaming OSC scanner — Phase 6a P4. //! -//! OSC scanner tailored to the daemon's architecture: +//! Originally written for the daemon, which was intentionally +//! libghostty-free: the UI reported raw OSC bytes via `ReportOsc` and +//! the daemon parsed everything it needed to route, so this scanner +//! emits `Title` for OSC 0/1/2 on top of the notification/cwd/color +//! classes even though the UI's libghostty handles titles itself. //! -//! * The UI's libghostty handles window-title OSC (0/1/2) itself, -//! so a scanner sitting next to it would only need to cover the -//! OSC classes libghostty doesn't surface (notifications, cwd, -//! color queries). +//! **That premise is obsolete — the daemon is gone and both Rust UIs +//! embed libghostty — but the conclusion still holds, for a different +//! reason.** libghostty's OSC parser cannot replace this one: +//! `GhosttyOscCommandData` exposes exactly one payload accessor +//! (`CHANGE_WINDOW_TITLE_STR`), identical in our pinned header and at +//! Ghostty tip. It discriminates 22 command *types* but hands back no +//! data for OSC 7 / 9 / 10-12 / 4 / 133 / 52 / 22 — seven of the eight +//! [`OscEvent`] variants. The policy below (percent-decode + +//! `file://` extraction, ConEmu OSC 9 sub-command filtering, OSC 52 +//! base64 decode with refuse-on-truncation, `MAX_BODY`, reply +//! synthesis) has no C-API counterpart and would survive regardless. //! -//! * This scanner instead sits in the daemon, which is -//! intentionally libghostty-free (see goal-rust-port-polish DL -//! choices). The UI reports raw OSC bytes via `ReportOsc` and the -//! daemon parses everything it needs to route. So this scanner -//! also emits `Title` for OSC 0/1/2, on top of the -//! notification/cwd/color classes. +//! Re-evaluate only if libghostty-vt grows `GHOSTTY_OSC_DATA_*` +//! accessors beyond window title — tracked as a watch item in +//! `docs/development/iced-migration-roadmap.md`. //! //! Architecture: //! diff --git a/crates/roost-vt/src/lib.rs b/crates/roost-vt/src/lib.rs index 0ed95813..78e31903 100644 --- a/crates/roost-vt/src/lib.rs +++ b/crates/roost-vt/src/lib.rs @@ -124,7 +124,7 @@ pub use mouse_encoder::{ mouse_action, mouse_button, MouseAction, MouseButton, MouseEncoder, MouseEvent, }; #[cfg(feature = "ffi")] -pub use render_state::{Cell, Colors, CursorInfo, CursorVisualStyle, RenderState, Style}; +pub use render_state::{Cell, Colors, CursorInfo, CursorVisualStyle, Dirty, RenderState, Style}; #[cfg(feature = "ffi")] pub use scroll::{PageDirection, PageRoute, ScrollDirection, ScrollRoute, TerminalScroll}; #[cfg(feature = "ffi")] diff --git a/crates/roost-vt/src/render_state.rs b/crates/roost-vt/src/render_state.rs index 3ecad119..9b82a482 100644 --- a/crates/roost-vt/src/render_state.rs +++ b/crates/roost-vt/src/render_state.rs @@ -5,11 +5,18 @@ //! and row-cells handles once. They're reused across frames. //! 2. `update(&terminal)` snapshots the current screen. //! 3. `walk(|cell| ...)` iterates rows × cells, calling the closure -//! once per cell. +//! once per cell; `walk_dirty` iterates only the rows that +//! changed and consumes the frame's damage. //! 4. `cursor()` / `colors()` extract additional per-frame data. //! -//! Matches `mac/Sources/Roost/RenderState.swift` 1:1 in shape — same -//! constructor pattern, same walk surface, same cursor info layout. +//! `mac/Sources/Roost/RenderState.swift` mirrors the constructor, +//! walk, and cursor-info shape — but the two have **intentionally +//! diverged** as of the dirty-tracking API below (`Dirty`, `dirty`, +//! `mark_full`, `dirty_rows`, `walk_dirty`). Swift deliberately does +//! not get dirty tracking: it is the daily driver, its full-grid +//! renderer is adequate, and the macOS-Iced evaluation may retire it, +//! so investing in its render path is potentially wasted work. Do not +//! "restore parity" here without that decision changing. use std::ptr; @@ -115,6 +122,28 @@ pub struct Cell { pub style: Style, } +/// Global dirty state after `update`. Maps `GhosttyRenderStateDirty`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Dirty { + Clean, + Partial, + Full, +} + +impl Dirty { + fn from_raw(v: sys::GhosttyRenderStateDirty) -> Self { + match v { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE => Self::Clean, + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_PARTIAL => Self::Partial, + // Anything we don't recognize — including a variant a future + // Ghostty adds — maps to `Full`, not `Clean`. Guessing "clean" + // for an unknown state would skip the rebuild and freeze the + // screen; guessing "full" only costs a redraw. + _ => Self::Full, + } + } +} + pub struct RenderState { handle: sys::GhosttyRenderState, row_iter: sys::GhosttyRenderStateRowIterator, @@ -269,46 +298,19 @@ impl RenderState { /// libghostty's contract says they're safe to re-bind via the next /// `_next` call without reallocation. pub fn walk(&mut self, terminal: &Terminal, mut f: impl FnMut(u32, Cell)) -> Result<()> { - // Rebind the row iterator to this frame's state. The C signature - // expects `GhosttyRenderStateRowIterator*` (pointer-to-handle slot), - // not the handle's value — the function writes into the slot to - // re-anchor the pre-allocated iterator at the new frame. Passing - // `self.row_iter as *mut _` would point at the iterator's IMPL - // and corrupt its internal state, leaving `..._next` returning - // false on every row (silent: no error, just zero cells walked). - // Mirrors `mac/Sources/Roost/RenderState.swift::walk`'s - // `withUnsafeMutablePointer(to: &self.rowIter)` pattern. - // SAFETY: state + iter handles non-null per constructor. - let rc = unsafe { - sys::ghostty_render_state_get( - self.handle, - sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_ROW_ITERATOR, - (&mut self.row_iter) as *mut _ as *mut _, - ) - }; - Error::from_result(rc)?; + self.bind_row_iterator()?; // Keep `terminal` alive across the walk so libghostty doesn't // drop allocations that back the iterators. Borrowing & makes // the lifetime explicit; the variable itself is intentionally // unused. let _ = terminal; - let mut row_idx: u32 = 0; - // SAFETY: iter handle non-null. - while unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { - // Bind this row's cells to row_cells. Same pointer-to-slot - // semantics as the row iterator above — pass `&mut`, not the - // handle value. - // SAFETY: iter + cells handles non-null. - let rc = unsafe { - sys::ghostty_render_state_row_get( - self.row_iter, - sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_CELLS, - (&mut self.row_cells) as *mut _ as *mut _, - ) - }; - if Error::from_result(rc).is_err() { - row_idx += 1; + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + if self.bind_row_cells().is_err() { continue; } @@ -319,11 +321,214 @@ impl RenderState { f(row_idx, cell); col = col.saturating_add(1); } - row_idx += 1; } Ok(()) } + /// Global dirty state. Pure read — clears nothing. + pub fn dirty(&self) -> Result { + let raw = self.read_u32(sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_DIRTY)?; + Ok(Dirty::from_raw(raw)) + } + + /// Raise the global dirty state to `Full`, forcing the next + /// `walk_dirty` to visit every row. Monotonic: this is the only + /// public way to move the dirty state at all, and it only ever + /// raises. Lowering lives solely inside `walk_dirty`, where both + /// layers are cleared together — which is what keeps libghostty's + /// two-layer footgun (clearing one layer does not clear the other) + /// structurally unreachable from safe code. + pub fn mark_full(&mut self) -> Result<()> { + self.set_global_dirty(sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FULL) + } + + /// Viewport row indices libghostty currently marks dirty. Pure with + /// respect to dirty state — clears nothing, on either layer. It does + /// rebind the cached row-iterator handle (`self.row_iter`) to walk the + /// rows, so it must not be called from inside a `walk` / `walk_dirty` + /// callback: that would re-anchor the iterator mid-iteration and + /// corrupt the caller's in-flight walk. Diagnostic / test accessor; + /// renderers want `walk_dirty`. + pub fn dirty_rows(&mut self, terminal: &Terminal) -> Result> { + self.bind_row_iterator()?; + // Keep `terminal` alive across the walk (see `walk`). + let _ = terminal; + + let mut rows = Vec::new(); + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + if self.read_row_dirty() { + rows.push(row_idx); + } + } + Ok(rows) + } + + /// Walk only the rows that changed, handing each row's complete cell + /// list to `f`. Visits every row when the global state is `Full`. + /// + /// **Consumes the frame's damage: clears BOTH dirty layers.** A + /// `Clean` frame visits nothing but still clears both, so the + /// contract is uniform regardless of what came back. + /// + /// Contract: `f` is called exactly once per visited row, with that + /// row's COMPLETE cell slice. It is never called mid-row, and never + /// for a row whose read failed — so a caller may replace its cached + /// row wholesale on each callback without a half-built row ever + /// landing in the cache. A row whose cells could not be read keeps + /// its dirty flag set, so the next frame retries it. + /// + /// Returns the global state as it was on ENTRY. + /// + /// On an early `Err` return some rows may already be cleared while + /// the global layer is not. That is safe by construction: residual + /// damage means the next frame redraws MORE than necessary, never + /// less. + pub fn walk_dirty( + &mut self, + terminal: &Terminal, + mut f: impl FnMut(u32, &[Cell]), + ) -> Result { + let global = self.dirty()?; + self.bind_row_iterator()?; + // Keep `terminal` alive across the walk so libghostty doesn't + // drop allocations that back the iterators (see `walk`). + let _ = terminal; + + let mut cells: Vec = Vec::new(); + let mut retry_pending = false; + for row_idx in 0u32.. { + // SAFETY: iter handle non-null. + if !unsafe { sys::ghostty_render_state_row_iterator_next(self.row_iter) } { + break; + } + + let row_dirty = self.read_row_dirty(); + let visit = match global { + Dirty::Clean => false, + // `render.h` does not promise that a `Full` frame also + // flags every row, so don't depend on it. + Dirty::Full => true, + Dirty::Partial => row_dirty, + }; + + if visit { + if self.bind_row_cells().is_err() { + // Leave this row's flag SET so the next walk retries + // it, and don't call `f` with a partial row. + retry_pending = true; + continue; + } + + cells.clear(); + let mut col: u16 = 0; + // SAFETY: row_cells handle non-null. + while unsafe { sys::ghostty_render_state_row_cells_next(self.row_cells) } { + cells.push(self.read_current_cell(col)); + col = col.saturating_add(1); + } + f(row_idx, &cells); + } + + if row_dirty { + self.clear_row_dirty()?; + } + } + + // A row we could not read kept its flag, but clearing the global + // layer to FALSE would strand it: the next `walk_dirty` reads + // `Clean` and visits nothing at all, so the retry the contract + // promises would never happen. Leave the frame `Partial` instead, + // which is exactly what the surviving row flags mean. + self.set_global_dirty(if retry_pending { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_PARTIAL + } else { + sys::GhosttyRenderStateDirty_GHOSTTY_RENDER_STATE_DIRTY_FALSE + })?; + Ok(global) + } + + /// Rebind `self.row_iter` to the current frame's state. + /// + /// The C signature expects `GhosttyRenderStateRowIterator*` (a + /// pointer-to-handle slot), not the handle's value — the call writes + /// into the slot to re-anchor the pre-allocated iterator at the new + /// frame. Passing `self.row_iter as *mut _` would point at the + /// iterator's IMPL and corrupt its internal state, leaving + /// `..._next` returning false on every row (silent: no error, just + /// zero cells walked). Mirrors + /// `mac/Sources/Roost/RenderState.swift::walk`'s + /// `withUnsafeMutablePointer(to: &self.rowIter)` pattern. + fn bind_row_iterator(&mut self) -> Result<()> { + // SAFETY: state + iter handles non-null per constructor. + let rc = unsafe { + sys::ghostty_render_state_get( + self.handle, + sys::GhosttyRenderStateData_GHOSTTY_RENDER_STATE_DATA_ROW_ITERATOR, + (&mut self.row_iter) as *mut _ as *mut _, + ) + }; + Error::from_result(rc) + } + + /// Rebind `self.row_cells` to the row the iterator currently sits + /// on. Same pointer-to-slot semantics as `bind_row_iterator`. + fn bind_row_cells(&mut self) -> Result<()> { + // SAFETY: iter + cells handles non-null per constructor. + let rc = unsafe { + sys::ghostty_render_state_row_get( + self.row_iter, + sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_CELLS, + (&mut self.row_cells) as *mut _ as *mut _, + ) + }; + Error::from_result(rc) + } + + /// Dirty flag of the row the iterator currently sits on. A failed + /// read reports `false`, which leaves the flag alone rather than + /// clearing damage we could not confirm. + fn read_row_dirty(&self) -> bool { + let mut out: bool = false; + // SAFETY: iter handle non-null; out is a real local. + let rc = unsafe { + sys::ghostty_render_state_row_get( + self.row_iter, + sys::GhosttyRenderStateRowData_GHOSTTY_RENDER_STATE_ROW_DATA_DIRTY, + (&mut out) as *mut bool as *mut _, + ) + }; + Error::from_result(rc).is_ok() && out + } + + fn clear_row_dirty(&mut self) -> Result<()> { + let value = false; + // SAFETY: iter handle non-null; value is a real local. + let rc = unsafe { + sys::ghostty_render_state_row_set( + self.row_iter, + sys::GhosttyRenderStateRowOption_GHOSTTY_RENDER_STATE_ROW_OPTION_DIRTY, + (&value) as *const bool as *const _, + ) + }; + Error::from_result(rc) + } + + fn set_global_dirty(&mut self, value: sys::GhosttyRenderStateDirty) -> Result<()> { + // SAFETY: handle non-null; value is a real local. + let rc = unsafe { + sys::ghostty_render_state_set( + self.handle, + sys::GhosttyRenderStateOption_GHOSTTY_RENDER_STATE_OPTION_DIRTY, + (&value) as *const _ as *const _, + ) + }; + Error::from_result(rc) + } + fn read_current_cell(&self, col: u16) -> Cell { let bg = self.read_cells_color( sys::GhosttyRenderStateRowCellsData_GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_BG_COLOR, diff --git a/crates/roost-vt/tests/render_dirty_test.rs b/crates/roost-vt/tests/render_dirty_test.rs new file mode 100644 index 00000000..15bdab21 --- /dev/null +++ b/crates/roost-vt/tests/render_dirty_test.rs @@ -0,0 +1,385 @@ +#![cfg(feature = "ffi")] +//! Pins the dirty-tracking semantics of libghostty's render state at our +//! pinned Ghostty SHA. Every expectation here was measured against the +//! real `libghostty-vt.a` before the wrapper was written (plan 017 §2.2); +//! a failure means libghostty's behavior moved, not that the wrapper is +//! merely mis-specified. + +use roost_vt::{ColorRgb, Dirty, RenderState, ScrollViewport, Terminal, TerminalOptions}; + +/// A terminal + render state that have never been updated: the render +/// state still reports `Full` on its first `update`. +fn fresh(cols: u16, rows: u16) -> (Terminal, RenderState) { + let t = Terminal::new(TerminalOptions { + cols, + rows, + max_scrollback: 500, + }) + .expect("Terminal::new"); + (t, RenderState::new().expect("RenderState::new")) +} + +/// A terminal + render state already drained to `Clean`, so the next +/// change is the only damage the test sees. +fn settled(cols: u16, rows: u16) -> (Terminal, RenderState) { + let (t, mut rs) = fresh(cols, rows); + settle(&mut rs, &t); + (t, rs) +} + +/// Drain the current damage and confirm the terminal has settled to +/// `Clean` — i.e. both dirty layers really were cleared. +fn settle(rs: &mut RenderState, t: &Terminal) { + rs.update(t).expect("update"); + rs.walk_dirty(t, |_, _| {}).expect("walk_dirty"); + rs.update(t).expect("update"); + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "terminal should settle to Clean after a walk_dirty + no-change update" + ); +} + +/// Row indices visited by one `walk_dirty`, plus the state it reported. +fn visit(rs: &mut RenderState, t: &Terminal) -> (Dirty, Vec) { + let mut rows = Vec::new(); + let state = rs + .walk_dirty(t, |row, _| rows.push(row)) + .expect("walk_dirty"); + (state, rows) +} + +fn row_text(cells: &[roost_vt::Cell]) -> String { + cells + .iter() + .map(|c| if c.text.is_empty() { " " } else { &c.text }) + .collect::() + .trim_end() + .to_string() +} + +#[test] +fn fresh_update_reports_full_and_visits_every_row() { + let (t, mut rs) = fresh(80, 24); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full, "walk_dirty returns the state on entry"); + assert_eq!(rows, (0..24).collect::>()); +} + +#[test] +fn settled_terminal_reports_clean_and_visits_nothing() { + let (t, mut rs) = settled(80, 24); + + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Clean); + // Zero rows visited is the proof that walk_dirty cleared BOTH layers: + // had it cleared only the global one, every row flag would survive + // and this walk would visit all 24. + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn single_row_write_reports_partial_on_that_row_only() { + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + + let mut visited: Vec<(u32, String)> = Vec::new(); + let state = rs + .walk_dirty(&t, |row, cells| visited.push((row, row_text(cells)))) + .expect("walk_dirty"); + + assert_eq!(state, Dirty::Partial); + assert_eq!(visited.len(), 1, "expected only row 0, got {visited:?}"); + assert_eq!(visited[0].0, 0); + assert_eq!(visited[0].1, "hello"); +} + +#[test] +fn row_flags_are_cleared_alongside_the_global_layer() { + // render.h's "extremely important detail": the global and per-row + // dirty layers are independent, so clearing one does not clear the + // other. If `walk_dirty` cleared only the global layer, row 0's flag + // would survive its first walk and row 0 would be redrawn on every + // subsequent frame forever. This test is what catches that. + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + let (_, first) = visit(&mut rs, &t); + assert_eq!(first, vec![0]); + + // Park the cursor off row 0 and settle, so the next write's cursor + // movement can't re-dirty row 0 for a reason unrelated to the + // footgun (§2.2 finding 4: a cursor move dirties both the row it + // leaves and the row it lands on). + t.vt_write(b"\x1b[11;1H"); + rs.update(&t).expect("update"); + let _ = visit(&mut rs, &t); + settle(&mut rs, &t); + + t.vt_write(b"\x1b[5;1HmarkerX"); + rs.update(&t).expect("update"); + let (state, second) = visit(&mut rs, &t); + + assert_eq!(state, Dirty::Partial); + assert!( + !second.contains(&0), + "row 0 must not be revisited — its flag was consumed by the first \ + walk_dirty; got {second:?}" + ); + // Row 4 is the write; row 10 is the cursor row it vacated. + assert_eq!(second, vec![4, 10]); +} + +#[test] +fn full_dirty_state_is_cleared_by_walk_dirty() { + // Full-layer counterpart to `row_flags_are_cleared_alongside_the_global_layer` + // above, which pins that the ROW layer would survive a walk that + // cleared only the global one. This test pins the other half: + // forcing the GLOBAL layer to Full and confirming `walk_dirty` + // clears it too, so a subsequent no-change frame settles to Clean + // instead of reporting Full forever. + let (t, mut rs) = settled(80, 24); + + rs.mark_full().expect("mark_full"); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..24).collect::>()); + + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Clean); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Clean); + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn dirty_rows_is_a_pure_read() { + let (mut t, mut rs) = settled(80, 24); + + t.vt_write(b"hello"); + rs.update(&t).expect("update"); + + let first = rs.dirty_rows(&t).expect("dirty_rows"); + let second = rs.dirty_rows(&t).expect("dirty_rows"); + assert_eq!(first, vec![0]); + assert_eq!(first, second, "dirty_rows must not consume anything"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + + let (_, rows) = visit(&mut rs, &t); + assert_eq!( + rows, first, + "walk_dirty still sees the rows dirty_rows read" + ); +} + +#[test] +fn mark_full_forces_a_full_visit() { + let (t, mut rs) = settled(80, 24); + + rs.mark_full().expect("mark_full"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..24).collect::>()); +} + +#[test] +fn theme_color_changes_report_full() { + let (mut t, mut rs) = settled(80, 24); + + // The Rust theme path — no vt bytes at all. + t.set_color_foreground(ColorRgb::new(1, 2, 3)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(4, 5, 6)) + .expect("set_color_background"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (_, rows) = visit(&mut rs, &t); + assert_eq!(rows.len(), 24); + + settle(&mut rs, &t); + t.set_color_palette(&[ColorRgb::new(9, 9, 9); 256]) + .expect("set_color_palette"); + rs.update(&t).expect("update"); + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (_, rows) = visit(&mut rs, &t); + assert_eq!(rows.len(), 24); +} + +#[test] +fn resize_reports_full_over_the_new_row_count() { + let (mut t, mut rs) = settled(80, 24); + + t.resize(100, 30, 8, 16).expect("resize"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..30).collect::>()); +} + +#[test] +fn viewport_scroll_reports_full() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..40 { + t.vt_write(format!("line-{i:03}\r\n").as_bytes()); + } + settle(&mut rs, &t); + + // A pure viewport move writes no cells, yet every visible row now + // shows different content — libghostty reports FULL, which is what + // lets a row cache trust it (§2.2 probe [S1]). + t.scroll_viewport(ScrollViewport::Delta(-10)); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..6).collect::>()); +} + +#[test] +fn output_driven_scroll_reports_full() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..6 { + t.vt_write(format!("\x1b[{};1Hrow-{i}", i + 1).as_bytes()); + } + // The marker loop's last write already leaves the cursor on row 6 + // (0-indexed 5), the last viewport row. + settle(&mut rs, &t); + + // Writing past the last row scrolls the viewport by one line via + // normal PTY output — no explicit `scroll_viewport` call. Per + // third_party/ghostty/src/src/terminal/render.zig:299-302 ("If our + // viewport pin changed, we do a full rebuild"), libghostty reports + // this the same way as `viewport_scroll_reports_full` above: a full + // rebuild, not an incremental per-row change. Streaming output that + // scrolls the viewport therefore gets no incremental benefit from + // dirty tracking. This test pins that expectation so a future + // Ghostty bump that changes it is noticed. + t.vt_write(b"\r\nSCROLLED"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Full); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Full); + assert_eq!(rows, (0..6).collect::>()); +} + +#[test] +fn in_place_write_without_scrolling_stays_partial() { + let (mut t, mut rs) = fresh(20, 6); + for i in 0..6 { + t.vt_write(format!("\x1b[{};1Hrow-{i}", i + 1).as_bytes()); + } + settle(&mut rs, &t); + + // No viewport-pin change here — the write lands in place on row 2, + // moving the cursor off row 6 (where the marker loop parked it). + t.vt_write(b"\x1b[2;1HINPLACE"); + rs.update(&t).expect("update"); + + assert_eq!(rs.dirty().expect("dirty"), Dirty::Partial); + let (state, rows) = visit(&mut rs, &t); + assert_eq!(state, Dirty::Partial); + // Row 1 (0-indexed) is the write; row 5 is the cursor row it + // vacated. This is the case where E3 actually saves work: only the + // touched rows are marked, not a full rebuild. + assert_eq!(rows, vec![1, 5]); +} + +#[test] +fn osc_default_color_change_reports_clean_known_limitation() { + // KNOWN LIMITATION, pinned deliberately. At our Ghostty pin, changing + // the default background via OSC 11 as PTY bytes marks NOTHING dirty + // — not the global layer, not a single row — even though every cell + // that inherits the default now renders differently. (The same change + // made through the Rust API, `set_color_background`, does report + // FULL; only the OSC-bytes path is silent.) + // + // This is why `roost-iced` must compare the default fg/bg itself and + // call `mark_full` when they move (plan 017 D3b) instead of trusting + // dirty tracking alone. + // + // If a future Ghostty bump starts reporting Full here, this test + // fails loudly — that is the point. At that time it can be relaxed + // and D3b's guard reconsidered. + // + // Both defaults are set before settling, mirroring what roost always + // does at attach — libghostty's colors reporting (render.zig's + // `orelse break :bg_fg`) only fully engages once both fg and bg + // defaults are set, so a test that left one unset would not pin the + // behavior roost actually experiences. + let (mut t, mut rs) = fresh(80, 24); + t.set_color_foreground(ColorRgb::new(0xAA, 0xAA, 0xAA)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(0x11, 0x11, 0x11)) + .expect("set_color_background"); + settle(&mut rs, &t); + + t.vt_write(b"\x1b]11;#123456\x07"); + rs.update(&t).expect("update"); + + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "OSC 11 reporting dirty would be an IMPROVEMENT — see the comment" + ); + assert!(rs.dirty_rows(&t).expect("dirty_rows").is_empty()); + let (_, rows) = visit(&mut rs, &t); + assert!(rows.is_empty(), "expected no rows visited, got {rows:?}"); +} + +#[test] +fn decscnm_swaps_reported_colors_without_marking_dirty() { + // Same class of limitation as the OSC 10/11 test above: libghostty's + // colors() swap on DECSCNM (reverse video) only applies when BOTH + // defaults are set — render.zig:326-339 does `orelse break :bg_fg` — + // and even then the swap itself marks nothing dirty. Since roost + // always sets both defaults at attach, this is the transition the + // iced consumer actually hits: it must compare the default fg/bg + // pair itself on every frame rather than trusting dirty state for + // this case. + let (mut t, mut rs) = fresh(80, 24); + t.set_color_foreground(ColorRgb::new(0xAA, 0xAA, 0xAA)) + .expect("set_color_foreground"); + t.set_color_background(ColorRgb::new(0x11, 0x11, 0x11)) + .expect("set_color_background"); + t.vt_write(b"hello"); + settle(&mut rs, &t); + + let before = rs.colors().expect("colors"); + assert_eq!(before.foreground, ColorRgb::new(0xAA, 0xAA, 0xAA)); + assert_eq!(before.background, ColorRgb::new(0x11, 0x11, 0x11)); + + t.vt_write(b"\x1b[?5h"); + rs.update(&t).expect("update"); + + let after = rs.colors().expect("colors"); + assert_eq!( + after.foreground, + ColorRgb::new(0x11, 0x11, 0x11), + "DECSCNM should swap the reported foreground to the old background" + ); + assert_eq!( + after.background, + ColorRgb::new(0xAA, 0xAA, 0xAA), + "DECSCNM should swap the reported background to the old foreground" + ); + + assert_eq!( + rs.dirty().expect("dirty"), + Dirty::Clean, + "the color swap itself must not mark anything dirty — see comment" + ); + assert!(rs.dirty_rows(&t).expect("dirty_rows").is_empty()); +} diff --git a/docs/development/iced-migration-roadmap.md b/docs/development/iced-migration-roadmap.md index 555674db..b37fa1c1 100644 --- a/docs/development/iced-migration-roadmap.md +++ b/docs/development/iced-migration-roadmap.md @@ -248,6 +248,163 @@ architecture cleanup that everything real-time depends on; 3f is done and (plan 016) — only 3h (polish parity II, user-directed) remains open on this track. +### Engine track (E) — shared renderer + robustness + +Deliberately **not** M3 slices. These span `roost-vt` / `roost-ui-model` +and land in all three UIs, so they are not part of "Iced functional +parity"; they run alongside M3/M4 rather than gating either. Lettered `E` +to avoid colliding with the parity inventory's P0/P1/P2 *priority* labels. + +Recorded 2026-08-06 from a Ghostty-comparison discovery pass. The headline +finding: **libghostty-vt's render-state dirty tracking is exposed in the +pinned header and wrapped by nobody** — not Swift, not GTK, not Iced. All +three walk the full grid on every update. + +The E-entries below were written as predictions before any of this +shipped; where the measured result corrected one, that correction is +recorded alongside it rather than quietly edited away. + +* **E1. Renderer baseline measurement — done, prediction corrected.** + The original plan was frame time under a full-screen scrolling TUI at + max window size. That workload is precisely the one E3 cannot move: + libghostty full-rebuilds its render state whenever the viewport pin + changes ("If our viewport pin changed, we do a full rebuild" — + `third_party/ghostty/src/src/terminal/render.zig:299-302`), so any + output that scrolls is a full rebuild by construction, independent of + our dirty-tracking wrapper. Measuring only that would have produced a + near-flat number and fed a false "no win here" into E4's go/no-go. + What actually shipped: deterministic per-tab counters + (`refresh_calls` / `refresh_nanos` / `rows_rebuilt` / `cells_walked`, + plus `draw_calls` / `draw_nanos` / `fill_text_calls` for the + draw path) and CPU-side span timings, exercised by three workloads — + W1 pointer-motion storm, W2 in-place TUI redraw, W3 scrolling stream + as an explicit control expected to stay flat — readable three ways: + CI unit tests, an `#[ignore]`d in-crate harness (`make perf-refresh`), + and the `app.render_stats` IPC op via `roostctl render-stats` + (`make perf-render-stats`), which is the only way to get draw-path + numbers at all (no unit test constructs a live `iced::Renderer`). + Locked-Mac caveat: presented frame rate is meaningless on a locked or + occluded Mac (macOS throttles presentation), which is why this + measures CPU spans and counters instead. See `tools/perf/README.md` + for the harness. +* **E2. Render-state dirty coverage — done.** Shipped in + `crates/roost-vt/src/render_state.rs`: `Dirty { Clean, Partial, Full }`, + `dirty()`, `mark_full()`, `dirty_rows()`, `walk_dirty()`. The footgun + (render.h's "extremely important detail" that clearing one dirty layer + does not clear the other) is handled by making consumption imply + reset — `walk_dirty` clears BOTH layers together, and there is + deliberately no public way to lower the dirty state otherwise: a + general `set_dirty` would have made `set_dirty(Clean)` the very + footgun this wrapper exists to prevent. `walk` is unchanged, so E2 is + purely additive and GTK is untouched by it. 14 tests in + `crates/roost-vt/tests/render_dirty_test.rs` pin the measured + semantics. + **Two-phase `begin_update` / `end_update` is NOT available at our pin** — + it landed upstream after `c74f6d5` (present at `../ghostty` tip and in + `../libghostty-rs`, absent from our generated bindings). It is an E8 + follow-on, not part of E2; don't plan around it. + `../libghostty-rs`'s `crates/libghostty-vt/src/render.rs` is MIT and a + useful reference for the dirty accessors regardless. +* **E3. Dirty-row snapshot rebuild — done for `roost-iced`.** Consumes + E2. `refresh_snapshot` (`app/terminal_tab.rs`) used to rebuild the + whole grid per PTY update, allocating `vec![vec![String::new(); cols]; + rows]` — a `String` per cell including blanks — plus a `String` per + `DrawCell`. Shipped shape: `TerminalSnapshot`'s `cells` + `rows_text` + became `grid: Vec>`, `DrawCell` lost its `row` field + so the row index has exactly one source of truth (the owning + `RenderedRow`'s position), and three invalidation guards force a full + rebuild outside `walk_dirty`'s own signal: grid size on both axes, + the default fg/bg pair, and a theme generation counter. + Measured (`refresh_snapshot`, `--release`, N=200/workload, before = + `f3e2657` pre-E3, after = this branch's HEAD, both built fresh in a + `git worktree` back-to-back): + + | workload | before ns/refresh | after ns/refresh | speedup | rows rebuilt/refresh | + |---|---|---|---|---| + | W1 pointer-motion storm | 107,605 | 956 | 113x | 32.00 → 0.16 | + | W2 in-place TUI redraw | 110,025 | 6,163 | 17.9x | 32.00 → 2.15 | + | W3 scrolling stream (control) | 116,609 | 57,473 | 2.0x | 32.00 → 27.50 | + + **Important limitation:** streaming/scrolling output gets **no + dirty-tracking benefit** — W3 barely moves in rows rebuilt (32.00 → + 27.50) because libghostty full-rebuilds on any viewport-pin change, per + E1's correction above. Its 2.0x clock gain is a *separate* effect: E3 + also deleted the dense `vec![vec![String::new(); cols]; rows]` + allocation, which cost a `String` per cell including blanks, so even + full rebuilds got cheaper. Judge W3 on rows/refresh staying high, not + on its timing. The dirty-tracking wins are concentrated in in-place TUI + redraws and the non-PTY refresh callers — above all mouse motion (W1), + which previously rebuilt the entire grid on every pointer event for + zero content change. + **libghostty limitation worked around:** `OSC 10`/`OSC 11` and DECSCNM + change the reported default colors while libghostty reports + `dirty=Clean` with zero rows flagged — the dirty API alone can't see + it — so the consumer must compare the default fg/bg pair itself + (the `cached_defaults` guard above). Tripwire tests in + `crates/roost-vt/tests/render_dirty_test.rs` will fail loudly if a + future Ghostty bump changes this. +* **E3b. Dirty-row rebuild for GTK + real `render_stats` — not started.** + Split out of E3, which originally read "GTK's `terminal_view.rs` gets + the same treatment." Two parts: `crates/roost-linux/src/terminal_view.rs` + adopts `walk_dirty` the same way `roost-iced` did, and + `app.render_stats` gets a real implementation — today it answers with + zeroed counters as a deliberate parity placeholder + (`crates/roost-linux/src/app.rs`'s `UiRequest::AppRenderStats` arm), + so the op's contract is identical on both Rust UIs while only one + collects real numbers. GTK is unaffected by the E1–E3 pass beyond that + placeholder, since `walk` itself was left unchanged. +* **E4. Run coalescing — future work, GO per E1's number.** Merge + adjacent cells sharing fg/bg/style into one `fill_text` run. A + `roost-ui-model` change, so GTK benefits too. Was "conditional on E1 + — do not start this without E1's number"; that number now exists: + a running (debug-build) app doing a 300-line scrolling burst on a full + screen issues ~2,410 `fill_text` calls per draw (one per visible + cell), and draw now dominates a refresh at ~1.37 ms vs. ~1 µs for an + idle refresh post-E3 (696 µs for a full refresh). `fill_text_calls` is + a deterministic counter (host- and build-independent); the nanosecond + figures are debug-build and indicative only. Not being done in this + pass. +* **E5. Sprite parity in Iced.** `mac/Sources/Roost/Sprite.swift` and + `crates/roost-linux/src/sprite.rs` draw U+2500–U+259F (box-drawing, + block elements) geometrically because font glyphs don't tile + pixel-perfectly across adjacent cells. **`roost-iced` has no sprite + path at all** — so TUI chrome shows hairline seams there and not in + either shipped UI. This is a regression against our own shipped Linux + UI, not a macOS concern. The Linux sibling is already Rust: move it to + a shared crate and add the draw call. Add the matching row to the + parity inventory (it has none today, which is why this went unnoticed). +* **E6. IME input.** `crates/roost-iced/src/input.rs` handles no `Ime` + events, so dead keys, CJK input, and the emoji picker are broken. + winit surfaces IME on both platforms — this is a Linux-shipping gap + too, not a macOS-only one. +* **E7. Crash robustness.** No `panic::set_hook` anywhere in `roost-iced` + or `roost-engine`; GTK ships to Linux users today with none. Add a + panic hook that logs + writes a crash file, and close [#299] (verified + release-build infinite loop on a malformed font) — promote it out of + the maintenance backlog. Both are entry gates for M6. +* **E8. Ghostty pin + zig bump** — *sequenced after the M6 direction + resolves.* `third_party/ghostty/build.sh` pins `c74f6d5` (2026-04-25) + against zig 0.15.2 (`.mise.toml`); `../libghostty-rs` pins `ab0b9da` + (2026-07-22, **603 commits ahead**) and needs zig 0.16.x. Wanted + eventually regardless. A large share of the cost is revalidating the + **Swift** build — `mac/Package.swift` links the same static archive — + so the price drops sharply if Swift is retiring. That is why this sits + after the M6 decision, not before it. Carries E2's deferred half: the + two-phase `begin_update` / `end_update` split arrives with the newer + pin, letting the renderer drop the terminal lock before the deferred + work — a latency win under heavy PTY output. +* **E9. `libghostty-rs` integration spike + library audit.** Depends on + E8. Checked out at `../libghostty-rs` (tip `72ac98f`, 2026-07-29, MIT, + not on crates.io — fine, `publish = false`). It is broader than + `roost-vt` (osc, kitty graphics, paste, sgr, screen, unicode, focus) + and more rigorous (borrow-checked lifetimes, typestate updates). Two + integration notes: `GHOSTTY_SOURCE_DIR` can point its `build.rs` at our + existing checkout, and a `pkg-config` feature can discover a + pre-built archive — worth a spike so we don't run zig twice. Adopting + it dissolves the `render_state.rs` ↔ `RenderState.swift` 1:1 parity + correspondence, which is a *cost* while Swift lives and a non-issue + after. Audit for further wins once integrated. + ### Maintenance backlog (filed, not scheduled) Work this migration surfaced that should not block a slice. Pull one in @@ -269,6 +426,7 @@ when it touches the code you are already in: | `roost-engine::facade` has no consumer; prove it or delete it (blocks M5) | [#286] | | `app/interactions.rs` at 2,960 lines — finer split when fixtures allow | [#288] | | swift-testing runner SIGABRT on fast value-check swarms (XCTest workaround) | [#289] | +| **OSC consolidation — watch item, do not act.** `roost-osc` cannot fold into libghostty's OSC parser: `GhosttyOscCommandData` exposes exactly one payload accessor (`CHANGE_WINDOW_TITLE_STR`), identical in our pinned header and in `../ghostty` tip, so a pin bump does not help. libghostty discriminates 22 command *types* but hands back no data for OSC 7 / 9 / 10-12 / 4 / 133 / 52 / 22 — 7 of the 8 events `OscEvent` needs. The custom parts (percent-decode + `file://` extraction, ConEmu OSC 9 sub-command filtering, OSC 52 base64 decode + refuse-on-truncation, `MAX_BODY`, reply synthesis) are policy with no C-API counterpart and would survive anyway. **Exit condition:** libghostty-vt adds `GHOSTTY_OSC_DATA_*` accessors beyond window title — then re-evaluate. | — | [#281]: https://github.com/charliek/roost/issues/281 [#282]: https://github.com/charliek/roost/issues/282 @@ -306,42 +464,20 @@ should re-verify every row against current behavior, close what shipped, and decide [#284] (visual-parity CI gate: required for M4 or waived). After plan 015, the open P1 set is expected to be exactly the 3e polish scope plus the upstream-blocked drops row ([#302]) — the audit confirms -that expectation rather than assuming it. - -### Possible direction — macOS side-by-side evaluation (not committed) - -Recorded 2026-08-05: the Iced work has landed better than expected, and -replacing the Swift app is now a *possible* direction rather than a -non-goal — but it is not guaranteed, a lot of testing stands between here -and any commitment, and **Swift remains the production daily driver -regardless** (guardrail #1 unchanged). Two consequences today: - -1. **Design Iced platform-clean now.** New roost-iced capability with a - native surface gets a per-OS backend seam rather than a Linux-only - shape — notifications (slice 3f) are the first instance: Linux D-Bus - ships; the macOS backend (`UNUserNotificationCenter`, which needs a - real .app bundle identity + code signature) is deferred, not - designed out. No macOS backend gets half-shipped from an unbundled - binary. -2. **M5 is frozen pending this direction** — see below. - -If the direction firms up, the evaluation vehicle is a parallel-install -signed+notarized DMG (the `ai.stridelabs.Roost.iced` profile is already -fully isolated for side-by-side running), gated on a robustness pass: -release-profile CI coverage for Iced, the [#299] swash release-hang fix -(a Mac daily driver meets the full macOS font universe), a panic-hook -crash/feedback story, and a mac-UX gap audit (menu bar, cmd-key -conventions, dock badge — plus 3e's vibrancy spike). Guardrail #3's -"absent from release artifacts" would be amended consciously at that -point (separate opt-in artifact, never bundled into the Swift release). +that expectation rather than assuming it. Amended 2026-08-06: add the new +box-drawing/sprite row (engine-track slice E5) to that expected set. It is +a genuine P1 gap against *both* references, and the fact that it went +unnoticed until a Ghostty-comparison pass — because the inventory had no +row for it — is exactly the drift the audit exists to catch. Treat +"is there a row for this at all?" as an audit question, not just "is this +row current?". ### M5 — Rust under Swift (exploration, frozen) -Frozen 2026-08-05 pending the possible-direction note above: if a -full Iced replacement is evaluated, a Swift-facing FFI boundary is -likely wasted investment — [#286] holds at "don't invest, don't -delete" until the direction resolves. Original exploration plan kept -below for when/if it resumes. +Frozen 2026-08-05 pending M6 below: if a full Iced replacement is +evaluated, a Swift-facing FFI boundary is likely wasted investment — +[#286] holds at "don't invest, don't delete" until the direction +resolves. Original exploration plan kept below for when/if it resumes. The Swift app's polish and daily use make replacement remote; the question is where shared Rust reduces duplication *without* slowdowns. @@ -363,6 +499,142 @@ is where shared Rust reduces duplication *without* slowdowns. 4. Decision gate with data; until then `IPCHandlerImpl.swift` / `Workspace.swift` remain authoritative on Mac. +### M6 — macOS Iced (evaluation → parity) + +**Runs parallel to M4, not after it.** The number follows M5 for document +order only; nothing here waits on shipping Iced to Linux users. + +Status (2026-08-06): replacing the Swift app is now **likely** rather than +merely possible — but it is not committed, it is a ways off, and +**Swift remains the production daily driver throughout** (guardrail #1 +unchanged). This section supersedes the earlier "Possible direction — +macOS side-by-side evaluation" note. Two standing consequences: + +1. **Design Iced platform-clean now.** New roost-iced capability with a + native surface gets a per-OS backend seam rather than a Linux-only + shape — notifications (slice 3f) are the first instance: Linux D-Bus + ships; the macOS backend is deferred, not designed out. No macOS + backend gets half-shipped from an unbundled binary. +2. **M5 stays frozen** while this is live. M6 is the *opposite* direction + from M5 (Iced replaces Swift vs. Rust under Swift); keep them separate + so the frozen thing stays frozen. + +**Entry gate:** E5 and E7 complete; release-profile CI coverage for Iced; +parity-inventory audit clean. Note the reference bar here is the **Swift +app**, which is consistently stricter than M4's GTK bar — the inventory +tracks both, and M6 is the superset. + +**Accepted regressions**, decided rather than discovered: accessibility. +AppKit gives the Swift sidebar and menus VoiceOver support for free; an +Iced canvas gives essentially none. Named here so it is a conscious trade. + +Guardrail #3's "absent from release artifacts" is amended consciously at +6a: a separate opt-in artifact, never bundled into the Swift release. + +Slices: + +* **6a. Bundling + parallel install.** The foundation — Sparkle, + notifications, and TCC testing all need real bundle identity, so + nothing below starts until this lands. `mac/scripts/bundle.sh` is + toolkit-agnostic apart from `swift build --show-bin-path`; fork or + parameterize it over (binary, bundle id, plist). `make-dmg.sh` needs no + change. `roost-iced` must also resolve its profile from its own bundle + id rather than calling `BundleProfile::iced()` unconditionally. + Decisions taken 2026-08-06: + * **Display name `Roost-Iced`** (`CFBundleName` / + `CFBundleDisplayName`; window title should match) — two apps called + "Roost" in the Dock and Cmd-Tab is genuinely confusing. + **Display-only.** `app_label` stays `Roost-iced` (lowercase `i`): + it drives the socket dir, the log dir, and the `identify` wire + response (`roost-ipc/src/messages.rs`), which roosttest asserts on, + and case-changing it is a no-op on macOS but a breaking path change + on Linux, where `roost-iced` also runs. Do not "fix" the + inconsistency. + * **Fresh, separate `state.json`** — no import, no migration. This is + already what `BundleProfile::iced()` does, so it is *zero* work. + Scoped to macOS side-by-side; it does **not** settle M4's Linux + adopt-or-migrate question, where Iced eventually replaces GTK for + the same users. + * **Sparkle/appcast deferred to 6c.** 6a ships with no auto-update + (`SUEnableAutomaticChecks` is already `false`). + Side-by-side is mostly already solved and needs no new machinery: + distinct bundle ids, per-profile socket/state/log paths with tests + asserting distinctness (`roost-ipc/src/paths.rs`), per-profile + single-instance locks. **Claude hooks need no per-app configuration** — + `ROOST_SOCKET` is injected into every PTY child (`roost-engine/src/pty.rs`, + `PtySupervisor.swift`) and sits at precedence #2 in target resolution, + above `--target` and auto-detect, so a Claude session in an Iced tab + dials the Iced socket automatically; `claude_settings_document()` + (`roost-cli/src/main.rs`) bakes no socket or profile into + `claude-settings.json`. One operational wrinkle to note in the install + output, not fix in code: `claude install` writes `self_exe()`, so with + two bundles the hook file points at whichever `roostctl` ran it last. + Two bundle ids also mean two entries in System Settings › Notifications. +* **6b. Native shim seam.** One decision with three consumers (6c/6d/6e): + call AppKit via `objc2` (already in the lockfile through winit) or + build a small Swift static library behind a C ABI. Leaning Swift lib — + it amortizes across Sparkle, menus, and notifications, and the Swift + toolchain is already a build dependency. Decide with a spike, not in + the abstract. +* **6c. Sparkle auto-update.** `mac/Package.swift` + `App.swift` use + ~two API calls (`SPUStandardUpdaterController`, `checkForUpdates(_:)`); + `bundle.sh` already embeds and inside-out-signs the framework, and + `release.yml` already EdDSA-signs and publishes the appcast. The + evaluation build needs a **separate feed or none** so the two apps do + not offer each other's updates. Design note for the eventual cutover: + Sparkle does not care what language wrote the app, so an Iced build + shipping under the *same* bundle id with the same `SUPublicEDKey` and a + higher `CFBundleVersion` upgrades existing Swift installs in place. +* **6d. Menu bar.** winit installs none, so Iced on macOS currently has + no menus at all. `App.swift` builds ~35 items across App/File/View/ + Edit/Window plus a dynamic Window menu of tabs and projects. Options: + `muda` (designed to sit alongside winit) or hand-rolled NSMenu via + `objc2-app-kit`. The keybind story is *better* in Rust — the table + already lives in `roost-ui-model`, so menu equivalents and the terminal + key encoder read one source instead of Swift re-deriving them. Highest + volume, lowest risk; also where "custom options later" becomes cheap + once menu items are just `Message` variants. +* **6e. Desktop notifications, macOS backend.** Closes [#303]. The seam + from slice 3f is backend-agnostic already (`notifications.rs`: worker, + per-tab replace semantics, click routing); only `mod backend` is + missing. `UNUserNotificationCenter` requires a bundled, signed app — + hence the 6a dependency, and it cannot be validated from `make + run-iced`. Match `mac/Sources/Roost/DesktopNotifications.swift` + semantics. Replace-by-server-id becomes UN's stable per-tab identifier, + which is simpler than the D-Bus version. +* **6f. Window vibrancy.** May partly land in 3h, which already owns the + spike. Worth knowing before starting: there is **no `NSVisualEffectView` + anywhere in the Swift source** — the sidebar's translucency is AppKit's + implicit source-list material (`outline.style = .sourceList` plus + `scrollView.drawsBackground = false` and no pane fill), and the Swift + code only ever works *around* it. So there is nothing to port 1:1; + Iced must build it explicitly. The seam is + `iced::window::run(id, |w: &dyn Window| …)` — a main-thread + `HasWindowHandle`, with `raw-window-handle` at 0.6.2, the version + `window-vibrancy` wants. Same call is the hook for Sparkle init, NSMenu + install, and dock badge. Do **not** use `window::Settings { blur }`: + winit's macOS impl calls the private `CGSSetWindowBackgroundBlurRadius` + SPI and blurs the whole window, not a region. The effect view sits + behind the wgpu surface, so the terminal region must stay opaque. +* **6g. macOS platform hygiene.** Individually small, collectively a + pass. Entitlements + purpose strings first: Roost is the TCC + *responsible app* for every child process in a tab, so without + `device.audio-input` / `device.camera` / `automation.apple-events` a + `/voice` or `osascript` in a tab fails **silently** — no prompt, no + error (see the rationale comment in `mac/Resources/Roost.entitlements`, + including which entitlements we deliberately omit). Then: Dock badge + + dock menu, `NSApplicationDelegate` lifecycle (reopen-from-Dock, + open-file/URL, graceful terminate), activation policy, and Secure + Keyboard Entry (`EnableSecureEventInput` — a terminal convention + neither app has today). +* **6h. macOS verification tier.** What makes the parity claim honest + instead of hand-verified: a CGEvent real-input harness ([#285] — the + uinput tier is Linux-only, a gap that matters far more once Mac is a + target), `e2e-iced-mac` as a required gate, and release-profile CI + coverage for Iced. Remember the enumerated-list trap: new roosttest + modules need `ICED_E2E_TESTS` in the `Makefile` *and* the `ci.yml` + lists, or they never run. + ## Gauntlet operating notes * One milestone slice per pass; every pass ends in PR(s) watched to green diff --git a/docs/development/iced-parity-inventory.md b/docs/development/iced-parity-inventory.md index fbf578f9..d0ad321e 100644 --- a/docs/development/iced-parity-inventory.md +++ b/docs/development/iced-parity-inventory.md @@ -148,6 +148,7 @@ product polish, and P2 is an optional native/toolkit refinement. | Terminal scrollback | Wheel/page navigation scrolls retained history locally when mouse reporting is off; alternate-screen behavior follows terminal modes | Closed: wheel and bare PageUp/PageDown page navigation both route through the shared GTK/Iced `roost-vt` policy — retained history, exact bottom state, next-terminal-key snap, mouse-report precedence, alternate-screen arrows/forwarding, and a full-viewport local page move that preserves selection and bypasses snap only on the local route. Swift Mac has no PageUp/PageDown scrollback route (deliberate Rust-UI-first divergence; no prior reference behavior existed) | closed | Physical X11 wheel under both renderers; `roost-vt::route_page` unit/fixture coverage plus both-UI (GTK/Iced) adapter fixtures (selection preserved, zero local PTY bytes, byte-identical Forward path); physical PageUp/PageDown segment in `iced_clipboard_check.py` | | Terminal typography | Configured family/size, baseline and cell metrics stable across styles and graphemes | Renderer-measured size and installed-family selection now reflow every live tab atomically, persist through the shared config policy, and reach new/restored tabs; focused glyph baseline/style comparison remains | P1 | Latin/wide/combined/style fixture under wgpu and tiny-skia; shared GTK/Iced font selection E2E | | Terminal cursor/selection/link | Shared colors and modes with reference-like cursor, selection, and link feedback | Functional coverage exists; geometry/color comparison remains incomplete | P1 | Focused cursor/selection/link screenshots plus existing real-input gates | +| Terminal box-drawing/block glyphs | Both shipped UIs draw U+2500–U+257F and U+2580–U+259F geometrically rather than from the font, because font glyphs do not tile pixel-perfectly across adjacent cells — `mac/Sources/Roost/Sprite.swift` and `crates/roost-linux/src/sprite.rs`, both ports of Ghostty's `font/sprite/draw/{block,box}.zig` and kept in lockstep per the CLAUDE.md parity rule | **No sprite path in `roost-iced` at all** — box-drawing and block elements fall through to font glyphs, so TUI chrome shows hairline seams (most visible in wordmark/logo art) that neither shipped UI has. Row added 2026-08-06; the gap existed unnoticed because this inventory had no box-drawing row. Tracked as engine-track slice E5: the Linux sibling is already Rust, so the work is move-to-shared-crate plus a draw call | P1 | Seam-free capture of a box-drawing/block fixture under both renderers, cross-checked against the GTK and Mac references; codepoint-dispatch unit coverage mirroring the existing Swift/Rust sprite tests | | Palette placement | Centered, elevated compact panel over an undimmed terminal, styled semantic rows, keyboard focus, and scrolling | Closed for the visual-feasibility slice: exact reference neutrals, border/shadow, content-sized 660 pt card capped at 500 pt, compact command/agent/notification/provider rows, shortcut hints, fuzzy-match accents, disabled state, and a narrow neutral scrollbar | closed | Five named GTK/Iced captures; focus/scroll tests plus real row/card/outside pointer routing | | Empty/loading/error states | Deliberate shell placeholders without changing hierarchy | Plain `Starting terminal…`; status errors are appended to the sidebar | P1 | Seeded empty/loading/error snapshots and recovery tests | | Hover/focus/disabled states | Subtle per-control hover and visible focus without global blue fills | Mostly inherited stock theme states | P1 | Renderer-neutral state-style unit tests plus real pointer/keyboard capture | diff --git a/docs/reference/cli.md b/docs/reference/cli.md index b4a585ec..7d78083b 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -196,6 +196,19 @@ roostctl screenshot > shot.png # raw PNG bytes to stdout `--scale` is `1` (default, logical window size) or `2`. With `--out` the CLI writes the file and prints the dimensions + byte count to stderr; without it, the raw PNG bytes go to stdout (nothing else is printed, so the stream stays binary-clean). Backed by the `app.screenshot` IPC op — see [ipc.md](ipc.md). +## `render-stats` + +Read the running UI's render-path counters — the only way to measure the real draw path, since `TerminalWidget::draw` needs a live renderer no unit test can construct. + +```bash +roostctl render-stats # running totals since process start +roostctl render-stats --reset # read, then zero for a clean next delta +``` + +Prints one counter per line plus two derived averages (`ns_per_refresh`, `ns_per_draw`, shown as `-` when the matching call count is zero). `--reset` zeroes the counters **after** the read, so the read-reset / run workload / read pattern gives you a delta directly. + +`refresh_*` covers the snapshot rebuild that walks libghostty's render state (with `rows_rebuilt` / `cells_walked` measuring what it touched); `draw_*` and `fill_text_calls` cover the widget draw pass. Note that `roostctl screenshot` re-renders the window and so inflates the draw counters — read before capturing, or reset after. The GTK UI reports all zeros (no instrumentation yet). Backed by the `app.render_stats` IPC op — see [ipc.md](ipc.md). + ## `palette` subcommands Drive the command-palette overlay: open it, inspect its rows, filter, activate a row, dismiss. Activating a row runs the **same** command its keybind would (a command row's id is its keybind action), so this is a command-dispatch surface, not just a UI poke. Each subcommand prints the resulting palette state (a `>` marks the highlighted row); `--json` emits the structured result. diff --git a/docs/reference/ipc.md b/docs/reference/ipc.md index 70b24cac..cd0b4a7f 100644 --- a/docs/reference/ipc.md +++ b/docs/reference/ipc.md @@ -604,6 +604,49 @@ during a drag are transient UI state, not part of this contract. Ungated, read-only — always available, matching `app.window_metrics`. +### `app.render_stats` + +Read the running UI's render-path counters. This is the only way to +measure the real draw path: `TerminalWidget::draw` needs a live +renderer, which unit tests cannot construct. + +Request: `{"params": {"reset": false}}`. `reset` defaults to `false`, +so `{"params": {}}` — or no `params` at all — is a plain read. + +Response: +```json +{"refresh_calls": "412", "refresh_nanos": "51500000", + "rows_rebuilt": "9888", "cells_walked": "790400", + "draw_calls": "377", "draw_nanos": "94250000", + "fill_text_calls": "9048"} +``` + +Every counter is a **string-wrapped int64** (the same +`string_int64` convention the envelope `id` uses, above): +the nanosecond accumulators pass 2^53 after roughly 104 days of +measured render time, and the rest ride the same convention so the +shape is uniform. All are running totals since process start, or since +the last `reset: true`. + +`refresh_calls` / `refresh_nanos` cover the snapshot rebuild that walks +libghostty's render state; `rows_rebuilt` and `cells_walked` are what +that walk touched. `draw_calls` / `draw_nanos` / `fill_text_calls` +cover the widget draw pass. + +`reset: true` zeroes the counters **after** the read, so a caller can +read-reset, run a workload, then read the delta directly. + +Caveat: `app.screenshot` re-renders the window, so taking a screenshot +inflates the three draw counters. Read before capturing, or reset +after. + +Ungated — always available, matching `tab.dump_resolved`. Not +read-only: `reset: true` reads the counters and then zeroes them. +The GTK UI answers with the same shape, all counters zero: its +renderer has no instrumentation yet, and a uniform contract across +both Rust UIs beats an op one of them refuses. CLI: `roostctl +render-stats [--reset]`. + ### `sidebar.set_width` *(test-only — gated)* **Requires `ROOST_TEST_MODE=1` set in the UI's launch environment.** diff --git a/tests/ipc-vectors/app.render_stats.request.json b/tests/ipc-vectors/app.render_stats.request.json new file mode 100644 index 00000000..17d61756 --- /dev/null +++ b/tests/ipc-vectors/app.render_stats.request.json @@ -0,0 +1,7 @@ +{ + "id": "21", + "op": "app.render_stats", + "params": { + "reset": false + } +} diff --git a/tests/ipc-vectors/app.render_stats.response.json b/tests/ipc-vectors/app.render_stats.response.json new file mode 100644 index 00000000..480b272b --- /dev/null +++ b/tests/ipc-vectors/app.render_stats.response.json @@ -0,0 +1,13 @@ +{ + "id": "21", + "ok": true, + "result": { + "refresh_calls": "412", + "refresh_nanos": "51500000", + "rows_rebuilt": "9888", + "cells_walked": "790400", + "draw_calls": "377", + "draw_nanos": "94250000", + "fill_text_calls": "9048" + } +} diff --git a/tools/README.md b/tools/README.md index 7adfd7ed..4d46c9ff 100644 --- a/tools/README.md +++ b/tools/README.md @@ -2,7 +2,9 @@ Three layers, by *what they can verify* and *how they drive the app*. Reach for the highest layer that can answer your question — it's faster, -more deterministic, and more portable. +more deterministic, and more portable. Layers 1-3 verify *correctness*; +[`perf/`](perf/README.md) measures *cost* instead and is not part of this +ladder — see its own README. ``` tools/ @@ -11,6 +13,10 @@ tools/ input/ Layer 3 — real OS input injection, platform-specific. linux/ uinput key/pointer + clipboard + single-monitor (COSMIC/Wayland). (mac/) CGEvent equivalent — planned. + perf/ Render-path cost — a sibling axis, not a layer (see intro above). + roosttest_unit/ (non-tier — fast unit tests for the harness wiring itself) + shed/ (non-tier — Apple VZ Linux microVM driver for Linux testing from a Mac) + wayland/ (non-tier — Wayland-specific test support) ``` | Layer | Dir | Drives via | Verifies | Platforms | CI | diff --git a/tools/perf/README.md b/tools/perf/README.md new file mode 100644 index 00000000..739840b6 --- /dev/null +++ b/tools/perf/README.md @@ -0,0 +1,142 @@ +# Roost render performance harness (`tools/perf/`) + +Measures the **cost** of the Iced UI's render path — not correctness. +`tools/perf/` is a sibling of `tools/roosttest/`, `tools/screenshot/`, and +`tools/input/` (see [`../README.md`](../README.md)), not a fourth tier of +that ladder: those three verify *behavior*; this measures *how much CPU +work it took*. + +Two readouts, backed by the counters `crates/roost-iced/src/perf.rs` +instruments: + +| Readout | What it reads | Needs a running UI? | +|---|---|---| +| `cargo test -p roost-iced --release -- --ignored --nocapture` | Per-tab `TabRenderStats` (`refresh_calls`, `refresh_nanos`, `rows_rebuilt`, `cells_walked`) | No — an in-crate `#[ignore]`d test | +| `tools/perf/render-stats.sh ` (→ `roostctl render-stats`) | The process-global aggregate, same fields plus `draw_calls` / `draw_nanos` / `fill_text_calls` | Yes | + +## The in-crate test: `cargo test -p roost-iced --release -- --ignored --nocapture` + +`roost-iced` has no `[lib]` target +(`crates/roost-iced/Cargo.toml`), so nothing outside the crate can import +`TerminalTab` — an external bench binary isn't an option. The harness is +instead an `#[ignore]`d test, +`crates/roost-iced/src/app/perf_bench.rs::refresh_snapshot_perf_harness`, +reusing the same `attach_test_terminal` fixture the rest of the app's +unit tests use. It is genuinely `#[ignore]`d: a normal `cargo test -p +roost-iced` (what CI runs) never executes it. Always run it `--release` +— debug-profile timings aren't representative and aren't comparable +across runs. + +It runs three workloads, N=200 refreshes each, and prints one block of +`key value` lines per workload. **The format is stable and greppable +on purpose** — a before/after comparison (see below) diffs two runs of +this output. + +### The three workloads + +- **W1 — pointer-motion storm.** 200 refreshes with **no terminal + mutation** between them. This is the shape + `crates/roost-iced/src/app/interactions.rs:1341` produces: + `refresh_snapshot` runs on every single mouse-motion event over a + terminal, even though nothing in the grid changed. **This is the + headline case** — today it rebuilds the entire grid for zero-content + motion. +- **W2 — in-place TUI redraw.** 200 iterations, each writing to a couple + of fixed rows via absolute cursor positioning (`\x1b[{row};1H...`) so + the viewport never scrolls, then refreshing. The vim/htop shape. +- **W3 — scrolling stream (CONTROL).** 200 iterations of plain + `line\r\n` output that scrolls the viewport, then refreshing. + **This workload is the control: expect little or no gain in + rows-rebuilt, ever.** libghostty full-rebuilds its render state + whenever the viewport's scroll pin changes + (`third_party/ghostty/src/src/terminal/render.zig:299-302`), so no + amount of dirty-tracking on our side reduces the rows it hands back — + E3 measured 32.00 -> 27.50 rows/refresh here, against 32.00 -> 0.16 + for W1. + + Its *timing* can still improve, and did: 2.0x (116,609 -> 57,473 + ns/refresh) in the E3 measurement, because removing the dense + `vec![vec![String::new(); cols]; rows]` allocation made even a full + rebuild cheaper. So judge W3 on **rows_per_refresh staying high**, not + on its clock. A W3 whose rows/refresh collapses means rows are being + served from a stale cache across a viewport move — that is a bug, not + a win. + +### What it can't measure + +This harness only exercises `refresh_snapshot` — the snapshot rebuild +that walks libghostty's render state. It cannot produce the `draw_*` / +`fill_text_calls` counters: `TerminalWidget::draw` needs a live iced +`Renderer`, which only the windowing system hands it, and no unit test +constructs one. For those, use `render-stats.sh` against a running app. + +### Before/after comparison + +A single run's numbers mean little on their own; what matters is the +delta between two runs of the *same machine, same moment* — CPU +frequency scaling, thermal throttling, and background load all move the +absolute numbers around run to run. Compare two runs taken back to back +(e.g. via a `git worktree` checking out two commits side by side, so +each build is fresh and neither run waits on the other's cache) rather +than trusting a number captured hours or days apart. + +```bash +cargo test -p roost-iced --release -- --ignored --nocapture > before.txt +# ...checkout the candidate change... +cargo test -p roost-iced --release -- --ignored --nocapture > after.txt +diff before.txt after.txt +``` + +Read `rows_per_refresh` first — it's the direct measure of "how much of +the grid did this refresh actually touch." Pre-E3 every workload +reported the full 32 rows/refresh; the point of dirty tracking is +driving W1 and W2 toward 0 while W3 stays high. + +## The running-app readout: `tools/perf/render-stats.sh` + +```bash +tools/perf/render-stats.sh iced # interactive: prompt before reading +tools/perf/render-stats.sh iced 10 # reset, sleep 10s, read +tools/perf/render-stats.sh mac +tools/perf/render-stats.sh gtk # GTK has no instrumentation yet — reports all zeros +``` + +Resets the counters, waits for you to exercise the running UI (either +interactively or for a fixed duration), then prints `roostctl +render-stats`'s delta since the reset. `--target` follows the rest of +`tools/`: `mac|gtk|iced` (see [`../screenshot/README.md`](../screenshot/README.md) +for the per-target launch/socket details this script reuses via +`../screenshot/lib.sh`). + +This is the *only* way to read `draw_calls` / `draw_nanos` / +`fill_text_calls` — they require a live iced `Renderer`, which only a +running UI has. `refresh_*` / `rows_rebuilt` / `cells_walked` are also +available here (same fields the in-crate test prints), aggregated across +every tab in the process rather than per-tab. + +## Two traps to know about before trusting a number + +- **The locked-Mac caveat.** External/presented frame rate is + meaningless on a locked or occluded Mac — macOS throttles + presentation regardless of how much CPU work the app is doing, so a + wall-clock or FPS-style measurement taken against a locked/backgrounded + window tells you about the OS's compositor policy, not about Roost. + This is exactly why this harness measures CPU-side spans + (`refresh_nanos`, `draw_nanos`) and deterministic counters + (`rows_rebuilt`, `cells_walked`, `draw_calls`) instead of frame rate. + There is no workaround for this — presented-frame timing on a + locked/occluded window is not a signal this harness can produce, so it + doesn't try. +- **The screenshot trap.** `iced::window::screenshot` re-renders the + window, so `roostctl screenshot` and everything in + [`../screenshot/`](../screenshot/README.md) inflate `draw_calls` / + `draw_nanos` / `fill_text_calls` just by having run. If a screenshot + happens inside your measurement window, either read the counters + *before* taking it, or `roostctl render-stats --reset` (or + `render-stats.sh`'s reset step) *after* it, before the window you + actually care about. + +See `crates/roost-iced/src/perf.rs`'s module doc for the underlying +counters, and [`docs/reference/cli.md`](../../docs/reference/cli.md) / +[`docs/reference/ipc.md`](../../docs/reference/ipc.md) for the `roostctl +render-stats` / `app.render_stats` wire contract. diff --git a/tools/perf/render-stats.sh b/tools/perf/render-stats.sh new file mode 100755 index 00000000..92b704ae --- /dev/null +++ b/tools/perf/render-stats.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# Read a running Iced UI's render-path counters (`roostctl render-stats`) +# around a measurement window, so the printed numbers are a clean delta +# instead of a running total since process start. +# +# tools/perf/render-stats.sh iced # interactive: prompt before reading +# tools/perf/render-stats.sh iced 10 # reset, sleep 10s, read +# +# Only the *running-app* half of the perf harness: `draw_calls` / +# `draw_nanos` / `fill_text_calls` need a live iced `Renderer`, which only +# a running UI has (see tools/perf/README.md). The `refresh_*` / +# `rows_rebuilt` / `cells_walked` counters this also prints are the same +# ones the in-crate `cargo test -p roost-iced --release -- --ignored +# --nocapture` harness measures without a UI — reach for that instead +# when you don't need the draw-path numbers. +# +# --target follows the rest of tools/: mac|gtk|iced (see ../screenshot/). +# The GTK UI has no render-path instrumentation yet, so it reports all +# zeros — that is expected, not a bug in this script. +set -euo pipefail +source "$(cd "$(dirname "${BASH_SOURCE[0]}")/../screenshot" && pwd)/lib.sh" + +TARGET="${1:?usage: render-stats.sh [duration_seconds]}" +DURATION="${2:-}" + +ut_init "${TARGET}" +ut_alive || { + echo "error: ${TARGET} UI is not running — launch it first (tools/screenshot/launch.sh ${TARGET})" >&2 + exit 1 +} + +echo "==> resetting ${TARGET} render-stats counters" >&2 +rc render-stats --reset >/dev/null + +if [[ -n "${DURATION}" ]]; then + echo "==> waiting ${DURATION}s — exercise the app now (scroll, type, resize...)" >&2 + sleep "${DURATION}" +else + read -rp "Exercise the app now, then press Enter to read the counters... " _unused +fi + +echo "==> ${TARGET} render-stats since reset:" >&2 +rc render-stats