neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(editor-core): normalize rewritten selections 

Adopt Q#GB6's clamp-or-clear rule in both window-coordinate
normalization paths. Preserve shortened selections, clear only those
collapsed by a moved endpoint, and pin both outcomes through the real
generated-write and view-rebuild callers.

Make listview refresh rely on the generated-write notification before
reseating, so Stage 1 criterion 7's fan-out mutation bites both
adopters. Align criteria 5, 11, and 12 with framing revision 7.
This commit is contained in:
Levi Neuwirth 2026-07-29 12:26:08 -04:00
parent dab0c184e1
commit 5d92348054
5 changed files with 197 additions and 185 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
builtin/runtime/listview.lua
View File

@ -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,
}

77
src/editor_core.rs
View File

@ -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);
}
}
}

58
tests/dired_acceptance.rs
View File

@ -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:?}"
);
}

107
tests/listview_acceptance.rs
View File

@ -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

130
tests/terminal_copy_mode_acceptance.rs
View File

@ -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"
);
}