diff --git a/context/code/cli.md b/context/code/cli.md index e1c20ca3..d4cc9a68 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -151,7 +151,75 @@ atomicity is preserved by keeping the existing whole-line `parse_line` as a still parses, and a line that did not still does not, so the only behavioural change is which cursor the commands see. +**That atomicity claim was false as written** (improvise-2bac), and §8 is what +makes it true. The gate runs the pure arm; the loop runs the interactive arm. A +gate is only a gate if *every argument list the pure arm accepts, the +interactive arm accepts too* — and `commit-cell-edit` broke it: +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +passed the gate, applied the first segment, and then refused the second with +`no cell at cursor`. Half the line landed. + The TUI's `:` line had the same defect and is fixed the same way, with one limitation that is not new: `ExecuteCommand` returns one effect batch, so the segments of `:a . b` all see the pre-batch cursor, exactly as a `Binding::Sequence` does (gotchas 21 and 23). + +## 8 — the interactive arm is `parse`, then context; it is never a second parser + +#151 fixed three instances of *a registry with two construction arms demotes +whichever caller reaches the weaker one*, one patch each. A post-merge review +found the fourth (improvise-2bac): `commit-cell-edit `, +whose interactive arm read only ``, took the value from the `edit` buffer +and the key from the cursor. So a script that wrote 5 to `Region/East` +**deleted the cell under the cursor** — the empty buffer, committed — with exit +0 and nothing on stderr. The fifth was already in the tree: +`toggle-panel-and-focus false false` ignored both flags and toggled. + +The shape rather than the instances is what changed. `CmdRegistry::register` +used to take two free functions, `parse(args)` and `interactive(args, ctx)`, +and nothing related them; each command re-derived its arguments twice and every +disagreement was silent. It now takes three pieces, and the interactive arm is +**built from `parse`**, not beside it: + +- `parse(args) -> C` — the only function that reads arguments a caller supplied. +- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and + never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is + handed the built command and **not the args**, so it cannot discard them. +- `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused. + This is where "no coordinates means the cursor" lives: `clear-cell` bare, and + `commit-cell-edit ` bare, which is what the keymap binds. + +Two consequences hold by construction rather than by test. **Whatever `parse` +accepts, the interactive arm accepts** — `contextualize` is infallible — so the +§7 gate cannot pass a segment the loop then refuses. And **an accepted argument +list cannot be overridden by the cursor**, because the only function that sees +both args and context never runs on an accepted list. `register_pure` and +`register_nullary` already had one arm; they are the `contextualize = identity, +fallback = None` case. + +What is still representable, stated rather than hidden: + +- `contextualize` can overwrite a field `parse` set. It would have to name the + field, which is a visible line in review, where the old `|_args, ctx|` was an + invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor` + fails if `commit-cell-edit`'s does. +- A `fallback` sees the args `parse` refused and could ignore some of them. + `commit-cell-edit`'s refuses anything but a lone mode, so the malformed + `commit-cell-edit editing 5` is an error on the `:` line — which has no pure + gate — rather than a commit of the edit buffer to the cursor cell. +- The gate is **stricter** than the loop wherever a fallback exists: a bare + `clear-cell` is refused in a script. That is the safe direction — the whole + line is refused — and it is unchanged from before #151. + +`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm +defaulted the omitted flags to `true` and a contextualizer cannot tell an +omitted flag from an explicit one. So the omission is now in the type — +`target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is +resolved in `execute` against the same context. That is the honest home: the +decision depends on context, and `execute` is where context is. + +Guards: `every_argument_list_the_gate_accepts_the_run_accepts_too` enumerates +**every registration** against every candidate argument list, in a context whose +cursor sits on no cell — the context in which the old `commit-cell-edit` arm +refused. It lists no commands, so a new registration is covered the day it +lands. It is backed by the process-level tests in `tests/headless_cli.rs`. diff --git a/context/code/command.md b/context/code/command.md index 02de2469..987df348 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2489,12 +2489,13 @@ line separated by `.`, coordinate pairs written `Category/Item`, and quoted stri available for the rare command that needs to enumerate all views, such as the view panel. -`InteractiveFn` constructs a `Cmd` from the interactive context (keymap dispatch). -It receives both the keymap args and the interactive context, so commands can -combine text arguments (a panel name) with runtime state (whether that panel is -currently open). `register_pure` registers a command that does not need interactive -context: called interactively with args it delegates to `parse`, and without args it -errors. +The interactive arm (keymap dispatch, scripts, the `:` line) is built **from** +`parse`: `register` takes `parse`, a `contextualize` that fills context-only +fields of the command `parse` built without seeing the args, and an optional +`fallback` reached only for argument lists `parse` refused. So an accepted +argument list is never overridden by context (improvise-2bac; the argument is +`context/code/cli.md` §8). `register_pure` is the case with nothing to +contextualize and no fallback: both arms are `parse`. `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index bf5f7818..4c89ddee 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -229,12 +229,94 @@ the syntax gate and then cleared whatever the cursor was sitting on — the orig 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 +arms went 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. +carries no arguments. That helper is gone since improvise-2bac: the cursor is +now the registry's `fallback`, reached only when `parse` refused, so explicit +coordinates win by construction (`context/code/cli.md` §8). 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"]`. + +### commit_cell_edit_writes_the_named_cell_and_spares_the_cursors + +improvise-2bac, found by the post-merge adversarial review of PR #151: the +fourth instance of that PR's own lesson. `commit-cell-edit` was registered with +two free arms. The pure arm read ` `; the interactive arm +read only ``, took the value from the `edit` buffer and the key from the +cursor. Once #151 made scripts construct through the interactive arm, +`commit-cell-edit editing 5 Region/East` passed the syntax gate and then +committed an **empty** buffer to the cursor cell — deleting it — with exit 0 +and nothing on stderr. + +Reproduced at `40b8913` before the fix, through the real binary: the document +came back as `["Region=East = 3", "Region=North = 1"]` — `South`, the cell the +cursor had been moved onto, was gone and `East` was unchanged. + +The fixture writes three cells and **moves the cursor to `South`** in the same +line before committing to `East`, so the cursor sits on neither the target nor +the origin: a test with the cursor on the target is right by accident, and one +that only asserts the target misses the destruction. It asserts all three. + +Injected rivals, run: (a) `contextualize` for `CommitAndAdvance` overwriting +`key` and `value` from the cursor and the buffer — fails this test and +`a_line_whose_gate_passes_runs_whole`; (b) the registry dispatching to the +fallback first whenever one exists (the pre-fix shape) — fails this test, +`a_line_whose_gate_passes_runs_whole`, `an_explicit_coordinate_still_beats_the_cursor` +and both registry unit tests below. + +### a_line_whose_gate_passes_runs_whole + +improvise-2bac's second symptom, reported on the ticket: `context/code/cli.md` +§7 said the whole-line `parse_line` gate preserved line atomicity. It did not, +wherever the interactive arm refused an argument list the pure arm accepted. +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +printed `Parse error: commit-cell-edit: no cell at cursor` after `Foo` had +already been added and saved: half the line landed. + +The assertion is on `Parse error` and on the committed cell, **not** on the exit +code. The line exits 1 both before #151 and after this fix, for a different and +pre-existing reason: the grid has no rows, so `commit-cell-edit`'s advance +emits `AbortChain`, and the CLI maps an aborted batch to exit 1 +(`context/code/cli.md` §3). Asserting success would have made this test fail on +the correct behaviour. + +### a_malformed_commit_line_is_refused_whole + +The other half of atomicity, which must survive the fix: a segment the gate +refuses (`commit-cell-edit editing 5` — a value and no coordinates) refuses the +whole line, so the preceding `add-category Foo` never lands. It passed before +the fix and must keep passing; its job is to catch a "fix" that loosens the +pure arm to make the gate agree. + +### commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor + +The unit-level form of the headless test, against `CmdRegistry::interactive` +directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an +explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key +and value. Rival (a) above — a `contextualize` that overwrites the parsed +fields — fails it; this is the one hole the new registry shape leaves +representable, so it is the one that needs a test. + +### every_argument_list_the_gate_accepts_the_run_accepts_too + +Enumerates **every** registration (`CmdRegistry::names`, no hand-kept list) +against every candidate argument list, and requires the interactive arm to +accept whatever the pure arm accepts, in a context whose cursor sits on **no +cell** — a category with no items. That context is what made the old +`commit-cell-edit` arm refuse, so a context with a cell under the cursor would +have passed the bug. The assertion that the cursor has no cell is checked +first; a plain `Workbook::new` does not satisfy it, which is how the fixture +came to add the empty category. Rival (b) fails it. + +### explicit_open_and_focus_arguments_beat_the_toggle + +The fifth instance, named on improvise-2bac as plausible and not yet run: +`toggle-panel-and-focus false false` ignored both flags and toggled, so +on a **closed** panel it opened it. The fixture closes every panel first, +because on an open-and-focused panel the toggle also closes it and the test +would pass against the bug. Rival: `execute` ignoring `target` and always +toggling — fails this test for every panel. diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 4af2470f..491718cb 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -139,9 +139,12 @@ pub trait Cmd: Debug + Send + Sync { /// Factory that constructs a Cmd from text arguments (headless/script). pub type ParseFn = fn(&[String]) -> Result, String>; -/// Factory constructing a Cmd from the interactive context. It takes both the keymap -/// args and the context, so a command can combine text args with runtime state. -pub type InteractiveFn = fn(&[String], &CmdContext) -> Result, String>; +/// Fills the context-only fields of a command `parse` already built. It never +/// sees the args, so it cannot discard them — `context/code/cli.md` §8. +pub type ContextualizeFn = fn(C, &CmdContext) -> C; + +/// Builds a command from context, reached ONLY for args `parse` refused. +pub type FallbackFn = fn(&[String], &CmdContext) -> Result; type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; @@ -183,13 +186,26 @@ impl CmdRegistry { name } - /// Register a command with both a text parser and an interactive constructor. - /// The name is derived from a prototype command instance. - pub fn register(&mut self, prototype: &dyn Cmd, parse: ParseFn, interactive: InteractiveFn) { + /// Register a command whose interactive arm is `parse` then `contextualize`, so + /// args `parse` accepts are honoured whole — `context/code/cli.md` §8. + pub fn register( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + contextualize: ContextualizeFn, + fallback: Option>, + ) { self.entries.push(CmdEntry { name: prototype.name(), - parse: Box::new(parse), - interactive: Box::new(interactive), + parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), + interactive: Box::new(move |args, ctx| { + let built = match (parse(args), fallback) { + (Ok(cmd), _) => Ok(contextualize(cmd, ctx)), + (Err(_), Some(fallback)) => fallback(args, ctx), + (Err(err), None) => Err(err), + }; + built.map(|c| Box::new(c) as Box) + }), }); } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 29d1153d..1d27a42f 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -20,8 +20,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: true, - focused: true, + target: Some((true, true)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -40,8 +39,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: false, - focused: false, + target: Some((false, false)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -531,20 +529,24 @@ mod tests { #[derive(Debug)] pub struct TogglePanelAndFocus { pub panel: Panel, - pub open: bool, - pub focused: bool, + /// Explicit `(open, focused)`; `None` toggles: close if open and focused, else open and focus. + pub target: Option<(bool, bool)>, } impl Cmd for TogglePanelAndFocus { fn name(&self) -> &'static str { "toggle-panel-and-focus" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { + let (open, focused) = self.target.unwrap_or_else(|| { + let shown = ctx.panel_open(self.panel) && ctx.mode.panel() == Some(self.panel); + (!shown, !shown) + }); let mut effects: Vec> = Vec::new(); effects.push(Box::new(effect::SetPanelOpen { panel: self.panel, - open: self.open, + open, })); - if self.focused { + if focused { effects.push(effect::change_mode(self.panel.mode())); } else { effects.push(effect::change_mode(AppMode::Normal)); diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index de1d2985..6ef243dc 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,15 +22,50 @@ 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)) +/// The cell under the cursor: what a coordinate command falls back to when given none. +fn cursor_key(ctx: &CmdContext) -> Result { + ctx.cell_key().clone().ok_or("no cell at cursor".into()) +} + +/// A `Move` built from args takes the live cursor as its origin. +fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move { + Move { + cursor: CursorState::from_ctx(ctx), + ..cmd } } +/// A commit built from args still advances from the live cursor. +fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance { + CommitAndAdvance { + cursor: CursorState::from_ctx(ctx), + ..cmd + } +} + +/// `` alone: commit the edit buffer to the cell under the cursor. +fn commit_at_cursor( + args: &[String], + ctx: &CmdContext, + advance: AdvanceDir, +) -> Result { + let [mode] = args else { + return Err("takes a mode alone, or a mode, a value and coordinates".into()); + }; + Ok(CommitAndAdvance { + edit_mode: parse_mode_name(mode)?, + value: read_buffer(ctx, "edit"), + key: cursor_key(ctx)?, + advance, + cursor: CursorState::from_ctx(ctx), + }) +} + +/// For a command with no context-only fields: `parse`'s result is used as built. +fn as_parsed(cmd: C, _ctx: &CmdContext) -> C { + cmd +} + /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -51,14 +86,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("clear-cell requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(ClearCellCommand { + Ok(ClearCellCommand { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(ClearCellCommand { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(ClearCellCommand { + key: cursor_key(ctx)?, + }) + }), ); r.register_pure(&AddFormulaCmd(vec![]), AddFormulaCmd::parse); r.register_pure(&FitMeasure::prototype(), FitMeasure::parse); @@ -80,16 +117,8 @@ pub fn default_registry() -> CmdRegistry { r.register_pure(&ExportCmd(vec![]), ExportCmd::parse); r.register_pure(&WriteCmd(vec![]), WriteCmd::parse); r.register_pure(&HelpCmd(vec![]), HelpCmd::parse); - r.register( - &HelpPageNextCmd(vec![]), - HelpPageNextCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPageNextCmd(vec![]))), - ); - r.register( - &HelpPagePrevCmd(vec![]), - HelpPagePrevCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPagePrevCmd(vec![]))), - ); + r.register_pure(&HelpPageNextCmd(vec![]), HelpPageNextCmd::parse); + r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); // ── Navigation (unified Move) ────────────────────────────────────── r.register( @@ -102,22 +131,14 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(dr, dc), cursor: CursorState::default(), cmd_name: "move-selection", - })) - }, - |args, ctx| { - require_args(args, 2)?; - let dr = args[0].parse::().map_err(|e| e.to_string())?; - let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(dr, dc), - cursor: CursorState::from_ctx(ctx), - cmd_name: "move-selection", - })) + }) }, + move_from_cursor, + None, ); // Jump-to-edge commands: first/last row/col macro_rules! reg_jump { @@ -133,7 +154,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: $name, }, |_| { - Ok(Box::new(Move { + Ok(Move { kind: if $to_end { MoveKind::ToEnd($is_row) } else { @@ -141,19 +162,10 @@ pub fn default_registry() -> CmdRegistry { }, cursor: CursorState::default(), cmd_name: $name, - })) - }, - |_, ctx| { - Ok(Box::new(Move { - kind: if $to_end { - MoveKind::ToEnd($is_row) - } else { - MoveKind::ToStart($is_row) - }, - cursor: CursorState::from_ctx(ctx), - cmd_name: $name, - })) + }) }, + move_from_cursor, + None, ); }; } @@ -170,21 +182,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(n, 0), cursor: CursorState::default(), cmd_name: "scroll-rows", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(n, 0), - cursor: CursorState::from_ctx(ctx), - cmd_name: "scroll-rows", - })) + }) }, + move_from_cursor, + None, ); r.register( &Move { @@ -195,28 +200,21 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Page(dir), cursor: CursorState::default(), cmd_name: "page-scroll", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Page(dir), - cursor: CursorState::from_ctx(ctx), - cmd_name: "page-scroll", - })) + }) }, + move_from_cursor, + None, ); r.register( &EnterAdvance { cursor: CursorState::default(), }, |_| { - Ok(Box::new(EnterAdvance { + Ok(EnterAdvance { cursor: CursorState { row: 0, col: 0, @@ -227,13 +225,12 @@ pub fn default_registry() -> CmdRegistry { visible_rows: 20, visible_cols: 8, }, - })) + }) }, - |_, ctx| { - Ok(Box::new(EnterAdvance { - cursor: CursorState::from_ctx(ctx), - })) + |_, ctx| EnterAdvance { + cursor: CursorState::from_ctx(ctx), }, + None, ); // ── Cell operations ────────────────────────────────────────────────── @@ -245,14 +242,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("yank requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(YankCell { + Ok(YankCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(YankCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(YankCell { + key: cursor_key(ctx)?, + }) + }), ); r.register( &PasteCell { @@ -262,14 +261,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(PasteCell { + Ok(PasteCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(PasteCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(PasteCell { + key: cursor_key(ctx)?, + }) + }), ); // clear-cell is registered above (unified: ctx.cell_key() or explicit coords) @@ -313,21 +314,19 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("drill-into-cell requires Cat/Item coordinates".into()); } - Ok(Box::new(DrillIntoCell { + Ok(DrillIntoCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(DrillIntoCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(DrillIntoCell { + key: cursor_key(ctx)?, + }) + }), ); r.register_nullary(|| Box::new(ViewNavigate { forward: false })); - r.register( - &ViewNavigate { forward: true }, - |_| Ok(Box::new(ViewNavigate { forward: true })), - |_, _| Ok(Box::new(ViewNavigate { forward: true })), - ); + r.register_nullary(|| Box::new(ViewNavigate { forward: true })); r.register_pure(&EnterMode(AppMode::Normal), |args| { require_args(args, 1)?; Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) @@ -345,38 +344,20 @@ pub fn default_registry() -> CmdRegistry { r.register( &TogglePanelAndFocus { panel: Panel::Formula, - open: true, - focused: true, + target: None, }, |args| { // Parse: toggle-panel-and-focus [open] [focused] require_args(args, 1)?; let panel = parse_panel(&args[0])?; - let open = args.get(1).map(|s| s == "true").unwrap_or(true); - let focused = args.get(2).map(|s| s == "true").unwrap_or(open); - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - // Default interactive: if already open+focused -> close, else open+focus - let currently_open = ctx.panel_open(panel); - let currently_focused = ctx.mode.panel() == Some(panel); - let (open, focused) = if currently_open && currently_focused { - (false, false) // close - } else { - (true, true) // open + focus - }; - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) + let target = args.get(1).map(|s| { + let open = s == "true"; + (open, args.get(2).map(|s| s == "true").unwrap_or(open)) + }); + Ok(TogglePanelAndFocus { panel, target }) }, + as_parsed, + None, ); r.register( &TogglePanelVisibility { @@ -386,20 +367,16 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; - Ok(Box::new(TogglePanelVisibility { + Ok(TogglePanelVisibility { panel, currently_open: false, - })) + }) }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - let currently_open = ctx.panel_open(panel); - Ok(Box::new(TogglePanelVisibility { - panel, - currently_open, - })) + |cmd, ctx| TogglePanelVisibility { + currently_open: ctx.panel_open(cmd.panel), + ..cmd }, + None, ); r.register( &CyclePanelFocus { @@ -408,19 +385,18 @@ pub fn default_registry() -> CmdRegistry { view_open: false, }, |_| { - Ok(Box::new(CyclePanelFocus { + Ok(CyclePanelFocus { formula_open: false, category_open: false, view_open: false, - })) + }) }, - |_, ctx| { - Ok(Box::new(CyclePanelFocus { - formula_open: ctx.formula_panel_open, - category_open: ctx.category_panel_open, - view_open: ctx.view_panel_open, - })) + |_, ctx| CyclePanelFocus { + formula_open: ctx.formula_panel_open, + category_open: ctx.category_panel_open, + view_open: ctx.view_panel_open, }, + None, ); r.register( &MovePanelCursor { @@ -433,29 +409,26 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(MovePanelCursor { + Ok(MovePanelCursor { panel, delta, current: 0, max: 0, - })) + }) }, - |args, ctx| { - require_args(args, 2)?; - let panel = parse_panel(&args[0])?; - let delta = args[1].parse::().map_err(|e| e.to_string())?; - let (current, max) = match panel { + |cmd, ctx| { + let (current, max) = match cmd.panel { Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), }; - Ok(Box::new(MovePanelCursor { - panel, - delta, + MovePanelCursor { current, max, - })) + ..cmd + } }, + None, ); r.register_nullary(|| Box::new(DeleteFormulaAtCursor)); r.register_nullary(|| Box::new(AddRecordRow)); @@ -490,11 +463,7 @@ pub fn default_registry() -> CmdRegistry { // ── Grid operations ────────────────────────────────────────────────── r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: true })); - r.register( - &ToggleGroupAtCursor { is_row: false }, - |_| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - |_, _| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - ); + r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: false })); r.register_nullary(|| Box::new(HideSelectedRowItem)); // ── Text buffer ────────────────────────────────────────────────────── @@ -537,27 +506,16 @@ pub fn default_registry() -> CmdRegistry { return Err("commit-cell-edit requires a mode, value, and coords".into()); } let edit_mode = parse_mode_name(&args[0])?; - Ok(Box::new(CommitAndAdvance { + Ok(CommitAndAdvance { key: parse_cell_key_from_args(&args[2..]), value: args[1].clone(), advance: AdvanceDir::Down, cursor: CursorState::default(), edit_mode, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Down, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) + }) }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)), ); r.register( &CommitAndAdvance { @@ -568,19 +526,8 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), }, |_| Err("commit-and-advance-right requires context".into()), - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Right, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) - }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); r.register( &CommitBuffer { @@ -589,13 +536,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; - Ok(Box::new(CommitBuffer { kind })) + Ok(CommitBuffer { kind }) }, - |args, ctx| { + as_parsed, + Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; - Ok(Box::new(CommitBuffer { kind })) - }, + Ok(CommitBuffer { kind }) + }), ); r.register_nullary(|| Box::new(CommitExport)); r.register_nullary(|| Box::new(CommitSave)); @@ -641,6 +589,9 @@ mod tests { vec!["1", "1"], vec!["formula", "1"], vec!["editing", "5", "Region", "East"], + vec!["editing", "5", "Region/East"], + vec!["formula", "false", "false"], + vec!["Region/East"], ] .into_iter() .map(|v| v.into_iter().map(String::from).collect()) @@ -802,6 +753,67 @@ mod tests { "only {checked} ex-command examples were found; the scan has stopped seeing them" ); } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor() { + let mut app = crate::ui::app::App::new(two_cat_model(), None); + app.view_state.buffers.insert("edit".into(), "decoy".into()); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + let target = CellKey::new(vec![ + ("Month".into(), "Feb".into()), + ("Type".into(), "Clothing".into()), + ]); + assert_ne!( + ctx.cell_key().as_ref(), + Some(&target), + "the cursor is elsewhere" + ); + let args: Vec = ["editing", "5", "Type/Clothing", "Month/Feb"] + .map(String::from) + .into(); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("commit-cell-edit", &args, &ctx) + .expect("commit-cell-edit resolves") + ); + assert!( + dbg.contains(&format!("key: {target:?}")), + "wrong cell: {dbg}" + ); + assert!(dbg.contains("value: \"5\""), "wrong value: {dbg}"); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn every_argument_list_the_gate_accepts_the_run_accepts_too() { + let r = default_registry(); + let mut wb = crate::workbook::Workbook::new("Empty"); + wb.add_category("Foo").expect("a category with no items"); + let app = crate::ui::app::App::new(wb, None); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + assert!(ctx.cell_key().is_none(), "the cursor must sit on no cell"); + let mut accepted = 0; + for name in r.names().collect::>() { + for args in candidate_args() { + if r.parse(name, &args).is_err() { + continue; + } + accepted += 1; + if let Err(e) = r.interactive(name, &args, &ctx) { + panic!("the gate accepts `{name} {args:?}` but the run refuses it: {e}"); + } + } + } + assert!(accepted > 50, "only {accepted} accepted pairs were tried"); + } } #[cfg(test)] @@ -880,6 +892,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: false"), @@ -897,6 +910,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: true"), @@ -905,4 +919,29 @@ mod panel_open_tests { ); } } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn explicit_open_and_focus_arguments_beat_the_toggle() { + for panel in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + app.view_state.formula_panel_open = false; + app.view_state.category_panel_open = false; + app.view_state.view_panel_open = false; + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let args = [panel.name().to_string(), "false".into(), "false".into()]; + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + .execute(&ctx) + ); + assert!( + dbg.contains("open: false") && !dbg.contains("open: true"), + "`{} false false` on a closed panel toggled it open: {dbg}", + panel.name() + ); + } + } } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index f5ae9bb6..f0751f16 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -357,3 +357,86 @@ fn an_explicit_coordinate_still_beats_the_cursor() { "the named cell survived: {left:?}" ); } + +/// North=1, South=2, East=3 on the row axis; the cursor starts on North. +fn three_cells(scratch: &Scratch, name: &str) -> PathBuf { + let doc = three_rows(scratch, name); + 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"); + doc +} + +fn value_of(doc: &Path, item: &str) -> Option { + cell_lines(doc) + .into_iter() + .find(|l| l.contains(&format!("Region={item}"))) + .and_then(|l| l.rsplit(" = ").next().map(str::to_string)) +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn commit_cell_edit_writes_the_named_cell_and_spares_the_cursors() { + let scratch = Scratch::new("commit-explicit"); + let doc = three_cells(&scratch, "commit.improv"); + + let out = run_cmd( + &["move-selection 1 0 . commit-cell-edit editing 5 Region/East"], + &doc, + ); + assert!(out.status.success(), "the line failed: {}", stderr_of(&out)); + let cells = cell_lines(&doc); + assert_eq!(value_of(&doc, "East").as_deref(), Some("5"), "{cells:?}"); + assert_eq!( + value_of(&doc, "South").as_deref(), + Some("2"), + "the cell under the cursor was touched: {cells:?}" + ); + assert_eq!( + value_of(&doc, "North").as_deref(), + Some("1"), + "the origin cell was touched: {cells:?}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_line_whose_gate_passes_runs_whole() { + let scratch = Scratch::new("commit-atomic"); + let doc = scratch.path("atomic.improv"); + let out = run_cmd( + &["add-category Foo . commit-cell-edit editing 5 Foo/x"], + &doc, + ); + assert!( + !stderr_of(&out).contains("Parse error"), + "a line the gate accepted was refused part-way: {}", + stderr_of(&out) + ); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + text.lines() + .any(|l| l.contains("Foo=x") && l.ends_with("= 5")), + "the commit did not land: {text}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_malformed_commit_line_is_refused_whole() { + let scratch = Scratch::new("commit-malformed"); + let doc = seeded(&scratch, "malformed.improv"); + let out = run_cmd(&["add-category Foo . commit-cell-edit editing 5"], &doc); + assert!(!out.status.success(), "a malformed line exited 0"); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + !text.contains("Foo"), + "the first half of a refused line landed: {text}" + ); +}