diff --git a/docs/compile-mode-framing.md b/docs/compile-mode-framing.md index f5b7f7c..5127747 100644 --- a/docs/compile-mode-framing.md +++ b/docs/compile-mode-framing.md @@ -1,7 +1,33 @@ # Compile-mode — framing (Arc 5 stage 1, terminal) -**Revision 11 — 2026-07-13. Status: implemented on branch -`compile-mode` (PR #113); revisions 7–11 fold in PR rounds 1–5.** +**Revision 12 — 2026-07-14. Status: implemented on branch +`compile-mode` (PR #113); revisions 7–12 fold in PR rounds 1–6.** + +Revision 12 (PR #113 round 6, findings 1–3): render-view attachment +is idempotent and split-complete. Overlays expose an +`overlay_identity` (the span store's allocation address); +`Window::ensure_overlay` attaches a store-backed render view AT MOST +once per window — pre-fix every switch into the buffer blindly +pushed another copy onto EVERY matching window, so passive panes +accumulated duplicates, each cloning all spans and rescanning the +buffer per frame. A same-buffer split copies clonable overlays to +the new pane via `clone_for_split` (splits fire no switch hook and +started with an empty overlay list — the new compilation pane +rendered unstyled). The translator ignores pure no-op edits +(buffers deliberately broadcast them): pre-fix each interior no-op +split the containing span into adjacent fragments — unbounded list +growth, and a no-op at a UTF-8 continuation byte minted a +mid-codepoint span boundary. And the overlay handle has a teardown +path: `handle:dispose()` idempotently detaches the buffer-attached +translator and removes every window render view over its store — +the documented lifetime contract is one handle per buffer +incarnation (compile/REPL need no disposal; repeated creation on a +long-lived buffer must dispose retired handles or every edit keeps +paying for abandoned translators). Bites: split+bounce per-cell +acceptance (immediate post-split assert, before any switch could +heal the pane), no-op fragmentation Lua twin, dispose Lua twin; +units for genuine insertion, no-op ignore, split copy, and +ensure-once. Revision 11 (PR #113 round 5, findings 1–3): style-span coordinate translation belongs to the BUFFER, not to views — a new @@ -240,8 +266,12 @@ Everything below was verified by reading the code, not the roadmap. exactly once per edit — Revision 11) plus render-only window overlays on windows *currently showing* the buffer; buffer switches clear window overlays; `attach_style_overlay(buf, - handle)` re-attaches the render view. The handle has `add`, - `clear`, `clear_before`, `spans`. + handle)` re-attaches the render view, idempotently per window via + the store identity, and same-buffer splits copy the render view to + the new pane (Revision 12). The handle has `add`, `clear`, + `clear_before`, `spans`, and idempotent `dispose` (teardown of the + translator + every window render view; one handle per buffer + incarnation needs no disposal). - **Buffer-switch hooks**: `buffer.after-switch` exists and fires on the ordinary switch paths (recentf subscribes, `builtin/runtime/recentf.lua:54`). **`pmacs.editor.jump_back` does diff --git a/src/editor.rs b/src/editor.rs index 0d03af2..0f23c07 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -5778,6 +5778,68 @@ mod tests { ); } + /// PR #113 round-6 finding 1: a same-buffer split copies + /// store-backed render overlays to the new pane (splits fire no + /// switch hook and started from an empty overlay list), and + /// per-window attachment is idempotent via the store identity. + #[test] + fn same_buffer_split_copies_style_overlays_and_attach_is_idempotent() { + use crate::overlay::{BufferStyleOverlay, SharedBufferStyleSpans}; + use crate::window::Orientation; + use std::sync::{Arc, Mutex}; + + let s = fresh_with(b"hello\n"); + let store: SharedBufferStyleSpans = Arc::new(Mutex::new(Vec::new())); + { + let mut core = s.core.borrow_mut(); + let win = core.active_window_mut(); + win.ensure_overlay(Box::new(BufferStyleOverlay::new(Arc::clone(&store)))); + // Second ensure over the SAME store: no duplicate. + win.ensure_overlay(Box::new(BufferStyleOverlay::new(Arc::clone(&store)))); + assert_eq!( + win.overlay_kinds() + .iter() + .filter(|k| **k == "buffer_style_overlay") + .count(), + 1, + "ensure_overlay must be idempotent per store" + ); + } + // Same-buffer split: the new pane carries a copy. + let new_id = s + .core + .borrow_mut() + .split_active(Orientation::Horizontal, true); + { + let core = s.core.borrow(); + let win = core.windows.get(&new_id).expect("split window"); + assert_eq!( + win.overlay_kinds() + .iter() + .filter(|k| **k == "buffer_style_overlay") + .count(), + 1, + "a same-buffer split must copy the render overlay" + ); + } + // Fresh-buffer split: no copy (different buffer, different + // styling). + let scratch_id = s + .core + .borrow_mut() + .split_active(Orientation::Horizontal, false); + let core = s.core.borrow(); + let win = core.windows.get(&scratch_id).expect("scratch window"); + assert_eq!( + win.overlay_kinds() + .iter() + .filter(|k| **k == "buffer_style_overlay") + .count(), + 0, + "a fresh-buffer split carries nothing" + ); + } + // ---- T M2.12: mouse input ---------------------------------------------- fn mouse(kind: crossterm::event::MouseEventKind, row: u16, col: u16) -> MouseEvent { diff --git a/src/editor_core.rs b/src/editor_core.rs index 64b3211..0ef913b 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -2109,9 +2109,21 @@ impl EditorCore { (new_id, TextView::new(buf)) }; let new_id = WindowId::next(); - let new_window = Window::new(new_id, buffer_id, text_view); - self.windows.insert(new_id, new_window); + let mut new_window = Window::new(new_id, buffer_id, text_view); let active = self.active_window_id(); + // A same-buffer split starts from an empty overlay list and + // fires no switch hook, so store-backed render overlays + // (ANSI styling on a compile buffer) would silently vanish + // from the new pane (PR #113 round-6 finding 1). Views that + // carry across splits say so via `clone_for_split`. + if same_buffer && let Some(src) = self.windows.get(&active) { + for overlay in &src.overlays { + if let Some(copy) = overlay.clone_for_split() { + new_window.overlays.push(copy); + } + } + } + self.windows.insert(new_id, new_window); self.active_layout_mut() .split_window(active, orientation, new_id); new_id diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 2501c9e..f0f08cb 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1743,9 +1743,22 @@ pub struct InterceptHandleLua { #[derive(Clone)] /// Lua handle for a shared buffer-byte style overlay. +/// +/// Lifetime contract (PR #113 round-6 finding 3): the buffer-attached +/// translator lives until the buffer dies OR `dispose()` is called. +/// One handle per buffer incarnation (the compile-mode and REPL +/// discipline) needs no disposal — the buffer's death frees it; +/// repeated `add_style_overlay` calls on a LONG-LIVED buffer must +/// `dispose()` retired handles, or every edit keeps paying for every +/// abandoned translator. pub struct StyleOverlayHandleLua { /// Shared style spans rendered by every attached overlay view. spans: crate::overlay::SharedBufferStyleSpans, + /// Buffer the translator was attached to. + buffer: BufferId, + /// The buffer-attached translator's view id — retained so + /// `dispose()` can detach it. + translator: crate::buffer::ViewId, } impl FromLua for StyleOverlayHandleLua { @@ -1821,6 +1834,29 @@ impl UserData for StyleOverlayHandleLua { } Ok(out) }); + + // Idempotent teardown (round-6 finding 3): detaches the + // buffer-attached translator (so later edits stop paying for + // it) and removes every window render view over this store. + // Without this, a retired handle's translator lived until + // the buffer died — permanent per-edit cost growth for + // repeated creation on a long-lived buffer. Safe to call + // twice; safe after the buffer is gone. + methods.add_method("dispose", |lua, this, ()| { + let id = crate::overlay::style_store_identity(&this.spans); + if let Some(core) = lua.app_data_ref::() { + let mut core = core.borrow_mut(); + for win in core.windows.values_mut() { + win.overlays.retain(|v| v.overlay_identity() != Some(id)); + } + let registry = core.registry.clone(); + let mut r = registry.borrow_mut(); + if let Ok(buf) = r.get_mut(this.buffer) { + buf.detach_view(this.translator); + } + } + Ok(()) + }); } } @@ -2922,9 +2958,6 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result mlua::Result { let spans = Arc::new(Mutex::new(Vec::new())); - let handle = StyleOverlayHandleLua { - spans: Arc::clone(&spans), - }; // Coordinate translation lives on the BUFFER // (PR #113 round-5 finding 1): buffer-attached // views see every edit exactly once — Lua bypass @@ -2933,13 +2966,18 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result>>; +/// Identity of a span store: the allocation address, stable for the +/// `Arc`'s lifetime. Every overlay/translator over the same store +/// reports this via [`View::overlay_identity`], which is what makes +/// per-window attachment idempotent and disposal able to find every +/// window copy (PR #113 round-6 findings 1 and 3). +#[must_use] +pub fn style_store_identity(spans: &SharedBufferStyleSpans) -> usize { + Arc::as_ptr(spans) as usize +} + /// View that renders buffer-byte style annotations. /// /// Unlike [`StyleSpanOverlay`], this overlay stores byte ranges rather @@ -241,6 +251,17 @@ impl View for BufferStyleSpanTranslator { let old_end = edit.range.end; let old_len = old_end - old_start; let new_len = edit.inserted_len; + // Buffers deliberately broadcast no-op edits (empty insert / + // empty-range delete — buffer.rs's "callers that count the + // call" contract). Nothing moved, so there is nothing to + // translate; falling through would split any span containing + // the position into two adjacent fragments per call — + // unbounded growth for repeated no-ops, and a fragment + // boundary mid-codepoint for a no-op at a continuation byte + // (round-6 finding 2). + if old_len == 0 && new_len == 0 { + return Ok(()); + } let mut spans = self.spans.lock().expect("style spans mutex poisoned"); let mut adjusted = Vec::with_capacity(spans.len()); for span in spans.drain(..) { @@ -273,9 +294,25 @@ impl View for BufferStyleSpanTranslator { fn kind(&self) -> &'static str { "buffer_style_span_translator" } + + fn overlay_identity(&self) -> Option { + Some(style_store_identity(&self.spans)) + } } impl View for BufferStyleOverlay { + fn kind(&self) -> &'static str { + "buffer_style_overlay" + } + + fn overlay_identity(&self) -> Option { + Some(style_store_identity(&self.spans)) + } + + fn clone_for_split(&self) -> Option> { + Some(Box::new(self.clone())) + } + fn render(&mut self, buf: &Buffer, viewport: Viewport, cells: &mut CellGrid<'_>) { let spans = self .spans @@ -850,4 +887,63 @@ mod tests { "a fully-overwritten span produces no fragments" ); } + + #[test] + fn translator_splits_a_span_around_a_genuine_insertion() { + // A real EditOp::Insert (not a replacement) inside a span: + // the left fragment stays, the right fragment shifts by the + // inserted length, and the inserted bytes inherit nothing. + use crate::buffer::EditOp; + let mut buf = Buffer::from_bytes(BufferId::next(), "t", b"abcdef"); + let store: SharedBufferStyleSpans = Arc::new(Mutex::new(vec![BufferStyleSpan { + start: 0, + end: 6, + style: red(), + }])); + buf.attach_view(Box::new(BufferStyleSpanTranslator::new(Arc::clone(&store)))); + buf.apply_edit(EditOp::Insert { + pos: 3, + bytes: b"XY", + }) + .expect("insert applies"); + assert_eq!( + spans_of(&store), + vec![(0, 3), (5, 8)], + "insertion splits the span; inserted bytes are unstyled" + ); + } + + #[test] + fn translator_ignores_pure_noop_edits() { + // Buffers deliberately broadcast no-op edits (empty insert / + // empty-range delete). PR #113 round-6 finding 2: falling + // through split a containing span into two adjacent + // fragments per call — unbounded growth for repeated no-ops + // at distinct positions, and a fragment boundary + // mid-codepoint for a no-op at a UTF-8 continuation byte. + use crate::buffer::EditOp; + use crate::rope::Range; + let mut buf = Buffer::from_bytes(BufferId::next(), "t", "ab\u{e9}def".as_bytes()); + let store: SharedBufferStyleSpans = Arc::new(Mutex::new(vec![BufferStyleSpan { + start: 0, + end: 7, + style: red(), + }])); + buf.attach_view(Box::new(BufferStyleSpanTranslator::new(Arc::clone(&store)))); + // Distinct interior positions, including 3 — the é's + // continuation byte. + for pos in [1, 2, 3, 4, 5] { + buf.apply_edit(EditOp::Insert { pos, bytes: b"" }) + .expect("no-op insert applies"); + buf.apply_edit(EditOp::Delete { + range: Range::new(pos, pos), + }) + .expect("no-op delete applies"); + } + assert_eq!( + spans_of(&store), + vec![(0, 7)], + "no-op edits must not fragment or move spans" + ); + } } diff --git a/src/view.rs b/src/view.rs index 139c599..313f411 100644 --- a/src/view.rs +++ b/src/view.rs @@ -236,6 +236,27 @@ pub trait View { fn kind(&self) -> &'static str { "unknown" } + + /// Identity of the shared resource this overlay renders, if any + /// (PR #113 round-6 findings 1 and 3). Two overlay instances + /// backed by the same store report the same value, which lets a + /// window attach a resource-backed overlay AT MOST once + /// ([`crate::window::Window::ensure_overlay`]) and lets disposal + /// remove every window copy. The default `None` opts out: such + /// views are never deduplicated or bulk-removed. + fn overlay_identity(&self) -> Option { + None + } + + /// A copy of this overlay for a freshly split window showing the + /// same buffer (round-6 finding 1: a same-buffer split starts + /// with an empty overlay list and fires no switch hook, so + /// without this the new pane rendered unstyled). `None` (the + /// default) means the view does not carry across splits; + /// store-backed render overlays return a clone. + fn clone_for_split(&self) -> Option> { + None + } } // --------------------------------------------------------------------------- diff --git a/src/window.rs b/src/window.rs index 316ecc4..22add47 100644 --- a/src/window.rs +++ b/src/window.rs @@ -232,6 +232,25 @@ impl Window { self.overlays.push(view); } + /// Push `view` unless an overlay with the same + /// [`View::overlay_identity`] is already attached — attachment + /// of store-backed overlays must be idempotent per window + /// (PR #113 round-6 finding 1: repeated switches into a buffer + /// stacked duplicate render views on passive panes, each cloning + /// every span and rescanning the buffer per frame). Views + /// without an identity always push. + pub fn ensure_overlay(&mut self, view: Box) { + if let Some(id) = view.overlay_identity() + && self + .overlays + .iter() + .any(|v| v.overlay_identity() == Some(id)) + { + return; + } + self.overlays.push(view); + } + /// Stable kind identifiers of every overlay on this window, in /// push order. Test seam used by `pmacs.window._overlay_kinds()` /// to verify that a specific overlay type actually attached diff --git a/tests/compile_mode_acceptance.rs b/tests/compile_mode_acceptance.rs index 9a37c76..c59edf8 100644 --- a/tests/compile_mode_acceptance.rs +++ b/tests/compile_mode_acceptance.rs @@ -2361,6 +2361,158 @@ fn r5f2_partial_rewrite_preserves_untouched_styling() { ); } +// --------------------------------------------------------------------------- +// PR #113 round 6 — bite tests +// --------------------------------------------------------------------------- + +/// Count of `buffer_style_overlay` render views on the ACTIVE window. +fn active_style_overlay_count(s: &EditorState) -> i64 { + eval( + s, + r#" + local n = 0 + for _, k in ipairs(pmacs.window._overlay_kinds()) do + if k == "buffer_style_overlay" then n = n + 1 end + end + return n + "#, + ) +} + +#[test] +fn r6f1_split_panes_stay_styled_with_one_attachment_each() { + use pmacs::cell::Color; + // A styled compilation split into two panes: the split copies + // the render view (splits fire no switch hook and pre-fix left + // the new pane unstyled), and repeated switches of one pane must + // not stack duplicates on the passive pane (pre-fix every + // re-attach blindly pushed to EVERY matching window — unbounded + // render cost). Round-6 finding 1. + let dir = tempfile::tempdir().unwrap(); + let script = write_script( + dir.path(), + "red.sh", + "printf '\\033[31mhello\\033[0m world\\n'\n", + ); + let mut s = editor(); + compile_and_finish(&mut s, &format!("sh {script}"), dir.path()); + exec(&s, "pmacs.window.split_horizontal()"); + // IMMEDIATELY after the split — before any switch could heal it + // through the attach-to-all-matching-windows path — both panes + // must be styled with one attachment each. This is the split + // half of the finding: pre-fix the new pane had NO render view. + for pane in 0..2 { + assert_eq!( + active_style_overlay_count(&s), + 1, + "pane {pane} post-split: exactly one render attachment" + ); + let cells = render_active_window_to_grid(&mut s, 12, 60); + let row = styled_row(&cells, 12, 60, "hello world", 5); + assert!( + row.iter().all(|(_, fg)| *fg == Color::Indexed(1)), + "pane {pane} post-split: 'hello' renders red; got {row:?}" + ); + exec(&s, "pmacs.window.focus_next()"); + } + // Bounce the ACTIVE pane away and back three times; the passive + // pane keeps showing *compilation* through every re-attach. This + // is the accumulation half: pre-fix every re-attach blindly + // pushed another render view onto the passive pane. + exec(&s, r#"bounce_buf = pmacs.buffer.create("*bounce*")"#); + for _ in 0..3 { + exec(&s, "pmacs.window.switch_buffer(bounce_buf)"); + exec( + &s, + r#" + for _, id in ipairs(pmacs.buffer.list()) do + if pmacs.describe.buffer(id).name == "*compilation*" then + pmacs.window.switch_buffer(id) + end + end + "#, + ); + } + for pane in 0..2 { + assert_eq!( + active_style_overlay_count(&s), + 1, + "pane {pane} post-bounce: exactly one render attachment" + ); + let cells = render_active_window_to_grid(&mut s, 12, 60); + let row = styled_row(&cells, 12, 60, "hello world", 5); + assert!( + row.iter().all(|(_, fg)| *fg == Color::Indexed(1)), + "pane {pane} post-bounce: 'hello' renders red; got {row:?}" + ); + exec(&s, "pmacs.window.focus_next()"); + } +} + +#[test] +fn r6f2_noop_edits_do_not_fragment_spans() { + // Buffers deliberately broadcast no-op edits; the translator + // must ignore them (round-6 finding 2). Pre-fix each interior + // no-op split the containing span into two adjacent fragments — + // unbounded growth, and position 3 (the é's continuation byte) + // minted a mid-codepoint span boundary. + let s = editor(); + let (count, start, end): (i64, i64, i64) = eval( + &s, + r#" + local buf = pmacs.buffer.create("*noop-spans*") + local ov = pmacs.buffer.add_style_overlay(buf) + buf:insert(0, "ab\195\169def") + ov:add(0, 7, { fg = 1 }) + for _, pos in ipairs({ 1, 2, 3, 4, 5 }) do + buf:insert(pos, "") + buf:delete(pos, pos) + end + local spans = ov:spans() + return #spans, spans[1].start, spans[1]["end"] + "#, + ); + assert_eq!( + (count, start, end), + (1, 0, 7), + "no-op edits must neither fragment nor move spans" + ); +} + +#[test] +fn r6f3_handle_dispose_detaches_translator_and_render_views() { + // Teardown path (round-6 finding 3): dispose() detaches the + // buffer-attached translator (later edits stop translating) and + // removes every window render view over the handle's store; + // calling it twice is safe. Pre-fix a dropped handle's + // translator lived until the buffer died. + let s = editor(); + let (live, stale, render_views): (i64, i64, i64) = eval( + &s, + r#" + local buf = pmacs.buffer.create("*disposable*") + pmacs.window.switch_buffer(buf) + local ov = pmacs.buffer.add_style_overlay(buf) + buf:insert(0, "abcdef") + ov:add(0, 6, { fg = 1 }) + buf:insert(0, "xx") -- live translator: span shifts + local live = ov:spans()[1].start + ov:dispose() + buf:insert(0, "yy") -- detached: span must NOT move + local stale = ov:spans()[1].start + ov:dispose() -- idempotent + local n = 0 + for _, k in ipairs(pmacs.window._overlay_kinds()) do + if k == "buffer_style_overlay" then n = n + 1 end + end + return live, stale, n + "#, + ); + assert_eq!(live, 2, "pre-dispose edits translate the span"); + assert_eq!(stale, 2, "post-dispose edits must not reach the store"); + assert_eq!(render_views, 0, "dispose removes the window render views"); +} + #[test] fn r5f3_tracked_line_start_matches_the_scan_across_transitions() { // Round-5 finding 3 is a performance fix — the per-CR/BS/erase