From 877d3e193bdea0bc98d957160f3dbc45c24969fc Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 03:57:32 -0700 Subject: [PATCH] refactor(help): a chord is recognised by its modifiers, not its spelling (improvise-thpe) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `compact` tested whether a label it had just formatted began with "Ctrl-" or "Alt-". No false positive today — the reviewer enumerated every `format_key_label` output — so this is the shape, not the behaviour. `Hint::keys` is now `(KeyCode, KeyModifiers)` and `compact` takes the pairs, so the string is not in scope to parse; `Hint::labels` and the help pages format last through the one spelling in `keymap::key_label`. The regression guard is the signature, as it is for `Checked`. `Keymap::labels_for` — the "throw the modifiers away" step the ticket names — has no caller left and is deleted: a consolidation that keeps its old path keeps the defect available. The rule also got wider and total over the bitset: a chord is any modifier beyond shift. Injected a temporary `KeyModifiers::SUPER` spelled `Super-x` and ran it — the old filter keeps it, `compact` drops it. Binding dump byte-identical across the change (4406 lines). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/protocol.md | 7 +- context/test-evidence/protocol.md | 40 ++++++++ .../improvise-command/src/command/keymap.rs | 2 +- crates/improvise-protocol/src/help.rs | 92 +++++++++++++++---- crates/improvise-protocol/src/keymap.rs | 9 -- src/ui/which_key.rs | 4 +- 6 files changed, 121 insertions(+), 33 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index c8810e9b..c0dde99a 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1364,7 +1364,12 @@ That is the price of the keys being true. ### The three rules the rendering follows -1. `hint_line` **drops chorded keys** (`Ctrl-`/`Alt-` prefixed). `T` advertises +1. `hint_line` **drops chorded keys** — any key held with a modifier beyond + shift, asked of the **modifiers** and never of the label (improvise-thpe). + `Hint::keys` is `(KeyCode, KeyModifiers)` and `compact` takes the pairs, so + the string it used to parse is not in scope to parse; `Hint::labels` and the + help pages format last, through the one spelling in `keymap::key_label`. + `T` advertises tile select; its four Ctrl-arrow aliases would push the rest of the line off the screen and teach nobody anything. A client wanting all of them reads `hints()`, which keeps them. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 44487bd5..6dcded7e 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -462,3 +462,43 @@ against an enum that really did have the variant. Proved discriminating by re-adding the variant with an empty `apply` arm: the retired spelling parses again and the test fails on `expect_err`. + +### a_chord_is_recognised_by_its_modifiers_and_not_by_its_label + +`help::compact` dropped a chorded key from a status-bar hint by testing whether +the formatted label started with `"Ctrl-"` or `"Alt-"` — parsing a string it had +just formatted. Finding 3 of the adversarial review of PR #50, filed as +improvise-thpe and deliberately not fixed there: the reviewer enumerated every +`format_key_label` output and none begins with either, so there was **no false +positive**. The defect was the shape. + +**The regression guard is the signature**, as it is for `Checked` (gotcha 43): +`compact` now takes `&[(KeyCode, KeyModifiers)]`, so the string it used to parse +is not in scope to parse. `keys_for` returns the pairs and `Hint::keys` carries +them; `help.rs` formats last, through `Hint::labels` and the one spelling in +`keymap::key_label`. `Keymap::labels_for` — the "throw the modifiers away into a +string" step the ticket names — has **no caller left and is deleted**, because a +consolidation that keeps its old path keeps the defect available +(`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`). + +The rule also got wider, and that is the part a test can state: a chord is any +modifier **beyond shift**, which is total over the bitset, where the string test +enumerated two prefixes. Shift alone is not a chord — it is how a key is named +rather than how it is held, which `S-Tab` is the case for. + +**Injected fault, run rather than argued.** A `KeyModifiers::SUPER` bit and a +`key_label` arm spelling it `Super-x` were added temporarily. The old filter +keeps it — `["T", "Super-x"]`, two survivors where one is wanted — while +`compact` returns `"T"`. That is the ticket's own reproduction, executed, and it +is the only way to see the difference: with `KeyModifiers` as shipped, every +chord's label *does* begin with `Ctrl-` or `Alt-`, so no permanent test can +separate the two rules. Both the bit and the probe were removed afterwards. + +One behaviour that had to be preserved deliberately: `keys_for` used to `dedup` +**labels**, and deduping the pairs instead would let two patterns that spell the +same key both reach the status bar. It now dedups by label explicitly. + +The binding dump is byte-identical across this change (4406 lines, `diff` +clean), which is what `doctrine(binding_dump_diff)` asks of anything touching +the keymap: the dump's prefix rows go through `Hint`, so they had to be checked +rather than assumed. diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index da91159a..2ec4c216 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -861,7 +861,7 @@ mod binding_dump { let rows: Vec<(Vec, &str)> = improvise_protocol::help::which_key(sub) .into_iter() - .map(|hint| (hint.keys, hint.command)) + .map(|hint| (hint.labels(), hint.command)) .collect(); format!("prefix {rows:?}") } diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 18a53b29..9c110afb 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -1,19 +1,34 @@ //! What a binding is called and what it does, in words: the status-bar hint, //! the which-key popup and the help pages. See `context/code/protocol.md` §8. -use crate::keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey}; +use crate::key::{KeyCode, KeyModifiers}; +use crate::keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey, key_label}; use crate::{AppMode, HELP_PAGE_COUNT}; /// One advertised binding. `command` is the semantic value — a localized or /// non-terminal front end keys off it and ignores `caption`. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Hint { - /// The keys that reach it, derived from the keymap and never spelled. - pub keys: Vec, + /// The keys that reach it, as the keymap holds them. A renderer spells them + /// with [`Hint::labels`]; nothing reads a modifier back out of a label. + pub keys: Vec<(KeyCode, KeyModifiers)>, pub command: &'static str, pub caption: &'static str, } +impl Hint { + /// This hint's keys, spelled. The one spelling is `keymap::key_label`. + pub fn labels(&self) -> Vec { + self.keys.iter().map(|(c, m)| key_label(c, *m)).collect() + } +} + +/// A key held with a modifier the status bar will not spell. Asked of the +/// modifiers, never of the label — `context/code/protocol.md` §8. +fn is_chord(mods: KeyModifiers) -> bool { + mods != KeyModifiers::NONE && mods != KeyModifiers::SHIFT +} + /// Short uppercase name for the title bar. Moved off [`AppMode`] so the enum /// that crosses the wire carries no English (improvise-k099). pub fn mode_label(mode: &AppMode) -> &'static str { @@ -187,11 +202,11 @@ pub fn hint_line(mode: &AppMode) -> String { /// The status bar's rendering of one hint's keys: chords dropped, the rest /// run together when every one is a single character. §8 says why. -fn compact(keys: &[String]) -> String { - let plain: Vec<&str> = keys +fn compact(keys: &[(KeyCode, KeyModifiers)]) -> String { + let plain: Vec = keys .iter() - .map(String::as_str) - .filter(|k| !k.starts_with("Ctrl-") && !k.starts_with("Alt-")) + .filter(|(_, mods)| !is_chord(*mods)) + .map(|(code, mods)| key_label(code, *mods)) .collect(); match plain.iter().all(|k| k.chars().count() == 1) { true => plain.concat(), @@ -201,13 +216,19 @@ fn compact(keys: &[String]) -> String { /// Every key that reaches `command` with `args` in `mode`, parents included, /// sorted by label so the answer does not move between processes (gotcha 18). -pub fn keys_for(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Vec { +pub fn keys_for( + mode: &ModeKey, + command: &str, + args: Option<&[String]>, +) -> Vec<(KeyCode, KeyModifiers)> { let mut keys = match Keymaps::shared().get(mode) { - Some(keymap) => keymap.labels_for(command, args), + Some(keymap) => keymap.keys_running(command, args), None => Vec::new(), }; - keys.sort(); - keys.dedup(); + keys.sort_by_key(|(code, mods)| key_label(code, *mods)); + // By label, not by pattern: two patterns can spell the same key, and the + // status bar drew it twice when they did. + keys.dedup_by_key(|(code, mods)| key_label(code, *mods)); keys } @@ -227,13 +248,13 @@ pub fn which_key(keymap: &Keymap) -> Vec { Binding::Prefix(_) => return None, }; Some(Hint { - keys: vec![crate::keymap::key_label(code, *mods)], + keys: vec![(*code, *mods)], command, caption: describe(command), }) }) .collect(); - hints.sort_by(|a, b| a.keys.cmp(&b.keys)); + hints.sort_by_key(Hint::labels); hints } @@ -317,7 +338,7 @@ fn keys( match found.is_empty() { true => None, false => Some(HelpLine::Keys { - keys: found, + keys: found.iter().map(|(c, m)| key_label(c, *m)).collect(), description, }), } @@ -827,7 +848,7 @@ mod tests { .expect("Normal advertises the cursor"); for key in ["h", "j", "k", "l"] { assert!( - nav.keys.iter().any(|k| k == key), + nav.labels().iter().any(|k| k == key), "{key} is bound to move-selection and must appear: {:?}", nav.keys ); @@ -864,19 +885,50 @@ mod tests { /// keys that are all chords leave nothing to draw. #[test] fn the_compact_rendering_drops_chords_and_says_nothing_when_all_are() { - let of = |ks: &[&str]| compact(&ks.iter().map(|k| (*k).to_string()).collect::>()); - assert_eq!(of(&["h", "j", "k", "l"]), "hjkl"); - assert_eq!(of(&["Esc", "Tab"]), "Esc/Tab"); + const NONE: KeyModifiers = KeyModifiers::NONE; + const CTRL: KeyModifiers = KeyModifiers::CONTROL; + const ALT: KeyModifiers = KeyModifiers::ALT; + let ch = |c: char, m| (KeyCode::Char(c), m); + assert_eq!(compact(&['h', 'j', 'k', 'l'].map(|c| ch(c, NONE))), "hjkl"); assert_eq!( - of(&["T", "Ctrl-\u{2190}", "Alt-x"]), + compact(&[(KeyCode::Esc, NONE), (KeyCode::Tab, NONE)]), + "Esc/Tab" + ); + assert_eq!( + compact(&[ch('T', NONE), (KeyCode::Left, CTRL), ch('x', ALT)]), "T", "a chord is not worth the width it costs in a status bar" ); assert_eq!( - of(&["Ctrl-d", "Ctrl-u"]), + compact(&[ch('d', CTRL), ch('u', CTRL)]), "", "and all-chords is empty rather than a bare colon" ); + assert_eq!( + compact(&[(KeyCode::BackTab, KeyModifiers::SHIFT)]), + "S-Tab", + "shift alone is not a chord — it is how a key is named, not held" + ); + } + + /// A chord is dropped by its MODIFIERS, not by its spelling — the defect + /// improvise-thpe closed. See `context/test-evidence/protocol.md`. + #[test] + fn a_chord_is_recognised_by_its_modifiers_and_not_by_its_label() { + let exotic = KeyModifiers::CONTROL | KeyModifiers::ALT | KeyModifiers::SHIFT; + assert!( + is_chord(exotic), + "any modifier beyond shift is a chord, whatever it is called" + ); + assert_eq!( + compact(&[ + (KeyCode::Char('T'), KeyModifiers::NONE), + (KeyCode::F(5), exotic) + ]), + "T", + "the label is built from the modifiers, so nothing may read them \ + back out of it" + ); } /// A caption is looked up by **command name**, so a localized front end diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 23bda05b..8621c757 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -235,15 +235,6 @@ impl Keymap { keys } - /// The same keys, spelled. One walk, so a caller asking "which keys" and - /// a caller asking "how are they written" cannot get different answers. - pub fn labels_for(&self, command: &str, args: Option<&[String]>) -> Vec { - self.keys_running(command, args) - .iter() - .map(|(code, mods)| key_label(code, *mods)) - .collect() - } - /// Look up the binding for a key in this keymap's own bindings. fn lookup_local(&self, key: KeyCode, mods: KeyModifiers) -> Option<&Binding> { self.bindings diff --git a/src/ui/which_key.rs b/src/ui/which_key.rs index 09547217..11139aa3 100644 --- a/src/ui/which_key.rs +++ b/src/ui/which_key.rs @@ -28,7 +28,7 @@ impl Widget for WhichKeyWidget<'_> { let content_width = self .hints .iter() - .map(|hint| hint.keys.concat().len() + 2 + hint.caption.len()) + .map(|hint| hint.labels().concat().len() + 2 + hint.caption.len()) .max() .unwrap_or(10); let popup_w = (content_width as u16 + 4).min(area.width); // +4 for border + padding @@ -58,7 +58,7 @@ impl Widget for WhichKeyWidget<'_> { break; } let y = inner.y + i as u16; - buf.set_string(inner.x + 1, y, hint.keys.concat(), key_style); + buf.set_string(inner.x + 1, y, hint.labels().concat(), key_style); let cmd_x = inner.x + 4; // fixed column for what the key does if cmd_x < inner.x + inner.width { buf.set_string(cmd_x, y, hint.caption, cmd_style);