diff --git a/context/code/clients.md b/context/code/clients.md index 167409fe..df31efb3 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -552,12 +552,16 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it. `text`**: the drawn text has been through `format_f64`, so a stored 1200 reads `"1,200"` under a comma-grouping format and committing it would store the *string* `"1,200"`. `tests/lockstep.rs` found exactly that. -- **Drill does not go upstream.** Every `Upstream` is a black hole, and a key - that visibly does nothing is the worst of the three answers. Drill is - session-local residue and reaching it needs a `CellKey` this client cannot - build, so the honest answer is to say so: a `SetStatus`, which is a view-half - command — `Sessions::apply` folds it into the asker's own mirror and - broadcasts nothing, so it reaches this user and no one else. +- **Drill does not go upstream — by *either* door.** Every `Upstream` is a black + hole, and a key that visibly does nothing is the worst of the three answers. + Drill is session-local residue and reaching it needs a `CellKey` this client + cannot build, so the honest answer is to say so: a `SetStatus`, which is a + view-half command — `Sessions::apply` folds it into the asker's own mirror and + 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. - **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"` @@ -587,6 +591,89 @@ applied it (`"A"`, not shift+`"a"`), and the shared keymap's `lookup_local` retries character keys without modifiers for that very reason, so keeping the flag would only make an exact binding miss. +### The pointer: `Click`, and the four rules it obeys + +A click is resolved here for the same reason a keypress is — a front end paints +and wires, it does not decide (design-principles §5). `resolve::click` takes a +`Click` naming a widget and an index, and returns the `Command`s. `web/app.js` +attaches listeners and knows nothing else; there is one entry point per widget +on `Client`, which was the user's ruling on improvise-7ybz. + +**Rule 1 — a click navigates to what was clicked, and activates it where +activation is something the client can express.** Every arm composes the same +shape: `focus` the panel, `SetPanelCursor` to the row, then whatever `Enter` +does on that row. The activation is resolved **against the clicked index, never +against the cursor**, because the `SetPanelCursor` ahead of it has not been +folded when the resolver runs — the trap that reads as "the cursor lands on +`Plan` and the document switches to `Default`", silent and wrong in the half a +user does not watch. + +The second clause of rule 1 is what makes the formula panel look inconsistent +and is in fact the rule holding. `ModeKey::ViewPanel` binds `Enter` to +`switch-view-at-cursor`; `ModeKey::FormulaPanel` binds `a`/`n`/`o` and +`d`/`Delete` and **no `Enter` at all**. So a view-row click switches and a +formula-row click only navigates. Opening the editor on a formula click would +be an activation *invented for the pointer that the keyboard does not have* — +a second derivation of what a formula row means, which is the disease §5 names. +**The guard for this must be aimed at the keymap, not at the click**: asserting +that the click emits no `FormulaEdit` only restates the author's reading, and a +reviewer demonstrated the gap by adding `Enter -> enter-mode formula-edit` to +`FormulaPanel` and watching the whole workspace stay green while the click and +the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does` +asserts the binding's absence and names `click_formula_row` as what must change +if it ever appears. + +**Rule 2 — a click is all of its intents or none of them** (improvise-hkr8). +Each `click_*` returns `Option>` and `click` does the single +`unwrap_or_default`. The case that matters is an activation the client cannot +compose: `filter_to_item_at` goes through `model`, which goes through +`BasisCache::claim`, which returns `None` until the server has published a basis +(gotcha 43). Before the fix the `None` was swallowed per-intent, so the click +kept its navigation prefix, dropped its activation, and reported success — while +the *keyboard* handled the identical `None` correctly by answering +`KeyOutcome::Upstream`. Note that `None` at the `click` level means something +different and benign: the whole click composed nothing, which is the invariant +rather than a breach of it. + +**Rule 3 — a click never mutates the document on its own.** The user's ruling on +the tile bar, and it is deliberately stated over *every* click rather than over +tiles: the first click on a tile selects it and enters `TileSelect`, and only a +second click on the **already-selected** tile cycles its axis. The cost accepted +is two clicks for the common action and a **deliberate divergence from the +keyboard**, where `Enter` cycles directly — record it as chosen, not as an +inconsistency to tidy, or someone will read the binding and "fix" the pointer to +match. What it buys is worth more than the keystroke: a tile cycle is a +`LayoutGeometry` change, which bumps `cursor_epoch` and therefore resets **every +connected client's cursor** (gotcha 24), so a single stray click used to cost +every other viewer their position. "Already selected" is `mode == TileSelect` +**and** `tile_cat_idx == index`; the index alone is not enough, because +`tile_cat_idx` is 0 before anything has selected anything and tile 0 would then +cycle on its first click. + +**Rule 4 — the shared helper is the idiom, and breaking it is how the two arms +drift.** `click_category_row` calls `filter_to_item_at`, `click_tile` calls +`tile_axis_at`, `click_view_row` calls `switch_view_at` — each is `foo_at(index)` +with the keyboard's `foo_at_cursor` calling `foo_at(cursor)`. `click_view_row` +originally re-spelled `SwitchView + ChangeMode(Normal)` inline, so a host-side +change to `SwitchViewAtCursor` was forced into the keyboard arm by +`tests/lockstep.rs` and left the click behind **silently**. A helper that returns +only the *name* rather than the whole activation is what invites that. + +### The category panel opens at launch + +`ViewState::default()` sets `category_panel_open: true` — the user's ruling, and +it is the **shared** default rather than a browser one. A browser-only default +would be the same defect as the bug improvise-wsaw fixed, wearing different +clothes: "the page ignores the flag" and "the page overrides the flag" are one +mistake. The consequence is that the **TUI** opens with the panel too, since +`src/draw.rs` draws it off the same field. + +Two tests, because either alone passes against the bug wsaw fixed: the panel is +visible with no keypress, *and* `C` still hides it. Injecting "the page ignores +the flag" fails the second; injecting "the default is false" fails the first. +A test that only asserts the sidebar is visible cannot tell the flag being +honoured from the flag being ignored. + --- ## `improvise-wasm-client` — the crate, the fold, the render model diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index a01651d2..f07176b7 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1101,14 +1101,22 @@ addressed to precisely one. That is the same argument that removed `ModelCommand::Drill` would reintroduce the bug that removal fixed. It needs the **addressed** form — ADR 001 layer 2. -**The two entry points fail differently, and that asymmetry is the finding.** -`i`/`a` on an aggregate declines **out loud**: `edit_or_drill` answers the -`CellId::Aggregate` arm with a status the user can read. But `>` -(`drill-into-cell`) is a name `resolve::step` does not handle, so it becomes +**The two entry points used to fail differently, and that asymmetry was the +finding.** `i`/`a` on an aggregate declines **out loud**: `edit_or_drill` +answers the `CellId::Aggregate` arm with a status the user can read. But `>` +(`drill-into-cell`) was a name `resolve::step` did not handle, so it became `KeyOutcome::Upstream` → `ClientFrame::Key` → `Host::handle`, whose `Key` arm is an explicit empty vec — **a black hole, not a slow path** (gotcha 47). Pressing -`>` in the browser does nothing whatsoever: no status, no mode change, nothing. -That is why this reads as "never confirmed working" rather than as a refusal. +`>` in the browser did nothing whatsoever: no status, no mode change, nothing. +That is why this read as "never confirmed working" rather than as a refusal. + +**That silence is now closed, and the closing is what this file ships.** `step` +answers `"drill-into-cell"` with a `SetStatus` saying drill is not available in +the browser yet — an interim the wording of which is load-bearing, since it must +be true both today and on the day improvise-gd28 lands. The wire vocabulary is +still absent; only the *silence* was fixed. Note for gd28: when the real drill +arrives, a leftover `"drill-into-cell"` arm in `step` would **shadow** it with +nothing failing, because a name that resolves never reaches the host. **The TUI is the control and it works**: 12 drill tests pass across `cmd/grid.rs`, `cmd/mode.rs`, `ui/effect.rs` and `ui/app.rs`. So this is @@ -1124,20 +1132,45 @@ becomes an aggregate. The precondition is then **asserted** (`td.editable` count 0) rather than assumed, so neither test can pass against a grid of Records. -**Both injections, run.** Replacing the aggregate arm with `None` — silencing -the honest refusal — fails **both** tests, and the second fails on its -*`check_the_checker` control*, which is the point: with nothing answering, the -control can no longer tell "`>` is silent" from "the page is dead", and the test -correctly refuses to pass. Adding `"drill-into-cell" => Some(vec![SetStatus(…)])` -to `step` — making `>` resolve — fails **only** the second test and leaves the +**Both injections, run** — against the tree *before* the interim, which is what +made them two faults rather than one. Replacing the aggregate arm with `None` — +silencing the honest refusal — failed **both** tests, and the second failed on +its *`check_the_checker` control*, which is the point: with nothing answering, +the control can no longer tell "`>` is silent" from "the page is dead", and the +test correctly refuses to pass. Adding `"drill-into-cell" => Some(vec![SetStatus(…)])` +to `step` — making `>` resolve — failed **only** the second test and left the first green. Two faults, two different failure signatures, neither test passing -for the other's reason. +for the other's reason. That second injection is now the shipped code, which is +why the test it broke was rewritten rather than kept. That control is load-bearing and not decoration: a test asserting *nothing 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 + +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". + +**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. + +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. + ### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked improvise-iioi, and it is the **same trap** as the category click one level @@ -1163,10 +1196,30 @@ and `d`/`Delete` to delete, and **no `Enter` at all**. So the rule yields navigation and nothing else, and that is the rule applying rather than the fix stopping half way. -The assertion is explicit that no `FormulaEdit` mode change appears, because the -tempting "improvement" is to open the editor on a click — which would be an -activation **invented** for the pointer that the keyboard does not have, and -therefore a second derivation of what a formula row does. +This test asserts the *exact* command vector, which is the whole of what it can +say. It once carried a second, explanatory `assert!` that no `FormulaEdit` mode +change appears — **which could never have been the failing assertion**, because +the `assert_eq!` above it pins the same three commands and aborts first. A +reviewer demonstrated it by injecting a `ChangeMode(FormulaEdit)` and watching +the panic land on the `assert_eq!` with the explanatory message never printed. +It was a comment wearing an `assert!`, and the argument now lives in +`context/code/clients.md` where a comment run cannot hold it. + +### the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does + +The guard that the removed assertion was pretending to be. The asymmetry — a +view-row click activates, a formula-row click does not — is a fact about the +**keymap**, and nothing tied the click to it. The reviewer proved the gap by +adding `Enter -> enter-mode formula-edit` to `ModeKey::FormulaPanel` and running +the whole workspace: **1381 passing, nothing failed**, while `click_formula_row` +still activated nothing. The click and the keyboard then disagree about what a +formula row does, silently. + +Injecting that same binding now fails this test and only this test. The failure +message names `click_formula_row` as what must change, because the person who +adds an `Enter` binding is not the person reading `resolve.rs`'s click arms. The +positive half (`ViewPanel` *does* bind `Enter`) is there so the test cannot pass +by asserting that no panel binds anything. #### harness/tests/pointer.spec.js — the two panel tests added for improvise-iioi @@ -1199,3 +1252,102 @@ one session — a false negative and, worse, a false *positive*. A path-hash default was considered and rejected: it still silently reuses when two agents share a worktree path, where an explicit opt-in cannot. CI is unaffected (it never reused) and a developer wanting the old speed sets the variable. + +### a_click_that_cannot_compose_its_activation_composes_nothing + +improvise-hkr8. `click_category_row` ended with +`filter_to_item_at(state, index).unwrap_or_default()`, and that `None` arm is +not "nothing to do" — it is `BasisCache::claim` answering that the server has +published no basis yet (gotcha 43), so the document change cannot be composed. +The click therefore kept its **navigation prefix** (focus the panel, move the +cursor), dropped its activation, and reported success. The keyboard handles the +identical `None` correctly, by mapping `step(..) == None` to +`KeyOutcome::Upstream`. + +The fixture has to do two things a careless one would not. It **expands +`Region`** so that row 1 is an `Item` row — a header row activates via +`ToggleCatExpand`, which needs no basis and so cannot show the bug — and it +asserts both preconditions (`cat_tree_entry_at` is an `Item`, `claim` is `None`) +rather than assuming them, because an empty `ClientState` makes every +state-dependent refusal look like the one under test. + +Injecting the old `unwrap_or_default()` fails this test and only this test. + +### the_same_click_composes_everything_once_a_basis_is_published + +The positive arm of the one above, and it is what stops the fix from being +"return nothing more often". With a basis published, the same click composes all +six commands: open, focus, move the cursor, two `Command::Model`s and the +status. Without this, deleting the activation entirely would pass hkr8's test. + +### a_first_tile_click_selects_without_touching_the_document + +The user's ruling: a click selects, a second click cycles. The fixture publishes +a basis **deliberately**, so the absence of a `Command::Model` is a decision +rather than a client that could not compose one — pointed at an unpublished +basis this test would pass for hkr8's reason instead of its own. + +Both halves are asserted, because "the axis did not change" passes against a +page where nothing happened at all: the positive half requires `SetTileCatIdx` +plus `ChangeMode(TileSelect)`, the negative half requires no `Command::Model`. + +**Four injections, four distinct signatures.** "First click also cycles" fails +this test, `the_first_click_on_tile_zero_selects_it_like_any_other` and +`clicking_a_different_tile_selects_it_rather_than_cycling_it` — and **not** +`a_second_click_on_the_selected_tile_cycles_its_axis`. "Second click does not +cycle" fails only that last one. "`already_selected` reads the index alone" +fails only the tile-zero test. Reverting hkr8 fails only the hkr8 test. No +injection passes for another's reason. + +### the_first_click_on_tile_zero_selects_it_like_any_other + +The trap in the ruling's own wording. "A second click on the already-selected +tile" reads as an index comparison, and `tile_cat_idx` is **0 before anything +has selected anything** — so an index-only test would read tile 0 as already +selected and cycle it on its very first click, making the non-mutation +guarantee true of every tile but one. The condition is `mode == TileSelect` +**and** `tile_cat_idx == index`. + +### clicking_a_different_tile_selects_it_rather_than_cycling_it + +The other half of the same trap, from the opposite side: without it, "a click +does not mutate" would hold only for the first click of a session, since every +click after the first is a "second click" in the loose reading. Clicking A then +B must select B. + +### a_fresh_session_opens_with_the_category_panel + +The user's ruling that the category panel is visible at launch, implemented as +the **shared** `ViewState::default()` rather than a browser special case — a +browser-only default is "the page overrides the flag", which is the same defect +as the "the page ignores the flag" bug improvise-wsaw fixed. The consequence, +accepted deliberately, is that the TUI opens with the panel too. + +The test also asserts the *other two* panels stay shut, so it cannot pass +against a default that simply opens everything. + +Two existing tests had to change, and both were pinning the old decision by +**inheriting** it rather than stating it. +`panel_focus_cycles_from_the_focused_panel_not_the_first` is about skipping a +*closed* panel and took "the category panel is closed" from the default; it now +spells that precondition. `with_categories()` is used by tests about a click +*opening* the panel, so it now shuts it explicitly. Neither is the decision +being re-made — the decision is re-made only in the harness, deliberately. + +#### harness/tests/pointer.spec.js — the launch state and the tile ruling + +Two states, one mechanism: the sidebar is visible with no keypress, **and** `C` +still hides it. Either alone passes against the bug wsaw fixed. Proved by +injection in a real browser: setting `el.categories.hidden = false` (the page +ignoring the flag) fails on `toBeHidden`; setting the shared default back to +`false` fails on `toBeVisible`. Different signatures, different halves. + +Note the first `C` now **focuses** rather than opens, since the panel is already +open — `toggle-panel-and-focus` closes only when the panel is both open and +focused — so the test presses it twice and the hide is the second press. + +The tile tests were rewritten rather than adjusted, because the previous one +asserted the behaviour the ruling reverses; its failure against the new code was +itself the injection. The first-click test waits out a timeout before asserting +the axis did not move, since a cycle would arrive asynchronously and an +instantaneous assertion would pass against a slow one. diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 2ccac1ef..8ba13e21 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -372,7 +372,7 @@ impl Default for ViewState { search_query: String::new(), search_mode: false, formula_panel_open: false, - category_panel_open: false, + category_panel_open: true, view_panel_open: false, cat_panel_cursor: 0, view_panel_cursor: 0, diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index bda2c3ea..253974ff 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -82,57 +82,52 @@ pub enum Click { Cell { row: usize, col: usize }, } -/// One rule for all three widgets: **go there, then do what `Enter` does -/// there.** Why that rather than three answers: `context/code/clients.md`. +/// One rule for all the widgets: **go there, then do what `Enter` does +/// there.** Why that rather than one answer each: `context/code/clients.md`. pub fn click(state: &ClientState, target: Click) -> Vec { - match target { + // `None` here means the whole click composed nothing, which is the + // invariant rather than a breach of it (improvise-hkr8) — clients.md. + let resolved = match target { Click::CategoryRow(index) => click_category_row(state, index), Click::ViewRow(index) => click_view_row(state, index), Click::FormulaRow(index) => click_formula_row(state, index), Click::Tile(index) => click_tile(state, index), Click::Cell { row, col } => click_cell(state, row, col), - } + }; + resolved.unwrap_or_default() } /// Focus the panel, move its cursor, then switch — what `V` then `Enter` do, /// in one gesture. `switch-view-at-cursor` is the activation. -fn click_view_row(state: &ClientState, index: usize) -> Vec { - let Some(summary) = state.views.views.get(index) else { - return Vec::new(); - }; +fn click_view_row(state: &ClientState, index: usize) -> Option> { let mut commands = focus(state, Panel::View); commands.push(Command::SetPanelCursor { panel: Panel::View, cursor: index, }); // Off `index`, never the cursor: `SetPanelCursor` is not folded yet. - commands.push(Command::SwitchView(summary.name.clone())); - commands.push(Command::ChangeMode(AppMode::Normal)); - commands + commands.extend(switch_view_at(state, index)?); + Some(commands) } /// The formula panel binds **no** `Enter`, so the rule yields navigation and /// nothing else. That is the rule applying, not an omission — clients.md. -fn click_formula_row(state: &ClientState, index: usize) -> Vec { - if state.formulas.formulas.get(index).is_none() { - return Vec::new(); - } +fn click_formula_row(state: &ClientState, index: usize) -> Option> { + state.formulas.formulas.get(index)?; let mut commands = focus(state, Panel::Formula); commands.push(Command::SetPanelCursor { panel: Panel::Formula, cursor: index, }); - commands + Some(commands) } /// Focus the panel, move its cursor, then activate — the three things the /// keyboard needs three keystrokes for, and `filter-to-item` is the activation. -fn click_category_row(state: &ClientState, index: usize) -> Vec { +fn click_category_row(state: &ClientState, index: usize) -> Option> { // A click past the last row is a click on the panel's empty space, which // is not a row and must not move the cursor onto one. - if cat_tree_entry_at(state, index).is_none() { - return Vec::new(); - } + cat_tree_entry_at(state, index)?; let mut commands = focus(state, Panel::Category); commands.push(Command::SetPanelCursor { panel: Panel::Category, @@ -140,35 +135,41 @@ fn click_category_row(state: &ClientState, index: usize) -> Vec { }); // Against `index`, never the cursor: the `SetPanelCursor` above is not // folded yet, so the cursor still reads the old row. Evidence file. - commands.extend(filter_to_item_at(state, index).unwrap_or_default()); - commands + commands.extend(filter_to_item_at(state, index)?); + Some(commands) } -/// Select the tile and cycle its axis — `TileSelect` plus what `Enter` does -/// in it. A tile the bar does not have is not a click. -fn click_tile(state: &ClientState, index: usize) -> Vec { - if state.tiles.tiles.get(index).is_none() { - return Vec::new(); +/// Select the tile; a **second** click on the already-selected one cycles its +/// axis. Why a click never mutates on its own: `context/code/clients.md`. +fn click_tile(state: &ClientState, index: usize) -> Option> { + state.tiles.tiles.get(index)?; + // Mode *and* index, because `tile_cat_idx` is 0 before anything selects + // anything — index alone would cycle tile 0 on its first click. + let already_selected = + state.view.mode == AppMode::TileSelect && state.view.tile_cat_idx == index; + match already_selected { + true => tile_axis_at(state, index, None), + false => { + let mut commands = vec![Command::SetTileCatIdx(index)]; + if state.view.mode != AppMode::TileSelect { + commands.push(Command::ChangeMode(AppMode::TileSelect)); + } + Some(commands) + } } - let mut commands = vec![Command::SetTileCatIdx(index)]; - if state.view.mode != AppMode::TileSelect { - commands.push(Command::ChangeMode(AppMode::TileSelect)); - } - commands.extend(tile_axis_at(state, index, None).unwrap_or_default()); - commands } /// Move the selection to a clicked cell. Activation is the page's inline /// editor, which is the pointer's `i` — `context/code/clients.md`. -fn click_cell(state: &ClientState, row: usize, col: usize) -> Vec { +fn click_cell(state: &ClientState, row: usize, col: usize) -> Option> { let cursor = state.view.cursor(); let GridExtent { rows, cols } = state.grid.extent; // The page paints a window; `SetSelected` is absolute (gotcha 42). let (absolute_row, absolute_col) = (row + cursor.row_offset, col + cursor.col_offset); if absolute_row >= rows || absolute_col >= cols { - return Vec::new(); + return None; } - scrolled_to(state, absolute_row, absolute_col) + Some(scrolled_to(state, absolute_row, absolute_col)) } /// Open a panel and focus it, emitting only what is not already true — so a @@ -310,13 +311,9 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op "delete-category-at-cursor" => delete_at_cat_cursor(state), // ── the view panel ─────────────────────────────────────────── - "switch-view-at-cursor" => Some(match view_at_cursor(state) { - Some(name) => vec![ - Command::SwitchView(name), - Command::ChangeMode(AppMode::Normal), - ], - None => Vec::new(), - }), + "switch-view-at-cursor" => { + Some(switch_view_at(state, state.view.view_panel_cursor).unwrap_or_default()) + } "create-and-switch-view" => { let name = format!("View {}", state.views.views.len() + 1); Some(vec![ @@ -871,6 +868,16 @@ fn view_at_cursor(state: &ClientState) -> Option { .map(|summary| summary.name.clone()) } +/// `SwitchViewAtCursor::execute`'s whole answer, at an arbitrary index. The +/// keyboard passes its cursor and a click passes the row — clients.md. +fn switch_view_at(state: &ClientState, index: usize) -> Option> { + let summary = state.views.views.get(index)?; + Some(vec![ + Command::SwitchView(summary.name.clone()), + Command::ChangeMode(AppMode::Normal), + ]) +} + /// `DeleteViewAtCursor::execute`, including the cursor step back that only /// happens away from the top of the list. fn delete_view_at_cursor(state: &ClientState) -> Option> { @@ -1042,10 +1049,28 @@ mod tests { } } + /// The user's ruling: the category panel is open at launch. It is the + /// *shared* default, so both front ends open with it — clients.md. + #[test] + fn a_fresh_session_opens_with_the_category_panel() { + let fresh = ViewState::default(); + assert!( + fresh.category_panel_open, + "a client that has pressed nothing can already see its categories" + ); + assert!( + !fresh.formula_panel_open && !fresh.view_panel_open, + "and only that one — the ruling names the category panel alone" + ); + } + // ── the pointer (improvise-7ybz) ──────────────────────────────── fn with_categories() -> ClientState { let mut state = ClientState::default(); + // Explicitly shut: these tests are about a click *opening* the panel, + // which the launch default would otherwise answer for them. + state.view.category_panel_open = false; state.fold(&Command::Project(Projection::CategoryTree( crate::CategoryTreeUpdate::Categories(vec![ crate::CategoryRow { @@ -1122,6 +1147,164 @@ mod tests { assert!(click(&ClientState::default(), Click::FormulaRow(0)).is_empty()); } + /// Two tiles and a **published basis**, so a missing `Command::Model` is + /// a decision rather than a client that could not compose one. + fn with_tiles() -> ClientState { + let mut state = ClientState::default(); + state.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![Projection::Basis(published())], + }); + state.tiles.tiles = vec![ + crate::TileEntry { + category: "Region".into(), + axis: Axis::Row, + }, + crate::TileEntry { + category: "Product".into(), + axis: Axis::Column, + }, + ]; + state + } + + /// The user's ruling: a click **selects**, it never mutates. The basis is + /// published here, so nothing stops the cycle but the rule itself. + #[test] + fn a_first_tile_click_selects_without_touching_the_document() { + let state = with_tiles(); + let commands = click(&state, Click::Tile(1)); + assert_eq!( + commands, + vec![ + Command::SetTileCatIdx(1), + Command::ChangeMode(AppMode::TileSelect), + ], + "select and enter TileSelect, and stop there" + ); + assert!( + !commands.iter().any(|c| matches!(c, Command::Model { .. })), + "a click must not change the document; the axis cycle needs a second one" + ); + } + + /// `tile_cat_idx` is 0 before anything has selected anything, so the + /// already-selected test needs the **mode** as well as the index. + #[test] + fn the_first_click_on_tile_zero_selects_it_like_any_other() { + let state = with_tiles(); + assert_eq!(state.view.tile_cat_idx, 0, "the default index is the trap"); + let commands = click(&state, Click::Tile(0)); + assert_eq!( + commands, + vec![ + Command::SetTileCatIdx(0), + Command::ChangeMode(AppMode::TileSelect), + ] + ); + assert!( + !commands.iter().any(|c| matches!(c, Command::Model { .. })), + "index alone would read tile 0 as already selected and cycle it" + ); + } + + /// The second click is on the **already-selected** tile, not merely the + /// second click of the session — otherwise the guarantee holds once. + #[test] + fn a_second_click_on_the_selected_tile_cycles_its_axis() { + let mut state = with_tiles(); + state.view.mode = AppMode::TileSelect; + state.view.tile_cat_idx = 1; + assert_eq!( + click(&state, Click::Tile(1)), + vec![ + Command::Model { + basis: state.basis.claim([]).unwrap(), + cmd: ModelCommand::CycleAxis("Product".into()), + }, + // `Column.next()` is `Page` — the *new* axis, as `Enter` says. + Command::SetStatus("Product → Page".into()), + ] + ); + } + + /// Clicking A then B selects B. Cycling B would make "a click does not + /// mutate" true only of the first click in a session. + #[test] + fn clicking_a_different_tile_selects_it_rather_than_cycling_it() { + let mut state = with_tiles(); + state.view.mode = AppMode::TileSelect; + state.view.tile_cat_idx = 0; + let commands = click(&state, Click::Tile(1)); + assert_eq!( + commands, + vec![Command::SetTileCatIdx(1)], + "already in TileSelect, so only the index moves" + ); + assert!(!commands.iter().any(|c| matches!(c, Command::Model { .. }))); + } + + /// improvise-hkr8: a click is all of its intents or none of them. With no + /// published basis the activation cannot be composed, so nothing is. + #[test] + fn a_click_that_cannot_compose_its_activation_composes_nothing() { + let mut state = with_categories(); + state.view.expanded_cats.insert("Region".into()); + assert!( + matches!( + cat_tree_entry_at(&state, 1), + Some(CatTreeEntry::Item { .. }) + ), + "row 1 is an item row, whose activation is a document change" + ); + assert!( + state.basis.claim([]).is_none(), + "and this client has been published no basis" + ); + assert!( + click(&state, Click::CategoryRow(1)).is_empty(), + "the focus and the cursor move must not survive the activation's failure" + ); + } + + /// The same click, once the basis has arrived: all four intents. + #[test] + fn the_same_click_composes_everything_once_a_basis_is_published() { + let mut state = with_categories(); + state.view.expanded_cats.insert("Region".into()); + state.fold(&Command::Project(Projection::Basis(published()))); + let basis = state.basis.claim([]).unwrap(); + assert_eq!( + click(&state, Click::CategoryRow(1)), + vec![ + Command::SetPanelOpen { + panel: Panel::Category, + open: true + }, + Command::ChangeMode(AppMode::CategoryPanel), + Command::SetPanelCursor { + panel: Panel::Category, + cursor: 1 + }, + Command::Model { + basis: basis.clone(), + cmd: ModelCommand::SetAxis { + category: "Region".into(), + axis: Axis::Page + }, + }, + Command::Model { + basis, + cmd: ModelCommand::SetPageSelection { + category: "Region".into(), + item: "North".into() + }, + }, + Command::SetStatus("Filter: Region = North".into()), + ] + ); + } + fn with_panels() -> ClientState { let mut state = ClientState::default(); state.fold(&Command::Project(Projection::ViewList(vec![ @@ -1185,11 +1368,26 @@ mod tests { }, ] ); + } + + /// The asymmetry is a fact about the keymap, so it is asserted against + /// the keymap — not against a reading of it. See `context/code/clients.md`. + #[test] + fn the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does() { + let keymaps = Keymaps::shared(); assert!( - !commands - .iter() - .any(|c| matches!(c, Command::ChangeMode(AppMode::FormulaEdit { .. }))), - "there is no Enter binding to mirror; opening an editor would be invented" + keymaps + .lookup(&ModeKey::FormulaPanel, KeyCode::Enter, KeyModifiers::NONE) + .is_none(), + "if FormulaPanel gains an Enter binding, click_formula_row must \ + activate too — the click mirrors Enter, and this is the only \ + thing tying the two together" + ); + assert!( + keymaps + .lookup(&ModeKey::ViewPanel, KeyCode::Enter, KeyModifiers::NONE) + .is_some(), + "and the view panel's click activates because its Enter does" ); } @@ -1525,9 +1723,12 @@ mod tests { /// to `enter-mode normal` inside a panel, so no key reaches them. #[test] fn panel_focus_cycles_from_the_focused_panel_not_the_first() { + // Spelled out rather than inherited: this test is about skipping a + // *closed* panel, so the default's own answer must not decide it. let open_two = |mode: AppMode| ViewState { mode, formula_panel_open: true, + category_panel_open: false, view_panel_open: true, ..ViewState::default() }; @@ -1546,8 +1747,12 @@ mod tests { vec![Command::ChangeMode(AppMode::FormulaPanel)], "and from the last, it wraps" ); + let nothing_open = ViewState { + category_panel_open: false, + ..ViewState::default() + }; assert!( - cycle_panel_focus(&ViewState::default()).is_empty(), + cycle_panel_focus(¬hing_open).is_empty(), "with nothing open there is nowhere to go" ); } diff --git a/harness/tests/pointer.spec.js b/harness/tests/pointer.spec.js index 76f756ee..686f7ede 100644 --- a/harness/tests/pointer.spec.js +++ b/harness/tests/pointer.spec.js @@ -9,11 +9,12 @@ async function openApp(page) { await page.locator("#grid").focus(); } -/// Open the category panel the way a user does, and leave the keyboard in -/// Normal — so a later click has a focus change to make. +/// Leave the panel open and the keyboard in Normal — so a later click has a +/// focus change to make. It opens at launch, so `C` here only focuses it. async function openPanelThenLeave(page) { - await page.keyboard.press("Shift+C"); await expect(page.locator("#categories")).toBeVisible(); + await page.keyboard.press("Shift+C"); + await expect(page.locator("#mode")).toHaveText("CATEGORIES"); await page.keyboard.press("Escape"); await expect(page.locator("#mode")).toHaveText("NORMAL"); await expect(page.locator("#categories")).toBeVisible(); @@ -22,17 +23,23 @@ async function openPanelThenLeave(page) { test.describe("the category sidebar obeys category_panel_open", () => { // improvise-wsaw. The flag existed from the start and app.js never read it, // so the sidebar was drawn always and `C` changed only the mode label. - test("the sidebar is hidden until C, and C hides it again", async ({ page }) => { + // The launch state is the user's later ruling; the toggle is wsaw's. Both + // are asserted, because either alone passes against the bug wsaw fixed. + test("the sidebar is open at launch, C focuses it, and C again hides it", async ({ + page, + }) => { await openApp(page); - await expect(page.locator("#categories")).toBeHidden(); + await expect(page.locator("#categories")).toBeVisible(); + await expect(page.locator("#categories li").first()).toBeVisible(); + await expect(page.locator("#mode")).toHaveText("NORMAL"); + // Open but not focused, so the first press focuses rather than closes. await page.keyboard.press("Shift+C"); await expect(page.locator("#categories")).toBeVisible(); await expect(page.locator("#mode")).toHaveText("CATEGORIES"); - await expect(page.locator("#categories li").first()).toBeVisible(); - // The same key closes it, because `toggle-panel-and-focus` closes only - // when the panel is already both open AND focused. + // And now it closes — which is the half that proves the page reads the + // flag rather than drawing the sidebar unconditionally. await page.keyboard.press("Shift+C"); await expect(page.locator("#categories")).toBeHidden(); await expect(page.locator("#mode")).toHaveText("NORMAL"); @@ -85,17 +92,37 @@ test.describe("clicking the category panel", () => { }); test.describe("clicking a tile", () => { - // The tile bar is keyboard-reachable only through `T` plus arrows; a click - // is the whole interaction, so it selects AND cycles. - test("a click selects the tile and cycles its axis", async ({ page }) => { + // The user's ruling: a click SELECTS, a second click CYCLES. Every click in + // the UI is non-mutating by default; the cost is two for the common action. + test("the first click selects the tile and changes nothing", async ({ page }) => { await openApp(page); const product = page.locator("#tiles .tile").filter({ hasText: "Product" }); await expect(product).toHaveText("Product Row"); await product.click(); + // The positive half: something did happen, so a green test is not the + // dead-page shape. `Product` is selected and the mode moved. await expect(page.locator("#mode")).toHaveText("TILES"); await expect(product).toHaveClass(/selected/); + + // And the negative half: the document did not move. Waited out rather + // than asserted instantly, since a cycle would arrive asynchronously. + await page.waitForTimeout(500); + await expect(product).toHaveText("Product Row"); + await expect( + page.locator("#grid thead th").filter({ hasText: "Espresso" }), + ).toHaveCount(0); + }); + + test("a second click on the selected tile cycles its axis", async ({ page }) => { + await openApp(page); + const product = page.locator("#tiles .tile").filter({ hasText: "Product" }); + await product.click(); + await expect(product).toHaveClass(/selected/); + + await product.click(); + // Row → Column is `Axis::next`; the page does not spell the rotation. await expect(product).toHaveText("Product Col", { timeout: 10_000 }); // And the grid moved with it, not just the label. @@ -103,6 +130,24 @@ test.describe("clicking a tile", () => { page.locator("#grid thead th").filter({ hasText: "Espresso" }).first(), ).toBeVisible(); }); + + test("clicking a different tile selects it rather than cycling it", async ({ + page, + }) => { + await openApp(page); + const product = page.locator("#tiles .tile").filter({ hasText: "Product" }); + const quarter = page.locator("#tiles .tile").filter({ hasText: "Quarter" }); + await product.click(); + await expect(product).toHaveClass(/selected/); + + await quarter.click(); + + // The guarantee has to hold for every click, not only a session's first. + await expect(quarter).toHaveClass(/selected/); + await page.waitForTimeout(500); + await expect(quarter).toHaveText("Quarter Pag"); + await expect(product).toHaveText("Product Row"); + }); }); test.describe("clicking a grid cell", () => {