fix(fold): address PR #142 review round 1

- **Finding 1 (bug):** a delete starting exactly at a fold's `end`
  removed the `\n` that `end` names — the last hidden line's terminator —
  but the strictly-after arm (`os >= e`) kept the fold, leaving a mid-line
  end. `translate`'s after-arm is now `os > e || (os == e && old_len == 0)`
  so a pure insert at `e` still stays outside while a delete at `e` falls
  to the drop arm, symmetric with the head side. New unit test
  `delete_starting_at_tail_boundary_drops_fold` (bite-verified).
- **Finding 2:** `pmacs.buffer.kill` didn't clean the fold registry.
  Added `FoldRegistry::forget_buffer(id)` (id-keyed; the view died with the
  buffer) and wired it into `after_buffer_removed`, mirroring the
  keymap/config cleanup; the registry is now stashed as Lua app-data.
  `forget(&mut Buffer)` is clarified as the revert/reload reset.
- **Finding 3:** `fold.close-all` now moves the invoking point to the head
  when it closes a fold around it (Q#FD3); the data-API `fold` exemption
  (programmatic, no invoking point) is named in the module doc.
- Nits: dropped the dead `!(both empty)` conjunct in `fold_state_msg`;
  replaced the trivial fresh-registry assert; added coverage for the
  stale-tree refuse via a fold command, the read-only-buffer rejection
  (Q#FD11), and unfold normalizing an arbitrary range.

Gates green: fmt, clippy --workspace --all-targets, --lib (1786),
--features crdt (1962), folding_acceptance (24), m4 (skip basedpyright),
required-GPU, git diff --check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5BkezMppbpCgGAYk2ftxV
This commit is contained in:
Levi Neuwirth 2026-07-23 13:42:20 -04:00
parent 3b411dbb2a
commit 9691dd4e9f
5 changed files with 148 additions and 21 deletions

View File

@ -231,8 +231,12 @@ impl FoldStore {
start: shift(s), start: shift(s),
end: shift(e), end: shift(e),
}) })
} else if os >= e { } else if os > e || (os == e && old_len == 0) {
// Strictly after the fold (an insert at exactly `e` too). // Strictly after the fold, OR a pure insert at exactly `e`
// (left outside). A *delete* starting at `e` removes the
// `\n` that `e` names — the terminator of the last hidden
// line — so it destroys the tail and falls through to the
// drop arm below, symmetric with the head side.
Some(ByteRange { start: s, end: e }) Some(ByteRange { start: s, end: e })
} else if os > s && oe < e { } else if os > s && oe < e {
// Strictly inside the interior — the fold still hides a // Strictly inside the interior — the fold still hides a
@ -348,14 +352,24 @@ impl FoldRegistry {
} }
/// Drop the buffer's store and detach its translator view — the /// Drop the buffer's store and detach its translator view — the
/// content-replacement (revert/reload) and buffer-close reset. Named /// content-replacement (revert/reload) reset, where the buffer survives
/// bytes no longer exist, so revalidation is not attempted (Q#FD8). /// but its bytes are replaced wholesale, so the view must come off too
/// (a later fold re-attaches a fresh one). Named bytes no longer exist,
/// so revalidation is not attempted (Q#FD8, framing acceptance 8).
pub fn forget(&self, buffer: &mut Buffer) { pub fn forget(&self, buffer: &mut Buffer) {
if let Some(entry) = self.stores.borrow_mut().remove(&buffer.id()) { if let Some(entry) = self.stores.borrow_mut().remove(&buffer.id()) {
buffer.detach_view(entry.view); buffer.detach_view(entry.view);
} }
} }
/// Drop the store for a buffer that has already been removed (the
/// `pmacs.buffer.kill` path). The buffer — and its attached translator
/// view — is gone, so only the map entry needs clearing; there is no
/// view to detach.
pub fn forget_buffer(&self, buf: BufferId) {
self.stores.borrow_mut().remove(&buf);
}
/// Unfold every fold in `buf` containing `p`. The pre-edit hook the /// Unfold every fold in `buf` containing `p`. The pre-edit hook the
/// six `EditorCore` edit primitives call; a no-op when the buffer has /// six `EditorCore` edit primitives call; a no-op when the buffer has
/// no store (hence no folds). Returns the count unfolded. /// no store (hence no folds). Returns the count unfolded.
@ -732,6 +746,24 @@ mod tests {
assert!(store.is_empty()); assert!(store.is_empty());
} }
#[test]
fn delete_starting_at_tail_boundary_drops_fold() {
// A delete beginning exactly at `end` removes the `\n` that `end`
// names (the last hidden line's terminator), destroying the tail —
// it must drop, not survive with a mid-line end. Mirror of
// `insert_at_tail_boundary_leaves_fold_untouched` for a delete.
let mut store = FoldStore::new();
store.insert(r(10, 30));
store.translate(&edit(Range::new(30, 31), 0)); // delete one byte at `end`
assert!(store.is_empty());
// Same class: a delete starting at the boundary and extending past.
let mut store = FoldStore::new();
store.insert(r(10, 30));
store.translate(&edit(Range::new(30, 45), 0));
assert!(store.is_empty());
}
#[test] #[test]
fn containment_is_start_exclusive_end_inclusive() { fn containment_is_start_exclusive_end_inclusive() {
let store = { let store = {

View File

@ -23,8 +23,11 @@
//! Fold *creation* refuses against an absent or stale parse tree (Q#FD10) //! Fold *creation* refuses against an absent or stale parse tree (Q#FD10)
//! and validates the buffer kind / UTF-8 boundaries / >= 1-hidden-line //! and validates the buffer kind / UTF-8 boundaries / >= 1-hidden-line
//! rule (Q#FD11); a rejection reports on the status line and returns //! rule (Q#FD11); a rejection reports on the status line and returns
//! `false`. Folding a range containing the invoking frontend's point moves //! `false`. The **interactive** commands (`toggle`/`close`/`cycle`/
//! that point to the head line (Q#FD3). //! `close_all`) move the invoking frontend's point to the head line when
//! they fold a range around it (Q#FD3); the **data-API** `fold` is
//! deliberately exempt — a programmatic caller names an explicit buffer
//! and range with no invoking point to relocate.
use std::sync::{Arc, Mutex}; use std::sync::{Arc, Mutex};
@ -45,6 +48,10 @@ use crate::syntax::{ParseTreeBundle, SharedSyntaxRegistry};
mirroring install_config; splitting fragments the wiring" mirroring install_config; splitting fragments the wiring"
)] )]
pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Result<()> { pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Result<()> {
// Also stash the registry as app-data so the buffer-remove cleanup
// (`after_buffer_removed`) can drop a killed buffer's store, mirroring
// the keymap/config registries.
lua.set_app_data(fold_registry.clone());
let fold_mod = lua.create_table()?; let fold_mod = lua.create_table()?;
// ---- data API --------------------------------------------------------- // ---- data API ---------------------------------------------------------
@ -99,8 +106,7 @@ pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Resu
return Ok(true); return Ok(true);
} }
if let Ok(Some(bytes)) = document_bytes(lua, id) if let Ok(Some(bytes)) = document_bytes(lua, id)
&& let Some(normalized) = && let Some(normalized) = fold::normalize_arbitrary_range(&bytes, requested)
fold::normalize_arbitrary_range(&bytes, requested)
{ {
return Ok(lock(&store).remove(normalized)); return Ok(lock(&store).remove(normalized));
} }
@ -242,14 +248,17 @@ pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Resu
}; };
let targets = fold::top_level_fold_targets(&bundle); let targets = fold::top_level_fold_targets(&bundle);
let store = store_for(lua, &reg, id)?; let store = store_for(lua, &reg, id)?;
let mut s = lock(&store); let inserted: Vec<ByteRange> = {
let mut n = 0i64; let mut s = lock(&store);
for t in targets { targets.into_iter().filter(|t| s.insert(*t)).collect()
if s.insert(t) { };
n += 1; // close-all is interactive: if a newly-closed top-level
} // fold contains the invoking point, move it to the head
// (Q#FD3). At most one top-level fold can contain it.
for r in &inserted {
maybe_move_point(lua, id, *r);
} }
Ok(n) Ok(i64::try_from(inserted.len()).unwrap_or(i64::MAX))
})?, })?,
)?; )?;
} }

