From 0bdfc9b6ddd7e0f588055bbc0bf182ec36d5e98c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 15 Aug 2026 15:10:07 +0200 Subject: [PATCH] fix(panel): the terminal mapping half --- classification, domain, publication Three of the four terminal gaps. The fourth is recorded as owed rather than faked; see below. **THE STABLE CLASSIFICATION WAS INCOMPLETE.** Cursor motion still advanced the mapping revision: `restore_cursor`, `horizontal_tab`, `move_vertical`, `move_horizontal`, `set_col` and `set_row` all called `changed()`. Moving the caret denotes nothing new, and a child that merely repositions its cursor would have cancelled a drag. All six take the display-only path now. Worse, **rewriting the same glyph under another style advanced it**, which is precisely the control SS5b requires to hold. `write_character` now compares the glyph before writing --- sampled BEFORE `clear_wide_at`, which blanks a cell that is part of a wide pair and would otherwise make every rewrite look like a change. That ordering was found by instrumenting the failing row, not by reading the code. **THE SNAPSHOT CARRIED DOCUMENT-ONLY STATE FOR TERMINALS.** `view_top`, `view_left`, wrap, content columns, fold policy and folds describe a document projection and take no part in a terminal's, where the child's screen decides the mapping. They live inside the `Document` arm now; only common geometry --- buffer identity, rows, columns --- stays outside. **AND THE REVISION WAS NOT PUBLICATION-CONSISTENT.** `view_mapping_identity` read the LIVE screen revision while `projection_ref` returns the last PUBLISHED cells, so buffered output under synchronized-output would stamp displayed cells with authority they were never painted under --- a frontend echoing a generation matching nothing it can see. `ScreenProjection` carries `mapping_revision` now and the published value is what is read. **The witnesses were separated across the seam**, which review named exactly: `screen.rs` proved the counter, the daemon proved enum selection, and a `view_mapping_identity` returning a constant would have left both green. A daemon-level row now drives real events through a panel terminal and asserts the daemon's generation moves on a new glyph and holds across a style-only rewrite and across cursor motion. **OWED, NOT DONE: the scroll-anchor row.** The anchor is in the key, but three attempts failed to drive a scroll from this fixture --- `scroll_lines` wants a viewport the projection registers on its own schedule, and `scroll_view` with an explicit size reports no movement after forty line feeds. Recorded in the ledger rather than faked or quietly dropped: without it, a constant ANCHOR alongside a live revision still passes every terminal row that exists. The two test hooks are `#[doc(hidden)] pub`, not `#[cfg(test)]`, because the rows needing them are integration tests and those link the library without `cfg(test)`. Verified: focused suite 37/37, `cargo test --lib` 1945 green, clippy clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 11 +++ src/editor.rs | 54 +++++++----- src/terminal/screen.rs | 62 +++++++++++-- src/terminal/view.rs | 39 ++++++++- .../bottom_panel_stage2b_daemon_acceptance.rs | 86 +++++++++++++++++++ 5 files changed, 223 insertions(+), 29 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index a0e5a9d..32b3f3f 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -437,6 +437,17 @@ from #171 and #215. three of them lost to these two signatures. U9's synthetic-load control remains unrun and is the cheapest thing that would either implicate load or clear it. +- **OWED WITNESS — terminal scroll-anchor movement at the daemon + level.** The anchor is in the key (`PanelMappingContent::Terminal` + carries it) and the review asked for a row proving the daemon's + generation moves when it does. **Three attempts failed to drive a + scroll from the panel fixture**: `scroll_lines` needs a viewport the + panel projection registers on its own schedule, and `scroll_view` + with an explicit content size still reports no movement after forty + line feeds, so the history it would move into is not accumulating as + the fixture assumes. **Recorded rather than faked.** Without it, a + `view_mapping_identity` returning a constant ANCHOR while reporting a + live revision passes every terminal row that exists. - **Gates:** the four `bottom_panel_*` suites, the GUI 1a wire suite, `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`, and **`--protocol`**. diff --git a/src/editor.rs b/src/editor.rs index ecf6f0b..139bf84 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -396,12 +396,6 @@ pub struct PanelMappingSnapshot { /// inverts nothing like a 6×2 one. rows: u32, cols: u32, - view_top: usize, - view_left: u32, - wrap: crate::view::WrapMode, - content_cols: u32, - fold_projection: bool, - folds: Vec, content: PanelMappingContent, } @@ -422,6 +416,18 @@ pub enum PanelMappingContent { Document { /// The buffer's content revision, or `None` if it is gone. revision: Option, + /// Vertical viewport origin. + view_top: usize, + /// Horizontal viewport origin. GUI arc 1b makes this move. + view_left: u32, + /// Wrap mode: it decides how a source line becomes display rows. + wrap: crate::view::WrapMode, + /// Text width with the gutter reservation already subtracted. + content_cols: u32, + /// The owning frontend's fold projection policy. + fold_projection: bool, + /// Fold content, read at its source. + folds: Vec, }, /// A terminal panel: the screen's **mapping revision** and the /// view's scroll anchor. @@ -2545,27 +2551,33 @@ impl EditorState { .get(buffer_id) .ok() .map(crate::buffer::Buffer::revision); - PanelMappingContent::Document { revision } + PanelMappingContent::Document { + revision, + view_top: window.view_top, + view_left: window.view_left, + wrap: window.last_wrap, + content_cols: window.last_content_cols, + fold_projection: core + .views + .get(&frontend_id) + .is_some_and(|view| view.fold_projection), + // Read at their SOURCE — the registry's ranges — rather + // than through the derived `VisibleLineMap`, whose only + // public summary is `is_identity()`. Too coarse: a fold + // edit leaving the map non-identity still changes which + // source line a row shows. + folds: core.fold_registry.folds(buffer_id), + } }; + // Only COMMON geometry lives out here. Everything below is + // domain-specific: `view_top`, `view_left`, wrap, gutter width + // and folds describe a DOCUMENT projection and take no part in a + // terminal's, where the child's screen decides the mapping. Some(PanelMappingSnapshot { buffer_id, rows: size.rows, cols: size.cols, - view_top: window.view_top, - view_left: window.view_left, - wrap: window.last_wrap, - content_cols: window.last_content_cols, - fold_projection: core - .views - .get(&frontend_id) - .is_some_and(|view| view.fold_projection), - // Read at their SOURCE — the registry's ranges — rather than - // through the derived `VisibleLineMap`, whose only public - // summary is `is_identity()`. That is too coarse: a fold edit - // leaving the map non-identity still changes which source - // line a row shows. - folds: core.fold_registry.folds(buffer_id), content, }) } diff --git a/src/terminal/screen.rs b/src/terminal/screen.rs index 87d4140..516c5d7 100644 --- a/src/terminal/screen.rs +++ b/src/terminal/screen.rs @@ -114,6 +114,15 @@ pub struct ScreenProjection { pub title: Option, /// Screen generation represented by this projection. pub generation: u64, + /// §5b — the **mapping revision** this projection was published at. + /// + /// Carried here, not read live, because the two diverge while + /// synchronized output is held: `projection_ref` keeps returning the + /// last PUBLISHED cells while the live screen races ahead, so + /// stamping a frame with the live revision would give displayed + /// cells authority they were never painted under — a frontend would + /// then echo a generation that matches nothing it can see. + pub mapping_revision: u64, } /// Borrowed, publication-consistent row projection for in-process views. @@ -128,6 +137,8 @@ pub(crate) struct BorrowedScreenProjection<'a> { pub cursor: Option, pub title: Option<&'a str>, pub generation: u64, + /// §5b — the mapping revision this projection was published at. + pub mapping_revision: u64, } impl BorrowedScreenProjection<'_> { @@ -147,6 +158,7 @@ impl ScreenProjection { cursor: self.cursor, title: self.title.as_deref(), generation: self.generation, + mapping_revision: self.mapping_revision, } } } @@ -224,6 +236,7 @@ impl TerminalScreen { cursor: Some(CellCoord::new(0, 0)), title: None, generation: 0, + mapping_revision: 0, }; Ok(Self { size, @@ -599,6 +612,7 @@ impl TerminalScreen { .then(|| CellCoord::new(self.cursor.row as u32, self.cursor.col as u32)), title: self.title.as_deref(), generation: self.generation, + mapping_revision: self.mapping_revision, } } @@ -658,7 +672,7 @@ impl TerminalScreen { self.g0 = saved.g0; self.g1 = saved.g1; self.use_g1 = saved.use_g1; - self.changed(); + self.display_only_changed(); } fn write_text(&mut self, text: &str) { @@ -812,6 +826,24 @@ impl TerminalScreen { if self.modes.insert { self.insert_characters(width as u32); } + // §5b — did the GLYPH change, or only the pen? Rewriting the + // same character in a new colour repaints the cell without + // changing what the coordinate denotes, and a drag must survive + // it. + // + // Sampled BEFORE `clear_wide_at`, which blanks the cell when it + // is part of a wide pair — sampling after would compare the new + // glyph against a default and call every rewrite a change. + let glyph_changed = { + let row = self.cursor.row; + let col = self.cursor.col; + let cells = &self.active().rows[row].cells; + cells[col].glyph != Glyph::Char(ch) + || (width == 2 + && cells + .get(col + 1) + .is_none_or(|next| next.glyph != Glyph::Continuation)) + }; self.clear_wide_at(self.cursor.row, self.cursor.col); if width == 2 { self.clear_wide_at(self.cursor.row, self.cursor.col + 1); @@ -832,7 +864,11 @@ impl TerminalScreen { self.cursor.pending_wrap = false; } self.last_grapheme = Some((row, col)); - self.changed(); + if glyph_changed { + self.changed(); + } else { + self.display_only_changed(); + } } fn soft_wrap(&mut self) { @@ -886,7 +922,7 @@ impl TerminalScreen { .copied() .unwrap_or(cols - 1); self.cursor.pending_wrap = false; - self.changed(); + self.display_only_changed(); } fn move_vertical(&mut self, delta: i64) { @@ -903,7 +939,7 @@ impl TerminalScreen { }; self.cursor.row = moved.clamp(lo, hi); self.cursor.pending_wrap = false; - self.changed(); + self.display_only_changed(); } fn move_horizontal(&mut self, delta: i64) { @@ -915,13 +951,13 @@ impl TerminalScreen { }; self.cursor.col = moved.min(self.size.cols as usize - 1); self.cursor.pending_wrap = false; - self.changed(); + self.display_only_changed(); } fn set_col(&mut self, col: u32) { self.cursor.col = col.saturating_sub(1).min(self.size.cols - 1) as usize; self.cursor.pending_wrap = false; - self.changed(); + self.display_only_changed(); } fn set_row(&mut self, row: u32) { let base = if self.modes.origin { @@ -936,7 +972,7 @@ impl TerminalScreen { }; self.cursor.row = (base + row.saturating_sub(1) as usize).min(hi); self.cursor.pending_wrap = false; - self.changed(); + self.display_only_changed(); } fn set_position(&mut self, row: u32, col: u32) { self.set_row(row); @@ -1525,8 +1561,20 @@ impl TerminalScreen { .then(|| CellCoord::new(self.cursor.row as u32, self.cursor.col as u32)), title: self.title.clone(), generation: self.generation, + mapping_revision: self.mapping_revision, } } + /// Publish the current projection, for tests that drive events + /// directly instead of through the PTY reader. + /// + /// `#[doc(hidden)]` rather than `#[cfg(test)]`: the rows that need + /// it are integration tests, which link the library WITHOUT + /// `cfg(test)` and so cannot see a gated item. + #[doc(hidden)] + pub fn publish_for_test(&mut self) { + self.publish(); + } + fn publish(&mut self) { self.published = self.current_projection(); } diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 0719c6e..414d607 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -249,6 +249,34 @@ impl TerminalManager { self.screen_size(key.buffer_id) } + /// Apply one parsed event to a session's screen, for tests. + /// + /// Terminal output normally arrives on the PTY reader thread, which + /// no daemon-level test can drive deterministically. §5b's terminal + /// rows must nevertheless be witnessed **across the seam** — the + /// screen counter and the daemon's key are separately provable, and + /// a `view_mapping_identity` returning a constant would leave both + /// green — so this exists to join them. + /// + /// `#[doc(hidden)]` rather than `#[cfg(test)]`, because the rows + /// that need it are integration tests and those link the library + /// without `cfg(test)`. + #[doc(hidden)] + pub fn apply_event_for_test( + &mut self, + buffer_id: BufferId, + event: crate::ansi::AnsiEvent, + ) -> bool { + match self.sessions.get_mut(&buffer_id) { + Some(session) => { + session.screen.apply_event(event); + session.screen.publish_for_test(); + true + } + None => false, + } + } + /// §5b — the terminal's **mapping revision** plus its per-view /// scroll anchor: together, the identity of what a coordinate in /// this view denotes. @@ -265,7 +293,14 @@ impl TerminalManager { // `top` IS the anchor: `None` means following the live tail, // which is itself a distinct state from any pinned row. let anchor = self.views.get(&key).and_then(|view| view.top); - Some((session.screen.mapping_revision(), anchor)) + // The PUBLISHED revision, not the live one. While synchronized + // output is held, `projection_ref` keeps returning the last + // published cells while the screen races ahead — reading + // `screen.mapping_revision()` there would stamp displayed cells + // with authority they were never painted under, and the frontend + // would echo a generation matching nothing it can see. + let published = session.screen.projection_ref().mapping_revision; + Some((published, anchor)) } /// The shared screen's current size, read from the borrowed @@ -984,6 +1019,7 @@ mod tests { cursor: None, title: Some("shell".into()), generation: 7, + mapping_revision: 0, } } @@ -1264,6 +1300,7 @@ mod tests { cursor: None, title: None, generation: 2, + mapping_revision: 0, }; let mut state = TerminalViewState { top: Some(LogicalCellAnchor { diff --git a/tests/bottom_panel_stage2b_daemon_acceptance.rs b/tests/bottom_panel_stage2b_daemon_acceptance.rs index a0cda31..aa8dab0 100644 --- a/tests/bottom_panel_stage2b_daemon_acceptance.rs +++ b/tests/bottom_panel_stage2b_daemon_acceptance.rs @@ -1744,3 +1744,89 @@ fn the_mapping_snapshot_picks_the_terminal_domain_for_a_terminal_panel() { ); } } + +/// §5b — the terminal key **across the seam**. +/// +/// `screen.rs` proves the counter classifies events correctly, and the +/// row above proves the snapshot picks the terminal domain. Neither +/// notices a `view_mapping_identity` that returns a CONSTANT: the +/// classification is right, the branch is taken, and the daemon's key +/// still never moves. These rows join the two halves. +#[test] +fn g2_g3_a_terminal_panels_key_tracks_its_screen_and_anchor() { + use pmacs::ansi::AnsiEvent; + + let mut session = Session::new(); + session.declare(1, 24, 80); + exec( + &session.state, + "TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", \ + args = { \"-c\", \"sleep 30\" }, display = \"panel\" }", + ); + let _ = session.frame(); + + let buffer_id = { + let core = session.state.core.borrow(); + let side = core.side_window_for(FID).expect("side"); + core.windows[&side].buffer_id + }; + let feed = |session: &Session, event: AnsiEvent| { + assert!( + session + .state + .terminal_manager + .borrow_mut() + .apply_event_for_test(buffer_id, event), + "the panel's terminal session must exist" + ); + }; + + let start = mapping_generation(&mut session).expect("a terminal panel has a key"); + + // CHANGING: a glyph appears where none was. + feed(&session, AnsiEvent::Text("A".to_owned())); + let after_glyph = mapping_generation(&mut session).expect("a key"); + assert!( + after_glyph > start, + "new output changes what a coordinate denotes" + ); + + // STABLE: the same glyph rewritten under a different pen. This is + // the row that forced `write_character` to compare glyphs — a + // blanket bump made a recolour cancel the drag. + feed(&session, AnsiEvent::CursorPosition { row: 1, col: 1 }); + feed( + &session, + AnsiEvent::SetStyle(pmacs_protocol::Style::default()), + ); + feed(&session, AnsiEvent::Text("A".to_owned())); + assert_eq!( + mapping_generation(&mut session), + Some(after_glyph), + "rewriting the SAME glyph in another style repaints the cell \ + without moving what it denotes" + ); + + // STABLE: ordinary cursor motion. + feed(&session, AnsiEvent::CursorPosition { row: 2, col: 3 }); + assert_eq!( + mapping_generation(&mut session), + Some(after_glyph), + "moving the caret denotes nothing new — and these paths were \ + advancing the revision until §5b's terminal correction" + ); + + // NOT YET WITNESSED: scroll-anchor movement at this level. + // + // The anchor IS in the key — `PanelMappingContent::Terminal` carries + // it — but driving a scroll from here has defeated three attempts: + // `scroll_lines` needs a viewport the panel projection registers on + // its own schedule, and `scroll_view` with an explicit size still + // reports no movement after forty line feeds, so the history the + // scroll would move into is not accumulating the way this fixture + // assumes. + // + // Recorded as OWED rather than faked. Without it, a + // `view_mapping_identity` that returned a constant ANCHOR while + // reporting a live revision would pass every row above. +}