From 7e27de63d876588ff293f4eb24368f047a5cfba0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 18:06:11 +0200 Subject: [PATCH] fix(listview): item was load-bearing, and the id contract was two contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TWO REVIEW FINDINGS, both real, and neither reachable from the existing tree tests. 1. `item` WAS EFFECTIVELY REQUIRED. `render` writes `line_to_item[n] = row.item`, so that map is SPARSE whenever a row omits the optional `item` — and `seat_cursor` took `#` of it. A display-only tree (a grouping node with `on_visit` unused, which the API explicitly allows) made that length 0, so the cursor never left the header, TAB found no row, and folding was unusable. It now counts visible rows explicitly. The old tests could not catch this because every one of them supplies `item`: under the reverted fix `tr_5` fails `left: 0 / right: 1` while `tr_1` still passes. 2. THE ID CONTRACT WAS TWO CONTRACTS. The docs said "opaque, compared by equality". Selection does compare with `==`, honouring `__eq` — but collapse state stores ids as TABLE KEYS, and Lua indexes tables by raw identity, consulting no metamethod. So a table id would satisfy one half and quietly fail the other: after a refresh minted fresh id tables, the cursor would be restored and the fold silently lost. A divergence that shows up as a missing fold, arbitrarily later, with nothing pointing back at the id. Narrowed rather than generalized. Equality-aware collapse lookup is the alternative and it is worse: `hidden_by_ancestor` runs per row, so it turns a linear render quadratic to support a key type no consumer has asked for. The contract is now the one both halves can honour — string or number, compared by value — enforced by `check_ids` where rows enter (`open` and `refresh`), so a bad id is a named error at the call site instead of a lost fold much later. Q#TR3 in the framing records the narrowing and why. Verified: fmt, clippy, diff-check, --lib 1897/0, crdt 2082/0, listview 24/24, m4 150/0, gpu 221/0. Both fixes bitten independently. Co-Authored-By: Claude Opus 5 --- builtin/runtime/listview.lua | 47 ++++++++++++++++++++--- docs/tree-primitive-framing.md | 2 +- tests/listview_acceptance.rs | 69 ++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 51b07dd..4d6cd00 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -116,8 +116,8 @@ end -- re-asserts the lock, all inside one registry borrow. -- Tree support (docs/tree-primitive-framing.md, Q#TR1-TR4). -- --- A row MAY carry `depth` (0-based, structural) and `id` (opaque, --- consumer-supplied, compared by equality). Both optional: a row +-- A row MAY carry `depth` (0-based, structural) and `id` (a STRING or +-- NUMBER, consumer-supplied, compared by value). Both optional: a row -- without them behaves exactly as before, which is what keeps the -- three flat consumers byte-identical. -- @@ -158,15 +158,46 @@ local function hidden_by_ancestor(p, rows, i) return false end +-- Ids must be scalars, and the reason is not fussiness about types. +-- Selection compares them with `==`, which honours `__eq`; collapse +-- state stores them as TABLE KEYS, and Lua indexes tables by raw +-- identity, consulting no metamethod. A table id would therefore +-- satisfy one and quietly fail the other: after a refresh minted fresh +-- id tables, selection would be restored and the fold would be lost. +-- +-- Equality-aware collapse lookup is the alternative, and it is worse +-- here: `hidden_by_ancestor` runs per row and would turn a linear +-- render quadratic to support a key type no consumer has wanted. So +-- the contract is narrowed to the one both halves can honour, and +-- enforced where rows enter rather than discovered as a lost fold. +local function check_ids(rows) + for i, row in ipairs(rows) do + local k = type(row.id) + if row.id ~= nil and k ~= "string" and k ~= "number" then + error(string.format( + "listview: row %d has a %s id; ids must be a string or number " + .. "(collapse state keys a table by identity, so a %s id would " + .. "lose its fold across a refresh)", i, k, k)) + end + end + return rows +end + local function render(p, rows) local lines = { p.header } + p.visible = 0 p.line_to_item = {} p.line_to_row = {} for i, row in ipairs(rows) do if not hidden_by_ancestor(p, rows, i) then lines[#lines + 1] = row.text + -- SPARSE BY CONSTRUCTION: `item` is optional, and a display-only + -- row (a grouping header in a tree, say) supplies none, so this + -- key is simply absent for it. Nothing may take `#` of this + -- table; `visible` below is the row count. p.line_to_item[#lines - 1] = row.item p.line_to_row[#lines - 1] = row + p.visible = #lines - 1 end end pmacs.buffer.set_generated_contents(p.buffer, table.concat(lines, "\n")) @@ -188,7 +219,11 @@ end -- `switch_active_buffer` zeroes the window cursor, so a fresh switch -- puts us on the header; walk down from there. local function seat_cursor(p, line) - local count = #p.line_to_item + -- `p.visible`, NOT `#p.line_to_item`: that map is sparse whenever a + -- row omits the optional `item`, and `#` on a sparse table is not + -- the row count. Reading it there left a tree of display-only rows + -- with the cursor stranded on the header, where TAB finds no node. + local count = p.visible or 0 if count == 0 then return end local target = math.max(1, math.min(line or 1, count)) for _ = 1, target do @@ -248,7 +283,7 @@ local function ensure_panel(name) local buf = pmacs.buffer.create(actual) p = { requested_name = name, buffer = buf, line_to_item = {}, - line_to_row = {}, collapsed = {}, rows = {} } + line_to_row = {}, collapsed = {}, rows = {}, visible = 0 } panels[#panels + 1] = p -- Read-only (Q#P3): every non-bypass edit is rejected, with a NAMED -- error. Kept beside the rope lock, not replaced by it: the layering @@ -284,7 +319,7 @@ function pmacs.listview.open(spec) -- Keep the row array: collapse re-renders from it WITHOUT calling the -- consumer, which is what lets a panel with no `on_refresh` still -- expand and collapse (the outline has none -- framing §1.5a). - p.rows = spec.rows or {} + p.rows = check_ids(spec.rows or {}) p.collapsed = {} render(p, p.rows) -- Bottom-panel arc (Q#BP11b): the placement opt-in. `seat_cursor` and @@ -332,7 +367,7 @@ pmacs.command.define { -- the row set moves every line; the id survives it. local saved_row = p.line_to_row[saved] local saved_id = saved_row and saved_row.id - local rows = p.on_refresh() or {} + local rows = check_ids(p.on_refresh() or {}) p.rows = rows render(p, rows) -- `set_generated_contents` has already refreshed this window's diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 8343166..7e82093 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -21,7 +21,7 @@ toward the consumer keeping `text`. |---|---| | **Q#TR1** | **Extend `listview`.** A separate `treeview` would either duplicate ~200 lines of panel discipline (Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, quit chain, generated-buffer writes) or require *extracting* them from a shipped primitive first — the riskier change. Extending is backward-compatible by construction: absent `depth`/`id` give today's behaviour exactly, which the three flat consumers already produce. | | **Q#TR2** | **Primitive-owned collapse state**, keyed by row id, held in the panel record. Consumer-owned would make every consumer reimplement refresh survival. | -| **Q#TR3** | **Consumer-supplied `row.id`**, compared by equality; the primitive never derives one. The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | +| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | | **Q#TR4** | **Consumer keeps pre-rendered `text`**; `depth` is structural only. Also sidesteps the conflict with dired's fixed-width `_layout` column contract when it adopts. | **Acceptance 5 is decided too: byte-identity coverage is written**, diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 4c14dcb..84245bf 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -1075,6 +1075,75 @@ fn s1_14_no_bypass_write_or_name_keyed_identity_remains() { ); } +/// A tree row need not carry `item` — `on_visit` is optional, so a +/// display-only node (a grouping header) is a legitimate row. The +/// cursor must still seat on it. +/// +/// This bit: `line_to_item` is SPARSE when rows omit `item`, and +/// `seat_cursor` took `#` of it. For an all-display-only tree that +/// length is 0, so the cursor never left the header — where TAB finds +/// no row and answers "no node here", making the tree unfoldable. +#[test] +fn tr_5_a_tree_of_display_only_rows_is_still_navigable_and_foldable() { + let mut s = editor(); + exec( + &s, + r#"pmacs.listview.open { + name = "*tr5*", + header = "display-only TAB fold", + rows = { + { text = "root", depth = 0, id = "r" }, + { text = " kid", depth = 1, id = "rk" }, + }, + }"#, + ); + + // Seated on a data row, not stranded on the header. + let line: i64 = eval(&s, "return pmacs.editor.cursor_line()"); + assert_eq!( + line, 1, + "cursor seats on the first data row despite no `item`" + ); + + press(&mut s, KeyCode::Tab); + assert!( + !status(&s).contains("no node here"), + "TAB found the node: {}", + status(&s) + ); + assert!( + !active_text(&s).contains("kid"), + "and folded it: {:?}", + active_text(&s) + ); +} + +/// Q#TR3's contract is that ids compare by value. Collapse state keys +/// a Lua table, and table indexing consults no `__eq`, so a non-scalar +/// id would compare equal for selection and unequal for folding: a +/// refresh would restore the cursor and silently lose the fold. The +/// contract is narrowed to scalars and enforced where rows enter, +/// rather than left to surface as a lost fold much later. +#[test] +fn tr_6_a_non_scalar_id_is_rejected_where_rows_enter() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr6*", + header = "h", + rows = { { text = "a", depth = 0, id = {} } }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("ids must be a string or number"), + "rejected where rows enter, with a reason: {err}" + ); +} + // Isolated bootstrap storage roots (see the module docs): an // integration test is compiled without `cfg(test)`, so a raw // `EditorState::new()` would read the developer's real `init.lua` and