fix(vterm): address stage 3 review round 1
Five findings, all addressed. One was a real defect; one prediction did not reproduce and is documented as such rather than papered over. Hover no longer claims durable terminal control (finding 2, the real one). apply_terminal_gesture claimed the controller before dispatching, including for Move, which does nothing. A semantic frontend reports motion at pixel rate, so sweeping the mouse across a passive split's terminal took durable control, and the next layout sync resized the shared PTY to that background view's geometry — precisely the theft the controller rule exists to prevent. Bare motion no longer claims; every deliberate gesture still does. scripts/bite HEAD src/editor.rs on the new test is a clean behavioral bite. The terminal-mode presence-sweep skip is removed (finding 1), but the predicted failure did NOT reproduce. The review reasoned that skipping the sweep freezes last_broadcast at the abandoned document position. It does not: the buffer-follow clears the terminal declaration when it ships the snapshot, so terminal_active is false on the tick a window first shows a terminal, and the declaration cannot arrive until a later tick — the frontend learns the buffer id from that very snapshot. One truthful sweep always lands first. The real-daemon two-frontend test written to catch the freeze passes against the pre-fix tree; the bite is vacuous and the test is labelled a regression guard, not fix evidence. The skip goes anyway: it was load-bearing on tick ordering and bought nothing, and removing it makes "presence follows the frontend" structural. Terminal motion is deduplicated by cell (finding 3). Sub-cell motion resolved to the same coordinate and still crossed the wire, where every event is a daemon-side gesture. Press and release re-arm the memo so the first drag after a press still reports. Its unit test cannot bite — the seam did not exist pre-fix — and says so. Declarations record only once sent (finding 4). terminal_declaration_if_changed is now a pure query; note_terminal_declaration_sent records. A failed write is retried instead of suppressed as already-declared. The existing a35 test caught the contract change and now pins both halves. Unchanged frames skip revalidation (finding 5). The complete-payload comparison runs before validate; only validated frames are ever stored, so a frame equal to the baseline has already passed. The chrome tail is factored into terminal_chrome so both exits emit it identically. Gates: fmt; strict workspace clippy; 1,757 default + 1,933 CRDT library tests; Stage 1 9/10, Stage 2 4/4, Stage 3 5/7, statusline 7/8 (default/CRDT); M4 120; required GPU 128; workspace sweep 2,921 across 83 suites; diff check clean.
This commit is contained in:
parent
1d0b46f023
commit
50fd9a08e4
|
|
@ -84,11 +84,19 @@ If it does not, stop and repair the remote/fetch configuration.
|
|||
- `cargo test --workspace -- --skip basedpyright`: 2,919 passed across
|
||||
83 suites (19 ignored), one invocation;
|
||||
- `git diff --check`.
|
||||
- Open caveat: the required-GPU suite failed ONCE mid-session and did not
|
||||
reproduce across eight subsequent runs or the full sweep. The failing
|
||||
test's identity was not captured. Re-run `PMACS_REQUIRE_GPU=1 cargo test
|
||||
-p pmacs-gpu` a few times on review; if it recurs, capture the name.
|
||||
- Next: user review rounds on the PR.
|
||||
- Review round 1 addressed (framing §0.10): hover no longer claims durable
|
||||
terminal control (real defect, bite-verified); terminal motion dedupes by
|
||||
cell; declarations record only once sent; unchanged frames skip
|
||||
revalidation; the terminal-mode presence-sweep skip is removed. The
|
||||
review's predicted presence FREEZE did not reproduce — the buffer-follow
|
||||
clears the declaration before `render_frame`, so a truthful sweep always
|
||||
precedes terminal mode; that test is a labelled regression guard, not
|
||||
fix evidence.
|
||||
- Post-review gates: required GPU 128; workspace sweep 2,921 across 83
|
||||
suites; Stage 3 acceptance 5 default / 7 CRDT; everything else as above.
|
||||
- Closed caveat: the once-seen required-GPU failure did not reproduce in
|
||||
eight author runs plus four reviewer runs. Treated as environmental.
|
||||
- Next: further user review rounds on the PR.
|
||||
|
||||
Recovery worktree:
|
||||
|
||||
|
|
|
|||
|
|
@ -381,6 +381,54 @@ required GPU 127; one-invocation workspace sweep 2,919 passed across 83 suites
|
|||
required-GPU suite occurred once mid-session and did not reproduce across eight
|
||||
subsequent runs including the full sweep; its identity was not captured.
|
||||
|
||||
### 0.10 Stage 3 review round 1
|
||||
|
||||
PR #135's first review found no correctness blocker, confirmed the required-GPU
|
||||
suite clean across four runs (twelve total with the author's), and raised two
|
||||
design questions plus three minor notes. All five are addressed.
|
||||
|
||||
- **Presence while in terminal mode (finding 1) — fix kept, prediction not
|
||||
reproduced.** The review predicted that skipping the presence sweep freezes
|
||||
`last_broadcast` at the abandoned document position, leaving peers painting a
|
||||
stale caret. It does not: the buffer-follow clears the terminal declaration
|
||||
when it ships the snapshot, so `terminal_active` is false on the tick a window
|
||||
first shows a terminal, and the declaration cannot arrive until a later tick
|
||||
(the frontend learns the buffer id FROM that snapshot). One truthful sweep
|
||||
always lands first. A real-daemon two-frontend test written to catch the
|
||||
freeze passes against the pre-fix tree — the bite is VACUOUS, and it is
|
||||
labelled a regression guard rather than fix evidence. The skip is removed
|
||||
anyway: it was load-bearing on tick ordering and bought nothing, and its
|
||||
removal makes "presence follows the frontend" structural.
|
||||
- **Hover claimed durable control (finding 2) — real, fixed, bite-verified.**
|
||||
`apply_terminal_gesture` claimed the controller before dispatching, including
|
||||
for `Move`, which does nothing. A semantic frontend reports motion at pixel
|
||||
rate, so sweeping the mouse across a PASSIVE split's terminal took durable
|
||||
control and the next layout sync resized the shared PTY to that background
|
||||
view's geometry — exactly the theft the controller rule exists to prevent.
|
||||
Bare motion no longer claims; every deliberate gesture still does.
|
||||
`scripts/bite HEAD src/editor.rs` on
|
||||
`hover_does_not_steal_terminal_control_from_the_active_frontend` is a clean
|
||||
behavioral bite (assertion failure, not a compile error).
|
||||
- **Terminal motion is deduplicated by cell (finding 3).** Sub-cell motion
|
||||
resolved to the same coordinate and still crossed the wire, where each event
|
||||
is a daemon-side gesture. `State::terminal_motion_is_new` now gates it, and
|
||||
press/release re-arm the memo so the first drag after a press still reports.
|
||||
Its unit test cannot bite — the seam did not exist pre-fix — and says so.
|
||||
- **Declarations record only once sent (finding 4).**
|
||||
`terminal_declaration_if_changed` is now a pure query and
|
||||
`note_terminal_declaration_sent` records, so a failed write is retried instead
|
||||
of suppressed as already-declared. The existing `a35` test caught the contract
|
||||
change and now pins both halves.
|
||||
- **Unchanged frames are no longer re-validated (finding 5).** The complete
|
||||
payload comparison runs before `validate`; only validated frames are ever
|
||||
stored, so a frame equal to the baseline has already passed. The chrome tail
|
||||
is factored into `terminal_chrome` so both exits emit it identically.
|
||||
|
||||
Post-review gates: `cargo fmt --check`; strict workspace Clippy; 1,757 default +
|
||||
1,933 CRDT library tests; Stage 1 acceptance 9/10, Stage 2 4/4, Stage 3 5/7,
|
||||
statusline 7/8 (default/CRDT); M4 120; required GPU 128; workspace sweep 2,921
|
||||
across 83 suites (19 ignored); `git diff --check` clean.
|
||||
|
||||
## 1. Problem and ownership boundary
|
||||
|
||||
Pmacs can supervise a PTY and can parse enough ANSI to turn command output into
|
||||
|
|
|
|||
|
|
@ -683,9 +683,11 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 {
|
|||
0,
|
||||
);
|
||||
}
|
||||
if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() {
|
||||
if let Some((buffer_id, size)) = state.terminal_declaration_if_changed()
|
||||
&& client.send_terminal_resize(buffer_id, size).is_ok()
|
||||
{
|
||||
facts.declarations += 1;
|
||||
let _ = client.send_terminal_resize(buffer_id, size);
|
||||
state.note_terminal_declaration_sent(buffer_id, size);
|
||||
}
|
||||
}
|
||||
if state.terminal.is_some() {
|
||||
|
|
@ -706,11 +708,13 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 {
|
|||
if !sent_resize && facts.frames >= 2 {
|
||||
sent_resize = true;
|
||||
state.resize(700, 500);
|
||||
if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() {
|
||||
if let Some((buffer_id, size)) = state.terminal_declaration_if_changed()
|
||||
&& client.send_terminal_resize(buffer_id, size).is_ok()
|
||||
{
|
||||
facts.declarations += 1;
|
||||
facts.resized_cols = size.cols;
|
||||
facts.resized_rows = size.rows;
|
||||
let _ = client.send_terminal_resize(buffer_id, size);
|
||||
state.note_terminal_declaration_sent(buffer_id, size);
|
||||
}
|
||||
}
|
||||
if facts.resized_cols > 0 && facts.last_frame_cols == facts.resized_cols {
|
||||
|
|
@ -1217,6 +1221,12 @@ struct State {
|
|||
/// Whether an invalid terminal frame has already been reported.
|
||||
/// Bounds the log while a bad producer keeps sending.
|
||||
terminal_frame_error_latched: bool,
|
||||
/// Last terminal cell a motion or drag was reported at. Pixel-rate
|
||||
/// motion inside ONE cell is not new information for the daemon —
|
||||
/// the document drag path dedupes by hit byte for the same reason.
|
||||
/// Cleared on press, release, and every exit from terminal mode, so
|
||||
/// a gesture that returns to the same cell still reports.
|
||||
last_terminal_pointer_cell: Option<CellCoord>,
|
||||
/// One shaped buffer per planned text run. Rebuilt only when the
|
||||
/// plan changes, never per frame.
|
||||
terminal_text_buffers: Vec<Buffer>,
|
||||
|
|
@ -1496,8 +1506,9 @@ impl App {
|
|||
let Some((buffer_id, size)) = state.terminal_declaration_if_changed() else {
|
||||
return;
|
||||
};
|
||||
if let Err(e) = client.send_terminal_resize(buffer_id, size) {
|
||||
eprintln!("pmacs-gpu: send_terminal_resize failed: {e}");
|
||||
match client.send_terminal_resize(buffer_id, size) {
|
||||
Ok(()) => state.note_terminal_declaration_sent(buffer_id, size),
|
||||
Err(e) => eprintln!("pmacs-gpu: send_terminal_resize failed: {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1818,13 +1829,22 @@ impl ApplicationHandler<AppEvent> for App {
|
|||
if let Some((buffer_id, coord)) =
|
||||
self.terminal_pointer_hit(position.x, position.y)
|
||||
{
|
||||
let mods = translate_mods(self.modifiers);
|
||||
let kind = if dragging {
|
||||
ProtocolMouseKind::Drag(ProtocolMouseButton::Left)
|
||||
} else {
|
||||
ProtocolMouseKind::Move
|
||||
};
|
||||
self.send_terminal_pointer(buffer_id, coord, kind, mods);
|
||||
// Sub-cell motion resolves to the same cell and
|
||||
// carries nothing new. Report only on a cell
|
||||
// change, matching the document drag path's
|
||||
// hit-byte dedupe — otherwise pixel-rate motion
|
||||
// becomes pixel-rate wire traffic, and every one
|
||||
// of those is a daemon-side gesture.
|
||||
let state = self.state.as_mut().expect("checked above");
|
||||
if state.terminal_motion_is_new(coord) {
|
||||
let mods = translate_mods(self.modifiers);
|
||||
let kind = if dragging {
|
||||
ProtocolMouseKind::Drag(ProtocolMouseButton::Left)
|
||||
} else {
|
||||
ProtocolMouseKind::Move
|
||||
};
|
||||
self.send_terminal_pointer(buffer_id, coord, kind, mods);
|
||||
}
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
|
@ -1904,6 +1924,11 @@ impl ApplicationHandler<AppEvent> for App {
|
|||
ProtocolMouseKind::Up(ProtocolMouseButton::Left)
|
||||
}
|
||||
};
|
||||
// A press or release always reports, and it re-arms
|
||||
// the motion dedupe: the first drag after a press
|
||||
// must reach the daemon even at the cell the press
|
||||
// landed on.
|
||||
state.last_terminal_pointer_cell = None;
|
||||
if let Some((buffer_id, coord)) = self.terminal_pointer_hit(x, y) {
|
||||
self.send_terminal_pointer(buffer_id, coord, kind, mods);
|
||||
}
|
||||
|
|
@ -2813,6 +2838,7 @@ impl State {
|
|||
terminal: None,
|
||||
last_terminal_size_sent: None,
|
||||
terminal_frame_error_latched: false,
|
||||
last_terminal_pointer_cell: None,
|
||||
terminal_text_buffers: Vec::new(),
|
||||
terminal_text_renderer,
|
||||
};
|
||||
|
|
@ -3884,6 +3910,7 @@ impl State {
|
|||
self.terminal_text_buffers.clear();
|
||||
self.terminal_frame_error_latched = false;
|
||||
self.last_terminal_size_sent = None;
|
||||
self.last_terminal_pointer_cell = None;
|
||||
}
|
||||
|
||||
/// Drop shaping and geometry caches without leaving terminal mode.
|
||||
|
|
@ -4159,10 +4186,32 @@ impl State {
|
|||
if self.last_terminal_size_sent == Some((buffer_id, size)) {
|
||||
return None;
|
||||
}
|
||||
self.last_terminal_size_sent = Some((buffer_id, size));
|
||||
Some((buffer_id, size))
|
||||
}
|
||||
|
||||
/// Whether terminal motion at `coord` is new information.
|
||||
///
|
||||
/// Records the cell as reported either way, so a caller that skips
|
||||
/// the send still advances the memo. Press and release reset it
|
||||
/// (`last_terminal_pointer_cell = None`), which is what lets the
|
||||
/// first drag after a press reach the daemon even at the cell the
|
||||
/// press landed on.
|
||||
fn terminal_motion_is_new(&mut self, coord: CellCoord) -> bool {
|
||||
let changed = self.last_terminal_pointer_cell != Some(coord);
|
||||
self.last_terminal_pointer_cell = Some(coord);
|
||||
changed
|
||||
}
|
||||
|
||||
/// Record a declaration the caller actually put on the wire.
|
||||
///
|
||||
/// Separate from [`Self::terminal_declaration_if_changed`] so a
|
||||
/// FAILED send does not leave a size believed-declared: the daemon
|
||||
/// would still hold the old geometry while this frontend suppressed
|
||||
/// every retry as unchanged.
|
||||
fn note_terminal_declaration_sent(&mut self, buffer_id: BufferId, size: CellSize) {
|
||||
self.last_terminal_size_sent = Some((buffer_id, size));
|
||||
}
|
||||
|
||||
/// True while the completion popup is open **for the buffer this
|
||||
/// window currently shows** — the predicate the key gates (Esc,
|
||||
/// RET/TAB) and the render path share, so a stale mirror can
|
||||
|
|
@ -13117,9 +13166,20 @@ mod tests {
|
|||
.expect("a 400x300 window admits a cell grid");
|
||||
assert_eq!(first.0, buffer_id);
|
||||
assert!(first.1.rows >= 1 && first.1.cols >= 1);
|
||||
|
||||
// Review round 1, finding 4: the query does not record. Until
|
||||
// the caller confirms the send, the declaration stays PENDING,
|
||||
// so a failed write is retried rather than suppressed as
|
||||
// already-declared.
|
||||
assert_eq!(
|
||||
state.terminal_declaration_if_changed(),
|
||||
Some(first),
|
||||
"an unsent declaration must still be offered"
|
||||
);
|
||||
state.note_terminal_declaration_sent(first.0, first.1);
|
||||
assert!(
|
||||
state.terminal_declaration_if_changed().is_none(),
|
||||
"an unchanged size must be silent"
|
||||
"an unchanged size must be silent once it has been sent"
|
||||
);
|
||||
|
||||
// A real geometry change re-declares exactly once.
|
||||
|
|
@ -13128,6 +13188,7 @@ mod tests {
|
|||
.terminal_declaration_if_changed()
|
||||
.expect("a wider window is a new size");
|
||||
assert!(widened.1.cols > first.1.cols);
|
||||
state.note_terminal_declaration_sent(widened.0, widened.1);
|
||||
assert!(state.terminal_declaration_if_changed().is_none());
|
||||
|
||||
// A buffer switch forces a fresh declaration even at the same size.
|
||||
|
|
@ -13372,6 +13433,50 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// Review round 1, finding 3: terminal motion reports only on a cell
|
||||
/// change.
|
||||
///
|
||||
/// A unit test on the memo rather than on the wire: the send site
|
||||
/// lives in `window_event`, which needs a real winit event and a
|
||||
/// live attach client. This cannot bite against the pre-fix tree —
|
||||
/// the seam it calls did not exist there — so it is a contract pin,
|
||||
/// not fix evidence.
|
||||
#[test]
|
||||
fn terminal_motion_reports_once_per_cell_and_rearms_on_press() {
|
||||
let Some(mut state) = headless_or_skip(400, 300, "doc") else {
|
||||
return;
|
||||
};
|
||||
let buffer_id = BufferId::next();
|
||||
state.current_buffer_id = Some(buffer_id);
|
||||
state.apply_terminal_frame(plain_terminal_frame(buffer_id, "abcd", 8));
|
||||
|
||||
let cell = CellCoord::new(0, 2);
|
||||
assert!(state.terminal_motion_is_new(cell), "first sight of a cell");
|
||||
assert!(
|
||||
!state.terminal_motion_is_new(cell),
|
||||
"sub-cell motion inside one cell is not new information"
|
||||
);
|
||||
assert!(
|
||||
state.terminal_motion_is_new(CellCoord::new(0, 3)),
|
||||
"crossing into another cell reports"
|
||||
);
|
||||
|
||||
// A press/release re-arms the memo, so the first drag after a
|
||||
// press reaches the daemon even at the press cell.
|
||||
let press_cell = CellCoord::new(1, 1);
|
||||
assert!(state.terminal_motion_is_new(press_cell));
|
||||
state.last_terminal_pointer_cell = None;
|
||||
assert!(
|
||||
state.terminal_motion_is_new(press_cell),
|
||||
"a press re-arms the memo at its own cell"
|
||||
);
|
||||
|
||||
// Leaving terminal mode drops it too: the next terminal's cells
|
||||
// are a different grid entirely.
|
||||
state.exit_terminal_mode();
|
||||
assert!(state.last_terminal_pointer_cell.is_none());
|
||||
}
|
||||
|
||||
/// Acceptance 36: the terminal statusline metadata reaches the band
|
||||
/// as text, never as a host-title or control effect.
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -1087,10 +1087,9 @@ fn dispatcher_loop(
|
|||
};
|
||||
|
||||
// Vterm Stage 3 — a semantic frontend showing a terminal has
|
||||
// no document cursor: the identity buffer is empty, so both
|
||||
// presence and `CursorByte` would describe byte 0 of a
|
||||
// buffer with no text, painting a phantom peer caret over
|
||||
// the cell grid.
|
||||
// no document cursor: the identity buffer is empty, so a
|
||||
// `CursorByte` would describe byte 0 of a buffer with no
|
||||
// text and scroll the frontend's document view against it.
|
||||
let terminal_mode = semantic_states
|
||||
.get(fid)
|
||||
.is_some_and(crate::semantic_render::SemanticRenderState::in_terminal_mode);
|
||||
|
|
@ -1098,12 +1097,33 @@ fn dispatcher_loop(
|
|||
// T M10.6 per-frontend presence sweep. The snapshot is
|
||||
// computed from this frontend's view; the sweep then
|
||||
// produces broadcasts to OTHER multi-frontend recipients.
|
||||
let broadcasts = if terminal_mode {
|
||||
Vec::new()
|
||||
} else {
|
||||
let snapshot = build_presence_snapshot(editor, *fid);
|
||||
session_registry.sweep(&[(*fid, snapshot)])
|
||||
};
|
||||
//
|
||||
// Terminal mode does NOT skip this (PR #135 review finding 1).
|
||||
// `sweep` is diff-keyed on `last_broadcast`, so a skip can
|
||||
// only ever FREEZE a frontend's presence — it can never
|
||||
// retract it. Sweeping truthfully moves the presence into
|
||||
// the terminal identity buffer, which is what takes this
|
||||
// frontend's caret off every peer showing the document.
|
||||
//
|
||||
// Honest note on why the skip was not a live bug: the
|
||||
// buffer-follow above clears the terminal declaration when
|
||||
// it ships the snapshot, so `render_frame` reports
|
||||
// `terminal_active == false` on the tick a window first
|
||||
// shows a terminal, and the declaration cannot arrive until
|
||||
// a later tick (the frontend learns the buffer id FROM that
|
||||
// snapshot). Every entry into terminal mode is therefore
|
||||
// already preceded by one truthful sweep. The skip was
|
||||
// load-bearing on that ordering and bought nothing; removing
|
||||
// it makes "presence follows the frontend" structural
|
||||
// instead of a property of tick sequencing.
|
||||
//
|
||||
// The framing's "suppress presence for the terminal identity
|
||||
// buffer" is a RENDER rule — no peer overlay is painted
|
||||
// inside a terminal — which the GPU honors by not preparing
|
||||
// the decoration batch in terminal mode. It was never a
|
||||
// reason to stop telling peers where this frontend went.
|
||||
let snapshot = build_presence_snapshot(editor, *fid);
|
||||
let broadcasts = session_registry.sweep(&[(*fid, snapshot)]);
|
||||
|
||||
// T M11.6 — DispatchIdle signal. `crdt_replica` frontends
|
||||
// gate their optimistic-apply path on this; we ship it
|
||||
|
|
|
|||
|
|
@ -1951,6 +1951,18 @@ impl EditorState {
|
|||
(status.at_bottom, modes, screen_size)
|
||||
};
|
||||
|
||||
// Hover is not an act of taking over a terminal (PR #135 review
|
||||
// finding 2). Every other gesture is deliberate — a press, a
|
||||
// release, a drag, a wheel tick, a right-click — but bare motion
|
||||
// happens whenever a pointer crosses a window. A semantic
|
||||
// frontend reports motion at pixel rate, so claiming on `Move`
|
||||
// let merely sweeping the mouse across a PASSIVE split's
|
||||
// terminal take durable control, and the next layout sync then
|
||||
// resized the shared PTY to that background view's geometry.
|
||||
// That is precisely the theft the controller rule exists to
|
||||
// prevent.
|
||||
let claims_control = !matches!(kind, TerminalMouseKind::Move);
|
||||
|
||||
if !shift
|
||||
&& at_bottom
|
||||
&& modes.mouse_sgr
|
||||
|
|
@ -1958,12 +1970,16 @@ impl EditorState {
|
|||
&& coord.col < screen_size.cols
|
||||
&& let Some(bytes) = crate::terminal::input::encode_mouse(kind, coord, modifiers, modes)
|
||||
{
|
||||
self.claim_terminal_controller(key);
|
||||
if claims_control {
|
||||
self.claim_terminal_controller(key);
|
||||
}
|
||||
self.send_terminal_bytes(key.buffer_id, &bytes);
|
||||
return;
|
||||
}
|
||||
|
||||
self.claim_terminal_controller(key);
|
||||
if claims_control {
|
||||
self.claim_terminal_controller(key);
|
||||
}
|
||||
let mut manager = self.terminal_manager.borrow_mut();
|
||||
match kind {
|
||||
TerminalMouseKind::ScrollUp => {
|
||||
|
|
|
|||
|
|
@ -853,17 +853,26 @@ impl SemanticRenderState {
|
|||
|
||||
let mut out = Vec::new();
|
||||
let frame = snapshot.into_terminal_frame();
|
||||
// Complete-payload comparison FIRST, and not on
|
||||
// `screen_generation`: a scroll, a selection change, or a
|
||||
// process exit must reach the frontend even though the screen
|
||||
// itself is byte-identical.
|
||||
//
|
||||
// Comparing before validating is also what keeps the steady
|
||||
// state cheap. Only validated frames are ever stored, so a frame
|
||||
// equal to the baseline has already passed — re-running the
|
||||
// per-cell width and topology checks every tick would recompute
|
||||
// a verdict we hold.
|
||||
if self.last_terminal_frame.as_ref() == Some(&frame) {
|
||||
self.terminal_error_latched = false;
|
||||
out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation));
|
||||
return Some(out);
|
||||
}
|
||||
match frame.validate() {
|
||||
Ok(()) => {
|
||||
self.terminal_error_latched = false;
|
||||
// Complete-payload comparison, not `screen_generation`:
|
||||
// a scroll, a selection change, or a process exit must
|
||||
// reach the frontend even though the screen itself is
|
||||
// byte-identical.
|
||||
if self.last_terminal_frame.as_ref() != Some(&frame) {
|
||||
self.last_terminal_frame = Some(frame.clone());
|
||||
out.push(InstanceMessage::TerminalFrame(frame));
|
||||
}
|
||||
self.last_terminal_frame = Some(frame.clone());
|
||||
out.push(InstanceMessage::TerminalFrame(frame));
|
||||
}
|
||||
Err(error) => {
|
||||
// Never emit a malformed or truncated frame. The peer
|
||||
|
|
@ -880,15 +889,33 @@ impl SemanticRenderState {
|
|||
}
|
||||
}
|
||||
|
||||
out.extend(self.terminal_chrome(state, buffer_id, statusline_evaluation));
|
||||
Some(out)
|
||||
}
|
||||
|
||||
/// The buffer-independent chrome a terminal-mode frontend still
|
||||
/// needs, in the order the document path emits it.
|
||||
///
|
||||
/// Shared by both terminal-pass exits so an unchanged frame and a
|
||||
/// changed one ship exactly the same chrome — the suppression is
|
||||
/// about the FRAME, never about the status band going quiet.
|
||||
fn terminal_chrome(
|
||||
&mut self,
|
||||
state: &EditorState,
|
||||
buffer_id: BufferId,
|
||||
statusline_evaluation: Option<StatuslineEvaluation>,
|
||||
) -> Vec<InstanceMessage> {
|
||||
let mut out = Vec::new();
|
||||
out.extend(self.status_facts_msg(state, buffer_id));
|
||||
out.extend(self.menu_prompt_msg(state, buffer_id));
|
||||
out.extend(self.minibuffer_prompt_msg(state, buffer_id));
|
||||
out.extend(self.theme_facts_msg(state));
|
||||
out.extend(self.font_facts_msg(state));
|
||||
// Q#SL6/Q#SL8: face inventory must precede segment text.
|
||||
if let Some(evaluation) = statusline_evaluation {
|
||||
self.emit_statusline_segments(evaluation, &mut out);
|
||||
}
|
||||
Some(out)
|
||||
out
|
||||
}
|
||||
|
||||
/// Apply the lead evaluator's publication outcome to the v18 wire
|
||||
|
|
|
|||
|
|
@ -767,6 +767,278 @@ fn a37_real_daemon_real_pty_and_headless_gpu_render_one_terminal_session() {
|
|||
);
|
||||
}
|
||||
|
||||
/// Review round 1, finding 1: a frontend that enters terminal mode must
|
||||
/// still tell its peers it left the document.
|
||||
///
|
||||
/// **This is a regression guard, not a bite-verified fix.** The review
|
||||
/// predicted that skipping the presence sweep in terminal mode freezes
|
||||
/// `last_broadcast` at the abandoned document position. It does not, and
|
||||
/// this test passes against the pre-fix tree: the buffer-follow clears
|
||||
/// the terminal declaration when it ships the snapshot, so
|
||||
/// `terminal_active` is false on the tick a window first shows a
|
||||
/// terminal, and the declaration cannot arrive until a later tick — one
|
||||
/// truthful sweep always lands first. The skip was load-bearing on that
|
||||
/// ordering and bought nothing, so it is gone; this test pins the
|
||||
/// resulting invariant against a future reordering that would make the
|
||||
/// predicted freeze real.
|
||||
///
|
||||
/// Real daemon, real wire, two real frontends: presence delivery is a
|
||||
/// property of the dispatcher loop, not of any function it calls.
|
||||
#[cfg(feature = "crdt")]
|
||||
#[test]
|
||||
fn terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret() {
|
||||
use pmacs::protocol::{
|
||||
AttachRequest, FrontendCapabilities, Hello, Key, KeyEvent, PROTOCOL_VERSION, read_message,
|
||||
write_message,
|
||||
};
|
||||
use std::os::unix::net::UnixStream;
|
||||
|
||||
fn attach(daemon: &common::daemon::TestDaemon, semantic: bool) -> (Hello, UnixStream) {
|
||||
let mut stream = daemon.connect();
|
||||
stream
|
||||
.set_read_timeout(Some(Duration::from_secs(2)))
|
||||
.expect("read timeout");
|
||||
let hello: Hello = read_message(&mut stream).expect("read Hello");
|
||||
let req = AttachRequest {
|
||||
protocol_version: hello.protocol_version,
|
||||
frontend_capabilities: FrontendCapabilities {
|
||||
synchronized_output: false,
|
||||
unicode_smp: true,
|
||||
true_color: true,
|
||||
mouse: false,
|
||||
bracketed_paste: false,
|
||||
terminal_kind: Some("acceptance".into()),
|
||||
multi_frontend: true,
|
||||
crdt_replica: true,
|
||||
semantic_render: semantic,
|
||||
},
|
||||
initial_size: CellSize::new(24, 80),
|
||||
};
|
||||
write_message(&mut stream, &req).expect("write AttachRequest");
|
||||
(hello, stream)
|
||||
}
|
||||
|
||||
/// Pump one stream until `want` returns a value, or time out.
|
||||
fn pump<T>(
|
||||
stream: &mut UnixStream,
|
||||
what: &str,
|
||||
mut want: impl FnMut(&InstanceMessage) -> Option<T>,
|
||||
) -> T {
|
||||
let deadline = Instant::now() + Duration::from_secs(20);
|
||||
while Instant::now() < deadline {
|
||||
match read_message::<InstanceMessage>(stream) {
|
||||
Ok(msg) => {
|
||||
if let Some(found) = want(&msg) {
|
||||
return found;
|
||||
}
|
||||
}
|
||||
Err(_) => continue,
|
||||
}
|
||||
}
|
||||
panic!("timed out waiting for {what}");
|
||||
}
|
||||
|
||||
assert_eq!(PROTOCOL_VERSION, 19);
|
||||
let daemon = common::daemon::TestDaemon::spawn_with_env_and_init(
|
||||
&[
|
||||
("PMACS_INSTANCE_SEMANTIC_RENDER", "1"),
|
||||
("PMACS_INSTANCE_MULTI_FRONTEND", "1"),
|
||||
],
|
||||
PROBE_INIT_LUA,
|
||||
);
|
||||
|
||||
// B attaches FIRST. The presence sweep is diff-keyed, so A's very
|
||||
// first snapshot is its only guaranteed broadcast — if B were not
|
||||
// yet a registered recipient, that broadcast would reach nobody and
|
||||
// A's presence would sit unchanged (and unsent) forever after.
|
||||
let (_hello_b, mut b) = attach(&daemon, false);
|
||||
// A is the semantic frontend that will open a terminal; B is the
|
||||
// peer whose document view would keep painting A's stale caret.
|
||||
let (hello_a, mut a) = attach(&daemon, true);
|
||||
let a_id = hello_a.assigned_frontend_id;
|
||||
|
||||
// A declares a byte viewport so it is a live semantic session.
|
||||
let document = pump(&mut a, "A's first BufferSnapshot", |msg| match msg {
|
||||
InstanceMessage::BufferSnapshot { buffer_id, .. } => Some(*buffer_id),
|
||||
_ => None,
|
||||
});
|
||||
write_message(
|
||||
&mut a,
|
||||
&pmacs::protocol::FrontendEvent::Viewport {
|
||||
frontend_id: a_id,
|
||||
buffer_id: document,
|
||||
visible: pmacs::protocol::ByteRange { start: 0, end: 0 },
|
||||
generation: 0,
|
||||
},
|
||||
)
|
||||
.expect("A declares a viewport");
|
||||
|
||||
// B sees A in the document. Without this the later assertion could
|
||||
// pass vacuously against a peer that never had presence at all.
|
||||
let seen_in_document = pump(&mut b, "A's presence in the document", |msg| match msg {
|
||||
InstanceMessage::PresenceUpdate {
|
||||
frontend_id,
|
||||
buffer_id,
|
||||
..
|
||||
} if *frontend_id == a_id => Some(*buffer_id),
|
||||
_ => None,
|
||||
});
|
||||
assert_eq!(seen_in_document, document);
|
||||
|
||||
// A opens a terminal through the bound chord and declares its cells.
|
||||
write_message(
|
||||
&mut a,
|
||||
&pmacs::protocol::FrontendEvent::Key(KeyEvent {
|
||||
frontend_id: a_id,
|
||||
key: Key::Char('t'),
|
||||
mods: Modifiers::CTRL | Modifiers::ALT,
|
||||
timestamp_ns: 0,
|
||||
}),
|
||||
)
|
||||
.expect("A opens a terminal");
|
||||
let terminal = pump(&mut a, "A's terminal BufferSnapshot", |msg| match msg {
|
||||
InstanceMessage::BufferSnapshot { buffer_id, .. } if *buffer_id != document => {
|
||||
Some(*buffer_id)
|
||||
}
|
||||
_ => None,
|
||||
});
|
||||
write_message(
|
||||
&mut a,
|
||||
&pmacs::protocol::FrontendEvent::TerminalResize {
|
||||
frontend_id: a_id,
|
||||
buffer_id: terminal,
|
||||
size: CellSize::new(12, 40),
|
||||
},
|
||||
)
|
||||
.expect("A declares terminal cells");
|
||||
// A really is in terminal mode once a frame arrives.
|
||||
let framed = pump(&mut a, "A's first TerminalFrame", |msg| match msg {
|
||||
InstanceMessage::TerminalFrame(frame) => Some(frame.buffer_id),
|
||||
_ => None,
|
||||
});
|
||||
assert_eq!(framed, terminal);
|
||||
|
||||
// The finding: B must learn that A left the document.
|
||||
let seen_after = pump(
|
||||
&mut b,
|
||||
"A's presence leaving the document",
|
||||
|msg| match msg {
|
||||
InstanceMessage::PresenceUpdate {
|
||||
frontend_id,
|
||||
buffer_id,
|
||||
..
|
||||
} if *frontend_id == a_id && *buffer_id != document => Some(*buffer_id),
|
||||
_ => None,
|
||||
},
|
||||
);
|
||||
assert_eq!(
|
||||
seen_after, terminal,
|
||||
"A's presence must move into the terminal identity buffer, not freeze \
|
||||
at the document position it abandoned"
|
||||
);
|
||||
}
|
||||
|
||||
/// Review round 1, finding 2: hover must not claim durable control.
|
||||
///
|
||||
/// A semantic frontend reports motion at pixel rate, so if bare `Move`
|
||||
/// claimed the controller, sweeping the mouse across a PASSIVE split's
|
||||
/// terminal would take it — and the next layout sync would resize the
|
||||
/// shared PTY to that background view's geometry. Every deliberate
|
||||
/// gesture still claims; only motion does not.
|
||||
#[test]
|
||||
fn hover_does_not_steal_terminal_control_from_the_active_frontend() {
|
||||
let mut state = EditorState::new();
|
||||
let owner = FrontendId(71);
|
||||
let bystander = FrontendId(72);
|
||||
let terminal_buffer = open_terminal(&mut state, "sleep 30", 6, 20);
|
||||
tick_until(&mut state, Duration::from_secs(5), |state| {
|
||||
state
|
||||
.terminal_manager
|
||||
.borrow()
|
||||
.snapshot(terminal_buffer)
|
||||
.is_some()
|
||||
});
|
||||
let owner_window = attach_view(&state, owner, terminal_buffer);
|
||||
let bystander_window = attach_view(&state, bystander, terminal_buffer);
|
||||
|
||||
let owner_size = CellSize::new(6, 20);
|
||||
let bystander_size = CellSize::new(4, 12);
|
||||
let owner_key = TerminalViewKey::new(owner, owner_window, terminal_buffer);
|
||||
let bystander_key = TerminalViewKey::new(bystander, bystander_window, terminal_buffer);
|
||||
{
|
||||
let mut manager = state.terminal_manager.borrow_mut();
|
||||
assert!(manager.record_view_size(owner_key, owner_size));
|
||||
assert!(manager.record_view_size(bystander_key, bystander_size));
|
||||
}
|
||||
|
||||
// The owner takes control with a real press.
|
||||
assert!(state.dispatch_semantic_terminal_pointer(
|
||||
owner,
|
||||
terminal_buffer,
|
||||
CellCoord::new(0, 0),
|
||||
MouseKind::Down(MouseButton::Left),
|
||||
Modifiers::NONE,
|
||||
));
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().controller(terminal_buffer),
|
||||
Some(pmacs::terminal::TerminalController::from_view(owner_key)),
|
||||
"a press claims control"
|
||||
);
|
||||
|
||||
// The bystander merely hovers, repeatedly. Control must not move.
|
||||
for col in 0..4 {
|
||||
assert!(state.dispatch_semantic_terminal_pointer(
|
||||
bystander,
|
||||
terminal_buffer,
|
||||
CellCoord::new(0, col),
|
||||
MouseKind::Move,
|
||||
Modifiers::NONE,
|
||||
));
|
||||
}
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().controller(terminal_buffer),
|
||||
Some(pmacs::terminal::TerminalController::from_view(owner_key)),
|
||||
"hovering a passive view must not take durable control"
|
||||
);
|
||||
|
||||
// And the shared PTY keeps the controller's geometry: a stolen
|
||||
// controller would resize it to the bystander's smaller view.
|
||||
assert!(!state.sync_semantic_terminal_layout(bystander, terminal_buffer, bystander_size));
|
||||
assert_eq!(
|
||||
state
|
||||
.terminal_manager
|
||||
.borrow()
|
||||
.snapshot(terminal_buffer)
|
||||
.expect("snapshot")
|
||||
.size,
|
||||
owner_size,
|
||||
"a hovered-over passive view must not resize the shared screen"
|
||||
);
|
||||
|
||||
// A deliberate gesture from the bystander still claims, so the
|
||||
// hover exemption is narrow rather than a dead controller path.
|
||||
assert!(state.dispatch_semantic_terminal_pointer(
|
||||
bystander,
|
||||
terminal_buffer,
|
||||
CellCoord::new(0, 1),
|
||||
MouseKind::Down(MouseButton::Left),
|
||||
Modifiers::NONE,
|
||||
));
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().controller(terminal_buffer),
|
||||
Some(pmacs::terminal::TerminalController::from_view(
|
||||
bystander_key
|
||||
)),
|
||||
"a press still claims control"
|
||||
);
|
||||
|
||||
state
|
||||
.terminal_manager
|
||||
.borrow_mut()
|
||||
.terminate(terminal_buffer, &mut state.process_supervisor.borrow_mut())
|
||||
.expect("terminate child");
|
||||
}
|
||||
|
||||
/// Acceptance 30 (v18 half) and 28: a peer that negotiated v18 receives
|
||||
/// no terminal message at all and keeps the ordinary document path over
|
||||
/// the empty identity buffer.
|
||||
|
|
|
|||
Loading…
Reference in New Issue