Merge pull request #198 from fiddlerwoaroof/fix/w7qd-ssoe-help-guards

Grid-move and help-row guards that the #180 re-review found blind (w7qd, ssoe)

context/test-evidence/clients.md

@@ -4640,3 +4640,40 @@ it fails naming `End` and `$`. The general point: the fixture that made the
 laundering invisible (the edge) is also the one that makes a wrong-direction
 jump visible, so moving off the edge fixed one blind spot and opened the other,
 and each needs its own question.
+
+**improvise-w7qd, and the floor was the hole.** As first written the test
+covered `move-selection` and a four-entry jump table, behind `pressed >= 14`.
+Two things passed the whole workspace, both found in re-review of #180 at
+544744c: negating the `scroll-rows` and `page-scroll` arms, so `Ctrl-d`/`Ctrl-u`
+and `PgDn`/`PgUp` all scroll the wrong way (advertised, and still "acting" from
+the centre); and dropping the jump table *with* `$` sent to the first column,
+because the sixteen `move-selection` sequences met the floor on their own. The
+test now walks **every** command Normal binds. `scroll-rows` and `page-scroll`
+state their landing from their own argument exactly as `move-selection` does —
+from the centre of a 3x3 grid any step of at least one lands at `1 + d`
+clamped, so a sign is visible whatever the page size — and a command that moves
+the cursor (in the same view) without a stated landing **fails** rather than
+being skipped. So a dropped table entry is caught by derivation, not by a count:
+under the second rival it fails naming every jump as moving with nothing stating
+where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on
+`Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at
+least once guards the other direction — a binding vanishing — and is a named
+list on purpose, since that is the one thing no walk over bindings can see.
+
+**improvise-5kdr: the 3x3 fixture saw only a sign.** From the centre of a 3x3
+grid every step of one or more clamps to an edge, so the w7qd version stated
+`page-scroll` as `1 + d` — one row, which is not what it does — and passed
+because the true landing clamped to the same edge. Measured on #198 at 8772db2:
+`page-scroll` ignoring the page size together with `scroll-rows` moving one row,
+or `move-selection` moving `2d`, left the test green. The walk now stands at
+(10,10) of a 21x21 grid with an 8x8 window, far from every edge, so a landing
+shows magnitude: `10+d` for `move-selection` and `scroll-rows`, `10 + page·d`
+for `page-scroll`, 0/20 for the jumps. The page size is not written into the
+test: `grid_move`'s page rule moved into `page_rows(window_rows)` and the test
+asks it, so the rule has one home. Rivals, each run: `move-selection` doubled
+fails on `Up` (`move-selection ["-1","0"]`); the page-size-and-one-row pair
+fails on `PgUp` (`page-scroll ["-1"]`); the one-row `scroll-rows` alone fails on
+`Ctrl-u` (`scroll-rows ["-5"]`); the jump-table rival still fails naming every
+jump. The residual, stated: because the test reads `page_rows`, a change to the
+page rule itself moves the test with it — whether that rule is the TUI's is
+`tests/lockstep.rs`'s question, not this one's.

context/test-evidence/protocol.md

@@ -1159,3 +1159,35 @@ failed: ex_usage_is_placeholders("q  or press Q")`. The rule cost one row:
 `:add-items <cat> a b c ...` had three bare words and is now
 `:add-items <cat> <item> ...`. This test pins the rule the macro asks, in both
 directions, including that shipped usage and the rival.
