diff --git a/context/code/clients.md b/context/code/clients.md index c9daea61..b535916a 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -891,8 +891,23 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it. broadcasts nothing, so it reaches this user and no one else. This argument arrived as `edit_or_drill`'s, i.e. about `i`/`a` on an aggregate, and for a while it read as doctrine for `>` while `>` was still silent. Both doors now - refuse, and **they refuse with different sentences on purpose**: "this key is - not built yet" and "this cell is not yours to write" are different facts. + refuse, and **they name the same missing feature in the same words** — + `not_yet("drill")` — because they reach the same one. In the TUI, + `EditOrDrill` hands an aggregate straight to `DrillIntoCell`, so an aggregate + *is* writable there, by drilling; `i` on it is drill by another door. `i`'s + sentence adds only *why* it drilled rather than edited ("an aggregate is + edited by drilling in — …"), and `>` still says nothing about aggregates, + because it is refused whatever the cell is. + + This **reverses** what this bullet said until improvise-s8h7: that the two + refused with different sentences on purpose, "this key is not built yet" + against "this cell is not yours to write". The second half was false about + the application — nothing refuses a write to an aggregate; the TUI drills — + and the separate wording hid from a browser user that `i` and `>` were one + missing feature. The help page's `edit-or-drill` caption carried the same + error, calling `i` only "Edit current cell"; it now says it drills into an + aggregate, which is true in both front ends because the browser's refusal + names drill. Delivering drill is `improvise-obtu`'s, not this bullet's. - **The commit's write is not spelled again here.** It is `Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard commit and a browser mouse commit cannot disagree about what `""` or `"42"` diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fd39ad65..e130266a 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1389,28 +1389,74 @@ happened* passes against a page that has stopped listening, so it presses `i` afterwards and requires `#status` to change. Without that, this file would be folklore with a green tick. -### the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons +### an_aggregate_edit_names_the_same_missing_drill_as_the_drill_key -The Rust half of the interim, and the assertion that carries the whole weight. -Two keys on the same aggregate cell must both refuse and must **not** refuse -with the same sentence: `>` says drill is unavailable, `i` says drilling into an -aggregate needs the host. Collapsing them into one message would be the -tempting tidy-up and would lose the distinction between "this key does nothing -here yet" and "this cell is not one you may write". +improvise-s8h7. Until it, this test was +`the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons` and asserted +the opposite: that `i` and `>` on an aggregate must refuse with *different* +sentences, "this key does nothing here yet" against "this cell is not one you +may write". The second half was false about the application. `EditOrDrill` +hands an aggregated cell to `DrillIntoCell`, so in the TUI `i` on an aggregate +**is** drill — nothing refuses the write, it drills — and the browser's +separate wording ("drilling into an aggregate needs the host") hid that `i` and +`>` are one missing feature. The reasoning is `context/code/clients.md`. -**The injection, and why it is not the obvious one.** Replacing `>`'s sentence -with the *aggregate* sentence looks like it should fail on the first assertion — -and does not: that sentence contains both "drill" and "browser", so it clears -the substring check. What fires is the anti-aggregate assertion lower down. The -claim that the second assertion is the load-bearing one was doubted on review, -tested, and confirmed; record it here because the first assertion reads like the -discriminating one and is not. +So the test now asserts four things on one aggregate fixture: `>` names drill +and the browser; `>` does **not** mention aggregates (it is refused whatever the +cell is); `i`'s sentence **contains** `>`'s sentence verbatim; and `i`'s +sentence mentions "aggregate", so it still says why it drilled rather than +edited. The fixture is `client_on(CellId::Aggregate(..))` rather than the old +empty `in_mode` state, because `i` needs a cell under the cursor to reach the +`Aggregate` arm at all — on an empty grid it answers nothing and only `>` was +ever exercised. -The harness sibling (`drill.spec.js`) asserts the same distinction in a browser, -but its `not.toContainText("aggregate")` is **decoration under the only -injection that exists** — Playwright throws at the positive assertion above it -first, so that line is never reached. The Rust assertion is the guard; the -harness one is end-to-end confirmation that the string survives the wire. +**Three injections, each failing at a different assertion.** Run, not reasoned: + +- `i` says only `not_yet("drill")` — the "why" dropped. Fails at the last + assertion, "and `i` must still say why". This is the tempting over-tidy fix. +- `>`'s `declined` subject becomes "drilling into an aggregate". Fails at the + anti-aggregate assertion on `>`. Note that `i` still contains `>`'s *old* + sentence here, because `edit_or_drill` calls `not_yet("drill")` itself rather + than going through `declined` — so the containment assertion alone would not + have caught a `>` that started blaming the cell. +- `i` paraphrases ("… drill needs the host"). Fails at the containment + assertion: the same feature named in different words is exactly what the + ticket reported. + +The harness sibling (`drill.spec.js`) asserts the same wording in a browser. +Its `not.toContainText("aggregate")` on `>` is still **decoration under the +only injection that exists** — Playwright throws at the positive assertion +above it first — so the Rust assertion is the guard and the harness is +end-to-end confirmation that the string survives the wire. + +### the_edit_key_row_says_it_drills_into_an_aggregate + +improvise-s8h7, the help-page half. The `edit-or-drill` row on the Editing page +read "Edit current cell", while `i`/`a` on an aggregate drill in the TUI and +name drill in the browser. The row is found by its **keys** — the labels +`keys_for(Normal, "edit-or-drill", ["editing"])` returns — not by its caption, +and exactly one row must carry them, so a caption fix that also moved or +duplicated the row fails on the `let [..]` pattern rather than passing on +whichever copy was found first. Failed at HEAD on the caption; the injection +"Edit current cell, or drill" (drill named, aggregate not) fails it too, since +`>` is also drill and the row must say *when* `i` drills. + +### only_the_edit_key_is_captioned_as_editing_the_cell + +Found while fixing improvise-s8h7, not predicted by it. The row below the edit +key read `Enter Edit current cell (same as i)`, but Normal-mode Enter is bound +to `enter-advance` alone — not a `Binding::Sequence` — and `EnterAdvance` only +moves the cursor down, wrapping to the next column. The test takes every help +row captioned "Edit current cell" and requires its keys to be a subset of +`edit-or-drill`'s; at HEAD it failed naming `["Enter"]`. The fix moved the row +to the Navigation page's "Cursor movement" section with a caption that says +what it does. + +Its reach is stated rather than implied: it checks the one phrase that was +false. A caption that misdescribes Enter in *other* words passes it: a caption +is prose, and nothing checks it against the command it documents. That is a +neighbour of improvise-wk18, not the same gap — wk18 is about `literal()` rows +whose *keys* go unchecked; here the keys were right and the words were wrong. ### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 8143ec92..724a373c 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -439,6 +439,12 @@ fn navigation() -> Vec { Some(HelpLine::Heading("Cursor movement")), Some(HelpLine::Blank), keys(n, "move-selection", None, "Move one cell"), + keys( + n, + "enter-advance", + None, + "Move down, wrapping to the next column", + ), Some(literal("gg", "Jump to first row")), keys(n, "jump-last-row", None, "Jump to last row"), keys(n, "jump-first-col", None, "Jump to first column"), @@ -513,8 +519,12 @@ fn editing() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("Entering edit mode")), Some(HelpLine::Blank), - keys(n, "edit-or-drill", Some(&["editing"]), "Edit current cell"), - keys(n, "enter-advance", None, "Edit current cell (same as i)"), + keys( + n, + "edit-or-drill", + Some(&["editing"]), + "Edit current cell (drill into an aggregate)", + ), keys( e, "clear-buffer", @@ -950,6 +960,58 @@ mod tests { "an undescribed name falls back to itself rather than to silence" ); } + + fn rows() -> Vec<(Vec, &'static str)> { + pages() + .into_iter() + .flat_map(|page| page.lines) + .filter_map(|line| match line { + HelpLine::Keys { keys, description } => Some((keys, description)), + _ => None, + }) + .collect() + } + + fn labels_of(command: &str, args: Option<&[String]>) -> Vec { + keys_for(&ModeKey::Normal, command, args) + .iter() + .map(|(c, m)| key_label(c, *m)) + .collect() + } + + /// improvise-s8h7: `i` on an aggregate drills in the TUI, so the row + /// that documents it must say so. See `context/test-evidence/clients.md`. + #[test] + fn the_edit_key_row_says_it_drills_into_an_aggregate() { + let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); + let found: Vec<_> = rows().into_iter().filter(|(k, _)| *k == edit).collect(); + let [(_, description)] = &found[..] else { + panic!("expected one help row keyed {edit:?}, got {found:?}"); + }; + assert!( + description.contains("drill") && description.contains("aggregate"), + "{edit:?} drills into an aggregate and the row must say so: {description}" + ); + } + + /// improvise-s8h7: Normal's Enter is `enter-advance`, which only moves. + /// See `context/test-evidence/clients.md`. + #[test] + fn only_the_edit_key_is_captioned_as_editing_the_cell() { + let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); + let claims: Vec<_> = rows() + .into_iter() + .filter(|(_, d)| d.contains("Edit current cell")) + .collect(); + assert!(!claims.is_empty(), "the check needs a row to check"); + for (keys, description) in claims { + assert!( + keys.iter().all(|k| edit.contains(k)), + "{keys:?} is captioned {description:?} but only {edit:?} edit" + ); + } + } + /// Not an assertion: a printout for a human to read once. `cargo test -p /// improvise-protocol eyeball -- --nocapture --ignored`. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0bece83a..be549bf3 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -748,9 +748,10 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { }, Command::ChangeMode(mode), ]), - CellId::Aggregate(_) => Some(vec![Command::SetStatus( - "drilling into an aggregate needs the host — not in the browser yet".to_string(), - )]), + CellId::Aggregate(_) => Some(vec![Command::SetStatus(format!( + "an aggregate is edited by drilling in — {}", + not_yet("drill") + ))]), CellId::Derived { target } => Some(vec![Command::SetStatus(format!( "{target} is computed by a formula — edit the formula, not the cell" ))]), @@ -2435,28 +2436,38 @@ mod tests { ); } - /// The advertised key must say what is actually true. Two refusals, two - /// subjects — see `context/test-evidence/clients.md`. + /// `i` on an aggregate IS drill in the TUI, so both doors name one missing + /// feature; only `i` says why. See `context/test-evidence/clients.md`. #[test] - fn the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons() { - let mut state = in_mode(AppMode::Normal); - state.grid.extent = GridExtent { rows: 2, cols: 2 }; - state.window = (2, 2); - - let KeyOutcome::Local(drill) = resolve(&state, ev(KeyCode::Char('>'))) else { - panic!("`>` is advertised in the hint line; it must answer"); - }; - let [Command::SetStatus(said)] = &drill[..] else { - panic!("expected one status, got {drill:?}"); + fn an_aggregate_edit_names_the_same_missing_drill_as_the_drill_key() { + let aggregate = CellId::Aggregate( + serde_json::from_str("\"aa4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + ); + let state = client_on(aggregate); + let said = |key| match resolve(&state, ev(key)) { + KeyOutcome::Local(commands) => match &commands[..] { + [Command::SetStatus(said)] => said.clone(), + other => panic!("expected one status, got {other:?}"), + }, + other => panic!("{key:?} is advertised; it must answer, got {other:?}"), }; + let (drill, edit) = (said(KeyCode::Char('>')), said(KeyCode::Char('i'))); assert!( - said.contains("drill") && said.contains("browser"), - "the status must name drill and say it is the browser that lacks it: {said}" + drill.contains("drill") && drill.contains("browser"), + "the status must name drill and say it is the browser that lacks it: {drill}" ); assert!( - !said.contains("aggregate"), - "a sentence about aggregates is TRUE-SOUNDING about the wrong thing — \ - `>` is refused whatever the cell is: {said}" + !drill.contains("aggregate"), + "`>` is refused whatever the cell is, so it must not blame the cell: {drill}" + ); + assert!( + edit.contains(&drill), + "`i` reached the same missing feature and must name it the same way: \ + {edit:?} does not contain {drill:?}" + ); + assert!( + edit.contains("aggregate"), + "and `i` must still say why it drilled rather than edited: {edit}" ); } diff --git a/harness/tests/drill.spec.js b/harness/tests/drill.spec.js index d38124c3..c1c12039 100644 --- a/harness/tests/drill.spec.js +++ b/harness/tests/drill.spec.js @@ -36,12 +36,14 @@ async function anAggregateGrid(page) { test.describe("drill is refused in the browser, not broken", () => { // The honest half: `i`/`a` on an aggregate declines OUT LOUD, because // `edit_or_drill` answers the `CellId::Aggregate` arm with a status. - test("editing an aggregate says drill needs the host", async ({ page }) => { + test("editing an aggregate says it is edited by drilling, and drill is missing", async ({ + page, + }) => { await anAggregateGrid(page); await page.keyboard.press("i"); await expect(page.locator("#toast")).toContainText( - "drilling into an aggregate needs the host", + "an aggregate is edited by drilling in — drill is not available in the browser yet", ); // And it really is a refusal: no editor opened, no mode change. await expect(page.locator("#mode")).toHaveText("NORMAL"); @@ -63,16 +65,15 @@ test.describe("drill is refused in the browser, not broken", () => { ); await expect(page.locator("#mode")).toHaveText("NORMAL"); - // The two refusals have two SUBJECTS, and conflating them would be a - // true-sounding sentence about the wrong thing — worse than silence, - // because it stops the reader looking. `>` must not talk about aggregates. + // `>` is refused whatever the cell is, so it must not blame the cell. await expect(page.locator("#toast")).not.toContainText("aggregate"); - // And `i` still gives the OTHER refusal, which is what proves the two are - // distinct rather than one message reached by two keys. + // `i` reaches the SAME missing feature (in the TUI it drills), so it names + // it the same way and adds only why — improvise-s8h7, clients.md. await page.keyboard.press("i"); + await expect(page.locator("#toast")).toContainText("an aggregate is edited by drilling in"); await expect(page.locator("#toast")).toContainText( - "drilling into an aggregate needs the host", + "drill is not available in the browser yet", ); }); });