diff --git a/context/code/command.md b/context/code/command.md index bc294cce..5f4542a2 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3401,18 +3401,41 @@ one run in eight (measured: 46 of 400). So the arguments now include **every item of every regular category as a one-coordinate drill**, read off the parsed document. A one-coordinate drill spans every other category whole, so it matches the whole slice of records under that item, not one record picked by name. In -the demo that is four records and eight cells for each of the eight drills. The -test -also asserts that the widest candidate it actually passes matches at least four -records. That guard catches a demo with too little data, and it catches the -derived drills being dropped from the argument list. It does **not** catch the -comparison losing its wide drills some other way, because it measures what the -argument lists match, not what the built commands ordered (improvise-b9f2). The -review showed this with a rival: a `drill-into-cell` parser that refuses fewer -than two arguments sends every one-coordinate drill to the one-record cursor -fallback. The guard still reports four records, and with the store's sort -removed the law passed 38 of 300 runs. With the store's sort removed and the -parser intact, the law fails 400 of 400 runs. +the demo that is four records and eight cells for each of the eight drills. With +the store's sort removed, the law fails 400 of 400 runs. + +**The width guard measures what the comparison compared (improvise-b9f2, +improvise-btyw).** The test asserts that some `drill-into-cell` the comparison +actually built and compared froze at least four records. For each one it applies +those very effects to one scratch `App` and counts the records in its +`drill_state`. It counts them only when their `Debug` text, which carries their +order, appears in the text the comparison compared. The comparison reads `Debug` +while the scratch `App` reads the applied state, and those are two +representations. The review showed they can disagree: give `StartDrill` a +`Debug` that prints only a row count, and the frozen records are still four +while the compared text no longer holds their order. With the store's sort +removed the law then passed 300 of 300 runs, with the guard green. Requiring +the ordered records in the compared text ties the guard to what the comparison +could actually see. A first +version measured what the *argument lists* match in the store instead. That was +a parallel derivation, and the review beat it with a `drill-into-cell` parser +that refuses fewer than two arguments. Every one-coordinate drill then went to +the one-record cursor fallback, the argument lists still matched four records, +and with the store's sort removed the law passed 38 of 300 runs. + +Counting frozen records is not enough by itself. A drill over no coordinates +(any candidate with no `Cat/Item` argument, such as `["1", "1"]`) freezes +**every** record, and it reads them through `iter_cells` in insertion order, +never through the narrowing index where gotcha 18 lives. Under the parser rival +that whole-store drill still froze 16 records, and a guard counting it stayed +silent. So only a drill that **narrowed** the store counts: one that froze fewer +records than the store holds. With the parser rival in place, the guard fires on +every run ("froze 1 records"), and it fires on every run under the `Debug` rival +too ("froze 0 records"). + +What the guard still cannot see is a narrowing drill whose records are frozen in +an order that does not come from the store. It checks that the law had a wide +drill to compare, not why the drill's order would differ. **What it cannot see.** Only commands and arguments the fixture reaches. A command whose nondeterminism needs some other state, such as records mode, a diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 15ff6714..ebe02a28 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4749,3 +4749,58 @@ claims at commit. The commit was then *accepted*. With the refusal assertion disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the cell B cleared. Under that rival, the 0j0f test above failed as well. Both files were restored. + +### a_locally_resolved_page_scroll_lands_where_the_host_would_put_it + +**What was missing (improvise-5xbq).** `tests/lockstep.rs` compared only `j` +and `G` against the host. The browser's page rule (`resolve::page_rows`) and +the TUI's (`Move`'s `Page` arm, `visible_rows * 3 / 4`) were two copies, and +two faults injected into the browser's copy left the whole workspace green: +`page_rows` returning `window_rows / 2`, and `grid_move` paging by +`window_cols`. The second was invisible for a further reason: the only window +the page-rule test used was square, `(8, 8)`. + +**The fixture.** 20 rows by 3 columns; the host sees 8 rows and the client +announces `(8, 3)`. The three candidate rules then give three different +steps: 6 (three quarters of the rows), 4 (half the rows) and 2 (three quarters +of the columns). The walk starts off the edge (two `j`), then presses `PgDn`, +`PgUp`, `Ctrl-d` and `Ctrl-u`, including a `PgDn` that clamps at row 19. + +**Why it pins the rows as well as comparing the ends.** `page_rows` is now the +one home (the TUI's `Move` calls it), so a wrong rule moves host and client +together, and a comparison alone passes. Each press therefore also asserts the +row the TUI's rule names. + +**Injected, before and after the move to one home.** + +| Fault | Two copies | One home | +|---|---|---| +| `page_rows` returns `window_rows / 2` | fails at press 2 (`PgDn`), "client and host disagree" | fails at press 2, "not the row the TUI's page rule names"; `app.rs`'s `page_down_scrolls_by_three_quarters_visible` and `page_up_scrolls_backward` fail too | +| `grid_move` pages by `window_cols` | fails at press 2, "client and host disagree" | fails at press 2, "client and host disagree" | + +`Ctrl-d`/`Ctrl-u` are `scroll-rows ±5`, not the page rule. They are in the walk +because the ticket named them, and they move the start point of the next page +turn to a row the fixed rules alone would not reach. + +### a narrow window still shows how to page through help — `harness/tests/help.spec.js` + +**What broke (improvise-1c5m).** Below the width at which the app bar wraps, +`#help` (fixed, `inset: 2rem`, `z-index: 30`) covers `#hint`, and since PR #180 +Welcome carries no paging instruction of its own. So a new user on Welcome at +phone width could not see how to reach page two. + +**The fix is option (b) from the ticket.** The overlay draws `model.hint` in a +footer. In Help mode that is already `hint_line(Help)`, the same string `#hint` +carries, so it adds no derivation, and it sits outside `.help-body`, so +"Welcome is prose" still holds. The footer is `position: sticky` because the +overlay itself scrolls and Welcome is longer than a phone-height overlay. + +**What the case asserts.** At 600x800 on Welcome, it reads the hint string from +`#hint`, so the spec spells no keys. It then requires some element showing +exactly that string to be the topmost element at its own centre, which is the +`elementFromPoint` measurement the ticket made by hand. + +**Red, then a rival.** At base it failed with "the paging hint is under the +help overlay". With the fix and `position: sticky` changed to `static`, it +fails the same way: the footer exists but sits below the fold of a scrolling +overlay, so presence alone cannot satisfy the case. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 387f3cd1..4d64b414 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5509,14 +5509,41 @@ rivals it catches: still pin the store directly. - **The derived drills dropped from the argument list** (the `all.extend(one_coordinate_drills(model))` line commented out): the test - fails before comparing anything, with "the widest drill orders 1 records". - That is the only way of losing the wide drills the guard can see. It measures - what the argument lists match, not what the built commands ordered, so it can - pass while the comparison has lost its wide drills (improvise-b9f2). Review - rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, - which sends every one-coordinate drill to the one-record cursor fallback. The - guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 - runs. + fails with "the widest narrowing drill compared froze 1 records". +- **`drill-into-cell`'s parser changed to refuse fewer than two arguments** + (improvise-b9f2): every one-coordinate drill then goes to the one-record cursor + fallback. The first width guard measured what the argument lists match, so it + stayed silent. With gotcha 18 re-injected the law passed 38 of 300 runs in + review, and 31 of 300 here at `cfcc9bff`. The guard now applies the compared + drills' own effects to a scratch `App` and counts the records they froze. + Against this rival it fails on every run with "froze 1 records". + - A first version of the fix counted all frozen records and **still passed**. + A candidate with no `Cat/Item` argument drills the whole store (16 records), + and it reads them in insertion order through `iter_cells`, which the + injection cannot reach. So the guard counts only drills that froze fewer + records than the store holds. + - Measured at `7dfa90f1`, test binary in a loop: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + parser rival | **0 of 300** | all 300 | + | gotcha 18 alone | **0 of 300** | 0 (every failure a real divergence) | + | neither | 300 of 300 | 0 | +- **`StartDrill`'s `Debug` changed to print only its row count** + (`StartDrill(4 rows)`, improvise-btyw): the law compares effects by their + `Debug` text, and this rival takes the record order out of that text, while + the frozen `drill_state` still holds four records. At `7dfa90f1`, with gotcha + 18 re-injected, the law **passed 200 of 200** runs and the guard stayed green. + The guard now counts a drill only when its frozen records, as `Debug` prints + them, appear in the text the comparison compared. The same measurements after + the repair, 200 runs each: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + `Debug` rival | **0 of 200** | all 200 ("froze 0 records") | + | gotcha 18 + parser rival | **0 of 200** | all 200 | + | gotcha 18 alone | **0 of 200** | 0 (every failure a real divergence) | + | neither | 200 of 200 | 0 | - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 8f0a71e5..06135353 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -1,14 +1,14 @@ //! A command is a function of its context: two contexts built independently from //! equal data give equal effects — `context/code/command.md`, improvise-hzjo. -use std::collections::{BTreeSet, HashSet}; +use std::collections::BTreeSet; use super::core::{Cmd, CmdContext}; +use super::grid::DrillIntoCell; use super::registry::tests::candidate_args; use crate::command::key::{KeyCode, KeyModifiers}; use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; -use crate::model::record::record_of; use crate::ui::app::App; use crate::ui::effect::{self, Effect}; @@ -61,27 +61,23 @@ fn one_coordinate_drills(model: &Model) -> Vec> { drills } -/// How many distinct records the widest all-`Cat/Item` candidate matches. -fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { - let coordinates = |args: &Vec| -> Option> { - args.iter() - .map(|a| { - a.split_once('/') - .map(|(c, i)| (c.to_string(), i.to_string())) - }) - .collect() - }; - candidates - .iter() - .filter(|args| !args.is_empty()) - .filter_map(coordinates) - .map(|partial| { - let cells = model.data.matching_cells(&partial); - let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); - records.len() - }) - .max() - .unwrap_or(0) +/// Records a compared drill froze, counted only if it narrowed the store and its +/// ordered records are in `compared`, the text the law compared (improvise-btyw). +fn records_narrowed(scratch: &mut App, effects: Vec>, compared: &str) -> usize { + let store = scratch + .model_state + .workbook + .model + .data + .live_records() + .count(); + scratch.apply_effects(effects); + let frozen = scratch + .drill_state + .as_ref() + .filter(|drill| compared.contains(&format!("{:?}", drill.records))) + .map_or(0, |drill| drill.records.len()); + if frozen < store { frozen } else { 0 } } /// The registry's candidates, plus names the demo really has: a drill or a fit @@ -108,9 +104,9 @@ fn args_the_document_names(model: &Model) -> Vec> { all } -fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option>> { let cmd = ctx.registry.interactive(name, args, ctx).ok()?; - Some(format!("{:?}", cmd.execute(ctx))) + Some(cmd.execute(ctx)) } /// What running every registered command under `a` and under each of `others` found. @@ -120,20 +116,34 @@ struct Comparison { shown: Vec, never_built: Vec<&'static str>, compared: usize, + widest_drill: usize, } fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let mut found = Comparison::default(); let candidates = args_the_document_names(a.model); + let drill = DrillIntoCell { + key: CellKey::new(vec![]), + }; + let mut scratch = fixture(); for name in a.registry.names().collect::>() { let before = found.compared; for args in &candidates { - let Some(left) = effects_of(name, args, a) else { + let Some(effects) = effects_of(name, args, a) else { continue; }; + let left = format!("{effects:?}"); + if name == drill.name() { + found.widest_drill = + found + .widest_drill + .max(records_narrowed(&mut scratch, effects, &left)); + } found.compared += 1; for b in others { - let right = effects_of(name, args, b).unwrap_or_default(); + let right = effects_of(name, args, b) + .map(|e| format!("{e:?}")) + .unwrap_or_default(); if left != right { found.diverged.insert(name); found @@ -157,13 +167,12 @@ fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { .iter() .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) .collect(); - let model = contexts[0].model; - let widest = widest_drill(model, &args_the_document_names(model)); - assert!( - widest >= 4, - "the widest drill orders {widest} records, so the store's order is nearly a coin toss" - ); let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.widest_drill >= 4, + "the widest narrowing drill compared froze {} records, so the store's order is nearly a coin toss", + found.widest_drill + ); assert!( found.never_built.is_empty(), "no candidate args build these in the fixture, so the law never ran them: {:?}", diff --git a/crates/improvise-command/src/command/cmd/navigation.rs b/crates/improvise-command/src/command/cmd/navigation.rs index 5f13f2cb..14523b5c 100644 --- a/crates/improvise-command/src/command/cmd/navigation.rs +++ b/crates/improvise-command/src/command/cmd/navigation.rs @@ -118,7 +118,7 @@ impl Cmd for Move { } } MoveKind::Page(dir) => { - let delta = (self.cursor.visible_rows as i32 * 3 / 4).max(1) * dir; + let delta = improvise_protocol::resolve::page_rows(self.cursor.visible_rows) * dir; let nr = (self.cursor.row as i32 + delta).clamp(0, row_max) as usize; (nr, self.cursor.col) } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index d88b4e23..fa22ee24 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -620,7 +620,8 @@ enum GridMove { } /// Rows one `page-scroll` step moves: three quarters of the window, at least one. -fn page_rows(window_rows: usize) -> i32 { +/// The one home: the TUI's `Move` calls it too (improvise-5xbq). +pub fn page_rows(window_rows: usize) -> i32 { (window_rows as i32 * 3 / 4).max(1) } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 294b3fc0..07e51ceb 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -266,6 +266,70 @@ fn a_locally_resolved_cursor_move_lands_where_the_host_would_put_it() { ); } +/// Twenty rows by three columns, the host's window eight rows by three: not +/// square, so a rule keyed off the wrong axis lands somewhere else. +fn paging_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for i in 0..20 { + wb.add_item("Region", format!("R{i:02}")).unwrap(); + } + for m in ["A", "B", "C"] { + wb.add_item("_Measure", m).unwrap(); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + let mut app = App::new(wb, None); + app.term_height = 8 + 8; + app.rebuild_layout(); + app +} + +/// Page and half-page scrolls land in the same place on both ends, and in the +/// place the TUI's rule names. Why the fixture and the pinned rows: the evidence file. +#[test] +fn a_locally_resolved_page_scroll_lands_where_the_host_would_put_it() { + let window = (8, 3); + let mut app = paging_host(); + let mut client = client_of(&app, window); + let key = |code, mods| KeyEvent::new(code, mods); + let none = KeyModifiers::NONE; + let ctrl = KeyModifiers::CONTROL; + let presses = [ + (key(KeyCode::Char('j'), none), 1), + (key(KeyCode::Char('j'), none), 2), + (key(KeyCode::PageDown, none), 8), + (key(KeyCode::PageDown, none), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::Char('d'), ctrl), 13), + (key(KeyCode::PageDown, none), 19), + (key(KeyCode::Char('u'), ctrl), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::PageUp, none), 2), + ]; + for (press, (event, want_row)) in presses.into_iter().enumerate() { + match resolve(&client, event) { + KeyOutcome::Local(cmds) => { + for cmd in &cmds { + client.fold(cmd); + } + } + other => panic!("press {press}: the client must answer {event:?}, got {other:?}"), + } + reduce_key(&mut app, event); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "press {press} ({event:?}): client and host disagree" + ); + assert_eq!( + client.view.cursor().selected, + (want_row, 0), + "press {press} ({event:?}): not the row the TUI's page rule names" + ); + } +} + /// The same for `G`, the key `GridExtent` exists for: it is *defined* as "the /// last row", so a window-only client would have nothing to jump to. #[test] diff --git a/harness/tests/help.spec.js b/harness/tests/help.spec.js index 54b9c961..6a2893e6 100644 --- a/harness/tests/help.spec.js +++ b/harness/tests/help.spec.js @@ -132,4 +132,30 @@ test.describe("the help overlay draws the shared content", () => { await expect(rows_.last().locator("td.selected")).toHaveCount(1); await expect(page.locator("#grid tbody td.selected")).toHaveCount(1); }); + + // improvise-1c5m. Below the width at which the app bar wraps, `#help` covers + // `#hint`, and Welcome itself says nothing about paging. The contract: the + // mode's own hint is UNCOVERED somewhere while help is open. + test("a narrow window still shows how to page through help", async ({ page }) => { + await page.setViewportSize({ width: 600, height: 800 }); + await openHelp(page); + + // The string is the page's own `#hint`, i.e. `hint_line(Help)`; this spec + // spells no keys. Asserting it names paging keeps an empty hint out. + const hint = (await page.locator("#hint").textContent()).trim(); + expect(hint).toContain("next-page"); + + // An element showing exactly that string, topmost at its own centre: the + // overlay covering `#hint` is precisely what `elementFromPoint` sees. + const uncovered = await page.evaluate((text) => + [...document.querySelectorAll("body *")].some((node) => { + if (node.children.length > 0 || node.textContent.trim() !== text) return false; + const r = node.getBoundingClientRect(); + if (r.width === 0 || r.height === 0) return false; + const top = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2); + return top !== null && (top === node || node.contains(top)); + }), + hint); + expect(uncovered, "the paging hint is under the help overlay").toBe(true); + }); }); diff --git a/web/app.js b/web/app.js index 22e94a34..31289ce1 100644 --- a/web/app.js +++ b/web/app.js @@ -348,7 +348,13 @@ function paintHelp(model) { } } - el.help.replaceChildren(tabs, body); + // The mode's own hint again, as the TUI popup's footer: once the app bar + // wraps, the overlay covers `#hint` (improvise-1c5m). + const footer = document.createElement("p"); + footer.className = "help-footer"; + footer.textContent = model.hint; + + el.help.replaceChildren(tabs, body, footer); } /** The page bar: which item each fixed category is fixed to. Without it a diff --git a/web/style.css b/web/style.css index 88ed64cc..6d02e7d1 100644 --- a/web/style.css +++ b/web/style.css @@ -736,6 +736,17 @@ td.total { font-weight: 700; } +/* Sticky, so the paging keys stay in view however long the page scrolls. */ +.help-footer { + position: sticky; + bottom: -1.25rem; + margin: 0.75rem 0 0; + padding: 0.5rem 0; + border-top: 1px solid var(--line); + background: var(--bg); + color: var(--dim); +} + .help-body { display: grid; grid-template-columns: 16rem 1fr;