fix(terminal): implement the Q#BP7 growth re-arm and pin it honestly

PR #155 review round 2.

Finding 1 (must fix): Q#BP7 item 1 — "growth reaching the live tail
re-arms follow (top -> None), only when no selection is active" — was
never implemented. `at_bottom` is the instantaneous geometric readout
`scroll_offset == 0`, which a still-anchored view satisfies whenever it
happens to be tall enough to reach the tail, so the round-1 assertion
could not see the gap: the next rows the child printed pushed the
anchored view back into history.

`rearm_follow_on_growth` now clears `top` when a viewport-size
declaration makes the view cover the tail and no selection is frozen,
and every size-declaring path (`snapshot_for_view`, `record_view_size`,
`view_status_for_size`) routes through one `declare_view_size` helper so
grid and semantic declarations cannot disagree. `scroll_view` and
`begin_selection` deliberately stay out: they write `top` themselves,
and `scroll_view` already owns the scroll-driven arm.

New acc32b is the pin the review asked for: scroll into history, grow
past the tail, then release a SECOND burst of child output through a
filesystem gate and assert the view moved with it.

Finding 2: the PTY fixtures emitted LF-only output, which staircases
rightward until every row clips to blanks past the viewport width — so
the round-1 anchor assertions compared "" with "" and could not fail.
Both fixtures now emit CRLF, and each anchor comparison is guarded by
`assert!(!top_before.is_empty())`.

Finding 3: acc33's contrast case asserted nothing, and the behavior it
claimed was false as coded. With the re-arm in place it is true and now
asserted: clearing the selection at the same geometry re-arms follow and
leaves the frozen anchor.

Finding 4: `start_run` gated the panel branch on `display == "panel" or
already_in_panel(..)`, so an explicit `display = "current"` lost to the
inference — and that value is the documented user-facing opt-out from
the Stage 3 default flip. Now gated on OMISSION. acc19b gains the
explicit-"current" case.

Finding 5: `window_drag` is a `HashMap<FrontendId, WindowDragState>`, so
a peer's mode-line press can no longer steal or clear another
frontend's in-flight gesture, and concurrent drags are legal. Cleared on
detach. acc30c gains the mode-line-press case.

Minor: `pmacs.window.buffer()` resolves both arms through the acting
frontend using the shared `lookup_window` / `selected_window` validators
rather than re-implementing them beside an ambient `active_buffer_id()`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Levi Neuwirth 2026-07-24 18:32:23 -04:00
parent 7e1bfb6dc5
commit 4d44be5d7b
6 changed files with 276 additions and 75 deletions

View File

@ -836,7 +836,13 @@ local function start_run(slot, cmdline, opts)
-- the selected DOCUMENT window while the panel still shows it — the
-- duplicate presentation this arc removes elsewhere. Detect that the
-- buffer already owns the panel slot and keep it there.
if display == "panel" or already_in_panel(slot.buf) then
--
-- Gated on OMISSION, never on an explicit value: `display = "current"`
-- is the documented user-facing opt-out from the Stage 3 default flip,
-- so it must reach the raw switch even when the previous run was
-- panel-placed. The duplicate presentation that produces is the
-- escape hatch's documented cost (R3-rp2).
if display == "panel" or (display == nil and already_in_panel(slot.buf)) then
pmacs.window.display(slot.buf, { side = "bottom", select = false })
else
pmacs.window.switch_buffer(slot.buf)

View File

