From e13635638c8d75c642e734f7e68794671fb6c707 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sat, 19 Sep 2026 21:35:14 -0700 Subject: [PATCH] feat(browser): the browser reads the TUI's keymap, and `i` opens an editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the keymap move. `keys.rs`'s hand-written per-mode classifier is gone; `resolve` now looks the key up in `Keymaps::default_keymaps()` — the same table the terminal dispatches against — and what is left here is the narrow residue: turning a bound command NAME into a `Command`, for the subset a client holding no `CmdContext` can compose. That is a better seam than the one it replaces. The drift risk was "which key does what": 170 bindings, no direct test, invisible when wrong. It is now "which command names can I execute", which is a list — and `the_normal_mode_keys_that_still_need_the_host_are_these` becomes `the_command_names_that_still_need_the_host_are_these` to match, so a rebinding no longer invalidates it and a new binding for an already-executable command reaches the browser with no change here. Several gaps close by sharing rather than by code. Search-mode typing resolves locally for the first time (the table binds `append-char ["search"]`, and "search" is `ViewState::search_query` — gotcha 11's pair is mirrored and nothing else is). `Esc` resolves as the `Binding::Sequence` it is, all-or-nothing. A modified character in a text-entry mode now types, because the `AnyChar` fallback being reached for is the host's own — the old "unmodified only" rule was a hedge against a table this client no longer guesses at. And a key nothing binds is `Ignored` rather than forwarded into `Host::handle`'s empty vec. What is newly executable, and so what a user gets back: `edit-or-drill` (`i`/`a`), `commit-cell-edit`/`commit-and-advance-right` (Enter/Tab in `Editing`), `enter-mode` (`?`/F1, Esc), `enter-tile-select` (`T`), `toggle-panel-visibility` (Ctrl-F/C/V), `toggle-panel-and-focus` (F/C/V) and `transpose` (`t`) — four of which an independent browser smoke test had just found inert. `i` on an **aggregate** does not go upstream. Every upstream key is a black hole, and a keypress that visibly does nothing is the worst of the three answers, so it answers with a `SetStatus` naming drill. A view-half command is folded into the asker's own mirror and broadcast to nobody, so that message reaches this user and no one else. The write itself is not composed twice: the keyboard commit calls `socket::typed_cell_command`, the function click-to-edit already used, so Enter and a click cannot disagree about what `""` or `"42"` means. ## `CellDisplay::edit_text`, found by lockstep `tests/lockstep.rs` drove `i` through the host and the client over one grid and they disagreed by a comma: the host seeds its edit buffer from `App::display_value_at_cursor` (the stored `1200`) while the client had only `CellDisplay::text`, which has been through `format_f64` (`1,200`). Committing that stores the *string* `"1,200"` and the number is gone — display rounding fed back into the model, which design-principles §4 forbids outright. So the projection is enriched rather than the client taught to un-format: `CellDisplay::edit_text` is `display_value_of` on the wire, and it repairs **click**-to-edit too, which had been seeding its `` from the drawn text all along. ## Making the edit visible `RenderModel::editing` carries what the user has typed so far, or `None` when no edit is open. The decision — is this an editing mode, and which buffer does it type into — is made in Rust; the page draws the string in the selected cell. Without it the mode indicator would read INSERT while every keystroke vanished into a buffer nobody painted. `web/app.js` and `web/style.css` change accordingly, and are **unrun**: there is no browser here (gotcha 48). `node --check` only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH --- .../src/command/cmd/registry.rs | 20 +- crates/improvise-command/src/ui/app.rs | 9 +- crates/improvise-command/src/ui/project.rs | 9 + crates/improvise-protocol/src/lib.rs | 32 + crates/improvise-protocol/src/render.rs | 26 +- crates/improvise-wasm-client/src/keys.rs | 1228 ++++++++++++----- crates/improvise-wasm-client/src/render.rs | 62 + crates/improvise-wasm-client/src/socket.rs | 29 +- .../improvise-wasm-client/tests/lockstep.rs | 176 +++ web/app.js | 21 +- web/style.css | 7 + 11 files changed, 1257 insertions(+), 362 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index ead11991..f0e9e05b 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -4,23 +4,11 @@ use crate::ui::effect::Panel; /// Decode a mode-name string (as supplied by `enter-mode`/`edit-or-drill` /// keymap bindings) into an `AppMode`. +/// The table itself is `AppMode::from_name`, beside `AppMode` in the +/// protocol crate: the spellings are keymap vocabulary and the keymap is +/// shared, so the browser client decodes `enter-mode` from the same list. fn parse_mode_name(s: &str) -> Result { - match s { - "normal" => Ok(AppMode::Normal), - "help" => Ok(AppMode::Help), - "formula-panel" => Ok(AppMode::FormulaPanel), - "category-panel" => Ok(AppMode::CategoryPanel), - "view-panel" => Ok(AppMode::ViewPanel), - "tile-select" => Ok(AppMode::TileSelect), - "command" => Ok(AppMode::command_mode()), - "category-add" => Ok(AppMode::category_add()), - "editing" => Ok(AppMode::editing()), - "records-normal" => Ok(AppMode::RecordsNormal), - "records-editing" => Ok(AppMode::records_editing()), - "formula-edit" => Ok(AppMode::formula_edit()), - "export-prompt" => Ok(AppMode::export_prompt()), - other => Err(format!("Unknown mode: {other}")), - } + AppMode::from_name(s).ok_or_else(|| format!("Unknown mode: {s}")) } use super::cell::*; diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 48d69e2d..7765798b 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -470,7 +470,14 @@ impl App { .unwrap_or_default() } - fn display_value_of(&self, key: &CellKey) -> String { + /// The value an editor opened on `key` starts with — unformatted, and + /// so **not** `CellDisplay::text` (design-principles §4: display + /// rounding is view-only and must not feed back into the model). + /// + /// Crate-visible so `project::cell_display` can put it on the wire as + /// `CellDisplay::edit_text`; a browser that un-formatted the drawn text + /// instead would be a second derivation, and a lossy one. + pub(crate) fn display_value_of(&self, key: &CellKey) -> String { match crate::view::synthetic_record_info(key) { Some((idx, col)) => self .layout diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 7fd6db7b..3ea84702 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -265,6 +265,7 @@ fn cell_display(app: &App, row: usize, col: usize, comma: bool, decimals: u8) -> let text = layout.records_display(row, col).unwrap_or_default(); let numeric = !text.is_empty() && text.parse::().is_ok(); return CellDisplay { + edit_text: Some(text.clone()), text, numeric, error: false, @@ -282,6 +283,14 @@ fn cell_display(app: &App, row: usize, col: usize, comma: bool, decimals: u8) -> numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), id: cell_id(app, row, col), + // The *unformatted* value, which is what the TUI's own + // `display_value_at_cursor` seeds its edit buffer from. Projecting it + // rather than letting a client un-format `text` is the rule from + // gotcha 47: where a client cannot compose the command, enrich the + // projection. + edit_text: layout + .cell_key(row, col) + .map(|key| app.display_value_of(&key)), } } diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index f6d1bade..30235e1d 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -190,6 +190,38 @@ pub enum AppMode { } impl AppMode { + /// The mode a keymap's `enter-mode ` argument names. + /// + /// Lives beside `AppMode` rather than in the command layer's registry + /// because the *names* are part of the keymap vocabulary, and the keymap + /// is shared (see [`crate::keymap`]): a browser client resolving + /// `enter-mode` has to decode the same spellings the registry does, and + /// two tables would be the mirror that move existed to delete. + /// + /// Not every variant is nameable. `Quit`, `Help`, `ImportWizard`, + /// `ItemAdd` and the three panel modes that *are* nameable are exactly + /// what the registry accepted before the move, and this is that list + /// unchanged — `ItemAdd` is absent because its category is resolved from + /// the runtime mode, never from a script line (gotcha 20). + pub fn from_name(s: &str) -> Option { + match s { + "normal" => Some(Self::Normal), + "help" => Some(Self::Help), + "formula-panel" => Some(Self::FormulaPanel), + "category-panel" => Some(Self::CategoryPanel), + "view-panel" => Some(Self::ViewPanel), + "tile-select" => Some(Self::TileSelect), + "command" => Some(Self::command_mode()), + "category-add" => Some(Self::category_add()), + "editing" => Some(Self::editing()), + "records-normal" => Some(Self::RecordsNormal), + "records-editing" => Some(Self::records_editing()), + "formula-edit" => Some(Self::formula_edit()), + "export-prompt" => Some(Self::export_prompt()), + _ => None, + } + } + /// Short uppercase name shown in the title bar. pub fn label(&self) -> &'static str { match self { diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 82c0a223..7e8e8633 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -113,6 +113,25 @@ pub struct CellDisplay { /// category column renames a coordinate across the whole row, gotcha 1, /// which is a different intent from `SetCell`). pub id: Option, + /// What an editor opened on this cell should start with — the value as + /// the user last typed it, **not** as it is drawn. + /// + /// The two differ and the difference corrupts data. `text` has been + /// through `format_f64`, so a stored `1200` is drawn `1,200` under a + /// comma-grouping format; seeding an editor with that and committing it + /// parses as `CellValue::Text("1,200")`, and the number is gone. Display + /// rounding is view-only and must never be fed back into the model + /// (design-principles §4) — an editor's seed is exactly a feedback path. + /// + /// It is the projection of `App::display_value_at_cursor`, which is what + /// `EditOrDrill` seeds the TUI's buffer from, so both front ends start + /// from one string rather than each deriving one. `None` where there is + /// nothing to edit, which is the same set of positions `id` is `None` + /// for plus the cells whose value is absent. + /// + /// A browser found this: `tests/lockstep.rs` drove `i` through the host + /// and the client over the same grid and they disagreed by a comma. + pub edit_text: Option, } impl CellDisplay { @@ -124,20 +143,25 @@ impl CellDisplay { } pub fn number(s: impl Into) -> Self { + let text = s.into(); Self { - text: s.into(), + edit_text: Some(text.clone()), + text, numeric: true, error: false, id: None, } } + /// An error cell is not editable back into the model — its text is a + /// diagnostic, not a value — so it carries no `edit_text`. pub fn error(s: impl Into) -> Self { Self { text: s.into(), numeric: false, error: true, id: None, + edit_text: None, } } diff --git a/crates/improvise-wasm-client/src/keys.rs b/crates/improvise-wasm-client/src/keys.rs index 787465a2..371ab885 100644 --- a/crates/improvise-wasm-client/src/keys.rs +++ b/crates/improvise-wasm-client/src/keys.rs @@ -1,97 +1,92 @@ -//! Browser `KeyboardEvent` → improvise key, and the one question that -//! follows: can this client answer the keypress itself? +//! Browser `KeyboardEvent` → improvise key → `Command`. //! -//! # The keymap is the client's, and the server must not know it +//! # The keymap is not this module's //! -//! That is the user's ruling on improvise-bpq2, and it follows from the -//! standing principle — the model is shared, the UI is not, and a keymap is -//! UI. It buys something real: two clients may hold different bindings over -//! one document, which is impossible if the server owns the keymap. +//! It used to be. `improvise-bpq2` gave this file a hand-written classifier +//! that decided, per key and per mode, what a keypress meant — a second +//! spelling of `default_keymaps()`, kept in step by a lockstep test over a +//! hand-written list. The user's ruling is that the web frontend uses the +//! **same** keymap as the TUI, not a mirror of it, and +//! `doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)` +//! says why: two definitions held honest by a compatibility test are one +//! definition waiting to be made. //! -//! It also closed a question that had been answered the other way. A binding -//! names an **intent** resolved against a `CmdContext`; a `Command` is the -//! **resolved mutation** (gotcha 38), and a thin client holds no `Model` and -//! no `GridLayout` — so keys used to travel upstream unresolved. What made -//! that untenable is not latency. `improvise-9x6` demonstrated, in -//! `a_key_resolved_on_the_host_moves_state_no_client_can_hear`, that -//! **nothing in the projection stream carries the host's own `ViewState`**: a -//! client's slice moves only by folding `Command`s, and resolving a keypress -//! on the host emits none. Host-side resolution does not merely cost a round -//! trip, it **diverges** — and today it does not even do that, because -//! `Host::handle` answers `ClientFrame::Key` with an empty vec. An upstream -//! key is a **black hole**, not a slow path. +//! So the table moved to `improvise_protocol::keymap` and this module reads +//! it. Which key means what is now correct **by construction**: a rebinding +//! in `default_keymaps()` reaches the browser with no change here, and a new +//! binding for a command this module can already execute simply works. //! -//! So the measure of this module is how much of the keymap it can answer, -//! and every [`KeyOutcome::Upstream`] below is a key that currently *does -//! nothing at all* in a browser. The method for growing it is the ticket's: -//! walk `KeymapSet::default_keymaps()` binding by binding and ask what the -//! client would need; where the answer is a fact it does not hold, **enrich -//! the projection** rather than moving resolution back to the host. +//! # What is left here, and why it is the better boundary //! -//! # What is answered here, and what each one needed +//! A lookup yields a command **name and arguments**. Turning that into +//! something that runs needs a `CmdContext` — a `Model`, a `GridLayout`, an +//! `App` — which this client does not have. So *key → intent* is shared and +//! *intent → `Command`* is ours, for the subset we can compose. [`step`] is +//! that subset, and it is the list that replaces the old list of keys. //! -//! - **Minibuffer text entry.** The buffer a character lands in is -//! `AppMode::minibuffer().buffer_key`, which lives on the mode this client -//! already holds — and the host's keymap reads the *same* field, because -//! `TextEntry::new` takes the `AppMode` so the prompt and the keys cannot -//! name different buffers (gotcha 11, improvise-k8h). Derived from the same -//! fact, not copied from the rule. -//! - **Grid cursor movement**, which needed [`GridExtent`]. `GridCache` is -//! viewport-indexed, so a client could not tell "the last visible row" from -//! "the last row" and could not clamp; the extent is the whole grid and is -//! projected for exactly this. The *window* is the other half and is the -//! client's own fact — it is what this client told the host in its -//! `ClientFrame::Viewport`, recorded by `Client::set_viewport`. A client -//! that has not announced one resolves nothing here, which is truthful -//! rather than guessed. -//! - **Panel cursor movement**, which needed nothing. The ticket lists it as -//! a gap on the grounds that re-deriving the category flattening is how two -//! ends drift — but improvise-edp already moved that derivation into -//! `improvise_protocol::viewmodel::flatten_cat_tree`, which both ends call. -//! It is the *shared* derivation, not a second one. The formula and view -//! panels are flat lists in their own caches. -//! - **Tile cursor movement**, which needed nothing: `TileBarCache::tiles` is -//! the list `MoveTileCursor` clamps against. +//! That is a list of names rather than of keys, which is strictly better in +//! two ways: it is enumerable, and it does not go stale when a binding moves. //! -//! # Modifiers, and what the old rule was really about +//! # What this client can execute, and what each one needed //! -//! The rule used to read "this client resolves only unmodified characters", -//! justified by `KeyPattern::AnyChar` matching a `Char` under *any* -//! modifiers. That justification is narrower than the rule: `AnyChar` is -//! bound **only in the text-entry modes** (and `Any` only in `ImportWizard`). -//! In `Normal` there is no such fallback, so `Ctrl-d` has an exact binding -//! and mirroring it is a faithful subset, not a superset. The rule is -//! therefore kept where it was earned — a modified character in a text-entry -//! mode still goes upstream — and not applied where it was never true. +//! - **Text entry** (`append-char`, `pop-char`, `clear-buffer`). The buffer +//! is the binding's own argument now — no longer read off +//! `AppMode::minibuffer()` and hoped to agree with what the host's keymap +//! named. `TextEntry::new` derives one from the other (gotcha 11), so the +//! two were equal; they are now the *same string*. +//! - **Mode entry** (`enter-mode`). The name decodes through +//! `AppMode::from_name`, which moved next to `AppMode` for the same reason +//! the keymap did. +//! - **Grid, panel and tile cursors**, which needed `GridExtent` and the +//! window this client announced. Unchanged from bpq2 except that the key +//! they answer to is now the command name. +//! - **Entering an edit** (`edit-or-drill`), which needed ADR 002: the +//! branch `EditOrDrill` takes on "is this cell aggregated" is exactly the +//! `CellId` variant the server already projects. +//! - **Committing one** (`commit-cell-edit`, `commit-and-advance-right`), +//! which needed the record surrogate — again ADR 002 — and nothing else +//! for the plain-grid case. //! -//! # What is still upstream, and why +//! # What still needs the host //! -//! - **Prefix keys** (`g`, `y`, `Z`, and so everything under them: `gg`, -//! `gz`, `yy`, `ZZ`). `ViewState::transient_prefix` holds the *address* of -//! a prefix, never the sub-keymap (gotcha 13), and resolving one means -//! holding the keymap. A client that owns its keymap outright will own this -//! too; nothing here fakes it. -//! - **Anything naming a row, column, item or group.** `hide-selected-row-item` -//! needs the category a row label belongs to; `toggle-group-under-cursor` -//! needs group membership and collapsed state; `delete-formula-at-cursor` -//! needs a formula's parsed target and target category. None of the three -//! is projected, and `GridCache::row_labels` slash-joins a multi-level -//! tuple (gotcha 44), so there is nothing to take one apart with. -//! - **Anything touching storage** (`save`, `wq`, `commit-export`) or an -//! arbitrary command line (`:`). These are the host's by definition. -//! - **Backspace in `CommandMode`**, the single user of `TextEntry::rebind`: -//! it leaves the mode on an empty buffer instead of popping (gotcha 11), so -//! a client that popped locally would sit in `:` after the host had left it. -//! - **Search mode.** It is `Normal` plus a flag, and its query is its own -//! `ViewState` field rather than an entry in `buffers` (gotcha 11). +//! [`the_command_names_that_still_need_the_host_are_these`] names them, and +//! `Host::handle` answers a `Key` frame with an empty vec, so each one is a +//! binding that **does nothing at all** in a browser +//! (`doctrine(an_upstream_key_is_a_black_hole)`). The classes: +//! +//! - **Prefix keys.** `Command::SetTransientPrefix` exists and this client +//! could arm one; what it cannot do is what the *second* key then runs, +//! which is `yank`, `wq` and `toggle-col-group-under-cursor`. Arming a +//! prefix whose payload is unreachable is worse than not arming it. +//! - **Anything naming a row, column, item or group** — `hide-selected-row-item`, +//! `toggle-group-under-cursor`, `delete-formula-at-cursor`. Not projected, +//! and `GridCache::row_labels` slash-joins a multi-level tuple (gotcha 44), +//! so there is nothing to take one apart with. +//! - **Anything touching storage or an arbitrary command line.** The host's +//! by definition. +//! - **The records-mode commit.** A wide-records row is one coordinate tuple +//! and editing a category column moves every measure cell in it (gotcha 1); +//! `commit_plain_records_edit` needs `GridLayout::record_col` to know which. +//! - **Drill.** Session-local residue (gotcha 30), and the one case here that +//! does **not** go upstream silently — see [`step`]'s `edit-or-drill` arm. +use std::sync::OnceLock; + +use improvise_protocol::keymap::{Binding, Keymaps, ModeKey}; use improvise_protocol::viewmodel::flatten_cat_tree; use improvise_protocol::{ - AppMode, Command, GridExtent, KeyCode, KeyEvent, KeyModifiers, Panel, ViewState, + AppMode, CellId, Command, GridExtent, KeyCode, KeyEvent, KeyModifiers, ModelCommand, Panel, + ViewState, }; use crate::state::ClientState; +/// The one keymap, built once. +fn keymaps() -> &'static Keymaps { + static KEYMAPS: OnceLock = OnceLock::new(); + KEYMAPS.get_or_init(Keymaps::default_keymaps) +} + /// A browser `KeyboardEvent`, flattened to the fields that matter. /// /// `key` is `KeyboardEvent.key` — the printed character for a character key, @@ -156,9 +151,9 @@ impl BrowserKey { } /// Shift is deliberately dropped for character keys: the browser already - /// applied it (`"A"`, not shift+`"a"`), and the host's `lookup_local` - /// retries character keys without modifiers for that very reason. Keeping - /// the flag would only make an exact binding miss. + /// applied it (`"A"`, not shift+`"a"`), and the shared keymap's + /// `lookup_local` retries character keys without modifiers for that very + /// reason. Keeping the flag would only make an exact binding miss. fn modifiers(&self, code: KeyCode) -> KeyModifiers { let mut mods = KeyModifiers::NONE; if self.ctrl { @@ -196,15 +191,17 @@ pub enum KeyOutcome { /// frame and in this order (gotcha 36: the asker's own fold is its own; /// the host forwards only to the others). /// - /// A `Vec` because one keypress is one `Cmd` but several `Effect`s: a + /// A `Vec` because one keypress is one binding but several `Effect`s: a /// cursor move is `SetSelected` plus whichever scroll offsets follow it, - /// exactly as `viewport_effects` returns them. Never empty — a keypress - /// this client can answer with nothing is [`KeyOutcome::Ignored`]. + /// exactly as `viewport_effects` returns them, and a `Binding::Sequence` + /// is several commands to begin with. Never empty — a keypress this + /// client can answer with nothing is [`KeyOutcome::Ignored`]. Local(Vec), - /// Not resolvable from this client's state. Forwarded to the host, which - /// today answers nothing — see the module docs. + /// Bound to a command this client cannot compose. Forwarded to the host, + /// which today answers nothing — see the module docs. Upstream(KeyEvent), - /// Nothing to send. + /// Nothing to send: either nothing is bound, or what is bound resolves + /// to no change. Ignored, } @@ -224,85 +221,227 @@ impl KeyOutcome { /// Decide a keypress against this client's own state. /// -/// Pure. The dispatch is by mode, because the host's is: `ModeKey::from_app_mode` -/// picks one keymap and the keymaps do not overlap. Order matters in one -/// place — a panel's text-entry sub-mode reports both a minibuffer and a -/// panel (`AppMode::panel` counts `CategoryAdd` as the category panel), and -/// typing wins. +/// Pure. The lookup is the shared one: `ModeKey::from_app_mode` picks the +/// keymap — including the `SearchMode` override, which is `Normal` plus a +/// flag rather than a mode — and [`Keymaps::lookup`] walks parent keymaps +/// and the `AnyChar`/`Any` fallbacks exactly as the terminal's dispatch +/// does. What differs is only what happens to the **name** that comes back. pub fn resolve(state: &ClientState, event: KeyEvent) -> KeyOutcome { - let view = &state.view; - match view.mode { - // `SearchMode` is `Normal` plus a flag, so it must be tested before - // the mode and not as part of it. - _ if view.search_mode => KeyOutcome::Upstream(event), - _ if view.mode.minibuffer().is_some() => text_entry(view, event), - _ if view.mode.panel().is_some() => panel_cursor(state, event), - AppMode::Normal | AppMode::RecordsNormal => grid_cursor(state, event), - AppMode::TileSelect => tile_cursor(state, event), - // Everything else. A wildcard rather than an exhaustive list because - // upstream really is the default here, not a per-mode decision: a - // mode this client has never been taught is one it must not guess at. - _ => KeyOutcome::Upstream(event), + // `Quit` has no keymap, on purpose: `ModeKey::from_app_mode` is + // exhaustive so that a new mode without bindings is a compile error + // rather than a mode that swallows every key. + let Some(mode) = ModeKey::from_app_mode(&state.view.mode, state.view.search_mode) else { + return KeyOutcome::Ignored; + }; + // Nothing bound is not "the host might know better" — the host looks the + // key up in this same table. It is `Ignored`, and that is new: it used to + // go upstream into an empty vec. + let Some(binding) = keymaps().lookup(&mode, event.code, event.modifiers) else { + return KeyOutcome::Ignored; + }; + match binding { + // Arming a prefix is composable (`Command::SetTransientPrefix`); what + // the second key then runs is not. See the module docs. + Binding::Prefix(_) => KeyOutcome::Upstream(event), + Binding::Cmd { name, args } => match step(state, event, name, args) { + Some(commands) => KeyOutcome::of(commands), + None => KeyOutcome::Upstream(event), + }, + // All or nothing. A sequence is one keypress, and half of `Esc` — + // clearing the buffer without leaving the mode — is worse than none + // of it. Each step is resolved against the *unchanged* state, because + // the host resolves every step of a sequence against one immutable + // `CmdContext`; the commands are then folded in order. + Binding::Sequence(steps) => match sequence(state, event, steps) { + Some(commands) => KeyOutcome::of(commands), + None => KeyOutcome::Upstream(event), + }, } } -// ── minibuffer text entry ─────────────────────────────────────────────── +/// Every step of a `Binding::Sequence`, or none of them. +fn sequence( + state: &ClientState, + event: KeyEvent, + steps: &[(&'static str, Vec)], +) -> Option> { + let mut commands = Vec::new(); + for (name, args) in steps { + commands.extend(step(state, event, name, args)?); + } + Some(commands) +} -/// The rule reads `AppMode::minibuffer()`, which is the same field the -/// host's `TextEntry` keymap builder reads. -fn text_entry(view: &ViewState, event: KeyEvent) -> KeyOutcome { - let Some(minibuf) = view.mode.minibuffer() else { - return KeyOutcome::Upstream(event); - }; - // `read_buffer`/`write_buffer` treat the name "search" as the - // `search_query` field rather than an entry in `buffers`. No text-entry - // mode uses that name (`no_text_entry_mode_types_into_the_search_buffer` - // holds it), so the rule below does not need the special case — but it - // must not silently acquire it either. - if minibuf.buffer_key == "search" { - return KeyOutcome::Upstream(event); - } - let current = || { - view.buffers - .get(&minibuf.buffer_key) - .cloned() - .unwrap_or_default() - }; - let set = |value: String| { - KeyOutcome::Local(vec![Command::SetBuffer { - name: minibuf.buffer_key.clone(), - value, - }]) - }; - match event.code { - // A modified character is the one place the `AnyChar` argument still - // bites: in a text-entry mode the host's fallback catches `Ctrl-x` - // as `append-char` unless the mode binds it exactly, and mirroring - // that fallback order is not worth the round trip it saves. - KeyCode::Char(c) if event.modifiers == KeyModifiers::NONE => { - let mut value = current(); +/// The subset of command names this client can turn into `Command`s. +/// +/// `None` means "the host's", and every `None` is a dead binding in a +/// browser until something projects what it needs. Adding a name here is the +/// whole of growing what the browser can do. +/// +/// Each arm mirrors its `Cmd` line for line, **including where siblings +/// disagree** (`doctrine(mirror_the_cmd_line_for_line)`): `Move` emits +/// `SetSelected` even when it clamps and `MovePanelCursor` does not, and +/// tidying that into one rule would make this a second derivation. +fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Option> { + match name { + // ── the grid cursor ────────────────────────────────────────── + "move-selection" => grid_move(state, GridMove::Relative(num(args, 0)?, num(args, 1)?)), + "scroll-rows" => grid_move(state, GridMove::Relative(num(args, 0)?, 0)), + "jump-first-row" => grid_move(state, GridMove::ToStart(true)), + "jump-first-col" => grid_move(state, GridMove::ToStart(false)), + "jump-last-row" => grid_move(state, GridMove::ToEnd(true)), + "jump-last-col" => grid_move(state, GridMove::ToEnd(false)), + "page-scroll" => grid_move(state, GridMove::Page(num(args, 0)?)), + + // ── the other two cursors ──────────────────────────────────── + "move-panel-cursor" => panel_cursor(state, args.first()?, num(args, 1)?), + "move-tile-cursor" => tile_cursor(state, num(args, 0)?), + + // ── minibuffer text entry ──────────────────────────────────── + "append-char" => { + let KeyCode::Char(c) = event.code else { + return Some(Vec::new()); + }; + let mut value = read_buffer(&state.view, args.first()?); value.push(c); - set(value) + Some(vec![write_buffer(args.first()?, value)]) } - // CommandMode rebinds Backspace to a command that leaves the mode on - // an empty buffer, so it is not ours to answer. - KeyCode::Backspace - if event.modifiers == KeyModifiers::NONE - && !matches!(view.mode, AppMode::CommandMode { .. }) => - { - let mut value = current(); + "pop-char" => { + let mut value = read_buffer(&state.view, args.first()?); value.pop(); - set(value) + Some(vec![write_buffer(args.first()?, value)]) } - _ => KeyOutcome::Upstream(event), + "clear-buffer" => Some(vec![write_buffer(args.first()?, String::new())]), + + // ── mode entry ─────────────────────────────────────────────── + "enter-mode" => { + let mode = AppMode::from_name(args.first()?)?; + Some(entered(mode)) + } + + // ── panels ─────────────────────────────────────────────────── + "toggle-panel-visibility" => { + let panel = named_panel(args.first()?)?; + Some(vec![Command::SetPanelOpen { + panel, + open: !panel_open(&state.view, panel), + }]) + } + // The interactive arm of `TogglePanelAndFocus`: open-and-focus unless + // this panel is already both, in which case close and unfocus. The + // decision is made in `registry.rs` on the host and here on the + // client, from the same two `ViewState` facts. + "toggle-panel-and-focus" => { + let panel = named_panel(args.first()?)?; + let closing = panel_open(&state.view, panel) && state.view.mode.panel() == Some(panel); + Some(vec![ + Command::SetPanelOpen { + panel, + open: !closing, + }, + Command::ChangeMode(match closing { + true => AppMode::Normal, + false => panel.mode(), + }), + ]) + } + + // `EnterTileSelect` gates on `model.category_names().len() > 0`, + // which counts the **virtual** categories too — `_Index`, `_Dim` and + // `_Measure` always exist (gotcha 7), so the gate is invariantly + // true and there is nothing here to mirror it against. Re-deriving + // it from `CategoryTreeCache`, which carries the panel's rows rather + // than `category_names()`, would be a second derivation that + // disagrees on exactly the empty document. + "enter-tile-select" => Some(vec![ + Command::SetTileCatIdx(0), + Command::ChangeMode(AppMode::TileSelect), + ]), + + // ── the view's shape ───────────────────────────────────────── + // + // `TransposeAxes` also emits `mark_dirty`, which is host-local + // bookkeeping and has no wire variant (gotcha 45). The cursor reset + // is not this client's to send either: the effect calls + // `App::reset_cursor`, and `adopt_cursor_epoch` broadcasts + // `Command::ResetCursor` to *everyone* in answer (gotcha 24) — so + // sending one here would be a second, differently-addressed copy. + "transpose" => Some(vec![Command::Model { + basis: state.basis.claim([])?, + cmd: ModelCommand::TransposeAxes, + }]), + + // ── editing ────────────────────────────────────────────────── + "edit-or-drill" => edit_or_drill(state, AppMode::from_name(args.first()?)?), + "commit-cell-edit" => commit(state, AppMode::from_name(args.first()?)?, Advance::Down), + "commit-and-advance-right" => { + commit(state, AppMode::from_name(args.first()?)?, Advance::Right) + } + + _ => None, } } +/// The panel a `` argument names — `parse_panel`'s spellings, which +/// `Panel::name` is required to match (gotcha 12). +fn named_panel(name: &str) -> Option { + Panel::ALL.into_iter().find(|p| p.name() == name) +} + +fn panel_open(view: &ViewState, panel: Panel) -> bool { + match panel { + Panel::Formula => view.formula_panel_open, + Panel::Category => view.category_panel_open, + Panel::View => view.view_panel_open, + } +} + +/// One numeric argument, as the registry's interactive parsers read it. +fn num(args: &[String], i: usize) -> Option { + args.get(i)?.parse().ok() +} + +// ── the named buffers ─────────────────────────────────────────────────── +// +// `cmd::core::read_buffer`/`write_buffer` are the only code on the host that +// knows a runtime-named buffer may not be an entry in `buffers` at all +// (improvise-gdl): the name "search" is `ViewState::search_query`. The +// shared keymap binds `append-char ["search"]` in `SearchMode`, so this +// client needs the same pair — and having it is what lets search typing +// resolve here for the first time. + +fn read_buffer(view: &ViewState, name: &str) -> String { + match name { + "search" => view.search_query.clone(), + other => view.buffers.get(other).cloned().unwrap_or_default(), + } +} + +fn write_buffer(name: &str, value: String) -> Command { + match name { + "search" => Command::SetSearchQuery(value), + other => Command::SetBuffer { + name: other.to_string(), + value, + }, + } +} + +/// `EnterMode::execute`: clear the incoming mode's minibuffer, then change. +fn entered(mode: AppMode) -> Vec { + let mut commands = Vec::new(); + if let Some(mb) = mode.minibuffer() { + commands.push(write_buffer(&mb.buffer_key, String::new())); + } + commands.push(Command::ChangeMode(mode)); + commands +} + // ── the grid cursor ───────────────────────────────────────────────────── -/// How a key moves the grid cursor. A mirror of `navigation.rs`'s `MoveKind`, -/// spelled again because `improvise-command` is not on a browser's -/// dependency list and never will be. +/// A mirror of `navigation.rs`'s `MoveKind`, spelled again because +/// `improvise-command` is not on a browser's dependency list and never will +/// be. The *keys* that reach it are no longer spelled twice. #[derive(Debug, Clone, Copy)] enum GridMove { Relative(i32, i32), @@ -313,38 +452,7 @@ enum GridMove { Page(i32), } -/// The Normal-mode movement keys, as `default_keymaps()` binds them. -/// -/// `gg` (`jump-first-row`) is absent and that is not an oversight: it sits -/// behind the `g` prefix, which this client does not resolve. `Enter` -/// (`enter-advance`) is absent too — it aborts the rest of its batch at the -/// bottom-right corner, a control-flow fact about the host's effect chain -/// rather than a cursor position. -fn grid_move(event: KeyEvent) -> Option { - let plain = event.modifiers == KeyModifiers::NONE; - let ctrl = event.modifiers == KeyModifiers::CONTROL; - match event.code { - KeyCode::Char('h') | KeyCode::Left if plain => Some(GridMove::Relative(0, -1)), - KeyCode::Char('j') | KeyCode::Down if plain => Some(GridMove::Relative(1, 0)), - KeyCode::Char('k') | KeyCode::Up if plain => Some(GridMove::Relative(-1, 0)), - KeyCode::Char('l') | KeyCode::Right if plain => Some(GridMove::Relative(0, 1)), - KeyCode::Char('0') | KeyCode::Home if plain => Some(GridMove::ToStart(false)), - KeyCode::Char('$') | KeyCode::End if plain => Some(GridMove::ToEnd(false)), - KeyCode::Char('G') if plain => Some(GridMove::ToEnd(true)), - KeyCode::PageDown if plain => Some(GridMove::Page(1)), - KeyCode::PageUp if plain => Some(GridMove::Page(-1)), - // `scroll-rows`. Bound exactly, under CONTROL, with no `AnyChar` in - // this keymap to fall through to — so mirroring it is a subset. - KeyCode::Char('d') if ctrl => Some(GridMove::Relative(5, 0)), - KeyCode::Char('u') if ctrl => Some(GridMove::Relative(-5, 0)), - _ => None, - } -} - -fn grid_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { - let Some(kind) = grid_move(event) else { - return KeyOutcome::Upstream(event); - }; +fn grid_move(state: &ClientState, kind: GridMove) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let (window_rows, window_cols) = state.window; // Two shapes with nothing to say, kept apart from "not ours". An empty @@ -352,7 +460,7 @@ fn grid_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { // viewport has been sent no grid either, so there is nothing on screen // for a cursor to be in. Neither is a key the host could answer better. if rows == 0 || cols == 0 || window_rows == 0 || window_cols == 0 { - return KeyOutcome::Ignored; + return Some(Vec::new()); } let cursor = state.view.cursor(); let row_max = (rows - 1) as i32; @@ -372,7 +480,7 @@ fn grid_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { ((row + delta).clamp(0, row_max) as usize, cursor.selected.1) } }; - KeyOutcome::of(scrolled_to(state, nr, nc)) + Some(scrolled_to(state, nr, nc)) } /// `SetSelected`, plus whichever scroll offsets have to follow it to keep @@ -407,24 +515,17 @@ fn scrolled_to(state: &ClientState, nr: usize, nc: usize) -> Vec { // ── the panel cursors ─────────────────────────────────────────────────── -/// `move-panel-cursor ±1`, clamped against the same length the host -/// clamps against. +/// `MovePanelCursor::execute`, and deliberately line for line: an empty +/// panel produces nothing, a cursor past the end is clamped *before* the +/// delta is applied, and a move that lands where it started says nothing. /// /// The category panel's length is `flatten_cat_tree`, which depends on /// `expanded_cats` — per-client, which is why it is derived rather than sent, /// and why the derivation lives in `improvise-protocol` where both ends /// reach it (improvise-edp). The other two panels are flat lists. -fn panel_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { +fn panel_cursor(state: &ClientState, panel: &str, delta: i32) -> Option> { let view = &state.view; - let Some(panel) = view.mode.panel() else { - return KeyOutcome::Upstream(event); - }; - let plain = event.modifiers == KeyModifiers::NONE; - let delta: i32 = match event.code { - KeyCode::Char('j') | KeyCode::Down if plain => 1, - KeyCode::Char('k') | KeyCode::Up if plain => -1, - _ => return KeyOutcome::Upstream(event), - }; + let panel = named_panel(panel)?; let (current, max) = match panel { Panel::Formula => (view.formula_cursor, state.formulas.formulas.len()), Panel::Category => ( @@ -433,22 +534,15 @@ fn panel_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { ), Panel::View => (view.view_panel_cursor, state.views.views.len()), }; - KeyOutcome::of(moved_panel_cursor(panel, delta, current, max)) -} - -/// `MovePanelCursor::execute`, and deliberately line for line: an empty -/// panel produces nothing, a cursor past the end is clamped *before* the -/// delta is applied, and a move that lands where it started says nothing. -fn moved_panel_cursor(panel: Panel, delta: i32, current: usize, max: usize) -> Vec { if max == 0 { - return Vec::new(); + return Some(Vec::new()); } let clamped = current.min(max - 1); let new = (clamped as i32 + delta).clamp(0, (max - 1) as i32) as usize; - match new == current { + Some(match new == current { true => Vec::new(), false => vec![Command::SetPanelCursor { panel, cursor: new }], - } + }) } // ── the tile cursor ───────────────────────────────────────────────────── @@ -459,23 +553,174 @@ fn moved_panel_cursor(panel: Panel, delta: i32, current: usize, max: usize) -> V /// Unlike the panel cursor this emits even when the index does not move, /// because `MoveTileCursor` does — a fold that lands on the value already /// there is not worth a second rule. -fn tile_cursor(state: &ClientState, event: KeyEvent) -> KeyOutcome { - let plain = event.modifiers == KeyModifiers::NONE; - let delta: i32 = match event.code { - KeyCode::Char('l') | KeyCode::Right if plain => 1, - KeyCode::Char('h') | KeyCode::Left if plain => -1, - _ => return KeyOutcome::Upstream(event), - }; +fn tile_cursor(state: &ClientState, delta: i32) -> Option> { let count = state.tiles.tiles.len(); if count == 0 { - return KeyOutcome::Ignored; + return Some(Vec::new()); } let new = (state.view.tile_cat_idx as i32 + delta).clamp(0, (count - 1) as i32) as usize; - KeyOutcome::Local(vec![Command::SetTileCatIdx(new)]) + Some(vec![Command::SetTileCatIdx(new)]) } + +// ── editing a cell ────────────────────────────────────────────────────── + +/// The cell under the cursor, as this client's viewport-indexed cache holds +/// it. `None` when the selection is scrolled out of the window this client +/// was sent — which is not a state the cursor keys can produce, since they +/// scroll to follow, but is one a projection race can. +fn cell_at_cursor(state: &ClientState) -> Option<&improvise_protocol::CellDisplay> { + let cursor = state.view.cursor(); + let row = cursor.selected.0.checked_sub(cursor.row_offset)?; + let col = cursor.selected.1.checked_sub(cursor.col_offset)?; + state.grid.cell(row, col) +} + +/// `EditOrDrill::execute`, with ADR 002 answering the branch. +/// +/// The host asks whether a regular category sits on `Axis::None` and whether +/// the cursor is on a synthetic records cell. This client asks neither: the +/// server already published the answer as the `CellId` variant, and +/// `doctrine(a_demo_is_checked_against_the_type_not_the_axes)` says to read +/// the type rather than re-derive the rule. `Record` is editable, `Aggregate` +/// is a drill target. +/// +/// **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 (gotcha 30) 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. +fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { + let cell = cell_at_cursor(state)?; + match cell.id.as_ref()? { + // `edit_text`, never `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. + CellId::Record(_) => Some(vec![ + Command::SetBuffer { + name: "edit".to_string(), + value: cell.edit_text.clone().unwrap_or_default(), + }, + Command::ChangeMode(mode), + ]), + CellId::Aggregate(_) => Some(vec![Command::SetStatus( + "drilling into an aggregate needs the host — not in the browser yet".to_string(), + )]), + } +} + +/// Which way `CommitAndAdvance` moves after writing. +#[derive(Debug, Clone, Copy)] +enum Advance { + Down, + Right, +} + +/// `CommitAndAdvance::execute`, for the one case a thin client can compose. +/// +/// The host's version calls `commit_cell_value`, which forks three ways on +/// `synthetic_record_info` and `has_drill_state` (gotcha 1, gotcha 19). Two +/// of those forks need `GridLayout::record_col` and a staging map this client +/// does not have, so this covers the third: a plain grid cell the server +/// addressed with a `CellId::Record`. Anything else is the host's, and +/// [`the_command_names_that_still_need_the_host_are_these`] says so. +/// +/// The composition of the write itself 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"` means. +/// +/// After the write the mirror is line for line: +/// `change_mode(exit_mode_for(edit_mode))`, then `EnterAdvance` — which +/// **aborts the rest of the batch** at the bottom-right corner, so the +/// re-entry below is skipped and the user lands in normal mode — then +/// `EnterEditAtCursor`, which seeds the `edit` buffer from the cell it +/// landed on. The trailing `clear-buffer` that the keymap welds to every +/// commit then empties that buffer again (gotcha 11); that is what the TUI +/// does and mirroring it is the point. +fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option> { + let cursor = state.view.cursor(); + let (window_rows, window_cols) = state.window; + if window_rows == 0 || window_cols == 0 { + return None; + } + // Records mode is the fork this client does not have: a row is one + // coordinate tuple and a category column renames every measure cell in + // it, which needs `record_col`. + if state.view.mode.is_records() { + return None; + } + let text = read_buffer(&state.view, "edit"); + let mut commands = vec![Command::Model { + basis: state.basis.claim([record_at_cursor(state)?])?, + cmd: crate::socket::typed_cell_command(cell_at_cursor(state)?.id.as_ref()?, &text)?, + }]; + commands.push(Command::ChangeMode(AppMode::Normal)); + + let GridExtent { rows, cols } = state.grid.extent; + let row_max = rows.saturating_sub(1); + let col_max = cols.saturating_sub(1); + let (row, col) = cursor.selected; + let landed = match advance { + Advance::Down if row < row_max => Some((row + 1, col)), + Advance::Down if col < col_max => Some((0, col + 1)), + // `EnterAdvance` returns `AbortChain` at the bottom-right corner, so + // the re-entry never runs and the commit exits editing. That abort is + // a fact about the host's effect chain; here it is simply the arm + // that appends nothing more. + Advance::Down => None, + // `Right` has no abort: it clamps at the rightmost column and the + // re-entry always runs. + Advance::Right => Some((row, (col + 1).min(col_max))), + }; + let Some((nr, nc)) = landed else { + return Some(commands); + }; + commands.extend(scrolled_to(state, nr, nc)); + // `EnterEditAtCursor`: seed the buffer from where we landed, then change + // mode. Reading the *new* cell out of a cache that has not yet seen the + // write is correct — it is a different cell — and a cell scrolled out of + // the window seeds empty, as `display_value_at_cursor` does off the grid. + let seeded = seed_at(state, nr, nc); + commands.push(Command::SetBuffer { + name: "edit".to_string(), + value: seeded, + }); + commands.push(Command::ChangeMode(edit_mode)); + Some(commands) +} + +/// The surrogate naming the record under the cursor, or `None` when the +/// cursor is not on a writable cell. +fn record_at_cursor(state: &ClientState) -> Option { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Record(cell) => Some(cell.record), + CellId::Aggregate(_) => None, + } +} + +/// The display text at an absolute grid position, as the cache holds it. +fn seed_at(state: &ClientState, row: usize, col: usize) -> String { + let cursor = state.view.cursor(); + let Some(r) = row.checked_sub(cursor.row_offset) else { + return String::new(); + }; + let Some(c) = col.checked_sub(cursor.col_offset) else { + return String::new(); + }; + state + .grid + .cell(r, c) + .and_then(|cell| cell.edit_text.clone()) + .unwrap_or_default() +} + #[cfg(test)] mod tests { use super::*; + use improvise_protocol::{CellDisplay, GridUpdate, Projection, RecordCell, Snapshot}; fn ev(code: KeyCode) -> KeyEvent { KeyEvent::new(code, KeyModifiers::NONE) @@ -496,10 +741,7 @@ mod tests { } /// A client in `mode` with **empty caches** — no grid, no panels, no - /// tiles, and no announced window. Everything the grid/panel/tile - /// resolvers need is therefore absent, which is what makes this the right - /// fixture for the text-entry rules and the wrong one for the cursors - /// (those are driven against a real host in `tests/lockstep.rs`). + /// tiles, and no announced window. fn in_mode(mode: AppMode) -> ClientState { ClientState { view: ViewState { @@ -551,9 +793,9 @@ mod tests { assert_eq!(shifted.to_key_event().unwrap().code, KeyCode::BackTab); } - /// Shift is already in the character, and the host's lookup retries - /// character keys without modifiers — carrying the flag can only make an - /// exact binding miss. + /// Shift is already in the character, and the shared keymap's lookup + /// retries character keys without modifiers — carrying the flag can only + /// make an exact binding miss. #[test] fn shift_is_dropped_for_characters_and_kept_for_named_keys() { let shifted_char = BrowserKey { @@ -603,7 +845,7 @@ mod tests { assert_eq!(BrowserKey::new("Unidentified").to_key_event(), None); } - // ── local vs upstream ─────────────────────────────────────────── + // ── text entry ────────────────────────────────────────────────── #[test] fn typing_a_character_in_any_text_entry_mode_resolves_locally() { @@ -623,6 +865,27 @@ mod tests { } } + /// The old classifier sent a modified character upstream in a text-entry + /// mode, because the host's `AnyChar` fallback matches under **any** + /// modifier and a hand-written rule could only be a subset of it safely. + /// Reading the same table removes the question: `Ctrl-x` falls through to + /// `AnyChar` here exactly as it does there. + #[test] + fn a_modified_character_now_types_because_the_fallback_is_the_shared_one() { + let mut state = in_mode(AppMode::editing()); + state.view.buffers.insert("edit".into(), "R".into()); + assert_eq!( + resolve( + &state, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL) + ), + KeyOutcome::Local(vec![Command::SetBuffer { + name: "edit".into(), + value: "Rx".into(), + }]) + ); + } + #[test] fn backspace_pops_locally_everywhere_except_command_mode() { for mode in text_entry_modes() { @@ -650,97 +913,229 @@ mod tests { } } + /// `Esc` is a `Binding::Sequence` — clear the buffer, then leave — and + /// this client now resolves both halves. Half of it would be worse than + /// none: a cleared buffer in a mode the user is still in. #[test] - fn a_commit_or_cancel_key_is_never_resolved_locally() { - for mode in text_entry_modes() { - let state = in_mode(mode.clone()); - for code in [KeyCode::Enter, KeyCode::Esc, KeyCode::Tab, KeyCode::Up] { - assert_eq!( - resolve(&state, ev(code)), - KeyOutcome::Upstream(ev(code)), - "{} / {code:?} needs the host", - mode.label() - ); - } - } + fn escape_clears_the_buffer_and_leaves_the_mode() { + let mut state = in_mode(AppMode::editing()); + state + .view + .buffers + .insert("edit".into(), "half typed".into()); + assert_eq!( + resolve(&state, ev(KeyCode::Esc)), + KeyOutcome::Local(vec![ + Command::SetBuffer { + name: "edit".into(), + value: String::new(), + }, + Command::ChangeMode(AppMode::Normal), + ]) + ); } - /// Local resolution may be a strict subset of the host's behaviour and - /// never a superset; `AnyChar` matches under any modifier on the host, so - /// a modified character is left to it. + /// Search typing resolves here for the first time, and only because the + /// keymap is shared: `SearchMode` binds `append-char ["search"]`, and the + /// name "search" is `ViewState::search_query` rather than an entry in + /// `buffers` (gotcha 11, improvise-gdl). The old classifier sent every + /// search key upstream into an empty vec. #[test] - fn a_modified_character_goes_upstream_even_in_a_text_entry_mode() { - let state = in_mode(AppMode::editing()); - let event = KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL); - assert_eq!(resolve(&state, event), KeyOutcome::Upstream(event)); + fn search_typing_writes_the_query_and_not_a_buffer() { + let mut state = in_mode(AppMode::Normal); + state.view.search_mode = true; + state.view.search_query = "Nor".into(); + state.grid.extent = GridExtent { rows: 9, cols: 9 }; + state.window = (9, 9); + assert_eq!( + resolve(&state, ev(KeyCode::Char('t'))), + KeyOutcome::Local(vec![Command::SetSearchQuery("Nort".into())]), + "j/t in search mode types; it must not move the cursor" + ); + assert_eq!( + resolve(&state, ev(KeyCode::Esc)), + KeyOutcome::Upstream(ev(KeyCode::Esc)), + "exit-search-mode owns the query and is the host's" + ); } - /// The keys `Normal` mode still hands to the host, named one by one so - /// that closing one of these gaps has to come here and say so. + // ── what is bound and what is not ─────────────────────────────── + + /// The command **names** this client cannot execute, one by one, so that + /// closing one of these gaps has to come here and say so. /// - /// Every one is a binding whose `Command` needs something no projection - /// carries: a prefix's sub-keymap (`g`, `y`, `Z`), the category a row - /// label belongs to (`H`), group membership (`z`), the host's `Storage` - /// (`Ctrl-s`), an arbitrary command line (`:`), or the abort semantics of - /// an effect chain (`Enter`). See the module docs. + /// This replaced a list of *keys*, and that is the whole gain of sharing + /// the keymap: a rebinding no longer invalidates the list, and a new + /// binding for an already-executable command works in a browser with no + /// change to this crate. + /// + /// Each entry is driven through the key that reaches it, because a name + /// that is not bound anywhere would otherwise sit here forever looking + /// like a gap. #[test] - fn the_normal_mode_keys_that_still_need_the_host_are_these() { - let state = in_mode(AppMode::Normal); - for event in [ - ev(KeyCode::Char('g')), - ev(KeyCode::Char('y')), - ev(KeyCode::Char('Z')), - ev(KeyCode::Char('H')), - ev(KeyCode::Char('z')), - ev(KeyCode::Char('x')), - ev(KeyCode::Char('p')), - ev(KeyCode::Char('t')), - ev(KeyCode::Char('[')), - ev(KeyCode::Char(':')), - ev(KeyCode::Enter), - KeyEvent::new(KeyCode::Char('s'), KeyModifiers::CONTROL), + fn the_command_names_that_still_need_the_host_are_these() { + let mut state = in_mode(AppMode::Normal); + state.grid.extent = GridExtent { rows: 4, cols: 4 }; + state.window = (4, 4); + for (key, name) in [ + (ev(KeyCode::Char('g')), ""), + (ev(KeyCode::Char('y')), ""), + (ev(KeyCode::Char('Z')), ""), + (ev(KeyCode::Char('H')), "hide-selected-row-item"), + (ev(KeyCode::Char('z')), "toggle-group-under-cursor"), + (ev(KeyCode::Char('x')), "clear-cell"), + (ev(KeyCode::Char('p')), "paste"), + (ev(KeyCode::Char('[')), "page-prev"), + (ev(KeyCode::Char(']')), "page-next"), + (ev(KeyCode::Char('>')), "drill-into-cell"), + (ev(KeyCode::Char('<')), "view-back"), + (ev(KeyCode::Char('/')), "search"), + (ev(KeyCode::Char('n')), "search-navigate"), + (ev(KeyCode::Char('N')), "search-or-category-add"), + (ev(KeyCode::Char('R')), "toggle-records-mode"), + (ev(KeyCode::Char('P')), "toggle-prune-empty"), + (ev(KeyCode::Tab), "cycle-panel-focus"), + (ev(KeyCode::Enter), "enter-advance"), + ( + KeyEvent::new(KeyCode::Char('s'), KeyModifiers::CONTROL), + "save", + ), + ( + KeyEvent::new(KeyCode::Char('q'), KeyModifiers::CONTROL), + "force-quit", + ), ] { assert_eq!( - resolve(&state, event), - KeyOutcome::Upstream(event), - "{:?} was resolved locally; if that is deliberate, this list \ - is where it stops being a gap", - event.code + resolve(&state, key), + KeyOutcome::Upstream(key), + "{name} ({:?}) was resolved locally; if that is deliberate, \ + this list is where it stops being a gap", + key.code ); } } + /// The four keys an independent browser smoke test found inert, plus the + /// two the editing path needs. Each asserts the *shape* of what is now + /// composed, because "resolves locally" alone would pass on a wrong + /// command. + #[test] + fn the_keys_the_smoke_test_found_inert_now_resolve() { + let state = in_mode(AppMode::Normal); + + assert_eq!( + resolve(&state, ev(KeyCode::Char('?'))), + KeyOutcome::Local(vec![Command::ChangeMode(AppMode::Help)]), + "? opens help" + ); + assert_eq!( + resolve(&state, ev(KeyCode::Char('T'))), + KeyOutcome::Local(vec![ + Command::SetTileCatIdx(0), + Command::ChangeMode(AppMode::TileSelect), + ]), + "T enters tile select" + ); + assert_eq!( + resolve(&state, ev(KeyCode::Char('F'))), + KeyOutcome::Local(vec![ + Command::SetPanelOpen { + panel: Panel::Formula, + open: true, + }, + Command::ChangeMode(AppMode::FormulaPanel), + ]), + "F opens and focuses the formula panel" + ); + assert_eq!( + resolve( + &state, + KeyEvent::new(KeyCode::Char('f'), KeyModifiers::CONTROL) + ), + KeyOutcome::Local(vec![Command::SetPanelOpen { + panel: Panel::Formula, + open: true, + }]), + "Ctrl-f shows the panel without focusing it" + ); + } + + /// `F` on an already-focused panel closes it, which is the interactive + /// arm of `TogglePanelAndFocus` and not its script arm. + #[test] + fn a_focused_panel_toggles_shut() { + let mut state = in_mode(AppMode::FormulaPanel); + state.view.formula_panel_open = true; + assert_eq!( + resolve(&state, ev(KeyCode::Char('F'))), + KeyOutcome::Local(vec![ + Command::SetPanelOpen { + panel: Panel::Formula, + open: false, + }, + Command::ChangeMode(AppMode::Normal), + ]) + ); + } + + /// `t` is a document mutation, so it carries a basis — and until one has + /// been published there is nothing truthful to claim, which is the + /// fourth `None` shape `Client::set_cell` already has (ADR 001). + #[test] + fn transpose_needs_a_published_basis() { + let state = in_mode(AppMode::Normal); + assert_eq!( + resolve(&state, ev(KeyCode::Char('t'))), + KeyOutcome::Upstream(ev(KeyCode::Char('t'))), + "with no basis this client has nothing it may claim" + ); + + let client = client_on(record_cell()); + assert_eq!( + resolve(client.state(), ev(KeyCode::Char('t'))), + KeyOutcome::Local(vec![Command::Model { + basis: client.state().basis.claim([]).unwrap(), + cmd: ModelCommand::TransposeAxes, + }]) + ); + } + /// A mode this client was never taught answers nothing itself. `Help` and /// `ImportWizard` are the two: the wizard's whole keymap is one `Any` /// binding into `HandleWizardKey`, and a half-open dialogue is addressed /// to the session that opened it (gotcha 32). #[test] fn an_untaught_mode_resolves_nothing() { - for mode in [AppMode::Help, AppMode::ImportWizard] { - let state = in_mode(mode.clone()); - for code in [KeyCode::Char('j'), KeyCode::Down, KeyCode::Enter] { - assert_eq!( - resolve(&state, ev(code)), - KeyOutcome::Upstream(ev(code)), - "{} / {code:?}", - mode.label() - ); - } + for code in [KeyCode::Char('j'), KeyCode::Down, KeyCode::Enter] { + assert_eq!( + resolve(&in_mode(AppMode::ImportWizard), ev(code)), + KeyOutcome::Upstream(ev(code)), + "the wizard is the host's" + ); } } + /// A key nothing binds is `Ignored`, not `Upstream` — and that is new. + /// This client reads the same table the host does, so "nothing is bound" + /// is a fact it now holds rather than a guess it defers. + #[test] + fn an_unbound_key_is_ignored_rather_than_sent_into_the_void() { + let state = in_mode(AppMode::Normal); + assert_eq!(resolve(&state, ev(KeyCode::F(9))), KeyOutcome::Ignored); + assert_eq!( + resolve(&state, KeyEvent::new(KeyCode::Up, KeyModifiers::ALT)), + KeyOutcome::Ignored + ); + } + /// An empty widget is not an unanswerable key: the client knows perfectly - /// well that there is nowhere to go, and says `Ignored` rather than - /// forwarding a key the host would also answer with nothing. - /// - /// The three are separate because they are three different emptinesses — - /// a grid with no rows, a panel with no entries, a tile bar with no - /// tiles — and a resolver that forgot one would fail only on that one. + /// well that there is nowhere to go. #[test] fn an_empty_widget_answers_ignored_rather_than_forwarding() { for (mode, code) in [ (AppMode::Normal, KeyCode::Char('j')), (AppMode::TileSelect, KeyCode::Char('l')), + (AppMode::FormulaPanel, KeyCode::Char('j')), ] { assert_eq!( resolve(&in_mode(mode.clone()), ev(code)), @@ -749,24 +1144,11 @@ mod tests { mode.label() ); } - // A panel cursor over an empty panel says nothing for the same - // reason `MovePanelCursor` returns no effects when `max` is zero. - assert_eq!( - resolve(&in_mode(AppMode::FormulaPanel), ev(KeyCode::Char('j'))), - KeyOutcome::Ignored - ); } /// Which widgets say nothing about a move that goes nowhere is **not a /// rule of this module** — it is whatever the `Cmd` being mirrored does, /// and the three differ. - /// - /// `Move` clamps and emits `SetSelected` regardless, so a `k` at the top - /// row still says "be at row 0". `MovePanelCursor` returns no effects - /// when the cursor does not move. `MoveTileCursor` emits regardless, like - /// `Move`. Tempting to unify — and wrong: the value of mirroring line for - /// line is that a lockstep test compares two things that were derived the - /// same way, and a tidier client is a second derivation. #[test] fn a_move_that_goes_nowhere_follows_the_command_it_mirrors() { let mut grid = in_mode(AppMode::Normal); @@ -803,45 +1185,221 @@ mod tests { ); } - /// The host's `read_buffer`/`write_buffer` route the name "search" to - /// `search_query` rather than to `buffers`. This client does not carry - /// that special case, which is only safe while no text-entry mode uses - /// the name — search is `Normal` plus a flag, not a minibuffer mode. - #[test] - fn no_text_entry_mode_types_into_the_search_buffer() { - for mode in text_entry_modes() { - assert_ne!( - mode.minibuffer().unwrap().buffer_key, - "search", - "{} would need the search special case", - mode.label() - ); - } + // ── the editing path ──────────────────────────────────────────── + + fn surrogate() -> improvise_protocol::RecordHash { + serde_json::from_str("\"fe4c8e5e9cbd578e0d388f4b2b5a7879\"").expect("a surrogate is hex") } - /// Search mode is `Normal` with a flag, so the guard above cannot fire — - /// but if a future mode carried both, the flag wins and the key goes to - /// the host that owns the query. - /// - /// Driven in two modes on purpose. `editing()` is the one the buffer - /// guard would also catch, so on its own it cannot tell the flag from the - /// buffer name; `Normal` is the mode search actually occurs in, where - /// `j` would otherwise move the cursor instead of typing into the query. + fn record_cell() -> CellId { + CellId::Record(RecordCell { + record: surrogate(), + measure: Some("Revenue".into()), + }) + } + + /// A one-by-two grid whose first cell carries `id`, with a published + /// basis naming its record — the shape a client is in when the user is + /// about to type. + fn client_on(id: CellId) -> crate::Client { + let mut first = CellDisplay::number("1200"); + first.id = Some(id); + let mut client = crate::init(); + client.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![ + Projection::Grid(GridUpdate::RowLabels(vec!["North".into(), "South".into()])), + Projection::Grid(GridUpdate::ColLabels(vec!["Revenue".into()])), + Projection::Grid(GridUpdate::Cells(vec![ + ((0, 0), first), + ((1, 0), CellDisplay::number("900")), + ])), + Projection::Grid(GridUpdate::Extent(GridExtent { rows: 2, cols: 1 })), + Projection::Basis(published()), + ], + }); + client.state_mut().window = (2, 1); + client + } + + fn published() -> improvise_protocol::Basis { + improvise_protocol::basis::published(7, 3, [(surrogate(), 4)].into_iter().collect()) + } + + /// `i` on a `Record` cell seeds the edit buffer from what is on screen + /// and enters `Editing` — `EditOrDrill`'s non-aggregated branch, in the + /// order that command emits it. #[test] - fn search_mode_always_goes_upstream() { - for mode in [AppMode::editing(), AppMode::Normal] { - let mut state = in_mode(mode.clone()); - state.view.search_mode = true; - state.grid.extent = GridExtent { rows: 9, cols: 9 }; - state.window = (9, 9); - for code in [KeyCode::Char('q'), KeyCode::Char('j')] { - assert_eq!( - resolve(&state, ev(code)), - KeyOutcome::Upstream(ev(code)), - "{} / {code:?} belongs to the query, not to this client", - mode.label() - ); + fn i_on_a_record_cell_enters_editing_seeded_from_the_cell() { + let client = client_on(record_cell()); + assert_eq!( + resolve(client.state(), ev(KeyCode::Char('i'))), + KeyOutcome::Local(vec![ + Command::SetBuffer { + name: "edit".into(), + value: "1200".into(), + }, + Command::ChangeMode(AppMode::editing()), + ]) + ); + } + + /// `a` is bound to the same command with the same argument, so it must + /// behave identically — and it does so without a second arm here, which + /// is the point of sharing the table. + #[test] + fn a_and_i_are_one_binding_with_two_keys() { + let client = client_on(record_cell()); + assert_eq!( + resolve(client.state(), ev(KeyCode::Char('a'))), + resolve(client.state(), ev(KeyCode::Char('i'))) + ); + } + + /// `i` on an aggregate says so. It must not go upstream: `Host::handle` + /// answers a `Key` frame with an empty vec, so that would be a keypress + /// the user sees do nothing at all + /// (`doctrine(an_upstream_key_is_a_black_hole)`). + #[test] + fn i_on_an_aggregate_declines_out_loud() { + let client = client_on(CellId::Aggregate( + serde_json::from_str("\"aa4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + )); + let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Char('i'))) else { + panic!("an aggregate must not be a silent no-op"); + }; + assert!( + matches!(&commands[..], [Command::SetStatus(msg)] if msg.contains("drill")), + "expected a status naming drill, got {commands:?}" + ); + } + + /// Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]`, + /// and every part of that sequence shows up here — including the + /// trailing clear that empties the buffer `EnterEditAtCursor` has just + /// seeded. That ordering looks wrong and is exactly what the TUI does + /// (gotcha 11); mirroring it line for line is the contract. + #[test] + fn enter_commits_the_cell_advances_and_re_enters_editing() { + let mut client = client_on(record_cell()); + client.state_mut().view.mode = AppMode::editing(); + client + .state_mut() + .view + .buffers + .insert("edit".into(), "42".into()); + let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + // The write, composed through the same function click-to-edit uses. + assert_eq!( + commands[0], + Command::Model { + basis: published(), + cmd: ModelCommand::SetCell { + cell: RecordCell { + record: surrogate(), + measure: Some("Revenue".into()), + }, + value: improvise_protocol::CellValue::Number(42.0), + }, } - } + ); + assert_eq!(commands[1], Command::ChangeMode(AppMode::Normal)); + assert_eq!(commands[2], Command::SetSelected { row: 1, col: 0 }); + assert!( + commands.contains(&Command::ChangeMode(AppMode::editing())), + "the advance re-enters editing on the cell it landed on: {commands:?}" + ); + assert_eq!( + commands.last(), + Some(&Command::SetBuffer { + name: "edit".into(), + value: String::new(), + }), + "the keymap welds `clear-buffer` to every commit: {commands:?}" + ); + } + + /// An empty buffer is `ClearCell`, not `SetCell` of an empty string — + /// the decision `typed()` makes, reached from the keyboard through the + /// same function the mouse reaches it through. + #[test] + fn committing_an_empty_buffer_clears_the_cell() { + let mut client = client_on(record_cell()); + client.state_mut().view.mode = AppMode::editing(); + client + .state_mut() + .view + .buffers + .insert("edit".into(), String::new()); + let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + assert!(matches!( + &commands[0], + Command::Model { + cmd: ModelCommand::ClearCell(_), + .. + } + )); + } + + /// At the bottom-right corner `EnterAdvance` returns `AbortChain`, so + /// the re-entry is skipped and the commit leaves the user in normal + /// mode. That abort is a fact about the host's effect chain and it has + /// to be mirrored, or the browser would sit in an editor on a cell the + /// terminal has left. + #[test] + fn a_commit_in_the_last_cell_exits_editing_instead_of_advancing() { + // A one-by-one grid, so the only cell *is* the corner. + let mut cell = CellDisplay::number("1200"); + cell.id = Some(record_cell()); + let mut client = crate::init(); + client.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![ + Projection::Grid(GridUpdate::RowLabels(vec!["North".into()])), + Projection::Grid(GridUpdate::ColLabels(vec!["Revenue".into()])), + Projection::Grid(GridUpdate::Cells(vec![((0, 0), cell)])), + Projection::Grid(GridUpdate::Extent(GridExtent { rows: 1, cols: 1 })), + Projection::Basis(published()), + ], + }); + client.state_mut().window = (1, 1); + client.state_mut().view.mode = AppMode::editing(); + client + .state_mut() + .view + .buffers + .insert("edit".into(), "7".into()); + + let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + assert!( + matches!(&commands[0], Command::Model { .. }), + "the write still happens: {commands:?}" + ); + assert!( + !commands.contains(&Command::ChangeMode(AppMode::editing())), + "the corner must not re-enter editing: {commands:?}" + ); + assert!( + commands.contains(&Command::ChangeMode(AppMode::Normal)), + "it drops to normal instead: {commands:?}" + ); + } + + /// Records mode is the fork this client does not have (gotcha 1), so its + /// commit goes upstream rather than writing the wrong cell. + #[test] + fn a_records_mode_commit_is_the_hosts() { + let mut client = client_on(record_cell()); + client.state_mut().view.mode = AppMode::records_editing(); + assert_eq!( + resolve(client.state(), ev(KeyCode::Enter)), + KeyOutcome::Upstream(ev(KeyCode::Enter)) + ); } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index c44d8f4c..13ef10e9 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -56,6 +56,17 @@ pub struct RenderModel { pub categories: Vec, /// Whether the panel is open, so the page does not decide. pub category_panel_open: bool, + /// What the user has typed so far, while a keyboard edit is open on the + /// selected cell — `None` when none is. + /// + /// The page draws this in place of the selected cell's own text. The + /// *decision* — is an edit open, and what is in it — is made here, + /// because it is `AppMode::is_editing()` and the `edit` buffer, and a + /// second front end must not have to know either + /// (design-principles §5). Without it a browser that resolved `i` + /// locally would change its mode indicator and then swallow every + /// keystroke invisibly. + pub editing: Option, } /// A grid row: its header label and its cells. @@ -76,6 +87,10 @@ pub struct RenderCell { /// alone. pub editable: bool, pub selected: bool, + /// What an editor opened on this cell should start with — the stored + /// value, not the formatted one. See `CellDisplay::edit_text`: seeding + /// from `text` and committing turns `1200` into the string `"1,200"`. + pub edit_text: String, } impl RenderCell { @@ -86,6 +101,7 @@ impl RenderCell { error: cell.error, editable: matches!(cell.id, Some(CellId::Record(_))), selected, + edit_text: cell.edit_text.clone().unwrap_or_default(), } } } @@ -152,6 +168,11 @@ impl ClientState { .map(|(i, entry)| RenderCatRow::of(entry, i == self.view.cat_panel_cursor)) .collect(), category_panel_open: self.view.category_panel_open, + editing: self + .view + .mode + .is_editing() + .then(|| self.view.buffers.get("edit").cloned().unwrap_or_default()), } } } @@ -374,6 +395,43 @@ mod tests { assert_eq!(model.revision, client.revision()); } + /// A keyboard edit has to be *visible*, and what is visible is a + /// decision made here rather than in the page. + /// + /// The failure this rules out is quiet: a client that resolved `i` + /// locally would change its mode label, put the typing into + /// `buffers["edit"]`, and draw the cell's old value the whole time — so + /// the user sees a keystroke do nothing, types over it, and commits + /// something they never read. `None` outside an editing mode is the + /// other half: a stale buffer must not draw an editor that is not open. + #[test] + fn a_keyboard_edit_in_progress_is_part_of_the_render_model() { + let mut client = client_with_grid(ViewState::default()); + assert_eq!( + client.model().editing, + None, + "nothing is being edited in Normal" + ); + + client + .state_mut() + .fold(&Command::ChangeMode(improvise_protocol::AppMode::editing())); + client.state_mut().fold(&Command::SetBuffer { + name: "edit".into(), + value: "12".into(), + }); + assert_eq!(client.model().editing.as_deref(), Some("12")); + + client + .state_mut() + .fold(&Command::ChangeMode(improvise_protocol::AppMode::Normal)); + assert_eq!( + client.model().editing, + None, + "leaving the mode closes the editor even with the buffer still set" + ); + } + /// The whole model crosses as JSON, so the page needs no bindings. #[test] fn the_render_model_crosses_as_json() { @@ -384,5 +442,9 @@ mod tests { assert_eq!(value["rows"][0]["cells"][0]["editable"], true); assert_eq!(value["rows"][0]["cells"][1]["editable"], false); assert_eq!(value["col_labels"][0], "Revenue"); + assert_eq!( + value["rows"][0]["cells"][0]["edit_text"], "1200", + "an editor seeds from the stored value, never from the drawn one" + ); } } diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 7dd6cb3c..27dd49e2 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -77,6 +77,30 @@ fn typed(text: &str) -> Option { } } +/// The `ModelCommand` for "the user typed `text` into this cell". +/// +/// The one composition, shared by the mouse path ([`Client::typed_cell_frame`]) +/// and the keyboard path (`keys::commit`). Writing it twice is how a browser +/// commit by Enter and a browser commit by click would come to disagree about +/// what an empty string means. +/// +/// `None` for an aggregate: `ModelCommand::SetCell` does not take that kind +/// of id, so this is the pattern failing to bind rather than a check (ADR +/// 002). +pub(crate) fn typed_cell_command( + id: &improvise_protocol::CellId, + text: &str, +) -> Option { + let improvise_protocol::CellId::Record(cell) = id else { + return None; + }; + let cell = cell.clone(); + Some(match typed(text) { + Some(value) => ModelCommand::SetCell { cell, value }, + None => ModelCommand::ClearCell(cell), + }) +} + impl Client { /// The handshake frame, and every resize after it. /// @@ -124,10 +148,7 @@ impl Client { let basis = self.state().basis.claim([cell.record])?; Some(ClientFrame::command(Command::Model { basis, - cmd: match typed(text) { - Some(value) => ModelCommand::SetCell { cell, value }, - None => ModelCommand::ClearCell(cell), - }, + cmd: typed_cell_command(&improvise_protocol::CellId::Record(cell), text)?, })) } } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index e0cea325..1c0427be 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -428,3 +428,179 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() { } assert!(client.view.tile_cat_idx > 0, "the fixture moved nothing"); } + +// ── the editing path (improvise-bpq2's second half) ───────────────────── + +/// A document whose cells are **records**, so `i` has something to edit. +/// +/// One regular category on Row, `_Measure` on Column, nothing regular on +/// None — the shape gotcha 46 built `web/demo.improv` to satisfy, and the +/// reason is the same: with a regular category on None every cell is an +/// `Aggregate` and the whole editing path is unreachable. +fn editable_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.model.set_cell( + improvise_command::model::cell::CellKey::new(vec![ + ("Region".to_string(), "North".to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]), + improvise_protocol::CellValue::Number(1200.0), + ); + let mut app = App::new(wb, None); + app.term_height = 2 + 8; + app.rebuild_layout(); + app +} + +/// [`client_of`], plus the published basis a real client folds before it is +/// allowed to compose a write (ADR 001). The tiers are the ones this test +/// never checks — the host applies the keypress directly rather than the +/// client's `Command::Model` — but `BasisCache::claim` withholds until it +/// has *something*, so a client without one resolves no commit at all. +fn client_ready_to_edit(app: &App, window: (usize, usize)) -> ClientState { + let mut client = client_of(app, window); + let records: std::collections::BTreeMap<_, _> = client + .grid + .cells + .iter() + .flatten() + .filter_map(|cell| match cell.id.as_ref() { + Some(improvise_protocol::CellId::Record(r)) => Some((r.record, 0u64)), + _ => None, + }) + .collect(); + assert!( + !records.is_empty(), + "the fixture must project at least one Record cell, or nothing here is testing editing" + ); + improvise_protocol::WidgetCache::apply( + &mut client.basis, + &improvise_protocol::basis::published(0, 0, records), + ); + client +} + +/// The headline claim for the editing half: `i`, three characters and `Esc` +/// leave the client and the host in the same mode with the same buffers. +/// +/// `Esc` rather than `Enter` for the cancel arm, because it is a +/// `Binding::Sequence` and so exercises the all-or-nothing rule: half of it — +/// a cleared buffer in a mode the user has not left — is worse than none. +#[test] +fn entering_typing_and_cancelling_an_edit_agrees_with_the_host() { + let window = (2, 1); + let mut app = editable_host(); + let mut client = client_ready_to_edit(&app, window); + + for key in ["i", "9", "9", "9", "Escape"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", app.view_state.mode), + "{key}: client and host disagree about the mode" + ); + assert_eq!( + client.view.buffers, app.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + + assert!( + matches!(app.view_state.mode, AppMode::Normal), + "Esc must have left editing, or the walk above proved nothing" + ); +} + +/// `i` seeds the edit buffer from the cell's **display value**, and the two +/// ends must agree on what that is: the host reads +/// `App::display_value_at_cursor`, the client reads the text the server +/// projected into its `GridCache`. Those are two paths to one string, and +/// this is the assertion that they are the same string. +#[test] +fn the_buffer_i_seeds_is_the_value_the_host_would_seed() { + let window = (2, 1); + let mut app = editable_host(); + let mut client = client_ready_to_edit(&app, window); + + let event = BrowserKey::new("i").to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("i on a record cell must resolve locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + + assert_eq!( + client.view.buffers.get("edit"), + app.view_state.buffers.get("edit"), + "the seeded buffer must be the host's display value" + ); + assert_eq!( + client.view.buffers.get("edit").map(String::as_str), + Some("1200"), + "and the fixture must actually have a value, or this passes on two empties" + ); +} + +/// The commit: `Enter` in `Editing` moves both ends to the same mode, the +/// same cursor and the same buffers. +/// +/// What is *not* compared is the document, and deliberately: the client +/// answers with a `Command::Model`, which `reduce_view` drops, while +/// `reduce_key` writes through the effect pipeline. The write's composition +/// is asserted in `keys.rs` against `typed_cell`'s own — the shared one — so +/// what is left to prove here is the half a unit test cannot see, which is +/// that the advance and the re-entry land in the same place. +#[test] +fn a_locally_committed_edit_leaves_both_ends_in_the_same_place() { + let window = (2, 1); + let mut app = editable_host(); + let mut client = client_ready_to_edit(&app, window); + + for key in ["i", "7", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", app.view_state.mode), + "{key}: client and host disagree about the mode" + ); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "{key}: client and host disagree about the cursor" + ); + assert_eq!( + client.view.buffers, app.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + + assert_eq!( + app.view_state.cursor().selected, + (1, 0), + "the commit must have advanced a row, or the walk proved nothing" + ); +} diff --git a/web/app.js b/web/app.js index e0f0c44e..e850ef7c 100644 --- a/web/app.js +++ b/web/app.js @@ -232,15 +232,23 @@ function gridTable(model) { row.cells.forEach((cell, c) => { const td = tr.insertCell(); - td.textContent = cell.text; - td.classList.toggle("numeric", cell.numeric); + // A keyboard edit is open on the selected cell when `model.editing` is + // a string; it holds what has been typed so far. Both facts are + // decided in Rust (`RenderModel::editing`) — the page does not know + // which modes are editing modes or which buffer they type into. + // Without this the mode indicator would say INSERT while every + // keystroke vanished into a buffer nobody drew. + const typing = cell.selected && typeof model.editing === "string"; + td.textContent = typing ? model.editing + "\u258f" : cell.text; + td.classList.toggle("numeric", cell.numeric && !typing); td.classList.toggle("error", cell.error); td.classList.toggle("selected", cell.selected); + td.classList.toggle("typing", typing); // `editable` is the server's answer relayed through Rust: a stored // cell. An aggregate is a drill target and drill is not on the wire // yet, so it is simply not offered here. td.classList.toggle("editable", cell.editable); - if (cell.editable) td.addEventListener("click", () => edit(td, r, c)); + if (cell.editable) td.addEventListener("click", () => edit(td, r, c, cell.edit_text)); }); }); return table; @@ -248,10 +256,13 @@ function gridTable(model) { /** Open an inline editor. Committing sends a frame; the value that comes * back is the server's, never the string typed here. */ -function edit(td, row, col) { +function edit(td, row, col, seed) { if (editing) return; editing = { row, col }; - const original = td.textContent; + // `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn + // one. `td.textContent` has been through the number format, so a stored + // 1200 reads "1,200" and committing that stores the string "1,200". + const original = seed; const input = document.createElement("input"); input.value = original; td.replaceChildren(input); diff --git a/web/style.css b/web/style.css index 0c3e9f7f..c716d7f7 100644 --- a/web/style.css +++ b/web/style.css @@ -89,6 +89,13 @@ td.selected { outline: 1px solid var(--accent); } +/* A keyboard edit in progress. Left-aligned even in a numeric column, + because what is drawn is the text being typed rather than a value. */ +td.typing { + text-align: left; + outline: 2px solid var(--accent); +} + #grid { flex: 1; overflow: auto;