View File

@ -1466,6 +1466,9 @@ fn after_buffer_removed(lua: &Lua, id: BufferId) {
if let Some(config) = lua.app_data_ref::<config::SharedConfigRegistry>() { if let Some(config) = lua.app_data_ref::<config::SharedConfigRegistry>() {
config.borrow_mut().remove_buffer(id); config.borrow_mut().remove_buffer(id);
} }
if let Some(folds) = lua.app_data_ref::<crate::fold::SharedFoldRegistry>() {
folds.forget_buffer(id);
}
let callbacks = match lua.app_data_ref::<BufferRemoveCallbacks>() { let callbacks = match lua.app_data_ref::<BufferRemoveCallbacks>() {
Some(callbacks) => callbacks.take(id), Some(callbacks) => callbacks.take(id),
None => Vec::new(), None => Vec::new(),

View File

@ -1434,9 +1434,9 @@ impl SemanticRenderState {
let should_emit = match self.last_folds.get(&buffer_id) { let should_emit = match self.last_folds.get(&buffer_id) {
// First sight: speak only if there is a fold to show. // First sight: speak only if there is a fold to show.
None => !folds.is_empty(), None => !folds.is_empty(),
// A real change; `empty → empty` conveys nothing and is // Any change: `empty → empty` is byte-identical and suppressed,
// suppressed, `non-empty → empty` is a change worth sending. // `non-empty → empty` differs and emits one clearing frame.
Some(prev) => *prev != folds && !(folds.is_empty() && prev.is_empty()), Some(prev) => *prev != folds,
}; };
if !should_emit { if !should_emit {
return None; return None;

View File

@ -13,8 +13,7 @@ use std::sync::Arc;
use pmacs::buffer::{Buffer, BufferId, EditOp}; use pmacs::buffer::{Buffer, BufferId, EditOp};
use pmacs::editor::EditorState; use pmacs::editor::EditorState;
use pmacs::fold::{ use pmacs::fold::{
self, CycleOutcome, FoldStore, close_at, cycle_at, fold_target_at, open_at, CycleOutcome, FoldStore, close_at, cycle_at, fold_target_at, open_at, top_level_fold_targets,
top_level_fold_targets,
}; };
use pmacs::protocol::ByteRange; use pmacs::protocol::ByteRange;
use pmacs::syntax::{ParseTreeBundle, ParseView, SyntaxRegistry, run_parse}; use pmacs::syntax::{ParseTreeBundle, ParseView, SyntaxRegistry, run_parse};
@ -487,5 +486,89 @@ fn forget_drops_store_and_detaches_view() {
s.fold_registry.forget(reg.get_mut(id).unwrap()); s.fold_registry.forget(reg.get_mut(id).unwrap());
} }
assert!(s.fold_registry.store(id).is_none(), "the store is dropped"); assert!(s.fold_registry.store(id).is_none(), "the store is dropped");
assert!(fold::make_shared_fold_registry().folds(id).is_empty()); assert!(
s.fold_registry.folds(id).is_empty(),
"no folds survive the drop"
);
}
#[test]
fn forget_buffer_drops_store_on_kill() {
// The id-keyed reset the pmacs.buffer.kill path uses: the buffer (and
// its attached view) is gone, so only the map entry is cleared.
let s = EditorState::new();
let id = active_id(&s);
insert_into(&s, id, "line0\nline1\nline2\n");
{
let core = s.core.borrow();
let mut reg = core.registry.borrow_mut();
s.fold_registry
.store_or_attach(reg.get_mut(id).unwrap())
.lock()
.unwrap()
.insert(ByteRange { start: 5, end: 11 });
}
assert!(s.fold_registry.store(id).is_some());
s.fold_registry.forget_buffer(id);
assert!(s.fold_registry.store(id).is_none());
}
#[test]
fn stale_parse_tree_refuses_fold() {
// Q#FD10: an edit after the parse leaves `pending_edit_count() > 0`,
// so a fold command refuses (the settled coordinates are stale).
let s = EditorState::new();
let id = active_id(&s);
let src = "fn foo() {\n let x = 1;\n let y = 2;\n}\n";
insert_into(&s, id, src);
install_rust_parse(&s, id);
// A further edit accumulates a pending edit on the attached parse view.
insert_into(&s, id, "// stale\n");
s.core.borrow_mut().set_cursor_byte(20);
exec(&s, "pmacs.command.invoke('fold.close')");
let n: i64 = eval(&s, "return #pmacs.fold.folds(pmacs.window.buffer())");
assert_eq!(n, 0, "a stale parse tree refuses fold creation");
}
#[test]
fn read_only_buffer_is_rejected() {
// Q#FD11's "normal document buffer" guard: terminals are read-only, so
// a read-only buffer is not foldable.
let s = EditorState::new();
exec(
&s,
"b = pmacs.buffer.from_bytes('ro.rs', 'aaa\\nbbb\\nccc\\n')",
);
let id = {
let core = s.core.borrow();
let reg = core.registry.borrow();
reg.find_by_name("ro.rs").expect("buffer")
};
{
let core = s.core.borrow();
let mut reg = core.registry.borrow_mut();
reg.get_mut(id).unwrap().set_read_only(true);
}
let ok: bool = eval(&s, "return pmacs.fold.fold(b, { start = 3, ['end'] = 7 })");
assert!(!ok, "a read-only buffer is rejected (Q#FD11)");
}
#[test]
fn unfold_normalizes_an_arbitrary_range_to_a_stored_fold() {
let s = EditorState::new();
exec(
&s,
"b = pmacs.buffer.from_bytes('doc.rs', 'aaa\\nbbb\\nccc\\nddd\\n')",
);
let folded: bool = eval(&s, "return pmacs.fold.fold(b, { start = 3, ['end'] = 11 })");
assert!(folded);
// A *different* input range that normalizes to the same stored fold.
let unfolded: bool = eval(
&s,
"return pmacs.fold.unfold(b, { start = 1, ['end'] = 10 })",
);
assert!(unfolded, "unfold normalizes an arbitrary range");
let n: i64 = eval(&s, "return #pmacs.fold.folds(b)");
assert_eq!(n, 0);
} }