@ -161,11 +161,16 @@ pub struct EditorState {
/// Last left-button down event, used to synthesize terminal double
/// clicks from crossterm's plain Down/Up mouse event stream.
mouse_click: Option<MouseClickState>,
/// In-progress split-boundary drag (bottom-panel arc, Q#BP5), armed
/// In-progress split-boundary drags (bottom-panel arc, Q#BP5), armed
/// by a left press on a mode-line row that is an exposed segment of a
/// horizontal boundary. Lives beside `mouse_click`; selection is
/// untouched for the whole gesture.
window_drag: Option<WindowDragState>,
/// horizontal boundary. Selection is untouched for the whole gesture.
///
/// Keyed by frontend, unlike the older global `mouse_click` slot: the
/// daemon routes every attached grid frontend through one
/// `dispatch_mouse`, so a single slot would let one frontend's press
/// steal or clear another's in-flight gesture, and concurrent drags
/// are perfectly legal.
window_drag: HashMap<FrontendId, WindowDragState>,
}
#[derive(Default)]
@ -220,7 +225,6 @@ struct MouseClickState {
/// mutation mid-drag cannot move a boundary that no longer exists.
#[derive(Copy, Clone)]
struct WindowDragState {
frontend_id: FrontendId,
owner: WindowId,
last_row: u32,
}
@ -619,7 +623,7 @@ impl EditorState {
snippets,
statusline_registry,
mouse_click: None,
window_drag: None,
window_drag: HashMap::new(),
}
}
@ -875,6 +879,9 @@ impl EditorState {
/// Drop one detached frontend's pending key and terminal escape state.
pub fn detach_frontend_input(&mut self, frontend_id: FrontendId) {
self.dispatchers.remove(&frontend_id);
// A detached frontend cannot finish a divider gesture, and its
// `owner` window is about to stop being live (Q#BP5).
self.window_drag.remove(&frontend_id);
self.terminal_manager
.borrow_mut()
.detach_frontend(frontend_id);
@ -1930,17 +1937,16 @@ impl EditorState {
// grid frontend through this same dispatcher, so an unscoped
// check would let one frontend's in-flight drag cancel and
// swallow another frontend's clicks.
if self
.window_drag
.is_some_and(|drag| drag.frontend_id == frontend_id)
{
if self.window_drag.contains_key(&frontend_id) {
match ev.kind {
MouseEventKind::Drag(MouseButton::Left) => {
self.drag_window_boundary(frontend_id, cell_row, term_size);
}
// Any other event — release, a different button, a
// wheel notch — ends the gesture.
_ => self.window_drag = None,
// wheel notch — ends THIS frontend's gesture only.
_ => {
self.window_drag.remove(&frontend_id);
}
}
return;
}
@ -2076,11 +2082,20 @@ impl EditorState {
.views
.get(&frontend_id)
.is_some_and(|view| view.layout.boundary_below(owner).is_some());
self.window_drag = is_divider.then_some(WindowDragState {
frontend_id,
owner,
last_row: cell_row,
});
// Only this frontend's slot is written, and only its own press
// can clear it — a peer pressing some other window's mode line
// must not disarm an in-flight gesture here.
if is_divider {
self.window_drag.insert(
frontend_id,
WindowDragState {
owner,
last_row: cell_row,
},
);
} else {
self.window_drag.remove(&frontend_id);
}
}
/// Continue an armed divider drag (Q#BP5).
@ -2095,16 +2110,16 @@ impl EditorState {
cell_row: u32,
term_size: CellSize,
) {
let Some(drag) = self.window_drag else {
let Some(drag) = self.window_drag.get(&frontend_id).copied() else {
return;
};
if drag.frontend_id != frontend_id {
return;
}
self.window_drag = Some(WindowDragState {
last_row: cell_row,
..drag
});
self.window_drag.insert(
frontend_id,
WindowDragState {
last_row: cell_row,
..drag
},
);
let delta = i64::from(cell_row) - i64::from(drag.last_row);
let Ok(delta) = i32::try_from(delta) else {
return;

View File

@ -12365,30 +12365,23 @@ fn install_window_module(lua: &Lua, core: &SharedCore) -> mlua::Result<Table> {
// panel" without first selecting the panel.
win.set(
"buffer",
lua.create_function(move |lua, target: Option<u64>| -> mlua::Result<BufferIdLua> {
let Some(raw) = target else {
return Ok(BufferIdLua(cc.borrow().active_buffer_id()));
};
let fid = window_panel::acting_frontend(lua, &cc);
let core = cc.borrow();
let view = core.views.get(&fid).ok_or_else(|| {
mlua::Error::runtime("pmacs.window.buffer: acting frontend has no layout")
})?;
let id = view
.layout
.iter_ids()
.into_iter()
.find(|id| id.raw() == raw)
.ok_or_else(|| {
mlua::Error::runtime(format!(
"pmacs.window.buffer: window {raw} is not live in this frontend's layout"
))
})?;
core.windows
.get(&id)
.map(|window| BufferIdLua(window.buffer_id))
.ok_or_else(|| mlua::Error::runtime("pmacs.window.buffer: window not live"))
})?,
lua.create_function(
move |lua, target: Option<u64>| -> mlua::Result<BufferIdLua> {
// Both arms resolve through the ACTING frontend, using
// the same validator the rest of the window surface
// does — no ambient `active_buffer_id()` asymmetry.
let fid = window_panel::acting_frontend(lua, &cc);
let id = match target {
Some(raw) => window_panel::lookup_window(&cc, fid, raw)?,
None => window_panel::selected_window(&cc, fid)?,
};
cc.borrow()
.windows
.get(&id)
.map(|window| BufferIdLua(window.buffer_id))
.ok_or_else(|| mlua::Error::runtime("pmacs.window.buffer: window not live"))
},
)?,
)?;
}

View File

@ -189,7 +189,7 @@ fn parse_request(
/// Not `active_window_id()`, which resolves through the ambient active
/// frontend: every other id in this module is `fid`-scoped, and the two
/// only coincide because dispatch happens to set `active_frontend` first.
fn selected_window(core: &SharedCore, fid: FrontendId) -> mlua::Result<WindowId> {
pub(crate) fn selected_window(core: &SharedCore, fid: FrontendId) -> mlua::Result<WindowId> {
core.borrow()
.views
.get(&fid)
@ -199,7 +199,11 @@ fn selected_window(core: &SharedCore, fid: FrontendId) -> mlua::Result<WindowId>
/// Resolve a raw Lua window id, refusing one that is not live in the
/// acting frontend's layout (Q#BP11).
fn lookup_window(core: &SharedCore, fid: FrontendId, raw: u64) -> mlua::Result<WindowId> {
pub(crate) fn lookup_window(
core: &SharedCore,
fid: FrontendId,
raw: u64,
) -> mlua::Result<WindowId> {
let core = core.borrow();
let view = core
.views

View File

@ -132,8 +132,7 @@ impl TerminalManager {
last_bell_count: bell_count,
..TerminalViewState::default()
});
normalize_state(state, projection);
state.viewport_size = Some(viewport_size);
declare_view_size(state, projection, viewport_size);
Some(project_snapshot(
key.buffer_id,
viewport_size,
@ -234,8 +233,7 @@ impl TerminalManager {
last_bell_count: bell_count,
..TerminalViewState::default()
});
normalize_state(state, projection);
state.viewport_size = Some(viewport_size);
declare_view_size(state, projection, viewport_size);
let rows = retained_rows(projection);
let geometry = view_geometry(&rows, state, viewport_size.rows);
Some(TerminalViewStatus {
@ -289,8 +287,7 @@ impl TerminalManager {
last_bell_count: bell_count,
..TerminalViewState::default()
});
normalize_state(state, projection);
state.viewport_size = Some(viewport_size);
declare_view_size(state, projection, viewport_size);
true
}
@ -595,6 +592,56 @@ fn clamp_or_clear(rows: &RetainedRows<'_>, anchor: LogicalCellAnchor) -> Option<
.then(|| row_lead(first))
}
/// The shared viewport-size declaration path (bottom-panel arc, Q#BP7).
///
/// Normalize, then re-arm live-tail following when the newly declared
/// viewport reaches the tail, then record the size. Every path that
/// *declares* a size routes through here so grid and semantic
/// declarations cannot disagree; `scroll_view` and `begin_selection`
/// deliberately do not, because they write `top` themselves.
fn declare_view_size(
state: &mut TerminalViewState,
projection: BorrowedScreenProjection<'_>,
viewport_size: CellSize,
) {
normalize_state(state, projection);
rearm_follow_on_growth(state, projection, viewport_size.rows);
state.viewport_size = Some(viewport_size);
}
/// Q#BP7 item 1: **growth reaching the live tail re-arms follow.**
///
/// A height change is a viewport change, never a scroll change — `top`
/// is preserved verbatim — but once a taller viewport covers the tail,
/// staying anchored would leave the view frozen just short of the live
/// output while `at_bottom` reported `true`: `at_bottom` is the
/// instantaneous geometric readout `scroll_offset == 0`, so it cannot
/// distinguish "following" from "anchored, and currently tall enough to
/// reach". The next rows the child prints would then push the anchored
/// view back into history with nothing to explain it.
///
/// **Only when no selection is active** (R1-8): a historical selection
/// froze this anchor on purpose, and growth must not yank the user's
/// region out from under them. `scroll_view` already handles the
/// scroll-driven arm (`next == tail_start`), so during ordinary
/// scrolling `scroll_offset == 0` implies follow is already armed —
/// which makes this rule fire on exactly the growth (and shrink-back)
/// case it names, and be idempotent everywhere else.
fn rearm_follow_on_growth(
state: &mut TerminalViewState,
projection: BorrowedScreenProjection<'_>,
viewport_rows: u32,
) {
if state.top.is_none() || state.selection.is_some() || viewport_rows == 0 {
return;
}
let rows = retained_rows(projection);
if view_geometry(&rows, state, viewport_rows).scroll_offset == 0 {
state.top = None;
state.selection_froze_top = false;
}
}
fn normalize_state(state: &mut TerminalViewState, projection: BorrowedScreenProjection<'_>) {
if state
.alternate_active

View File

@ -1336,6 +1336,26 @@ fn acc19b_recompile_reuses_the_panel_instead_of_duplicating_into_the_document()
"…and did not duplicate itself into the document window"
);
// An EXPLICIT `display = "current"` still wins over the inference:
// it is the documented user-facing opt-out from the Stage 3 default
// flip, so it must reach the raw switch even while the panel holds
// this buffer. The resulting duplicate presentation is the escape
// hatch's documented cost (R3-rp2).
s.core
.borrow_mut()
.focus_window(FrontendId::LOCAL, document);
exec(&s, "pmacs.compile.run(\"true\", { display = \"current\" })");
assert_eq!(
s.core.borrow().windows[&document].buffer_id,
compilation,
"explicit \"current\" reached the raw switch"
);
assert_eq!(
s.core.borrow().windows[&panel].buffer_id,
compilation,
"…and the panel still holds it too — the escape hatch's cost"
);
// A compilation that is NOT in a panel keeps the pre-arc raw switch.
let s = editor();
exec(&s, "pmacs.compile.run(\"true\")");
@ -1818,6 +1838,15 @@ fn acc30c_an_armed_drag_does_not_swallow_another_frontends_mouse_events() {
"the peer's click reached its own window instead of being swallowed"
);
// …a peer press on a MODE-LINE row must not steal or clear the slot
// either — that press reaches the arming path, which a single global
// slot would let it overwrite.
s.dispatch_mouse(
other,
mouse(MouseEventKind::Down(MouseButton::Left), divider_row, 4),
CellSize::new(ROWS, COLS),
);
// …and LOCAL's gesture must still be armed and still work.
s.dispatch_mouse(
FrontendId::LOCAL,
@ -1827,7 +1856,7 @@ fn acc30c_an_armed_drag_does_not_swallow_another_frontends_mouse_events() {
assert_eq!(
fixed_rows_of(&s, panel),
Some(armed_rows.expect("armed rows") - 2),
"the peer's event did not cancel LOCAL's in-flight drag"
"the peer's events did not cancel or steal LOCAL's in-flight drag"
);
}
@ -1981,8 +2010,14 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() {
let mut s = editor();
exec(
&s,
// `printf '...\\r\\n'`, not `echo`: a PTY in the default mode
// does not translate LF to CRLF for us, so LF-only output
// staircases rightward and every row past the viewport width
// clips to blanks — which would make the anchor assertions below
// compare "" with "" and pass for any regression.
"TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", \
args = { \"-c\", \"for i in $(seq 1 200); do echo line$i; done; sleep 30\" }, \
args = { \"-c\", \"i=1; while [ $i -le 200 ]; do printf 'line%d\\\\r\\\\n' $i; \
i=$((i+1)); done; sleep 30\" }, \
display = \"panel\" }",
);
let panel = side_window(&s).expect("terminal panel");
@ -2001,8 +2036,13 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() {
let before_size = CellSize::new(11, COLS);
s.terminal_manager
.borrow_mut()
.scroll_view(key_before, before_size, 5);
.scroll_view(key_before, before_size, 30);
let top_before = first_visible_row(&s, key_before, before_size);
assert!(
!top_before.is_empty(),
"the anchor row must carry real text, or the equality below \
cannot fail for the regression it names"
);
exec(
&s,
&format!(
@ -2029,9 +2069,65 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() {
.at_bottom,
"…and a SHRINK cannot re-arm follow"
);
exec(&s, "pmacs.terminal.terminate(TERM_BUF)");
}
// Growth that reaches the live tail re-arms follow (Q#BP7 case 1),
// and later output then scrolls in.
/// Q#BP7 item 1 proper: **growth reaching the live tail re-arms follow**,
/// so later output scrolls in.
///
/// `at_bottom` alone cannot pin this — it is the instantaneous geometric
/// readout `scroll_offset == 0`, which a still-anchored view satisfies
/// whenever it happens to be tall enough to reach the tail. The pin has
/// to feed the child MORE output after the growth and assert the view
/// moved with it.
#[test]
fn acc32b_growth_reaching_the_tail_re_arms_follow_and_later_output_scrolls_in() {
let dir = tempfile::tempdir().expect("tempdir");
let gate = dir.path().join("gate");
// Inserted bare into the shell word: `tempfile` paths carry no
// spaces or quotes, and wrapping it would terminate the Lua string.
let gate_path = gate.display().to_string();
let mut s = editor();
// Two bursts with a filesystem gate between them, so "more output
// after the growth" is deterministic rather than a race.
exec(
&s,
&format!(
"TERM_BUF = pmacs.terminal.open {{ command = \"/bin/sh\", \
args = {{ \"-c\", \"i=1; while [ $i -le 60 ]; do printf 'first%02d\\\\r\\\\n' $i; \
i=$((i+1)); done; \
while [ ! -f {gate_path} ]; do sleep 0.02; done; \
i=1; while [ $i -le 40 ]; do printf 'second%02d\\\\r\\\\n' $i; \
i=$((i+1)); done; sleep 30\" }}, \
display = \"panel\" }}"
),
);
let panel = side_window(&s).expect("terminal panel");
let buffer: pmacs::lua_bindings::BufferIdLua = eval(&s, "return TERM_BUF");
let key_id = pmacs::terminal::TerminalViewKey::new(FrontendId::LOCAL, panel, buffer.0);
render(&s);
wait_for_terminal_text(&mut s, buffer.0, "first60", Duration::from_secs(10));
// Scroll back into history at a short viewport.
let short = CellSize::new(6, COLS);
assert!(
s.terminal_manager
.borrow_mut()
.scroll_view(key_id, short, 20)
);
let anchored = first_visible_row(&s, key_id, short);
assert!(!anchored.is_empty(), "the anchor row carries real text");
assert!(
s.terminal_manager
.borrow_mut()
.view_status(key_id)
.expect("status")
.scroll_offset
> 0,
"the view really is anchored in history"
);
// Grow the panel until the viewport covers the tail.
exec(
&s,
&format!(
@ -2041,17 +2137,36 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() {
);
render(&s);
s.sync_terminal_layout(FrontendId::LOCAL, CellSize::new(ROWS, COLS));
let grown = CellSize::new(40, COLS);
s.terminal_manager
.borrow_mut()
.snapshot_for_view(key_before, CellSize::new(22, COLS))
.snapshot_for_view(key_id, grown)
.expect("snapshot at the grown size");
assert!(
s.terminal_manager
.borrow_mut()
.view_status(key_before)
.expect("view status")
.at_bottom,
"growth reaching the live tail re-arms follow when no selection is frozen"
// Release the second burst. A view that merely LOOKS at-bottom while
// still anchored gets pushed back into history here; a re-armed one
// follows.
std::fs::write(&gate, b"go").expect("open the gate");
wait_for_terminal_text(&mut s, buffer.0, "second40", Duration::from_secs(10));
s.terminal_manager
.borrow_mut()
.snapshot_for_view(key_id, grown)
.expect("snapshot after the second burst");
let status = s
.terminal_manager
.borrow_mut()
.view_status(key_id)
.expect("status");
assert_eq!(
status.scroll_offset, 0,
"the view followed the live tail through the new output"
);
assert!(status.at_bottom);
assert_ne!(
anchored,
first_visible_row(&s, key_id, grown),
"…and its first visible row moved off the old anchor"
);
exec(&s, "pmacs.terminal.terminate(TERM_BUF)");
}
@ -2197,7 +2312,7 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() {
exec(
&s,
"TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", \
args = { \"-c\", \"i=0; while [ $i -lt 60 ]; do printf 'row%02d\\\\n' $i; \
args = { \"-c\", \"i=0; while [ $i -lt 60 ]; do printf 'row%02d\\\\r\\\\n' $i; \
i=$((i+1)); done; sleep 30\" }, \
display = \"panel\" }",
);
@ -2221,6 +2336,11 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() {
assert!(manager.begin_selection(key_id, view_size, CellCoord::new(0, 0)));
}
let top_before = first_visible_row(&s, key_id, view_size);
assert!(
!top_before.is_empty(),
"the anchor row must carry real text, or the equality below \
cannot fail for the regression it names"
);
// Grow the panel enough that following the tail WOULD reach it.
exec(
@ -2255,13 +2375,29 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() {
"follow is NOT re-armed while a selection is frozen"
);
// The contrast that makes this bite: clear the selection and the
// same geometry DOES re-arm follow.
s.terminal_manager.borrow_mut().clear_selection(key_id);
// The contrast that makes this bite: the freeze is owed to the
// SELECTION, so clearing it lets the next size declaration re-arm
// follow at the very same geometry. Without this, "no re-arm while
// selected" would also hold if the re-arm simply did not exist.
assert!(s.terminal_manager.borrow_mut().clear_selection(key_id));
s.terminal_manager
.borrow_mut()
.snapshot_for_view(key_id, grown)
.expect("snapshot after clearing");
let cleared = s
.terminal_manager
.borrow_mut()
.view_status(key_id)
.expect("view status after clearing");
assert!(
cleared.at_bottom && cleared.scroll_offset == 0,
"clearing the selection re-arms follow at the same geometry"
);
assert_ne!(
top_before,
first_visible_row(&s, key_id, grown),
"…and the view left the frozen anchor"
);
exec(&s, "pmacs.terminal.terminate(TERM_BUF)");
}