diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 668dce11..bf5f7818 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -213,3 +213,28 @@ commands — plus the empty segment in `a . . b`. is a shape an unknown *word* must not be able to reach: a silently skipped command in a script is the partial-and-silent failure this repo keeps finding. The error also names the word, matching what `CmdRegistry::parse` does. + +### an_explicit_coordinate_still_beats_the_cursor + +Found by the ChatGPT Codex review of PR #151 at head `64bcb82`, and correct: the +first version of improvise-6uva's fix introduced a regression it was not looking +for. Four registry entries — `clear-cell`, `yank`, `paste`, `drill-into-cell` — +were registered with `register`, and their **interactive** arms took `|_args, +ctx|`: they discarded the arguments and used `ctx.cell_key()`. That was harmless +while a script only ever reached the *pure* arm, which reads the coordinates. The +moment the headless loop re-parsed interactively, `clear-cell Region/East` passed +the syntax gate and then cleared whatever the cursor was sitting on — the origin, +`Region/North`. + +So improvise-6uva's own lesson had a **third** instance in its own fix: two +construction arms that mean different things demote whichever caller reaches the +weaker one, and here the "weaker" one was the arm that had the context. The four +arms now go through `key_from_args_or_cursor`: explicit coordinates win, the +cursor is the fallback, which is also what the keymap path wants since a binding +carries no arguments. + +The fixture writes three cells and clears the one the cursor is **not** on, so +the assertion separates "cleared the named cell" from "cleared the cursor cell" +— clearing `North` by name would have passed against the broken code. Restoring +the arg-discarding arm fails it: `the cursor cell was cleared instead of the +named one: ["Region=East = 3", "Region=South = 2"]`. diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 99905588..de1d2985 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,6 +22,15 @@ use super::text_buffer::*; use super::tile::*; use super::wizard::*; +/// Explicit coordinates win; the cursor is the fallback — `context/code/cli.md`. +fn key_from_args_or_cursor(args: &[String], ctx: &CmdContext) -> Result { + if args.is_empty() { + ctx.cell_key().clone().ok_or("no cell at cursor".into()) + } else { + Ok(parse_cell_key_from_args(args)) + } +} + /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -46,8 +55,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(ClearCellCommand { key })) }, ); @@ -240,8 +249,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(YankCell { key })) }, ); @@ -257,8 +266,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(PasteCell { key })) }, ); @@ -308,8 +317,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(DrillIntoCell { key })) }, ); diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 99c9427b..f5ae9bb6 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -317,3 +317,43 @@ fn a_script_cursor_move_survives_the_line_that_made_it() { run_cmd(&["move-selection 1 0", "hide-selected-row-item"], &doc); assert_eq!(hidden_line(&doc), "hidden: Region/South"); } + +fn cell_lines(doc: &Path) -> Vec { + std::fs::read_to_string(doc) + .expect("the document") + .lines() + .filter(|l| l.contains(" = ")) + .map(|l| l.trim().to_string()) + .collect() +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn an_explicit_coordinate_still_beats_the_cursor() { + let scratch = Scratch::new("explicit"); + let doc = scratch.path("cells.improv"); + run_cmd(&["add-category Region"], &doc); + run_cmd(&["add-items Region North South East"], &doc); + run_cmd( + &[ + "set-cell 1 Region/North", + "set-cell 2 Region/South", + "set-cell 3 Region/East", + ], + &doc, + ); + assert_eq!(cell_lines(&doc).len(), 3, "the fixture wrote three cells"); + + // The cursor is at the origin, which is North. Clearing East by name must + // clear East, not whatever the cursor happens to be sitting on. + run_cmd(&["clear-cell Region/East"], &doc); + let left = cell_lines(&doc); + assert!( + left.iter().any(|l| l.contains("North")), + "the cursor cell was cleared instead of the named one: {left:?}" + ); + assert!( + !left.iter().any(|l| l.contains("East")), + "the named cell survived: {left:?}" + ); +}