diff --git a/context/code/clients.md b/context/code/clients.md index b535916a..995b99f4 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1491,6 +1491,19 @@ what says it is closed**. That is the shape `col_groups` already uses for "this grid has no column groups" — an absent list rather than a flag the page has to interpret alongside a mode it would then have to understand. +**That shape is only sound where "open" implies "non-empty"**, and help is the +one place it does — its content is static. `views` and `formulas` borrowed it +and were wrong: a document with no formulas opened a formula panel that built +the same `RenderModel` as a closed one, so the page hid it under a mode reading +FORMULAS (improvise-df2w). Both are now `Option>` — `None` closed, +`Some(vec![])` open and empty — one field, so the two facts cannot disagree, +where a `formula_panel_open` flag beside the rows (the `category_panel_open` +shape) would have been two. The same mistake as improvise-xg1z: an absence +spent on encoding a second fact. What the page does **not** yet draw is the +TUI's empty-state line (`(no formulas — press 'n' to add)`); that string lives +in `src/ui/formula_panel.rs`, and copying it into `render.rs` would be a second +home for it. + The rows themselves are `improvise_protocol::help::pages()`, the same ones `src/ui/help.rs` draws. `RenderHelpRow` carries a `kind` string (`heading`, `keys`, `accent`, …) which is a *semantic* name the page maps to a diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e73939f4..e8c8bc86 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1130,8 +1130,8 @@ Normal and never closes the panel, exactly as `src/draw.rs` keeps drawing it off `view_panel_open` alone. Asserting it hidden would have pinned the browser to behaviour the TUI does not have. -The gating rule itself — an empty list is what says a panel is closed, the same -rule `help` follows — is asserted in Rust as well, by +The gating rule itself — `None` is what says a panel is closed (it was an empty +list, the rule `help` follows, until improvise-df2w) — is asserted in Rust as well, by `a_closed_panel_projects_no_rows_and_an_open_one_does` and `the_tile_bar_is_always_present_and_only_its_cursor_is_modal`, because a browser test cannot distinguish "closed" from "the cache is empty" and those two tests @@ -4231,6 +4231,33 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. +### `an_open_empty_panel_is_not_a_closed_one` + +improvise-df2w. `views` and `formulas` said "closed" by being empty, so an open +formula panel over a document with no formulas built a `RenderModel` identical +to a closed one, and the page hid it while the mode read FORMULAS. The test +compares the two models whole rather than asserting a field, so it does not +depend on how the difference is spelled. + +**Reproduced:** fails at HEAD before the fix with `left == right` on the formula +half — the two models were byte-identical. The existing +`a_closed_panel_projects_no_rows_and_an_open_one_does` could not see it, because +it fills both caches before opening the panels. + +### the formula panel opens over a document with no formulas + +The browser half of improvise-df2w, in `harness/tests/contract.spec.js`, over +`harness/fixtures/tall.improv`, which has no `## Formulas` section. It asserts +**both** directions, because each rival fails a different one. + +**Reproduced:** against a bundle built from the unfixed `render.rs` and +`app.js`, fails at `#formulas` `toBeVisible()`, immediately after `#mode` reads +FORMULAS — the exact disagreement the ticket describes. + +**Fault injected:** `paintPanel` writing `host.hidden = false` (a page that +stops inferring closed and simply never hides). Fails at the launch-time +`toBeHidden()` before the panel is opened. + ## improvise-ktlk — the overlay and the commit read one buffer PR #162 (improvise-xg1z) changed what the overlay **draws** for an `Editing` diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 8025d36b..fa48a6bd 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -42,11 +42,11 @@ pub struct RenderModel { pub categories: Vec, /// Whether the panel is open, so the page does not decide. pub category_panel_open: bool, - /// The view panel's rows, **empty unless it is open** — the same rule the - /// help overlay follows, so the page asks no question about the mode. - pub views: Vec, - /// The formula panel's rows, empty unless it is open. - pub formulas: Vec, + /// The view panel's rows, **`None` when it is closed** and possibly empty + /// when open: an absence is not an empty list (improvise-df2w). + pub views: Option>, + /// The formula panel's rows, on the same terms as `views`. + pub formulas: Option>, /// The tile bar, drawn on every frame as the TUI draws it. Which tile is /// `selected` is the mode's answer, not the page's. pub tiles: Vec, @@ -341,41 +341,39 @@ fn page_axes(state: &ClientState) -> Vec { .collect() } -/// The view panel's rows while it is open. The axis line is +/// The view panel's rows, or `None` while it is closed. The axis line is /// `viewmodel::view_axis_summary`, which the TUI's own panel calls too. -fn view_rows(state: &ClientState) -> Vec { - if !state.view.view_panel_open { - return Vec::new(); - } - state - .views - .views - .iter() - .enumerate() - .map(|(i, summary)| RenderViewRow { - name: summary.name.clone(), - axes: improvise_protocol::viewmodel::view_axis_summary(summary), - active: summary.name == state.view.active_view, - selected: i == state.view.view_panel_cursor, - }) - .collect() +fn view_rows(state: &ClientState) -> Option> { + state.view.view_panel_open.then(|| { + state + .views + .views + .iter() + .enumerate() + .map(|(i, summary)| RenderViewRow { + name: summary.name.clone(), + axes: improvise_protocol::viewmodel::view_axis_summary(summary), + active: summary.name == state.view.active_view, + selected: i == state.view.view_panel_cursor, + }) + .collect() + }) } -/// The formula panel's rows while it is open. -fn formula_rows(state: &ClientState) -> Vec { - if !state.view.formula_panel_open { - return Vec::new(); - } - state - .formulas - .formulas - .iter() - .enumerate() - .map(|(i, text)| RenderFormulaRow { - text: text.clone(), - selected: i == state.view.formula_cursor, - }) - .collect() +/// The formula panel's rows, or `None` while it is closed. +fn formula_rows(state: &ClientState) -> Option> { + state.view.formula_panel_open.then(|| { + state + .formulas + .formulas + .iter() + .enumerate() + .map(|(i, text)| RenderFormulaRow { + text: text.clone(), + selected: i == state.view.formula_cursor, + }) + .collect() + }) } /// The tile bar, always. Only the *selection* is conditional, exactly as @@ -797,34 +795,48 @@ mod tests { client } - /// An empty list is what says a panel is closed — the same rule the help - /// overlay follows, so the page asks no question about the mode. + /// `None` is what says a panel is closed, so the page asks no question + /// about the mode. #[test] fn a_closed_panel_projects_no_rows_and_an_open_one_does() { let mut client = client_with_panels(); let closed = client.model(); assert!( - closed.views.is_empty(), + closed.views.is_none(), "the cache is full but the panel is shut" ); - assert!(closed.formulas.is_empty()); + assert!(closed.formulas.is_none()); client.state_mut().view.view_panel_open = true; client.state_mut().view.formula_panel_open = true; client.state_mut().view.view_panel_cursor = 1; client.state_mut().view.active_view = "Default".into(); let open = client.model(); - assert_eq!(open.views.len(), 2); + let (views, formulas) = (open.views.unwrap(), open.formulas.unwrap()); + assert_eq!(views.len(), 2); assert_eq!( - open.views[0].axes, "R:Region", + views[0].axes, "R:Region", "the axis line is the shared derivation, and _Dim is not part of it" ); - assert!( - open.views[0].active, - "active is ViewState's, not the cache's" - ); - assert!(open.views[1].selected, "and selected is the panel cursor's"); - assert_eq!(open.formulas[0].text, "Profit = Revenue - Cost"); + assert!(views[0].active, "active is ViewState's, not the cache's"); + assert!(views[1].selected, "and selected is the panel cursor's"); + assert_eq!(formulas[0].text, "Profit = Revenue - Cost"); + } + + /// Open with nothing in it is not closed: the page must be able to tell + /// them apart from the model alone (improvise-df2w). + #[test] + fn an_open_empty_panel_is_not_a_closed_one() { + let mut client = init(); + let closed = client.model(); + client.state_mut().view.formula_panel_open = true; + let formulas_open = client.model(); + assert_ne!(formulas_open, closed, "an open, empty formula panel"); + + client.state_mut().view.formula_panel_open = false; + client.state_mut().view.view_panel_open = true; + let views_open = client.model(); + assert_ne!(views_open, closed, "an open, empty view panel"); } /// The tile bar is drawn always; only the *selection* waits for the mode, diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 34dd6743..718a4121 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -70,7 +70,7 @@ test.describe("every field the render model projects reaches the screen", () => await expect(page.locator("#categories")).toBeVisible(); await page.keyboard.press("Escape"); - // `views`: empty unless open, so the page asks no question about the mode. + // `views`: `null` unless open, so the page asks no question about the mode. await expect(page.locator("#views")).toBeHidden(); await page.keyboard.press("Shift+V"); await expect(page.locator("#views li")).toHaveCount(2); @@ -356,6 +356,27 @@ test.describe("a pointer event reaches the client and its answer is painted", () }); }); +test.describe("an open panel is drawn even when it has nothing in it", () => { + // improvise-df2w: `formulas` said "closed" by being empty, so a document with + // no formulas could open the panel and never see it. `tall.improv` has none. + test("the formula panel opens over a document with no formulas", async ({ page }) => { + await page.goto("/app/index.html?doc=df2w-empty.improv&seed=tall.improv"); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await page.locator("#grid").focus(); + await expect(page.locator("#formulas")).toBeHidden(); + + await page.keyboard.press("Shift+F"); + await expect(page.locator("#mode")).toHaveText("FORMULAS"); + await expect(page.locator("#formulas")).toBeVisible(); + await expect(page.locator("#formulas li")).toHaveCount(0); + + // And closed is still closed: the other direction, which a page that + // simply never hid the panel would fail. + await page.keyboard.press("Shift+F"); + await expect(page.locator("#formulas")).toBeHidden(); + }); +}); + test.describe("the selection is on screen", () => { // improvise-jq8l: `web/app.js` called `scrollIntoView` NOWHERE, so a selection // moved past the bottom of `#grid` simply stayed there, invisible, with no way diff --git a/web/app.js b/web/app.js index 2aeed8d0..b3c0149c 100644 --- a/web/app.js +++ b/web/app.js @@ -594,12 +594,12 @@ function edit(td, row, col, seed) { }); } -/** A side panel. An empty row list is what says it is closed — the page asks - * no question about the mode, exactly as `paintHelp` does not. */ +/** A side panel. `null` is what says it is closed, and `[]` is open and empty: + * two facts, never inferred from a count (improvise-df2w). */ function paintPanel(host, rows, build) { - host.hidden = rows.length === 0; + host.hidden = rows === null; const list = document.createElement("ul"); - rows.forEach((row, index) => { + (rows ?? []).forEach((row, index) => { const li = build(row); li.dataset.index = String(index); list.appendChild(li);