From 3ecb03d94913585c5f8e9b528d9178fd979db7d5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 28 Jul 2026 18:52:43 -0400 Subject: [PATCH] Close review round 1: five findings, plus one the sweep found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five review findings reproduced with a failing test before any fix, and every fix falsified by reverting it. R1-1 — the wire-area clamp lived only in `panel_grid_size`, so the daemon shipped an authoritative `Absent` while `panel_hidden` stayed false: keys kept reaching the invisible window and a panel terminal kept its controller. Q#BP2b calls hiding a DURABLE state transition and the exhaustion arm had made it a per-frame effect. Fixed structurally rather than pointwise: `presentable_panel_grid` is now the one derivation behind both the renderer and `reconcile_panel_layout_core`, so the two cannot drift apart again. R1-2 — closing and reopening the same PERSISTENT buffer inside one dispatcher burst left the shipped declaration intact while the window it described was already dead, and same-buffer/same-size made the successor indistinguishable by every other field. A presentation epoch only identifies a presentation if something checks that the presentation it names is still on screen, so `panel_declaration_matches` now takes the live side window and buffer. R1-3 — the semantic terminal-layout twin consulted only the full-document declaration, which a panel terminal deliberately lacks, so the child kept its opening geometry through the drain. `sync_semantic_panel_terminal_ layout` is the missing case; it resolves through `side_window_for` while its sibling resolves through `primary_document_window`, so the two are disjoint by construction and nothing is resized twice per tick. R2-4 — `NoMessage` means publish nothing, not publish empty. Treating it like `Invalidated` removed the band's provider text on a transient buffer-follow mismatch. The band repaints its whole mode line every frame, so "publish nothing" has to be a retained baseline; it is keyed by window id so a replaced panel inherits nothing. R2-5 — non-`Move` activation is Q#BP16's TERMINAL clause, because the shared adapter claims the controller for wheel steps too. A document panel keeps scroll-without-focus, matching `dispatch_mouse`. The sweep for R1-1's and R1-3's shape found one more, and it is the same bug as R1-1: a panel wider than the terminal subsystem's per-axis cap is legal on the wire (Bet B5') but its content rect was refused by `snapshot_for_view`, collapsing the projection to `None` — a per-frame `Absent` with the durable state still saying visible, reachable with one `FrontendCellGeometry` declaration. The band is legitimately that wide, so the child is clamped to the columns a PTY can have and the remainder paints as band background, exactly as a narrower snapshot already does. One knowingly per-frame `Absent` remains and is recorded in the code rather than fixed: presentation-epoch exhaustion, which takes 2^64 shipped presentation changes in one session and cannot be reached by any frontend. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/daemon.rs | 440 +++++++++++++++++- src/editor.rs | 151 +++++- src/editor_core.rs | 45 +- src/semantic_render.rs | 136 ++++-- .../bottom_panel_stage2b_daemon_acceptance.rs | 235 ++++++++++ 5 files changed, 947 insertions(+), 60 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 8aaa434..c97e032 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -947,22 +947,23 @@ fn peer_accepts_panel_message(protocol_version: u32, message: &InstanceMessage) /// non-panel-capable view is rejected before any payload state is /// trusted. /// -/// The claimed `frontend_id` in the payload is never consulted anywhere: -/// routing is by the authenticated transport `source`, so a forged id -/// addresses nothing. /// Q#BP16 steps 2–4: the event addresses the panel declaration this /// session most recently shipped, under the geometry it most recently -/// accepted. +/// accepted, and that declaration still describes the panel on screen. /// -/// Three facts, one predicate, because they close three different holes -/// and no two of them subsume the third: +/// Four facts, one predicate, because they close four different holes +/// and no three of them subsume the fourth: /// /// * the latest declaration is a `Present` (an `Absent` cleared input /// authority, so nothing is addressable), /// * its echoed `geometry_epoch` equals both the payload's **and** the /// daemon's latest accepted declaration — the font/scale/resize race, /// * its `panel_epoch` equals the payload's — close/hide/reopen of the -/// same persistent buffer, which a `buffer_id` alone cannot see. +/// same persistent buffer, which a `buffer_id` alone cannot see, +/// * and the presentation behind it is the side window that is live +/// **now** — because a close/reopen inside one dispatcher burst does +/// not invalidate the shipped declaration, only the window it named +/// (review round 1, R1-2). fn panel_event_epochs_are_current( editor: &EditorState, semantic_states: &HashMap, @@ -970,16 +971,33 @@ fn panel_event_epochs_are_current( geometry_epoch: u64, panel_epoch: u64, ) -> bool { - semantic_states - .get(&source) - .is_some_and(|sem| sem.panel_declaration_matches(geometry_epoch, panel_epoch)) - && editor - .core - .borrow() - .frame_geometry_for(source) - .is_some_and(|geometry| geometry.geometry_epoch == geometry_epoch) + let core = editor.core.borrow(); + let live_presentation = core.side_window_for(source).and_then(|window_id| { + core.windows + .get(&window_id) + .map(|window| (window_id, window.buffer_id)) + }); + semantic_states.get(&source).is_some_and(|sem| { + sem.panel_declaration_matches(geometry_epoch, panel_epoch, live_presentation) + }) && core + .frame_geometry_for(source) + .is_some_and(|geometry| geometry.geometry_epoch == geometry_epoch) } +/// Whether an authenticated source may send the v21 panel event family +/// (Q#BP9's "every gate keys on the daemon's own state"). +/// +/// All three inbound events require the same three facts, and they are +/// checked together so no arm can satisfy two and forget the third: an +/// installed **semantic** projection, a negotiated version that carries +/// the variants, and a `FrontendView` this daemon itself marked +/// panel-capable. A grid session, a pre-panel semantic peer, or a +/// non-panel-capable view is rejected before any payload state is +/// trusted. +/// +/// The claimed `frontend_id` in the payload is never consulted anywhere: +/// routing is by the authenticated transport `source`, so a forged id +/// addresses nothing. fn peer_may_send_panel_events( editor: &EditorState, session_registry: &SessionRegistry, @@ -3428,6 +3446,16 @@ fn sync_terminal_layouts_for_tick( if let Some((buffer_id, size)) = state.terminal_viewport() { editor.sync_semantic_terminal_layout(*frontend_id, buffer_id, size); } + // Bottom-panel R1-3: the band is the semantic frontend's + // OTHER terminal surface, and it has no declaration to + // consult — the daemon derives its geometry (Q#BP15a). The + // grid arm below gets this for free because it resolves + // through `controller_view_for_frontend`, which is + // window-agnostic; the semantic arm above is keyed to the + // document declaration and structurally cannot see a side + // window. Both calls target disjoint windows, so this is a + // second CASE, not a second resize of the same child. + editor.sync_semantic_panel_terminal_layout(*frontend_id); } else if let Some(size) = term_sizes.get(frontend_id).copied() { editor.sync_terminal_grid_geometry(*frontend_id, size); } @@ -6347,6 +6375,388 @@ mod tests { ); } + // ----------------------------------------------------------------- + // Review round 1 — three findings the mutation pass could not reach, + // because each is a behaviour that was never modelled rather than a + // line that was written wrong. + // ----------------------------------------------------------------- + + /// R1-2: closing and reopening the SAME persistent buffer inside one + /// dispatcher burst — before the next render can ship a new + /// declaration — must not let a stale gesture address the successor. + /// + /// Same buffer, same size, same geometry: `buffer_id` and the grid + /// bounds are identical on both sides, so the only thing that can + /// tell the two presentations apart is the presentation identity + /// itself (Q#BP16). Validating the last SHIPPED declaration alone is + /// not enough — it still describes the dead window. + #[test] + #[allow( + clippy::too_many_lines, + reason = "one close/reopen transaction plus both event kinds and the accepted counterpart" + )] + fn a_stale_panel_epoch_cannot_address_a_reopened_same_buffer_panel() { + let mut editor = crate::editor::EditorState::new(); + let fid = FrontendId(713); + let (document, panel) = semantic_panel_view(&editor, fid, true); + let first_panel = panel.expect("panel window"); + let mut semantic_states = HashMap::new(); + semantic_states.insert( + fid, + crate::semantic_render::SemanticRenderState::for_peer(fid, PROTOCOL_VERSION), + ); + editor.accept_semantic_frame_geometry(fid, 1, CellSize::new(24, 80)); + let (geometry_epoch, panel_epoch) = shipped_declaration(&editor, fid, &mut semantic_states); + let buffer_id = editor.core.borrow().windows[&first_panel].buffer_id; + + // Close and reopen the SAME buffer, with no render in between. + { + let mut core = editor.core.borrow_mut(); + core.active_frontend = fid; + core.focus_window(fid, first_panel); + assert!( + core.close_active(), + "closing a side window is legal even as the only other window" + ); + } + editor.reconcile_panel_layout(fid); + { + let mut core = editor.core.borrow_mut(); + let mut request = crate::editor_core::DisplayRequest::new(buffer_id); + request.side = Some(crate::window::Side::Bottom); + request.height = Some(4); + core.display_buffer(fid, &request) + .expect("reopen the panel"); + } + editor.reconcile_panel_layout(fid); + let second_panel = editor.core.borrow().side_window_for(fid).expect("reopened"); + assert_ne!( + second_panel, first_panel, + "fixture precondition: the successor is a different window" + ); + assert_eq!( + editor.core.borrow().windows[&second_panel].buffer_id, + buffer_id, + "…showing the SAME persistent buffer, which is what makes it \ + indistinguishable by buffer id" + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + document, + "fixture precondition: focus is on the document after the reopen" + ); + + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + FrontendEvent::PanelPointer { + frontend_id: fid, + geometry_epoch, + panel_epoch, + buffer_id, + coord: pmacs_protocol::CellCoord::new(0, 0), + kind: pmacs_protocol::MouseKind::Down(pmacs_protocol::MouseButton::Left), + mods: pmacs_protocol::Modifiers::default(), + }, + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + document, + "R1-2: a gesture aimed at the CLOSED presentation must not \ + activate its successor" + ); + + // …and the resize event follows the same ladder. + let before = editor.core.borrow().windows[&second_panel] + .params + .fixed_rows; + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + FrontendEvent::PanelResizeRows { + frontend_id: fid, + geometry_epoch, + panel_epoch, + rows: 9, + }, + ); + assert_eq!( + editor.core.borrow().windows[&second_panel] + .params + .fixed_rows, + before, + "R1-2: nor resize it" + ); + + // The accepted counterpart: once a frame describing the successor + // ships, the same gesture shape is honored. + let (fresh_geometry, fresh_panel_epoch) = + shipped_declaration(&editor, fid, &mut semantic_states); + assert_ne!( + fresh_panel_epoch, panel_epoch, + "the successor took a fresh presentation identity" + ); + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + FrontendEvent::PanelPointer { + frontend_id: fid, + geometry_epoch: fresh_geometry, + panel_epoch: fresh_panel_epoch, + buffer_id, + coord: pmacs_protocol::CellCoord::new(0, 0), + kind: pmacs_protocol::MouseKind::Down(pmacs_protocol::MouseButton::Left), + mods: pmacs_protocol::Modifiers::default(), + }, + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + second_panel, + "…and the CURRENT presentation is addressable" + ); + } + + /// R2-5: Q#BP16 keeps scroll-without-focus for a NON-terminal panel. + /// Non-`Move` activation is the terminal-specific clause, because the + /// shared terminal adapter claims the controller for wheel steps too. + #[test] + #[allow( + clippy::too_many_lines, + reason = "the two panel kinds are the discriminating pair and must share one fixture shape" + )] + fn a_wheel_step_focuses_a_terminal_panel_but_not_a_document_panel() { + use crate::terminal::TerminalSpec; + + // --- non-terminal panel: the wheel must NOT focus ------------- + let mut editor = crate::editor::EditorState::new(); + let fid = FrontendId(714); + let (document, panel) = semantic_panel_view(&editor, fid, true); + let panel = panel.expect("panel window"); + let mut semantic_states = HashMap::new(); + semantic_states.insert( + fid, + crate::semantic_render::SemanticRenderState::for_peer(fid, PROTOCOL_VERSION), + ); + editor.accept_semantic_frame_geometry(fid, 1, CellSize::new(24, 80)); + let (geometry_epoch, panel_epoch) = shipped_declaration(&editor, fid, &mut semantic_states); + let buffer_id = editor.core.borrow().windows[&panel].buffer_id; + let wheel = |buffer_id, geometry_epoch, panel_epoch| FrontendEvent::PanelPointer { + frontend_id: fid, + geometry_epoch, + panel_epoch, + buffer_id, + coord: pmacs_protocol::CellCoord::new(0, 0), + kind: pmacs_protocol::MouseKind::ScrollUp, + mods: pmacs_protocol::Modifiers::default(), + }; + + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + wheel(buffer_id, geometry_epoch, panel_epoch), + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + document, + "R2-5: wheel motion over a document panel scrolls without focus" + ); + + // …while a press still focuses it, so the assertion above is not + // "panel pointers do nothing". + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + FrontendEvent::PanelPointer { + frontend_id: fid, + geometry_epoch, + panel_epoch, + buffer_id, + coord: pmacs_protocol::CellCoord::new(0, 0), + kind: pmacs_protocol::MouseKind::Down(pmacs_protocol::MouseButton::Left), + mods: pmacs_protocol::Modifiers::default(), + }, + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + panel, + "click-to-focus is unchanged" + ); + + // --- terminal panel: every non-Move gesture DOES focus -------- + let mut editor = crate::editor::EditorState::new(); + let fid = FrontendId(715); + let (document, panel) = semantic_panel_view(&editor, fid, true); + let panel = panel.expect("panel window"); + let mut spec = TerminalSpec::new("/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.rows = 4; + spec.cols = 20; + let terminal_buffer = editor + .terminal_manager + .borrow_mut() + .open( + spec, + &mut editor.core.borrow_mut(), + &mut editor.process_supervisor.borrow_mut(), + ) + .expect("open panel terminal"); + { + let mut core = editor.core.borrow_mut(); + let text_view = { + let registry = core.registry.clone(); + let registry = registry.borrow(); + crate::text_view::TextView::new( + registry.get(terminal_buffer).expect("terminal buffer"), + ) + }; + let window = core.windows.get_mut(&panel).expect("panel window"); + window.buffer_id = terminal_buffer; + window.text_view = text_view; + } + let mut semantic_states = HashMap::new(); + semantic_states.insert( + fid, + crate::semantic_render::SemanticRenderState::for_peer(fid, PROTOCOL_VERSION), + ); + editor.accept_semantic_frame_geometry(fid, 1, CellSize::new(24, 80)); + let (geometry_epoch, panel_epoch) = shipped_declaration(&editor, fid, &mut semantic_states); + assert_eq!( + editor.core.borrow().views[&fid].active, + document, + "fixture precondition: the terminal panel starts passive" + ); + + dispatch_panel_event( + &mut editor, + fid, + PROTOCOL_VERSION, + &mut semantic_states, + &mut HashMap::new(), + wheel(terminal_buffer, geometry_epoch, panel_epoch), + ); + assert_eq!( + editor.core.borrow().views[&fid].active, + panel, + "R2-5: a TERMINAL panel activates on every non-Move gesture, \ + because the shared adapter claims the controller for wheel \ + steps too" + ); + } + + /// R1-3: a semantic frontend's PANEL terminal must be resized to the + /// daemon-derived content grid before the child's output is drained. + /// + /// The two terminal-layout syncs are twins and must stay alternatives + /// (that is why `sync_terminal_layouts_for_tick` exists), but the grid + /// twin resolves through `controller_view_for_frontend` and therefore + /// covers a side window for free, while the semantic twin consulted + /// only the full-document declaration — which a panel terminal + /// deliberately does not have. + #[test] + fn a_semantic_panel_terminal_is_resized_before_the_child_drain() { + use crate::terminal::{TerminalSpec, view::TerminalViewKey}; + + let mut editor = crate::editor::EditorState::new(); + let fid = FrontendId(716); + let (_document, panel) = semantic_panel_view(&editor, fid, true); + let panel = panel.expect("panel window"); + let mut spec = TerminalSpec::new("/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.rows = 4; + spec.cols = 20; + let terminal_buffer = editor + .terminal_manager + .borrow_mut() + .open( + spec, + &mut editor.core.borrow_mut(), + &mut editor.process_supervisor.borrow_mut(), + ) + .expect("open panel terminal"); + { + let mut core = editor.core.borrow_mut(); + let text_view = { + let registry = core.registry.clone(); + let registry = registry.borrow(); + crate::text_view::TextView::new( + registry.get(terminal_buffer).expect("terminal buffer"), + ) + }; + let window = core.windows.get_mut(&panel).expect("panel window"); + window.buffer_id = terminal_buffer; + window.text_view = text_view; + } + // The panel view owns the child: only the durable controller's + // declaration reaches the PTY, and the per-tick liveness pass + // releases a controller whose window is not focused, so the panel + // has to actually own focus. Registering the view first mirrors + // what the first projection does. + let key = TerminalViewKey::new(fid, panel, terminal_buffer); + editor.core.borrow_mut().focus_window(fid, panel); + { + let mut manager = editor.terminal_manager.borrow_mut(); + manager.record_view_size(key, CellSize::new(4, 20)); + assert!( + manager.claim_controller(key), + "fixture precondition: the panel view controls the child" + ); + } + + let mut semantic_states = HashMap::new(); + semantic_states.insert( + fid, + crate::semantic_render::SemanticRenderState::for_peer(fid, PROTOCOL_VERSION), + ); + editor.accept_semantic_frame_geometry(fid, 1, CellSize::new(24, 120)); + assert_eq!( + editor.core.borrow().panel_grid_size(fid), + Some(CellSize::new(4, 120)), + "fixture precondition: the daemon derives a 4x120 band, so its \ + CONTENT grid is 3x120" + ); + assert_eq!( + editor + .terminal_manager + .borrow() + .screen_size(terminal_buffer), + Some(CellSize::new(4, 20)), + "fixture precondition: the child still has its opening size" + ); + + sync_terminal_layouts_for_tick( + &mut editor, + &[fid], + &HashMap::from([(fid, CellSize::new(24, 120))]), + &semantic_states, + ); + + assert_eq!( + editor + .terminal_manager + .borrow() + .screen_size(terminal_buffer), + Some(CellSize::new(3, 120)), + "R1-3: the panel terminal adopts the daemon-derived content \ + grid at the tick's layout step, BEFORE tick_processes drains \ + the child" + ); + } + /// Q#BP9's write-loop gate, independent of the producer's own flag. #[test] fn the_panel_frame_write_gate_rejects_v20_independently() { diff --git a/src/editor.rs b/src/editor.rs index 613bcf7..4be27be 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1870,6 +1870,91 @@ impl EditorState { } } + /// Sync a semantic frontend's **panel** terminal to the + /// daemon-derived content grid (Q#BP7 / Q#BP15a). + /// + /// The sibling of [`Self::sync_semantic_terminal_layout`], and the + /// case review round 1 (R1-3) found missing. The two arms are + /// disjoint by construction rather than by discipline: + /// `sync_semantic_terminal_layout` resolves its window through + /// `primary_document_window`, so it can never reach a side window, + /// and this one resolves through `side_window_for`, so it can never + /// reach the document. Nothing is ever resized twice per tick — the + /// failure mode the extraction of `sync_terminal_layouts_for_tick` + /// exists to prevent. + /// + /// A panel terminal has **no** `FrontendEvent::TerminalResize` + /// declaration to consult: the daemon derives its geometry, the + /// frontend never asserts it (Q#BP15a). So the size comes from + /// `panel_grid_size` minus the panel's one mode line, and it must be + /// applied at the tick's layout step — before `tick_processes` + /// drains the child — or the program formats its output against a + /// geometry the band is not showing. + /// + /// Recording the view size is unconditional for a resolvable panel + /// (that is what gives a passive view its own clipped projection); + /// only the durable controller resizes the shared PTY. + /// + /// Returns whether the shared screen geometry actually changed. + pub fn sync_semantic_panel_terminal_layout(&mut self, frontend_id: FrontendId) -> bool { + let Some((window_id, buffer_id, content)) = ({ + let core = self.core.borrow(); + core.panel_grid_size(frontend_id).and_then(|size| { + let window_id = core.side_window_for(frontend_id)?; + let buffer_id = core.windows.get(&window_id)?.buffer_id; + Some(( + window_id, + buffer_id, + CellSize::new(size.rows.saturating_sub(1), size.cols), + )) + }) + }) else { + return false; + }; + if content.rows == 0 || content.cols == 0 { + return false; + } + if !self.terminal_manager.borrow().is_terminal(buffer_id) { + return false; + } + let key = TerminalViewKey::new(frontend_id, window_id, buffer_id); + let content = terminal_projection_size(content); + if !self + .terminal_manager + .borrow_mut() + .record_view_size(key, content) + { + return false; + } + let controls = self + .terminal_manager + .borrow() + .controller(buffer_id) + .is_some_and(|controller| controller.matches(key)); + if !controls { + return false; + } + if self.terminal_manager.borrow().screen_size(buffer_id) == Some(content) { + return false; + } + let (Ok(rows), Ok(cols)) = (u16::try_from(content.rows), u16::try_from(content.cols)) + else { + return false; + }; + let result = self.terminal_manager.borrow_mut().resize( + buffer_id, + rows, + cols, + &mut self.process_supervisor.borrow_mut(), + ); + if let Err(error) = result { + self.core.borrow_mut().status = error.to_string(); + false + } else { + true + } + } + /// Apply a semantic frontend's terminal-cell pointer gesture. /// /// The gesture must name the authenticated frontend's active @@ -1980,7 +2065,7 @@ impl EditorState { let snapshot = self .terminal_manager .borrow_mut() - .snapshot_for_view(key, content.size)?; + .snapshot_for_view(key, terminal_projection_size(content.size))?; paint_terminal_snapshot(&mut grid, content, &snapshot, &theme); let registry = self.core.borrow().registry.clone(); let reg = registry.borrow(); @@ -2106,11 +2191,27 @@ impl EditorState { /// epochs) belong to the caller, because only the session holds the /// declaration the frontend was actually looking at. /// - /// **Click-to-focus only in Stage 2B-2.** A `Down`/`Up`/wheel/context - /// gesture activates the panel; replaying it into selection, listview - /// rows, or child SGR reporting is parent acceptance 48, which needs - /// the GPU band and lands in Stage 2B-3. Bare hover neither focuses - /// nor claims anything, exactly as on the document terminal path. + /// **Activation is not uniform, and Q#BP16 says so explicitly.** A + /// **press** focuses any panel — that is click-to-focus, and + /// `Down(Right)` is the context-menu gesture, so both buttons count. + /// Everything else depends on what the panel holds: + /// + /// * a **terminal** panel activates on *every* non-`Move` gesture, + /// because the shared terminal adapter claims the controller for + /// wheel, press, drag, and release alike — leaving a wheel step + /// unactivated would hand the child to a window that does not own + /// focus; + /// * a **document** panel keeps today's **scroll-without-focus** + /// behaviour, matching `dispatch_mouse`, where a wheel notch moves + /// a viewport without selecting the window (and preserves a kill + /// chain for the same reason). + /// + /// Review round 1 (R2-5) found the terminal clause applied to both. + /// Bare hover neither focuses nor claims, on either kind. + /// + /// **Replay is out of scope in Stage 2B-2.** Driving selection, + /// listview rows, or child SGR reporting is parent acceptance 48, + /// which needs the GPU band and lands in Stage 2B-3. /// /// Returns whether the gesture was accepted. pub fn dispatch_semantic_panel_pointer( @@ -2126,6 +2227,7 @@ impl EditorState { if coord.row >= size.rows || coord.col >= size.cols { return false; } + let is_terminal = self.terminal_manager.borrow().is_terminal(buffer_id); let mut core = self.core.borrow_mut(); let Some(side) = core.side_window_for(frontend_id) else { return false; @@ -2133,7 +2235,12 @@ impl EditorState { if core.windows.get(&side).map(|window| window.buffer_id) != Some(buffer_id) { return false; } - if !matches!(kind, pmacs_protocol::MouseKind::Move) { + let activates = if is_terminal { + !matches!(kind, pmacs_protocol::MouseKind::Move) + } else { + matches!(kind, pmacs_protocol::MouseKind::Down(_)) + }; + if activates { core.focus_window(frontend_id, side); core.active_frontend = frontend_id; } @@ -3350,6 +3457,36 @@ const SCROLL_LINES: i32 = 3; /// therefore only appears when a line-number mode reserves a gutter. const FOLD_GUTTER_GLYPH: char = '▸'; +/// The largest viewport the terminal subsystem will actually project, +/// for a window content rect that may legitimately be larger. +/// +/// A panel deliberately does **not** inherit the terminal's per-axis PTY +/// caps (Bet B5'): a 4K surface at a small font is legitimately wider +/// than 512 columns, and `PanelFrame` answers only to the shared area +/// bound. The terminal *screen* keeps its own policy, so without this +/// clamp `snapshot_for_view` refused the panel's content rect, the whole +/// projection collapsed to `None`, and the band went per-frame `Absent` +/// while `panel_hidden` still said "visible" — review round 1's R1-1 +/// shape, found again by its own sweep. +/// +/// Clamping rather than hiding is the right answer because the band is +/// legitimately that wide: the child occupies the columns a PTY can +/// have, and the remainder paints as band background exactly as a +/// snapshot narrower than its window already does. Rows are shed for the +/// area bound rather than columns, so a wide band keeps its full width. +fn terminal_projection_size(content: CellSize) -> CellSize { + let cols = content + .cols + .min(u32::from(crate::terminal::MAX_TERMINAL_COLS)); + let rows = content + .rows + .min(u32::from(crate::terminal::MAX_TERMINAL_ROWS)); + let rows_within_area = + u32::try_from(crate::terminal::MAX_TERMINAL_VISIBLE_CELLS / (cols as usize).max(1)) + .unwrap_or(u32::MAX); + CellSize::new(rows.min(rows_within_area), cols) +} + /// One painted side window, ready to become a /// [`pmacs_protocol::panel::PanelFrame`] (bottom-panel Stage 2B-2). /// diff --git a/src/editor_core.rs b/src/editor_core.rs index 7292284..661b767 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -3398,10 +3398,33 @@ impl EditorCore { /// the area budget. #[must_use] pub fn panel_grid_size(&self, fid: FrontendId) -> Option { - let view = self.views.get(&fid)?; - if view.panel_hidden { + if self.views.get(&fid)?.panel_hidden { return None; } + self.presentable_panel_grid(fid) + } + + /// The panel grid this frontend's layout and geometry **could** + /// present, ignoring the cached `panel_hidden` bit. + /// + /// This is the single derivation behind both [`Self::panel_grid_size`] + /// and [`Self::reconcile_panel_layout_core`]'s satisfiability test, + /// and it is one function on purpose (review round 1, R1-1). When the + /// wire-area clamp lived only in the renderer, the daemon shipped an + /// authoritative `Absent` while `panel_hidden` stayed `false` — so + /// keys still reached the invisible window and a panel terminal kept + /// its controller. Q#BP2b is explicit that hiding is a **durable + /// state transition**, never a per-frame effect, and two derivations + /// of "can this panel be shown" is exactly how it became one. + /// + /// The area bound is a transport-safety limit rather than a frontend + /// policy, so it is applied uniformly rather than only on the + /// semantic path. It cannot bind for a grid frontend at any physically + /// reachable width — two rows fit until roughly 131,000 columns — so + /// one shared rule costs nothing and removes the drift. + #[must_use] + fn presentable_panel_grid(&self, fid: FrontendId) -> Option { + let view = self.views.get(&fid)?; self.side_window_for(fid)?; let geometry = view.frame_geometry?; let cols = geometry.total.cols; @@ -3410,9 +3433,6 @@ impl EditorCore { } let area_rows = self.frontend_area_rows(fid)?; let rows = self.panel_allocation(fid, area_rows)?; - // The wire's area bound is a transport-safety limit, not a - // policy: clamp rows against it rather than shipping a frame the - // shared validator would reject whole. let budget_rows = u32::try_from(pmacs_protocol::panel::MAX_PANEL_VISIBLE_CELLS / (cols as usize).max(1)) .unwrap_or(u32::MAX); @@ -3440,13 +3460,14 @@ impl EditorCore { return result; }; let was_hidden = self.views.get(&fid).is_some_and(|view| view.panel_hidden); - // Unknown geometry (a semantic view before Stage 2's declaration) - // and a zero-column frame are both non-presentable, and follow the - // hidden arm rather than being sized against a placeholder. - let satisfiable = self - .frontend_area_rows(fid) - .and_then(|rows| self.panel_allocation(fid, rows)) - .is_some(); + // Unknown geometry (a semantic view before Stage 2's declaration), + // a zero-column frame, a layout that cannot spare the rows, and a + // grid the shared wire budget cannot carry are ALL non-presentable + // and all follow the hidden arm. One derivation, shared with the + // renderer (R1-1): a condition that only the renderer knew about + // produced a blank band with the durable state still saying + // "visible". + let satisfiable = self.presentable_panel_grid(fid).is_some(); let Some(view) = self.views.get_mut(&fid) else { return result; }; diff --git a/src/semantic_render.rs b/src/semantic_render.rs index c42835c..47b85c4 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -352,6 +352,15 @@ pub struct SemanticRenderState { /// Whether an invalid panel frame was already reported since the last /// valid one. Bounds the log exactly like `terminal_error_latched`. panel_error_latched: bool, + /// The band's last PUBLISHED statusline segments and the side window + /// they belong to (review round 1, R2-4). + /// + /// The band repaints its whole mode line every frame, so + /// "publish nothing" has to be expressed as "paint what was published + /// last" — there is no wire-level suppression to fall back on the way + /// `StatuslineSegments` has. Keyed by window id so a replaced panel + /// never inherits its predecessor's provider text. + last_panel_statusline: Option<(WindowId, StatuslineWindowSegments)>, } /// The presentation identity a shipped [`PanelFrame`] carries. @@ -523,6 +532,7 @@ impl SemanticRenderState { panel_epoch_used: 0, panel_presentation: None, panel_error_latched: false, + last_panel_statusline: None, } } @@ -542,14 +552,37 @@ impl SemanticRenderState { } /// Whether the last shipped declaration is a `Present` whose epochs - /// both match an inbound panel event (Q#BP16 steps 2–4). + /// both match an inbound panel event **and** which still describes + /// the side window that is live now (Q#BP16 steps 2–4). /// /// Rolled into one predicate so no caller can check the geometry /// epoch and forget the presentation epoch: they close different /// holes and neither subsumes the other. + /// + /// `live_presentation` is the frontend's **current** side window and + /// its buffer. Comparing it is what closes review round 1's R1-2: + /// closing and reopening the same *persistent* buffer inside one + /// dispatcher burst leaves the shipped declaration intact while the + /// window it describes is already dead, and same-buffer/same-size + /// makes the successor indistinguishable by every other field. A + /// presentation epoch only identifies a presentation if something + /// checks that the presentation it names is still the one on screen. + /// + /// `None` means "no side window right now", which never matches — an + /// event cannot address a panel that does not exist. #[must_use] - pub fn panel_declaration_matches(&self, geometry_epoch: u64, panel_epoch: u64) -> bool { - self.panel_declaration().is_some_and(|frame| { + pub fn panel_declaration_matches( + &self, + geometry_epoch: u64, + panel_epoch: u64, + live_presentation: Option<(WindowId, BufferId)>, + ) -> bool { + let Some((window_id, buffer_id)) = live_presentation else { + return false; + }; + self.panel_presentation.is_some_and(|presentation| { + presentation.window_id == window_id && presentation.buffer_id == buffer_id + }) && self.panel_declaration().is_some_and(|frame| { frame.geometry_epoch == geometry_epoch && frame.panel_epoch == panel_epoch }) } @@ -924,10 +957,7 @@ impl SemanticRenderState { // side half is taken from this same evaluation before it is // consumed. let side_window = state.core.borrow().side_window_for(self.frontend_id); - let panel_statusline = statusline_evaluation - .as_ref() - .and_then(|evaluation| self.panel_statusline(evaluation, side_window)) - .cloned(); + let panel_statusline = self.panel_statusline(statusline_evaluation.as_ref(), side_window); if let Some(evaluation) = statusline_evaluation { self.emit_statusline_segments(evaluation, statusline_document_window, &mut out); } @@ -1074,10 +1104,7 @@ impl SemanticRenderState { // and suppressing the panel here would leave the peer's retained // band on screen with no way to clear it. let side_window = state.core.borrow().side_window_for(self.frontend_id); - let panel_statusline = statusline_evaluation - .as_ref() - .and_then(|evaluation| self.panel_statusline(evaluation, side_window)) - .cloned(); + let panel_statusline = self.panel_statusline(statusline_evaluation.as_ref(), side_window); if let Some(evaluation) = statusline_evaluation { self.emit_statusline_segments(evaluation, statusline_document_window, &mut out); } @@ -1156,23 +1183,64 @@ impl SemanticRenderState { /// order and could paint the document's status text into the panel's /// mode line. /// - /// An `Invalidated` evaluation yields `None`, which is the panel's - /// authoritative-empty: a callback that mutated layout or focus - /// invalidates the whole evaluation, so the band paints its plain - /// mode line rather than stale provider text. - fn panel_statusline<'a>( - &self, - evaluation: &'a StatuslineEvaluation, + /// The three outcomes are **not** interchangeable, and review round 1 + /// (R2-4) found two of them collapsed: + /// + /// * `Ready` is authoritative — including an empty result. It + /// replaces the retained baseline. + /// * `Invalidated` discards all evaluated text: a callback mutated + /// registry, layout, or focus mid-evaluation, so the band clears to + /// its plain mode line and the baseline dies with it. + /// * `NoMessage` means **publish nothing**. Phase 1 was already stale + /// — most reachably a buffer-follow mismatch, where the primary + /// document window has moved off the buffer the frontend declared. + /// The band therefore keeps what it last published. Treating this + /// like `Invalidated` *removes* provider text on a transient + /// condition that said nothing about it. + /// + /// The baseline is keyed by window id so it can never leak across a + /// panel replacement: a new side window starts with no retained text. + fn panel_statusline( + &mut self, + evaluation: Option<&StatuslineEvaluation>, side_window: Option, - ) -> Option<&'a StatuslineWindowSegments> { - let side_window = side_window?; + ) -> Option { + let Some(side_window) = side_window else { + // No band to publish for; drop any baseline so a later panel + // cannot inherit a dead window's text. + self.last_panel_statusline = None; + return None; + }; + let retained = |state: &Self| { + state + .last_panel_statusline + .as_ref() + .filter(|(window_id, _)| *window_id == side_window) + .map(|(_, segments)| segments.clone()) + }; + let Some(evaluation) = evaluation else { + // No evaluation ran at all (an unsupported peer, or a frame + // before the document viewport exists). Nothing was + // published, so nothing is retracted. + return retained(self); + }; match &evaluation.outcome { - StatuslineEvaluationOutcome::Ready(windows) => windows.iter().find(|window| { - window.context.frontend_id == self.frontend_id - && window.context.window_id == side_window - }), - StatuslineEvaluationOutcome::Invalidated { .. } - | StatuslineEvaluationOutcome::NoMessage(_) => None, + StatuslineEvaluationOutcome::Ready(windows) => { + let found = windows + .iter() + .find(|window| { + window.context.frontend_id == self.frontend_id + && window.context.window_id == side_window + }) + .cloned(); + self.last_panel_statusline = found.clone().map(|segments| (side_window, segments)); + found + } + StatuslineEvaluationOutcome::Invalidated { .. } => { + self.last_panel_statusline = None; + None + } + StatuslineEvaluationOutcome::NoMessage(_) => retained(self), } } @@ -1225,6 +1293,22 @@ impl SemanticRenderState { // Q#BP15: allocation is checked and exhaustion fails closed // to `Absent`. Wrapping would let a new panel inherit a live // identity and accept gestures aimed at its predecessor. + // + // **The one knowingly per-frame `Absent` left in this file**, + // and it is recorded rather than fixed. Review round 1's R1-1 + // established that a band cleared on the wire must also move + // the durable `panel_hidden` state, or keys keep reaching an + // invisible window; this arm cannot, because the producer + // holds session state and `panel_hidden` is recomputed by + // core reconciliation from geometry alone. Making it durable + // needs a new "presentation permanently unavailable" reason + // in `FrontendView`, which is machinery for a state that + // takes 2^64 shipped presentation changes in ONE session to + // reach — unlike the wire-area exhaustion in + // `presentable_panel_grid`, which any frontend can trigger + // with one declaration. If the epoch ever becomes + // frontend-supplied, this stops being unreachable and needs + // the durable arm. self.publish_absent_panel(out); return; }; diff --git a/tests/bottom_panel_stage2b_daemon_acceptance.rs b/tests/bottom_panel_stage2b_daemon_acceptance.rs index adf3774..ab0d36f 100644 --- a/tests/bottom_panel_stage2b_daemon_acceptance.rs +++ b/tests/bottom_panel_stage2b_daemon_acceptance.rs @@ -983,3 +983,238 @@ fn the_projection_paints_the_side_windows_own_buffer() { frame.validate().expect("a produced frame is a valid frame"); let _ = HashMap::::new(); } + +// --------------------------------------------------------------------------- +// Review round 1 — R1-1 and R2-4 +// --------------------------------------------------------------------------- + +/// R1-1: exhausting the wire-area budget must be a **durable** hide, not +/// a per-frame one. +/// +/// Q#BP2b is explicit that hiding is a durable state transition: a +/// render-time dodge still routes keys to an invisible window and leaves +/// the terminal controller claimed. `panel_grid_size` gained a budget +/// clamp that `reconcile_panel_layout_core` did not share, so the band +/// went `Absent` on the wire while `panel_hidden` stayed false — the +/// exact per-frame-effect shape the Stage 1 record warns about. +#[test] +fn r1_1_wire_area_exhaustion_is_a_durable_hide_not_a_blank_frame() { + let mut session = Session::new(); + session.declare(1, ROWS, COLS); + open_panel(&session, "*panel*", 4); + let panel = session.side_window().expect("side window"); + session.state.core.borrow_mut().focus_window(FID, panel); + assert!( + session.present().focused, + "fixture precondition: the panel owns focus while it is presentable" + ); + + // Wide enough that not even the structural two-row floor fits inside + // the shared area bound. + let max_cells = u32::try_from(pmacs_protocol::panel::MAX_PANEL_VISIBLE_CELLS).expect("bound"); + session.declare(2, ROWS, max_cells); + + assert_eq!( + session.frame(), + Some(PanelFramePayload::Absent), + "the band is cleared authoritatively" + ); + let core = session.state.core.borrow(); + assert!( + core.panel_hidden_for(FID), + "R1-1: …and the DURABLE hidden state must move with it, or keys \ + keep reaching an invisible window" + ); + assert_ne!( + core.views[&FID].active, panel, + "R1-1: focus must leave a panel that can no longer be presented" + ); +} + +/// R1-1's controller half: a panel terminal that stops being presentable +/// must have its child released, because the resize path merely returns +/// on zero content without releasing anything. +#[test] +fn r1_1_wire_area_exhaustion_releases_a_panel_terminals_controller() { + let mut session = Session::new(); + session.declare(1, ROWS, COLS); + exec( + &session.state, + "TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", \ + args = { \"-c\", \"sleep 30\" }, display = \"panel\" }", + ); + let panel = session.side_window().expect("terminal panel"); + let buffer: pmacs::lua_bindings::BufferIdLua = session + .state + .lua_host + .lua() + .load("return TERM_BUF") + .eval() + .unwrap(); + session.state.core.borrow_mut().focus_window(FID, panel); + let _ = session.present(); + let key = pmacs::terminal::TerminalViewKey::new(FID, panel, buffer.0); + assert!( + session + .state + .terminal_manager + .borrow_mut() + .claim_controller(key), + "fixture precondition: the panel view controls the child" + ); + + let max_cells = u32::try_from(pmacs_protocol::panel::MAX_PANEL_VISIBLE_CELLS).expect("bound"); + session.declare(2, ROWS, max_cells); + assert_eq!(session.frame(), Some(PanelFramePayload::Absent)); + + assert!( + session + .state + .terminal_manager + .borrow() + .controller(buffer.0) + .is_none(), + "R1-1: the child's controller is released with the durable hide" + ); + exec(&session.state, "pmacs.terminal.terminate(TERM_BUF)"); +} + +/// R2-4: `NoMessage` means publish **nothing**, so the band keeps the +/// text it last published. Treating it like `Invalidated` *removes* +/// provider text on a transient buffer-follow mismatch. +#[test] +fn r2_4_nomessage_retains_the_bands_published_segments() { + let mut session = Session::new(); + session.declare(1, ROWS, COLS); + open_panel(&session, "*panel*", 4); + let panel = session.side_window().expect("side window"); + let document_buffer = session.state.core.borrow().windows[&session.document].buffer_id; + session.render.set_viewport( + document_buffer, + pmacs::protocol::ByteRange { start: 0, end: 0 }, + 0, + ); + exec( + &session.state, + "pmacs.statusline.register { + name = 'probe', side = 'left', + fn = function(ctx) return 'W' .. tostring(ctx.window) end, + }", + ); + + let marker = format!("W{}", panel.raw()); + let mode_line = |session: &mut Session| { + rows_of(&session.present()) + .last() + .expect("mode line") + .clone() + }; + assert!( + mode_line(&mut session).contains(&marker), + "fixture precondition: a Ready evaluation paints the band's own \ + provider text" + ); + + // A buffer-follow mismatch: the primary document window moves off the + // buffer the frontend declared, so phase 1 is already stale and the + // evaluator returns NoMessage. + exec( + &session.state, + "pmacs.window.switch_buffer(pmacs.buffer.create(\"*elsewhere*\"))", + ); + // Force a repaint: without a content change the payload would be + // duplicate-suppressed and this would assert nothing. + set_panel_text(&session, "changed"); + assert!( + mode_line(&mut session).contains(&marker), + "R2-4: NoMessage publishes nothing, so the band keeps its last \ + published segments" + ); + + // The discriminating half: `Invalidated` DOES clear them, because a + // callback that mutated the registry invalidates all evaluated text. + // The evaluation has to reach the callback phase first, so re-declare + // the viewport onto the buffer the document window now shows — + // otherwise phase 1 stays stale and this would still be NoMessage. + let elsewhere = session.state.core.borrow().windows[&session.document].buffer_id; + session.render.set_viewport( + elsewhere, + pmacs::protocol::ByteRange { start: 0, end: 0 }, + 0, + ); + exec( + &session.state, + "SELF = pmacs.statusline.register { + name = 'self-unregistering', side = 'right', + fn = function() pmacs.statusline.unregister(SELF); return 'STALE' end, + }", + ); + set_panel_text(&session, "changed again"); + let after = mode_line(&mut session); + assert!( + !after.contains(&marker) && !after.contains("STALE"), + "an Invalidated evaluation discards ALL callback text, got {after:?}" + ); +} + +// --------------------------------------------------------------------------- +// Review round 1 sweep — the same defect shape, found elsewhere +// --------------------------------------------------------------------------- + +/// Sweep result: a panel **wider than the terminal subsystem's per-axis +/// cap** hosting a terminal reproduced R1-1's shape all over again. +/// +/// Bet B5' makes a panel wider than 512 columns legal on the wire — a 4K +/// surface at a small font is ordinary, and the frontend declares that +/// width itself. But the terminal screen keeps its own PTY policy +/// (`MAX_TERMINAL_COLS`), so `snapshot_for_view` refused the panel's +/// content rect, the projection returned `None`, and the band went +/// **per-frame `Absent` while `panel_hidden` stayed false** — keys still +/// reaching an invisible window, controller still claimed. +/// +/// The band is legitimately that wide, so hiding it would be the wrong +/// answer: the terminal projects into the columns it can occupy and the +/// remainder is band background, exactly as a snapshot narrower than its +/// window already paints. +#[test] +fn sweep_a_panel_wider_than_the_terminal_cap_still_presents_its_terminal() { + let wide = u32::from(pmacs::terminal::MAX_TERMINAL_COLS) + 88; + let mut session = Session::new(); + session.declare(1, ROWS, wide); + exec( + &session.state, + "TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", \ + args = { \"-c\", \"sleep 30\" }, display = \"panel\" }", + ); + let panel = session.side_window().expect("terminal panel"); + session.state.core.borrow_mut().focus_window(FID, panel); + + let frame = match session.frame() { + Some(PanelFramePayload::Present(frame)) => frame, + other => panic!( + "a legally wide panel must still present its terminal; got {other:?} \ + — and the durable state says hidden={}", + session.state.core.borrow().panel_hidden_for(FID) + ), + }; + assert_eq!( + frame.size.cols, wide, + "the band keeps the width the frontend declared (Bet B5')" + ); + frame + .validate() + .expect("and it is a frame the shared validator accepts"); + + // The durable state and the wire agree — which is the property R1-1 + // was really about. + assert!( + !session.state.core.borrow().panel_hidden_for(FID), + "a presented band is not durably hidden" + ); + assert_eq!( + session.state.core.borrow().views[&FID].active, + panel, + "…and focus is still legitimately in it" + ); + exec(&session.state, "pmacs.terminal.terminate(TERM_BUF)"); +}