+
+### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for
+
+improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed
+hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder
+could call it directly instead of through its `keys` parameter — and
+`help_bindings()` records only what goes through the parameter. Injected into
+`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current
+cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test
+exists for — and the whole workspace stayed green, because both guards that
+would object read `help_bindings()`.
+
+Two changes. `looked_up` is now private to the `key_row` module, whose one
+caller is the module's own `keys` — the lookup `pages()` hands every builder —
+so the reviewer's injection is `E0624: associated function looked_up is
+private`. That narrows the door without closing it: a builder can still reach
+the module-level `keys` by path (`self::keys(..)`) rather than through its
+shadowing parameter. So this test states the premise directly — the number of
+keys rows drawn equals the number of rows the builders asked for (every asked
+row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) —
+and the `self::keys` bypass fails it `51 != 50`.
+
+**improvise-aszt: a count is not the premise.** The test above compared
+*counts*, so a bypass that also drops one real row keeps them equal. Injected in
+`editing()`: call the parameter `keys(e, "clear-buffer", …)` and discard it with
+`let _ =`, then return `self::keys(n, "clear-cell", None, "Edit the current
+cell")` in its place. The asked row is recorded and never drawn, the drawn row
+is never recorded, 50 = 50, and the whole workspace stayed green. The test now
+compares **content in order**: the drawn keys rows must equal
+`help_bindings()` looked up through the same `keys`, row for row. The same
+injection now fails. What still passes is a bypass that draws exactly what the
+builders asked for, in the order they asked — which is harmless by definition.

crates/improvise-protocol/src/help.rs

@@ -324,11 +324,12 @@ pub struct HelpPage {
 }
 
 pub use key_row::KeyRow;
+use key_row::keys;
 
 /// The one door to a keys row: its fields are private to this module, and its
 /// only constructor looks the keys up.
 mod key_row {
-    use super::{ModeKey, key_label, keys_for, prefixed_labels};
+    use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels};
 
     #[derive(Debug, Clone, PartialEq, Eq)]
     pub struct KeyRow {
@@ -347,7 +348,7 @@ mod key_row {
 
         /// An unbound command yields no keys and no row, so the page never
         /// advertises a key that does not exist.
-        pub(super) fn looked_up(
+        fn looked_up(
             mode: &ModeKey,
             command: &str,
             args: Option<&[String]>,
@@ -361,17 +362,18 @@ mod key_row {
             (!keys.is_empty()).then_some(Self { keys, description })
         }
     }
-}
 
-/// A keys row, looked up in `mode`.
-fn keys(
-    mode: &ModeKey,
-    command: &'static str,
-    args: Option<&[&str]>,
-    description: &'static str,
-) -> Option<HelpLine> {
-    let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
-    KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
+    /// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`],
+    /// and the lookup `pages` hands every builder (improvise-ssoe).
+    pub(super) fn keys(
+        mode: &ModeKey,
+        command: &'static str,
+        args: Option<&[&str]>,
+        description: &'static str,
+    ) -> Option<HelpLine> {
+        let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
+        KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
+    }
 }
 
 /// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap.
@@ -1021,6 +1023,29 @@ mod tests {
         }
     }
 
+    /// improvise-ssoe: a keys row drawn but never asked for through the builders'
+    /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`.
+    #[test]
+    fn every_keys_row_drawn_is_one_the_builders_asked_for() {
+        let drawn: Vec<HelpLine> = pages()
+            .into_iter()
+            .flat_map(|page| page.lines)
+            .filter(|line| matches!(line, HelpLine::Keys(_)))
+            .collect();
+        // Content, in order, not a count: a bypass that also drops a real row
+        // keeps the count and changes this (improvise-aszt).
+        let asked: Vec<HelpLine> = help_bindings()
+            .iter()
+            .filter_map(|(mode, command, args, description)| {
+                let args: Option<Vec<&str>> = args
+                    .as_ref()
+                    .map(|a| a.iter().map(String::as_str).collect());
+                keys(mode, command, args.as_deref(), description)
+            })
+            .collect();
+        assert_eq!(drawn, asked);
+    }
+
     /// A row whose lookup finds no key is dropped silently, so a broken lookup
     /// looks like a shorter page. See `context/test-evidence/protocol.md`.
     #[test]

crates/improvise-protocol/src/resolve.rs

@@ -619,6 +619,11 @@ enum GridMove {
     Page(i32),
 }
 
+/// Rows one `page-scroll` step moves: three quarters of the window, at least one.
+fn page_rows(window_rows: usize) -> i32 {
+    (window_rows as i32 * 3 / 4).max(1)
+}
+
 fn grid_move(state: &ClientState, kind: GridMove) -> Option<Vec<Command>> {
     let GridExtent { rows, cols } = state.grid.extent;
     let (window_rows, window_cols) = state.window;
@@ -641,7 +646,7 @@ fn grid_move(state: &ClientState, kind: GridMove) -> Option<Vec<Command>> {
         GridMove::ToEnd(true) => (row_max as usize, cursor.selected.1),
         GridMove::ToEnd(false) => (cursor.selected.0, col_max as usize),
         GridMove::Page(dir) => {
-            let delta = (window_rows as i32 * 3 / 4).max(1) * dir;
+            let delta = page_rows(window_rows) * dir;
             ((row + delta).clamp(0, row_max) as usize, cursor.selected.1)
         }
     };
@@ -2362,47 +2367,90 @@ mod tests {
     #[test]
     fn every_grid_move_lands_where_its_binding_says() {
         let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal");
-        let mut moves = Vec::new();
-        bound_commands(keymap, &mut moves);
-        // A move's destination is its own args; a jump's is its name.
-        let mut expected: Vec<(String, Vec<String>, (usize, usize))> = moves
-            .into_iter()
-            .filter(|(name, _)| name == "move-selection")
-            .map(|(name, args)| {
-                let d = |i: usize| args[i].parse::<isize>().expect("a delta");
-                let to = ((1 + d(0)) as usize, (1 + d(1)) as usize);
-                (name, args, to)
-            })
-            .collect();
-        for (name, to) in [
-            ("jump-first-row", (0, 1)),
-            ("jump-last-row", (2, 1)),
-            ("jump-first-col", (1, 0)),
-            ("jump-last-col", (1, 2)),
-        ] {
-            expected.push((name.to_string(), Vec::new(), to));
-        }
-        let mut pressed = 0;
-        for (name, args, to) in expected {
-            let wanted = (!args.is_empty()).then_some(args.as_slice());
-            for sequence in sequences_running(keymap, &name, wanted) {
+        let mut bound = Vec::new();
+        bound_commands(keymap, &mut bound);
+        bound.sort();
+        bound.dedup();
+        // Far from every edge of a grid far larger than any step, so a landing
+        // shows magnitude as well as sign (improvise-5kdr); a jump's is its name.
+        const START: (usize, usize) = (10, 10);
+        const WINDOW: (usize, usize) = (8, 8);
+        let d = |a: &String| a.parse::<isize>().expect("a delta");
+        let at = |base: usize, by: isize| base.checked_add_signed(by).expect("inside the grid");
+        let page = page_rows(WINDOW.0) as isize;
+        let destination = |name: &str, args: &[String]| match name {
+            "move-selection" => Some((at(START.0, d(&args[0])), at(START.1, d(&args[1])))),
+            "scroll-rows" => Some((at(START.0, d(&args[0])), START.1)),
+            "page-scroll" => Some((at(START.0, page * d(&args[0])), START.1)),
+            "jump-first-row" => Some((0, START.1)),
+            "jump-last-row" => Some((20, START.1)),
+            "jump-first-col" => Some((START.0, 0)),
+            "jump-last-col" => Some((START.0, 20)),
+            _ => None,
+        };
+        let mut unstated = Vec::new();
+        let mut unpressed = Vec::new();
+        let mut pressed = std::collections::BTreeSet::new();
+        for (name, args) in &bound {
+            let to = destination(name, args);
+            let sequences = sequences_running(keymap, name, Some(args.as_slice()));
+            if to.is_some() && sequences.is_empty() {
+                unpressed.push(format!("{name} {args:?}"));
+            }
+            for sequence in sequences {
                 let mut state = furnished(AppMode::Normal);
+                state.grid.extent = GridExtent { rows: 21, cols: 21 };
+                state.window = WINDOW;
+                *state.view.cursor_mut() = crate::ViewCursor {
+                    selected: START,
+                    row_offset: 5,
+                    col_offset: 5,
+                };
+                let view = state.view.active_view.clone();
                 for key in &sequence {
                     if let KeyOutcome::Local(commands) = resolve(&state, *key) {
                         commands.iter().for_each(|c| state.fold(c));
                     }
                 }
-                assert_eq!(
-                    state.view.cursor().selected,
-                    to,
-                    "{sequence:?} runs `{name} {args:?}` from (1,1)"
-                );
-                pressed += 1;
+                let landed = state.view.cursor().selected;
+                if to.is_some() {
+                    pressed.insert(name.as_str());
+                }
+                match to {
+                    Some(to) => {
+                        assert_eq!(
+                            landed, to,
+                            "{sequence:?} runs `{name} {args:?}` from {START:?}"
+                        )
+                    }
+                    None if state.view.active_view == view && landed != START => {
+                        unstated.push(format!("{name} {args:?} -> {landed:?}"))
+                    }
+                    None => {}
+                }
             }
         }
         assert!(
-            pressed >= 14,
-            "only {pressed} keys pressed; the walk went blind"
+            unpressed.is_empty(),
+            "stated and never pressed: {unpressed:?}"
+        );
+        assert!(
+            unstated.is_empty(),
+            "these move the cursor and nothing states where to: {unstated:?}"
+        );
+        let every = [
+            "jump-first-col",
+            "jump-first-row",
+            "jump-last-col",
+            "jump-last-row",
+            "move-selection",
+            "page-scroll",
+            "scroll-rows",
+        ];
+        assert_eq!(
+            pressed.into_iter().collect::<Vec<_>>(),
+            every,
+            "each kind is pressed"
         );
     }