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