diff --git a/src/editor_core.rs b/src/editor_core.rs index 28fa0f4..ba4da86 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1846,6 +1846,10 @@ impl EditorCore { /// bounded by [`TextView::line_count`]. A replace can grow in bytes /// while collapsing many lines into one, so "the buffer shrank" is /// not a usable trigger for the second. + /// + /// The selection anchor is a **third** coordinate and it is dropped, + /// not clamped, when it no longer fits — see + /// [`Self::drop_stale_selection`] for why. pub fn notify_buffer_edit(&mut self, buffer_id: BufferId, edit: &Edit) { self.search_invalidate_for_edit(buffer_id, edit); let reg = self.registry.borrow(); @@ -1866,10 +1870,47 @@ impl EditorCore { if win.view_top > max_top { win.view_top = max_top; } + Self::drop_stale_selection(win, len); } } } + /// Drop `win`'s selection when its anchor no longer fits a buffer of + /// `len` bytes. + /// + /// **The anchor is dropped rather than clamped, and that asymmetry + /// with `cursor` is the point.** A window must always have a cursor, + /// so clamping one into range is the only available answer. A window + /// need not have a selection, and a *clamped* anchor asserts a region + /// boundary the user never placed — after a wholesale generated + /// rewrite the surviving offsets address unrelated bytes, so the + /// clamped region would be a selection of text nobody selected. + /// + /// This is not a new rule: `window.quit`'s restore already answers + /// exactly this question the same way, with + /// `selection.filter(|sel| sel.anchor <= len)` and a comment giving + /// this reason (`:3259`). Two call sites, one rule. + /// + /// Without it, `cursor`'s clamp is not enough to make the region + /// safe. `Window::region` orders `(anchor, cursor)`, so a stale + /// anchor above a clamped cursor still yields `hi > len`, and + /// `region_bytes` slices with it: reproduced as + /// `assertion failed: end <= self.len()` at `src/rope.rs:145` from + /// `EditorCore::clipboard_copy`. + /// + /// **PROVISIONAL WORDING.** The rule this implements belongs to + /// Q#GB6, and PR #188's revision 5 — the approved text at the time of + /// writing — does not mention the anchor at all. A revision request + /// carrying this defect is with that lane. If the landed revision + /// specifies clamping or translation instead, this function and its + /// pin change to match it; it must not be left as a third, + /// independently-worded description of the same rule. + fn drop_stale_selection(win: &mut Window, len: Position) { + if win.selection.is_some_and(|sel| sel.anchor > len) { + win.selection = None; + } + } + /// Force every window currently showing `buffer_id` to rebuild /// its [`TextView`] from scratch. /// @@ -1882,7 +1923,13 @@ impl EditorCore { /// what an end-to-end rewrite cost anyway. /// /// Cursor and `view_top` are clamped to the new buffer extent so - /// they don't dangle past the end after a shrinking rewrite. + /// they don't dangle past the end after a shrinking rewrite, and a + /// selection whose anchor no longer fits is dropped + /// ([`Self::drop_stale_selection`]). This function had the same + /// anchor gap [`Self::notify_buffer_edit`] did, and for the same + /// reason: clamping the cursor is not enough to make + /// [`Self::region_bytes`] safe, because `Window::region` orders the + /// pair and a stale anchor can still be the high end. pub fn rebuild_views_for(&mut self, buffer_id: BufferId) { let reg = self.registry.borrow(); let Ok(buffer) = reg.get(buffer_id) else { @@ -1899,6 +1946,7 @@ impl EditorCore { if win.view_top > max_top { win.view_top = max_top; } + Self::drop_stale_selection(win, len); } } } diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index b2011bb..261a073 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -1812,33 +1812,40 @@ fn dired_revert_still_repaints_after_the_lock() { ); } -/// Criterion 5 [fix-shape] --- the named intercept survives adoption and -/// is still what refuses an edit whenever the rope does not. +/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an +/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert +/// on the message text, which distinguishes them.* Bite: *an adopter +/// that deletes the intercept and relies on the rope passes 1-4 and +/// fails this.* /// -/// **Restated against the framing**, which asked for an ordinary edit -/// "refused by the INTERCEPT, not by the rope" and asserted on the -/// message text. That state is unreachable once the arc's lock is -/// installed: `Buffer::apply_edit` (`src/buffer.rs:773`) and -/// `Buffer::begin_edit` (`:725`) call `ensure_writable()` as their FIRST -/// statement, while the intercept chain runs later inside -/// `apply_edit_inner` (`:1072`), so the rope always answers first. The -/// criterion is therefore driven with the lock lifted --- the state the -/// intercept genuinely still covers, including the window between -/// `pmacs.buffer.create` and the first paint. +/// **PROVISIONAL --- this test does not currently satisfy that +/// criterion, and does not claim to.** The criterion's state proved +/// unreachable during Stage 1; a revision request carrying the evidence +/// is with PR #188, which owns this acceptance contract. Until that +/// revision lands and is re-approved this test stands in for criterion 5 +/// at the only point where the tree can express the distinction, and its +/// wording follows #188 rather than replacing it. **The framing's own +/// bite is preserved unchanged**: deleting `add_intercept` fails this +/// test. /// -/// *Bite:* unchanged --- an adopter that drops `add_intercept` and relies -/// on the rope alone passes criteria 3 and 4 and fails here. +/// The evidence handed to #188: `Buffer::apply_edit` +/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call +/// `ensure_writable()` as their FIRST statement, while the intercept +/// chain runs later inside `apply_edit_inner` (`:1072`), so once the +/// arc's lock is installed the rope always answers first. The stand-in +/// lifts the lock Rust-side, which is the state the intercept still +/// covers, including the window between `pmacs.buffer.create` and the +/// first paint. #[test] -fn dired_keeps_the_named_intercept_beside_the_rope_lock() { +fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { let td = fixture_dir(); let mut s = editor(); open_ok(&mut s, td.path(), "nil"); let listing = active_buffer_id(&s); let before = active_text(&s); - // With the lock on, the ROPE answers first, and its message is the - // one with the buffer id in it. Pinned so the restatement above - // cannot rot silently. + // The measurement reported to #188, pinned so it cannot rot while + // the revision is outstanding: with the lock on, the ROPE answers. type_char(&mut s, 'z'); assert!( status(&s).contains("(id BufferId("), @@ -1862,23 +1869,25 @@ fn dired_keeps_the_named_intercept_beside_the_rope_lock() { ); } -/// Criterion 7 [mutation] --- a repaint reaches the **window**, not just -/// the rope, pinned by painting a shrinking listing. +/// **Stage 1 criterion 7 [mutation]**, the dired half: *a refresh +/// reaches the window, not just the rope --- pinned by painting a +/// shrinking render (many rows -> one) and asserting row 1 is empty, for +/// each adopter.* Bite: *delete the `notify_buffer_edit_to_windows` call +/// in the `set_generated_contents` binding +/// (`src/lua_bindings/mod.rs:3092`).* +/// +/// The criterion says **for each adopter**, so the listview half is +/// `listview_acceptance::s1_7_a_shrinking_refresh_reaches_the_window`. +/// This half is the one that carries the framing's bite; the note on the +/// listview half records why, and that observation is with PR #188 as a +/// revision request rather than being settled here. /// /// A rope write is only half of an edit: the window holds a `TextView` /// line index that only `on_edit` maintains, so a write that reaches the /// rope without the fan-out leaves the two disagreeing, and the next -/// paint indexes the new rope with the old offsets. `dired.revert` is -/// the right driver because it paints and does **not** follow the paint -/// with a `window.switch_buffer` --- which rebuilds the `TextView` from -/// scratch and would mask the mutation. (`listview.refresh` and -/// `listview.open` both do switch, so the listview half of this -/// criterion cannot bite; the primitive's own pin is -/// `terminal_copy_mode_acceptance::acc16d`.) -/// -/// *Bite:* delete the `notify_buffer_edit_to_windows` call in the -/// `set_generated_contents` binding (`src/lua_bindings/mod.rs:3092`) and -/// the painted frame keeps rows the listing no longer has. +/// paint indexes the new rope with the old offsets. `dired.revert` +/// paints and does **not** follow the paint with a +/// `window.switch_buffer`. #[test] fn dired_a_shrinking_repaint_reaches_the_window() { let td = tempfile::tempdir().expect("tempdir"); diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 5740081..c5db6ed 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -123,6 +123,51 @@ fn set_read_only(s: &EditorState, id: BufferId, value: bool) { .set_read_only(value); } +/// Render the active window's text view into a cell grid. Criterion 7 is +/// pinned by PAINTING, because that is where a rope/window disagreement +/// bites: the rope is right and the screen is not. +fn paint_active_window(s: &EditorState, rows: u32, cols: u32) -> Vec { + use pmacs::cell::{Cell, CellGrid, CellSize}; + use pmacs::view::{View, Viewport}; + use pmacs::window::Rect; + + let mut core = s.core.borrow_mut(); + let active = core.active_window_id(); + let registry = core.registry.clone(); + let win = core.windows.get_mut(&active).expect("active window"); + let rect = Rect::new(0, 0, rows, cols); + let mut backing = vec![Cell::default(); (rows * cols) as usize]; + let reg = registry.borrow(); + let buf = reg.get(win.buffer_id).expect("buffer in registry"); + let viewport = Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: rect.origin, + cell_size: CellSize::new(rows, cols), + gutter_w: 0, + folds: None, + }; + let mut grid = CellGrid { + cells: &mut backing, + stride: cols, + size: CellSize::new(rows, cols), + }; + win.text_view.render(buf, viewport, &mut grid); + backing +} + +fn grid_row(cells: &[pmacs::cell::Cell], row: u32, cols: u32) -> String { + use pmacs::cell::Glyph; + (0..cols) + .map(|c| match cells[(row * cols + c) as usize].glyph { + Glyph::Char(ch) => ch, + _ => ' ', + }) + .collect::() + .trim_end() + .to_owned() +} + /// Open a three-row test panel whose visits record into `_G.VISITED`. fn open_test_panel(s: &mut EditorState) { s.lua_host @@ -332,38 +377,41 @@ fn s1_4_the_owners_refresh_still_works_after_the_lock() { ); } -/// Criterion 5 [fix-shape] --- the named intercept survives adoption and -/// is still the thing that refuses an edit whenever the rope does not. +/// **Stage 1 criterion 5 [fix-shape]**, as the framing states it: *an +/// ordinary edit is refused by the INTERCEPT, not by the rope --- assert +/// on the message text, which distinguishes them.* Bite: *an adopter +/// that deletes the intercept and relies on the rope passes 1-4 and +/// fails this.* /// -/// **Restated against the framing, which specified this criterion in a -/// form the tree cannot reach.** §6 Stage 1 criterion 5 asks for an -/// ordinary edit refused "by the INTERCEPT, not by the rope", asserted on -/// the message text. Once the arc's own lock is installed that state is -/// unreachable: `Buffer::apply_edit` (`src/buffer.rs:773`) and -/// `Buffer::begin_edit` (`:725`) both call `ensure_writable()` as their -/// FIRST statement, while the intercept chain runs later, inside -/// `apply_edit_inner` (`:1072`). Measured here: a self-insert on an -/// adopted panel now reports +/// **PROVISIONAL --- this test does not currently satisfy that +/// criterion, and does not claim to.** Implementing Stage 1 found the +/// criterion's state unreachable, with the evidence below; a revision +/// request is with PR #188, which owns this acceptance contract. Until +/// that revision lands and is re-approved, this test stands in for +/// criterion 5 by driving the same distinction at the only point where +/// the tree can express it, and its wording follows #188 rather than +/// replacing it. **The framing's own bite is preserved unchanged**: +/// deleting `add_intercept` fails this test. +/// +/// The evidence handed to #188: `Buffer::apply_edit` +/// (`src/buffer.rs:773`) and `Buffer::begin_edit` (`:725`) call +/// `ensure_writable()` as their FIRST statement, while the intercept +/// chain runs later inside `apply_edit_inner` (`:1072`), so once the +/// arc's lock is installed the rope always answers first. Measured on +/// this branch, a self-insert on an adopted panel reports /// ``insert failed: buffer `*test-panel*` (id BufferId(n)) is read-only`` -/// --- the rope's message --- and can never report the intercept's. -/// -/// So the criterion is driven at the one point where the two are -/// distinguishable, which is also the state it is actually protecting: -/// the rope lock lifted. That covers the real window between -/// `pmacs.buffer.create` and the first render, and any future Rust-side -/// lift. -/// -/// *Bite:* unchanged from the framing's --- an adopter that deletes the -/// `add_intercept` call and relies on the rope alone passes criteria 1-4 -/// and fails here, because with the lock lifted the `z` lands. +/// and can never report the intercept's message. The stand-in lifts the +/// lock Rust-side first --- the state the intercept still covers, +/// including the window between `pmacs.buffer.create` and the first +/// render. #[test] -fn s1_5_an_ordinary_edit_is_refused_by_the_named_intercept_not_only_the_rope() { +fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { let mut s = EditorState::new(); open_test_panel(&mut s); let panel = id_of(&s, "*test-panel*"); - // With the lock ON, the rope answers first and the message is its - // own. Pinned so the restatement above cannot rot silently. + // The measurement reported to #188, pinned so it cannot rot while + // the revision is outstanding: with the lock on, the ROPE answers. press(&mut s, KeyCode::Char('z')); assert!( status(&s).contains("(id BufferId("), @@ -440,6 +488,55 @@ fn s1_6_round_trip_input_survives_the_adoption() { ); } +/// **Stage 1 criterion 7 [mutation]**, the listview half: *a refresh +/// reaches the window, not just the rope --- pinned by painting a +/// shrinking render (many rows -> one) and asserting row 1 is empty, for +/// each adopter.* Bite: *delete the `notify_buffer_edit_to_windows` call +/// in the `set_generated_contents` binding +/// (`src/lua_bindings/mod.rs:3092`).* +/// +/// The criterion says **for each adopter**, so both halves exist; the +/// dired half is `dired_acceptance::dired_a_shrinking_repaint_reaches_the_window`. +/// +/// **This half asserts the content produced but does NOT carry the +/// framing's mutation bite, and says so rather than being quietly +/// dropped.** `listview.refresh` and `listview.open` both follow +/// `render` with `pmacs.window.switch_buffer`, which rebuilds the +/// window's `TextView` from scratch (`src/editor_core.rs:4854-4868`) and +/// so repaints correctly even with the fan-out deleted --- on `main` +/// with its `bypass_intercept` writes just as much as here. Verified by +/// applying the mutation: this test stays green, while the dired half +/// fails with `assertion failed: end <= self.len()`. That observation is +/// filed with PR #188, which owns the criterion; it is recorded here, +/// not resolved here. +#[test] +fn s1_7_a_shrinking_refresh_reaches_the_window() { + let mut s = EditorState::new(); + open_test_panel(&mut s); + let painted = paint_active_window(&s, 6, 24); + assert_eq!( + grid_row(&painted, 1, 24), + "alpha", + "precondition: three data rows paint" + ); + assert_eq!(grid_row(&painted, 3, 24), "gamma"); + + // `g` re-renders from three rows to one. + press(&mut s, KeyCode::Char('g')); + + let painted = paint_active_window(&s, 6, 24); + assert_eq!( + grid_row(&painted, 1, 24), + "delta", + "the refreshed row must paint" + ); + assert_eq!( + grid_row(&painted, 2, 24), + "", + "and nothing of the rows it replaced" + ); +} + /// Criterion 9 [`main`] --- a foreign buffer that happens to share the /// panel's name is never adopted (Q#GB13). /// @@ -524,11 +621,22 @@ fn s1_10_the_disambiguation_limit_raises_rather_than_adopting() { assert_eq!(mine, "mine", "and touch nothing"); } -/// Criterion 11 [`main`] --- a **disambiguated** panel still answers -/// `RET`, `g` and `q` (Q#GB18). +/// **Stage 1 criterion 11** --- a **disambiguated** panel still answers +/// `RET`, `g` and `q` (Q#GB18). The framing labels it `[main]` and names +/// its bite as *Q#GB13 landed without Q#GB18*. /// -/// This is the criterion that fails against Q#GB13 landed without -/// Q#GB18: disambiguation alone leaves the old lookup reading +/// **Recorded here as a MUTATION bite, because that is what it is.** On +/// `main` this test fails at its disambiguation *premise* --- `main` +/// adopts the foreign buffer, so the panel is never called +/// `*test-panel*<2>` and the `RET`/`g`/`q` assertions are never reached. +/// A revert therefore proves nothing about what the criterion asserts. +/// The bite the framing actually names is a mutation of this branch: +/// keep the disambiguation, restore a name-keyed `panel_for_buffer`. +/// Verified --- under that mutation the test fails at the `g` assertion. +/// The `[main]` label belongs to #188 and is reported to it; what the +/// tree claims is corrected here either way. +/// +/// Disambiguation alone leaves the old lookup reading /// `panels["*test-panel*<2>"]` for a record stored under /// `"*test-panel*"`, so all three commands return early. Every one of /// them fails **silently**, so the assertion is on the content each @@ -568,9 +676,17 @@ fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { ); } -/// Criterion 12 [`main`] --- the `q`-target capture is not inverted +/// **Stage 1 criterion 12** --- the `q`-target capture is not inverted /// (Q#GB18), which needs its own criterion because it fails **open** -/// rather than closed. +/// rather than closed. The framing labels it `[main]`. +/// +/// **Recorded here as a MUTATION bite**, for the same reason as +/// criterion 11: on `main` this test fails at its disambiguation +/// premise and never reaches the `q`-target assertion, so a revert is +/// not evidence for what it asserts. Under the mutation the framing +/// actually names --- a name-keyed `panel_for_buffer` beside the +/// disambiguation --- it fails at the assertion it exists for, +/// `q must never return into another panel`. Verified. /// /// `listview.open`'s guard reads "capture the current buffer as the `q` /// target, but never another panel (chained panels would trap `q` in a diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index c572166..fa3fd7d 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -1108,3 +1108,133 @@ fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { ); assert_eq!(top, 0, "the collapsed buffer has exactly one line"); } + +/// Generated-buffer immutability Stage 1 — the **selection anchor** is a +/// third window coordinate a generated rewrite invalidates, and clamping +/// the cursor alone does not make the region safe. +/// +/// **PROVISIONAL, and named as such.** This pins a defect found in +/// review of PR #191; the rule belongs to Q#GB6, whose approved text +/// (PR #188 revision 5) does not mention the anchor. A revision request +/// carrying this defect is with that lane. This test stands in for the +/// anchor clause of a revised Q#GB6 and must be reconciled with it — +/// including its verdict of *drop* rather than *clamp* — when the +/// revision lands. It is not an independent contract. +/// +/// `Window::region` orders `(anchor, cursor)`, so a stale anchor above a +/// clamped cursor is still the high end of the region, and +/// `region_bytes` slices the rope with it. Reproduced on this branch +/// before the fix: `assertion failed: end <= self.len()` at +/// `src/rope.rs:145`, reached from `EditorCore::clipboard_copy`. +/// +/// *Bite:* delete the `drop_stale_selection` call from +/// `notify_buffer_edit` and this panics rather than failing an +/// assertion. Note the anchor must be the **high** end: with the anchor +/// low and the cursor high the cursor clamp already covers it, so a +/// forward selection passes with the bug live. +#[test] +fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { + let state = EditorState::new(); + exec( + &state, + r" + GEN = pmacs.buffer.create('*anchor-probe*') + pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n') + pmacs.window.switch_buffer(GEN) + ", + ); + { + // A BACKWARD selection: anchor at the far end, point at the + // start. The forward one is not a discriminator. + let mut core = state.core.borrow_mut(); + core.begin_selection(30); + core.set_cursor_byte(0); + assert_eq!( + core.active_region(), + Some((0, 30)), + "precondition: a live 30-byte region" + ); + } + + exec(&state, r"pmacs.buffer.set_generated_contents(GEN, 'xy')"); + + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_buffer_len(), + 2, + "precondition: the buffer shrank" + ); + assert_eq!( + core.active_window().selection, + None, + "an anchor that no longer fits is dropped, not clamped" + ); + assert_eq!(core.active_region(), None, "so there is no region left"); + // The production consumer, not just the field: this is the call that + // panicked before the fix. + assert!( + !core.clipboard_copy(), + "copy must report 'no region' rather than slice past the rope" + ); +} + +/// The same anchor gap in the **other** function, driven through its own +/// real Lua path. +/// +/// `EditorCore::rebuild_views_for` had the identical defect and is a +/// separate exit: the `*help*` renderer rewrites end to end and calls it +/// rather than `notify_buffer_edit` (`src/lua_bindings/mod.rs:1650`, via +/// `pmacs.help.show_command`). Fixing one function and not the other +/// would leave a live panic reachable from `M-x` help, so this pins the +/// second exit rather than trusting that one call site implies the +/// other. +/// +/// Same PROVISIONAL status as `acc16h`: the rule is Q#GB6's and its +/// approved text does not yet carry the anchor. +/// +/// *Bite:* delete the `drop_stale_selection` call from +/// `rebuild_views_for` and this panics at `src/rope.rs:145`. `acc16h` +/// stays green under that mutation, which is why this test exists +/// separately. +#[test] +fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { + let state = EditorState::new(); + // 286 bytes, then 154: a real shrink through the help renderer. + exec( + &state, + r" + HELP = pmacs.help.show_command('cursor.down') + pmacs.window.switch_buffer(HELP) + ", + ); + let long_len: i64 = eval(&state, "return HELP:len()"); + { + let mut core = state.core.borrow_mut(); + let anchor = u64::try_from(long_len).expect("non-negative"); + core.begin_selection(anchor); + core.set_cursor_byte(0); + assert_eq!( + core.active_region(), + Some((0, anchor)), + "precondition: a live region anchored at the end" + ); + } + + exec(&state, "pmacs.help.show_command('editor.quit')"); + + let short_len: i64 = eval(&state, "return HELP:len()"); + assert!( + short_len < long_len, + "precondition: the help buffer shrank ({long_len} -> {short_len})" + ); + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_window().selection, + None, + "rebuild_views_for must drop an anchor that no longer fits" + ); + assert!( + !core.clipboard_copy(), + "copy must report 'no region' rather than slice past the rope" + ); +}