diff --git a/context/code/clients.md b/context/code/clients.md index 73c7cbde..146d0138 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -578,11 +578,43 @@ immutable `CmdContext`; the commands are then folded in order. ### What still needs the host -`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 +`resolve::NEEDS_THE_HOST` names them, and +`the_command_names_that_still_need_the_host_are_these` drives each through the +key that reaches it. `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 help overlay marks them, and it marks rather than omits** (improvise-uln5). +The overlay is shared content, and before this the browser painted every row +plainly, including six that do nothing there. That is worse than a gap: +improvise-koh6 showed that the keys typed after a dead one land on live Normal +bindings, and `i` opens an editor. The browser's `RenderHelpRow` now draws a row +whose command is on the list as `dim`, and appends "(not in the browser yet)" +to its description. It does the same for every ex-command row while `:` opens a +command line this client cannot run (`resolve::runs_the_command_line`, which is +`stranding`'s answer, not a second one). + +Marking was chosen over omitting for three reasons: +- **Both front ends draw the same rows.** A page with rows missing is a second + layout of the same content. +- **The feature stays findable.** The row still says it exists in the terminal. +- **It needs no change in `web/`.** `dim` is a kind the page already paints. + +The TUI is unaffected, because the marking lives in the browser's renderer. + +**One list, held from both sides.** `NEEDS_THE_HOST` is production data because +the renderer needs it. Two tests hold it to what `resolve` actually does: +- `the_command_names_that_still_need_the_host_are_these` fails if a name on it + resolves locally. +- `every_help_row_the_browser_cannot_run_is_one_it_marks` fails if a help row + is silent in the browser without being on the list, or on the list without + being silent. It also fails if a *hint line* ever names a listed command, + because the status bar is never marked. + +`every_advertised_command_answers_in_its_own_mode` therefore no longer restates +the black holes. It skips a silent row whose command is on the list, because +the other test owns it. + **An upstream key that the app also ADVERTISES is a different severity, and it had its own class of defect** (improvise-rmvx). A key nobody offers doing nothing is a gap; a key named in the mode's own hint line doing nothing reads as diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 1b893acb..8092cd6e 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -213,6 +213,45 @@ when Ctrl-S became a `Request`; putting the entry back fails with *"save stops being a gap"*. So the mechanism gotcha 47 asks for was watched failing rather than assumed. +**Since improvise-uln5 it iterates `resolve::NEEDS_THE_HOST`** rather than a +table of its own. It derives each name's keys from the Normal and search-mode +keymaps, and arms a prefix before pressing its second key. So the list the +browser's help marks and the list this test drives are the same list. +`exit-search-mode` joined it, because search mode's `Enter`/`Esc` are silent +too. Rival: add `move-selection`, which the browser runs, to the list. This test +fails, and so does `every_help_row_the_browser_cannot_run_is_one_it_marks`. + +### every_help_row_the_browser_cannot_run_is_one_it_marks + +improvise-uln5. Every help row is pressed, in its own mode, on the fixture +`furnished` builds. The commands whose keys are silent must be exactly the +commands on `NEEDS_THE_HOST`, which are the rows the browser draws marked. The +check runs in both directions, because each direction catches its own mistake: +- a silent row that is not on the list is an unmarked black hole; +- a listed row that acts is a false warning. + +It also asserts that no status-bar hint names a listed command, since +`hint_line` is never marked. + +Rivals, each run against the fixed tree: +- Drop `search` from the list: red. This test fails, and so does + `every_advertised_command_answers_in_its_own_mode`, which no longer skips `/`. +- Add `move-selection` to the list: red. This test fails, and so does the + list's own test. + +### a_help_row_the_browser_cannot_run_is_drawn_marked + +improvise-uln5, the display half, and the red test. On the Navigation page the +row "Start search — …" (key `/`) must render as `dim`, with "not in the browser" +in its description. On the Commands page every `:` row must say the same. A +row the browser runs ("Jump to last row") must stay a plain `keys` row with its +description unchanged, so a renderer that marks everything cannot pass. + +Red at `202e7eac`: the search row rendered as `keys`. Rival: the renderer's +`needs_the_host` check replaced by `true`. It is red here, and only here, which +is why the display has a test of its own and does not lean on the resolve-side +guard. + ### a_prompt_this_client_cannot_commit_is_never_entered The refusal list above is only half the property. `Upstream` says the key @@ -2105,7 +2144,10 @@ two cannot drift — including prefix sequences (`gg`, `yy`, `ZZ`), which it presses by arming the prefix first. That is what added the five host-only black holes (`/`, `n`, `H`, `z`, Normal's `Enter`) to the list: each was already in `the_command_names_that_still_need_the_host_are_these`, and the help page -advertised it anyway. +advertised it anyway. Since the rest of improvise-uln5 they are no longer listed +here: the browser marks those rows, and this test skips a silent row whose +command is on `resolve::NEEDS_THE_HOST`, which +`every_help_row_the_browser_cannot_run_is_one_it_marks` owns. *Which cells.* `furnished` stood on a record cell only, so `i` on an aggregate (s8h7's whole subject) was outside its reach. Normal mode is now pressed on a diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 19cc648b..5d0c7a52 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -335,6 +335,7 @@ mod key_row { #[derive(Debug, Clone, PartialEq, Eq)] pub struct KeyRow { keys: Vec, + command: &'static str, description: &'static str, } @@ -343,6 +344,12 @@ mod key_row { &self.keys } + /// The command the keys run, kept so a front end can ask whether it + /// can run it (improvise-uln5). + pub fn command(&self) -> &'static str { + self.command + } + pub fn description(&self) -> &'static str { self.description } @@ -351,7 +358,7 @@ mod key_row { /// advertises a key that does not exist. fn looked_up( mode: &ModeKey, - command: &str, + command: &'static str, args: Option<&[String]>, description: &'static str, ) -> Option { @@ -360,7 +367,11 @@ mod key_row { .map(|(c, m)| key_label(c, *m)) .collect(); keys.extend(prefixed_labels(mode, command, args)); - (!keys.is_empty()).then_some(Self { keys, description }) + (!keys.is_empty()).then_some(Self { + keys, + command, + description, + }) } } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 981b5227..4e115009 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -359,6 +359,34 @@ fn not_yet(what: &str) -> String { format!("{what} is not available in the browser yet") } +/// The command names this client has no arm for, so their keys go upstream to +/// a host that answers nothing (gotcha 47). The one list: see clients.md. +pub const NEEDS_THE_HOST: &[&str] = &[ + "enter-advance", + "exit-search-mode", + "force-quit", + "hide-selected-row-item", + "search", + "search-navigate", + "toggle-group-under-cursor", +]; + +/// Whether a key running `name` does nothing in this client. +pub fn needs_the_host(name: &str) -> bool { + NEEDS_THE_HOST.contains(&name) +} + +/// Whether `:` opens a command line this client can run; it cannot, so an +/// ex-command it advertises is unreachable (improvise-uln5). +pub fn runs_the_command_line() -> bool { + AppMode::from_name("command").is_some_and(|mode| stranding(&mode).is_none()) +} + +/// A help description, marked as naming something this client cannot do. +pub fn not_here(description: &str) -> String { + format!("{description} (not in the browser yet)") +} + /// The subset of command names this client can turn into `Command`s; `None` /// means "the host's". Each arm mirrors its `Cmd` line for line — clients.md. fn composed( @@ -2402,7 +2430,12 @@ mod tests { } } } - for (where_, got) in report.into_iter().filter(|(_, got)| *got != Heard::Acts) { + // A black hole the browser marks is the next test's, by NEEDS_THE_HOST. + let marked = |got: &Heard| *got == Heard::Silent && needs_the_host(command); + for (where_, got) in report + .into_iter() + .filter(|(_, got)| *got != Heard::Acts && !marked(got)) + { short .entry((where_, command, got)) .or_default() @@ -2426,14 +2459,6 @@ mod tests { "COMMAND: Enter execute-command Silent", "EXPORT: Enter commit-export Silent", "SAVE AS: Enter commit-save Silent", - "NORMAL SEARCH: Enter/Esc exit-search-mode Silent", - // Commands the browser has no arm for: upstream, so a black hole - // (improvise-uln5). Each is also in the host-only list below. - "NORMAL: Enter enter-advance Silent", - "NORMAL: H hide-selected-row-item Silent", - "NORMAL: / search Silent", - "NORMAL: n search-navigate Silent", - "NORMAL: z toggle-group-under-cursor Silent", // Honestly unavailable, and saying so in `declined`'s words. "FORMULAS: Del/d delete-formula-at-cursor Declines", "NORMAL: > drill-into-cell Declines", @@ -2667,41 +2692,93 @@ mod tests { /// closing one of these gaps has to come here and say so. Evidence file. #[test] 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 [ - // The three prefixes left with improvise-hs0u: arming is a - // `Command`, and the second key resolves in the sub-keymap. - (ev(KeyCode::Char('H')), "hide-selected-row-item"), - (ev(KeyCode::Char('z')), "toggle-group-under-cursor"), - // `>`, `R`, `P`, Ctrl-W and Ctrl-E are NOT here any more: each - // answers with a status of its own (improvise-gd28, improvise-rmvx). - - // `<` left too, and further: since improvise-cfly it composes a - // real `Command::ViewBack` rather than a refusal. - (ev(KeyCode::Char('/')), "search"), - (ev(KeyCode::Char('n')), "search-navigate"), - // `N` left this list with improvise-koh6: both its arms are - // answered here, one of them out loud. See clients.md. - (ev(KeyCode::Enter), "enter-advance"), - // Ctrl-S left this list with improvise-jz6w: it is a `Request`, - // not a `Command`, and the host has answered it since 368h. - ( - KeyEvent::new(KeyCode::Char('q'), KeyModifiers::CONTROL), - "force-quit", - ), - ] { - assert_eq!( - 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 + // `N` left with improvise-koh6, Ctrl-S with jz6w, and `>`, `R`, `P`, + // Ctrl-W, Ctrl-E and `<` with gd28, rmvx and cfly: see clients.md. + for name in NEEDS_THE_HOST { + let mut sequences = Vec::new(); + for (mode, search) in [(AppMode::Normal, false), (AppMode::Normal, true)] { + let key = ModeKey::from_app_mode(&mode, search).expect("a keyed mode"); + let keymap = Keymaps::shared().get(&key).expect("a keymap"); + for sequence in sequences_running(keymap, name, None) { + sequences.push((mode.clone(), search, sequence)); + } + } + assert!( + !sequences.is_empty(), + "{name} is bound nowhere this test looks" ); + for (mode, search, sequence) in sequences { + let mut state = in_mode(mode); + state.view.search_mode = search; + state.grid.extent = GridExtent { rows: 4, cols: 4 }; + state.window = (4, 4); + let (key, prefix) = sequence.split_last().expect("a key"); + for armed in prefix { + if let KeyOutcome::Local(commands) = resolve(&state, *armed) { + commands.iter().for_each(|c| state.fold(c)); + } + } + let key = *key; + assert_eq!( + resolve(&state, key), + KeyOutcome::Upstream(key), + "{name} ({:?}) was resolved locally; if that is deliberate, it \ + leaves NEEDS_THE_HOST, and the browser stops marking it", + key.code + ); + } } } + /// improvise-uln5: the help rows a browser cannot run are exactly the ones + /// it marks, and no hint names one. See `context/test-evidence/clients.md`. + #[test] + fn every_help_row_the_browser_cannot_run_is_one_it_marks() { + for mode in AppMode::all() { + for (command, _, _) in crate::help::advertised(&mode) { + assert!( + !needs_the_host(command), + "the {mode:?} hint line advertises {command}, which a browser \ + cannot run and which the hint line never marks" + ); + } + } + let mut silent = Vec::new(); + let mut marked = Vec::new(); + for (key, command, args, _) in crate::help::help_bindings() { + let (mode, search) = match &key { + ModeKey::SearchMode => (AppMode::Normal, true), + key => ( + AppMode::all() + .into_iter() + .find(|m| ModeKey::from_app_mode(m, false).as_ref() == Some(key)) + .expect("a help row's mode is reachable"), + false, + ), + }; + let keymap = Keymaps::shared().get(&key).expect("a keymap"); + for sequence in sequences_running(keymap, command, args.as_deref()) { + let mut state = furnished(mode.clone()); + state.view.search_mode = search; + if press(state, &sequence) == Heard::Silent { + silent.push(command); + } + } + if needs_the_host(command) { + marked.push(command); + } + } + for list in [&mut silent, &mut marked] { + list.sort(); + list.dedup(); + } + assert_eq!( + silent, marked, + "left: help rows whose keys do nothing in a browser; right: the rows \ + it marks as needing the terminal. They must be the same rows." + ); + } + /// improvise-jz6w: the outcome a resolver had no way to name. See /// `context/test-evidence/clients.md`. #[test] diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 8535f78b..6e916ec4 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -2,7 +2,7 @@ //! already made. The four silent ones are in `context/code/clients.md`. use improvise_protocol::viewmodel::{CatTreeEntry, flatten_cat_tree}; -use improvise_protocol::{AxisFrame, CellDisplay, CellId, FrameEntry}; +use improvise_protocol::{AxisFrame, CellDisplay, CellId, FrameEntry, resolve}; use serde::Serialize; use crate::{Client, ClientState}; @@ -423,22 +423,37 @@ impl RenderHelpRow { L::Heading(t) => prose("heading", t), L::Text(t) => prose("text", t), L::Accent(t) => prose("accent", t), - L::Keys(row) => Self { - keys: row.keys().join(" "), - description: row.description().to_string(), - kind: "keys", - }, + L::Keys(row) => Self::row( + row.keys().join(" "), + row.description(), + !resolve::needs_the_host(row.command()), + false, + ), L::Literal { keys, description, dim, - } => Self { - keys: (*keys).to_string(), - description: description.to_string(), - kind: match dim { - true => "dim", - false => "keys", - }, + } => Self::row( + (*keys).to_string(), + description, + resolve::runs_the_command_line(), + *dim, + ), + } + } + + /// A keys row. One this client cannot run is drawn dim and says so, since a + /// dead key followed by live ones edits the grid (improvise-uln5, koh6). + fn row(keys: String, description: &str, runs: bool, dim: bool) -> Self { + Self { + keys, + description: match runs { + true => description.to_string(), + false => resolve::not_here(description), + }, + kind: match dim || !runs { + true => "dim", + false => "keys", }, } } @@ -579,6 +594,50 @@ mod tests { FrameEntry::Item(vec![label.into()]) } + fn help_page(page: usize) -> Vec { + let view = ViewState { + mode: improvise_protocol::AppMode::Help, + help_page: page, + ..ViewState::default() + }; + help_rows(&view) + } + + /// improvise-uln5: a row whose key this client sends to a host that answers + /// nothing is drawn marked. Evidence: `context/test-evidence/clients.md`. + #[test] + fn a_help_row_the_browser_cannot_run_is_drawn_marked() { + let rows = help_page(1); + let row = |prefix: &str| { + let found = rows.iter().find(|r| r.description.starts_with(prefix)); + found.unwrap_or_else(|| panic!("no Navigation row reads {prefix:?}")) + }; + let search = row("Start search"); + assert_eq!(search.kind, "dim", "{search:?}"); + assert!( + search.description.contains("not in the browser"), + "{search:?}" + ); + let control = row("Jump to last row"); + assert_eq!( + control.kind, "keys", + "a row the browser runs is drawn plainly" + ); + assert_eq!(control.description, "Jump to last row"); + let commands = help_page(4); + let ex: Vec<_> = commands + .iter() + .filter(|r| r.keys.trim_start().starts_with(':')) + .collect(); + assert!( + !ex.is_empty(), + "precondition: the Commands page lists ex-commands" + ); + for row in ex { + assert!(row.description.contains("not in the browser"), "{row:?}"); + } + } + /// A run is opened by a HEADER, not by a name — improvise-h0iv, and both /// ends now ask the one rule. See the evidence file. #[test]