diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 49f19e5..3c83119 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -260,9 +260,13 @@ pmacs.command.define { local saved = pmacs.editor.cursor_line() local rows = p.on_refresh() or {} render(p, rows) - -- The wholesale rewrite leaves the window cursor at a stale byte - -- offset; re-enter the buffer to reset, then re-seat. - pmacs.window.switch_buffer(p.buffer) + -- `set_generated_contents` has already refreshed this window's + -- TextView. Re-seat through the editor primitives instead of + -- switching to the buffer it already shows: that redundant switch + -- rebuilt the TextView and hid a missing edit notification. + pmacs.editor.clear_selection() + pmacs.editor.set_view_top(0) + pmacs.editor.move_to_line(0) seat_cursor(p, saved) end, } diff --git a/src/editor_core.rs b/src/editor_core.rs index ba4da86..2a5691e 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1847,9 +1847,10 @@ impl EditorCore { /// 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. + /// The selection anchor is a **third** coordinate. It is clamped to + /// the buffer extent, and the selection is cleared only when moving + /// an endpoint collapses it — see + /// [`Self::clamp_cursor_and_selection`]. 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(); @@ -1863,52 +1864,33 @@ impl EditorCore { for overlay in &mut win.overlays { let _ = overlay.on_edit(buffer, edit); } - if win.cursor > len { - win.cursor = len; - } + Self::clamp_cursor_and_selection(win, len); let max_top = win.text_view.line_count().saturating_sub(1); 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. + /// Clamp `win`'s cursor and selection anchor to `len`, clearing the + /// selection only when the clamp collapses it. /// - /// **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; - } + /// This mirrors terminal selection normalization: a surviving, + /// shortened region remains selected, while a region whose content + /// disappeared does not become an accidental active-but-empty + /// selection. Looking at whether either endpoint moved distinguishes + /// that case from a zero-width selection the user created. + fn clamp_cursor_and_selection(win: &mut Window, len: Position) { + let old_cursor = win.cursor; + win.cursor = win.cursor.min(len); + win.selection = win.selection.and_then(|mut selection| { + let old_anchor = selection.anchor; + selection.anchor = selection.anchor.min(len); + let collapsed_by_clamp = selection.anchor == win.cursor + && (selection.anchor != old_anchor || win.cursor != old_cursor); + (!collapsed_by_clamp).then_some(selection) + }); } /// Force every window currently showing `buffer_id` to rebuild @@ -1923,13 +1905,9 @@ 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, 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. + /// they don't dangle past the end after a shrinking rewrite. + /// Selection normalization uses the same clamp-or-clear rule as + /// [`Self::notify_buffer_edit`]. pub fn rebuild_views_for(&mut self, buffer_id: BufferId) { let reg = self.registry.borrow(); let Ok(buffer) = reg.get(buffer_id) else { @@ -1939,14 +1917,11 @@ impl EditorCore { for win in self.windows.values_mut() { if win.buffer_id == buffer_id { win.text_view = TextView::new(buffer); - if win.cursor > len { - win.cursor = len; - } + Self::clamp_cursor_and_selection(win, len); let max_top = win.text_view.line_count().saturating_sub(1); 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 261a073..bff9134 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -1812,45 +1812,33 @@ fn dired_revert_still_repaints_after_the_lock() { ); } -/// **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.* +/// Stage 1 criterion 5 [`main`, and also fix-shape] — the rope lock +/// refuses an ordinary edit first, and the named dired intercept +/// survives behind it. /// -/// **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. -/// -/// 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. +/// The rope half asserts the exact `BufferError::ReadOnly` rendering and +/// byte identity. The lifted half distinguishes the intercept by its +/// `intercept rejected the edit` message, so deleting `add_intercept` +/// still fails with the rope guard intact. #[test] -fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { +fn dired_rope_lock_and_named_intercept_refuse_in_order() { let td = fixture_dir(); let mut s = editor(); open_ok(&mut s, td.path(), "nil"); let listing = active_buffer_id(&s); + let name = active_name(&s); let before = active_text(&s); - // 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("), - "with the lock on the rope refuses first; got {:?}", - status(&s) + assert_eq!( + status(&s), + format!("insert failed: buffer `{name}` (id {listing:?}) is read-only"), + "with the lock on, the rope must provide the exact refusal" + ); + assert_eq!( + active_text(&s), + before, + "the rope refusal leaves every byte unchanged" ); set_read_only(&s, listing, false); @@ -1864,8 +1852,14 @@ fn dired_provisional_keeps_the_named_intercept_beside_the_rope_lock() { ); let st = status(&s); assert!( - st.contains("dired.lua") && st.contains("is read-only"), - "and refuse it by NAME, not with the rope's message; got {st:?}" + st.starts_with("insert failed: intercept rejected the edit:") + && st.contains("dired.lua") + && st.contains("is read-only"), + "the lifted path must carry the named intercept refusal; got {st:?}" + ); + assert!( + !st.contains(&format!("buffer `{name}` (id {listing:?}) is read-only")), + "the lifted path must not masquerade as the rope refusal: {st:?}" ); } diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index c5db6ed..8c15629 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -377,46 +377,32 @@ fn s1_4_the_owners_refresh_still_works_after_the_lock() { ); } -/// **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.* +/// Stage 1 criterion 5 [`main`, and also fix-shape] — the rope lock +/// refuses an ordinary edit first, and the named intercept survives +/// behind it. /// -/// **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`` -/// 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. +/// Both halves are required. The first asserts the exact +/// `BufferError::ReadOnly` rendering and byte identity. The second lifts +/// the lock Rust-side and distinguishes the intercept by its +/// `intercept rejected the edit` message. Deleting `add_intercept` +/// therefore passes the rope half and fails the lifted half. #[test] -fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { +fn s1_5_the_rope_lock_and_named_intercept_refuse_in_order() { let mut s = EditorState::new(); open_test_panel(&mut s); let panel = id_of(&s, "*test-panel*"); + let before = active_text(&s); - // 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("), - "with the lock on, the ROPE refuses first; got {:?}", - status(&s) + assert_eq!( + status(&s), + format!("insert failed: buffer `*test-panel*` (id {panel:?}) is read-only"), + "with the lock on, the rope must provide the exact refusal" + ); + assert_eq!( + active_text(&s), + before, + "the rope refusal leaves every byte unchanged" ); set_read_only(&s, panel, false); @@ -430,8 +416,16 @@ fn s1_5_provisional_an_ordinary_edit_is_refused_by_the_named_intercept() { ); let st = status(&s); assert!( - st.contains("listview.lua") && st.contains("*test-panel* is read-only"), - "and refuse it by NAME, not with the rope's message; got {st:?}" + st.starts_with("insert failed: intercept rejected the edit:") + && st.contains("listview.lua") + && st.contains("*test-panel* is read-only"), + "the lifted path must carry the named intercept refusal; got {st:?}" + ); + assert!( + !st.contains(&format!( + "buffer `*test-panel*` (id {panel:?}) is read-only" + )), + "the lifted path must not masquerade as the rope refusal: {st:?}" ); } @@ -498,17 +492,10 @@ fn s1_6_round_trip_input_survives_the_adoption() { /// 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. +/// `listview.refresh` re-seats through the already-notified `TextView`; +/// it deliberately does not rebuild the view by switching to the buffer +/// it already shows. Deleting the notification fan-out therefore leaves +/// the old line index live and this paint assertion bites. #[test] fn s1_7_a_shrinking_refresh_reaches_the_window() { let mut s = EditorState::new(); @@ -621,20 +608,14 @@ fn s1_10_the_disambiguation_limit_raises_rather_than_adopting() { assert_eq!(mine, "mine", "and touch nothing"); } -/// **Stage 1 criterion 11** --- a **disambiguated** panel still answers +/// Stage 1 criterion 11 [`main`] — 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*. /// -/// **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. +/// On `main` the test first fails at the disambiguation premise because +/// ownership is absent. The framing's narrower pre-image is also pinned: +/// keep disambiguation but restore a name-keyed `panel_for_buffer`, and +/// the test reaches the consumer checks and fails at `g`. /// /// Disambiguation alone leaves the old lookup reading /// `panels["*test-panel*<2>"]` for a record stored under @@ -676,17 +657,13 @@ fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { ); } -/// **Stage 1 criterion 12** --- the `q`-target capture is not inverted +/// Stage 1 criterion 12 [`main`] — the `q`-target capture is not inverted /// (Q#GB18), which needs its own criterion because it fails **open** /// 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. +/// On `main` the ownership premise fails first. With disambiguation kept +/// and only `panel_for_buffer` restored to name-keyed lookup, the test +/// reaches and fails the `q`-target assertion the criterion exists for. /// /// `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 fa3fd7d..aeba1ce 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -1109,17 +1109,9 @@ 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. +/// Generated-buffer immutability Stage 1 criterion 8c [`main`] — the +/// selection anchor is a third window coordinate normalized by Q#GB6's +/// clamp-or-clear rule. /// /// `Window::region` orders `(anchor, cursor)`, so a stale anchor above a /// clamped cursor is still the high end of the region, and @@ -1127,13 +1119,16 @@ fn acc16g_a_line_collapsing_generated_write_clamps_view_top() { /// 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. +/// Both outcomes matter: clamping a backward selection from 0..30 into +/// 0..2 preserves the shortened region, while clamping a forward +/// selection from 2..30 collapses both endpoints at 2 and clears it. +/// Clearing every stale anchor passes the crash check but fails the +/// first half; clamping without the collapsed check fails the second. +/// +/// *Bite:* delete `clamp_cursor_and_selection` from +/// `notify_buffer_edit`; the first copy reaches the stale-anchor panic. #[test] -fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { +fn acc16h_a_shrinking_generated_write_clamps_or_clears_the_selection() { let state = EditorState::new(); exec( &state, @@ -1164,22 +1159,55 @@ fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { 2, "precondition: the buffer shrank" ); + assert_eq!( + core.active_window() + .selection + .map(|selection| selection.anchor), + Some(2), + "the stale anchor is clamped into the new extent" + ); + assert_eq!( + core.active_region(), + Some((0, 2)), + "a non-collapsed selection survives as the shortened region" + ); + assert!( + core.clipboard_copy(), + "the production consumer copies the valid shortened region" + ); + drop(core); + + // The other result: cursor clamping moves 30 to the anchor at 2, so + // the selected content is gone and no empty active selection remains. + exec( + &state, + r"pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n')", + ); + { + let mut core = state.core.borrow_mut(); + core.begin_selection(2); + core.set_cursor_byte(30); + assert_eq!( + core.active_region(), + Some((2, 30)), + "precondition: a forward 28-byte region" + ); + } + exec(&state, r"pmacs.buffer.set_generated_contents(GEN, 'xy')"); + let mut core = state.core.borrow_mut(); assert_eq!( core.active_window().selection, None, - "an anchor that no longer fits is dropped, not clamped" + "a cursor clamp that collapses the region clears the selection" ); - 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" + "there is no collapsed region to copy" ); } -/// The same anchor gap in the **other** function, driven through its own -/// real Lua path. +/// Criterion 8c's second clamp site, 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 @@ -1189,15 +1217,13 @@ fn acc16h_a_shrinking_generated_write_drops_a_stale_selection_anchor() { /// 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. +/// This site also asserts both halves: a clamp can preserve the +/// shortened region, and an anchor that clamps exactly onto the cursor +/// clears it. *Bite:* delete `clamp_cursor_and_selection` from +/// `rebuild_views_for`; the first copy panics at `src/rope.rs:145` while +/// `acc16h` stays green. #[test] -fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { +fn acc16i_a_shrinking_view_rebuild_clamps_or_clears_the_selection() { let state = EditorState::new(); // 286 bytes, then 154: a real shrink through the help renderer. exec( @@ -1227,14 +1253,50 @@ fn acc16i_a_shrinking_view_rebuild_drops_a_stale_selection_anchor() { short_len < long_len, "precondition: the help buffer shrank ({long_len} -> {short_len})" ); + let short = u64::try_from(short_len).expect("non-negative"); + let mut core = state.core.borrow_mut(); + assert_eq!( + core.active_window() + .selection + .map(|selection| selection.anchor), + Some(short), + "the anchor is clamped to the shorter help buffer" + ); + assert_eq!( + core.active_region(), + Some((0, short)), + "the non-collapsed region survives the rebuild" + ); + assert!( + core.clipboard_copy(), + "copy consumes the clamped region without slicing past the rope" + ); + drop(core); + + // Grow the same help buffer, then choose an anchor that the next + // short render will clamp exactly onto the cursor. + exec(&state, "pmacs.help.show_command('cursor.down')"); + { + let mut core = state.core.borrow_mut(); + let long = u64::try_from(long_len).expect("non-negative"); + core.begin_selection(long); + core.set_cursor_byte(short); + assert_eq!( + core.active_region(), + Some((short, long)), + "precondition: a region whose anchor exceeds the next extent" + ); + } + exec(&state, "pmacs.help.show_command('editor.quit')"); + 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" + "an anchor clamp that collapses the region clears the selection" ); assert!( !core.clipboard_copy(), - "copy must report 'no region' rather than slice past the rope" + "the collapsed region is not retained as active-but-empty" ); }