From cece34a1d456869198f8b488e0772fd97ebc6e2f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 15 Apr 2026 22:44:13 -0700 Subject: [PATCH] refactor(command): parameterize mode-related commands and effects Make mode-related commands and effects mode-agnostic by passing the target mode as an argument instead of inspecting the current application mode. - `CommitAndAdvance` now accepts `edit_mode` . - `EditOrDrill` now accepts `edit_mode` . - `EnterEditAtCursorCmd` now accepts `target_mode` . - `EnterEditAtCursor` effect now accepts `target_mode` . Update the command registry to parse mode names from arguments and pass them to the corresponding commands. Add tests to verify the new mode-passing behavior. Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf) --- src/command/cmd/commit.rs | 39 +++++++++- src/command/cmd/mode.rs | 149 +++++++++++++++++++++++------------- src/command/cmd/registry.rs | 90 ++++++++++++---------- src/ui/effect.rs | 52 +++++++++++-- 4 files changed, 232 insertions(+), 98 deletions(-) diff --git a/src/command/cmd/commit.rs b/src/command/cmd/commit.rs index e4c67f1..706c692 100644 --- a/src/command/cmd/commit.rs +++ b/src/command/cmd/commit.rs @@ -118,6 +118,35 @@ mod tests { assert!(effects.is_empty()); } + /// `CommitAndAdvance` must thread its `edit_mode` through to the + /// trailing `EnterEditAtCursor` effect so the post-commit re-edit lands + /// in the mode the keymap requested. The command never reads ctx.mode. + #[test] + fn commit_and_advance_threads_edit_mode_to_enter_edit_at_cursor() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let mut bufs = HashMap::new(); + bufs.insert("edit".to_string(), "42".to_string()); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.buffers = &bufs; + // ctx.mode stays Normal — the command must not look at it. + let key = ctx.cell_key().unwrap(); + let cmd = CommitAndAdvance { + key, + value: "42".to_string(), + advance: super::AdvanceDir::Down, + cursor: super::CursorState::from_ctx(&ctx), + edit_mode: AppMode::records_editing(), + }; + let effects = cmd.execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + dbg.contains("EnterEditAtCursor") && dbg.contains("RecordsEditing"), + "Expected trailing EnterEditAtCursor with RecordsEditing target, got: {dbg}" + ); + } + #[test] fn commit_export_produces_export_and_normal_mode() { let m = two_cat_model(); @@ -234,12 +263,18 @@ pub enum AdvanceDir { /// Commit a cell edit, advance the cursor, and re-enter edit mode. /// Subsumes the old `CommitCellEdit` (Down) and `CommitAndAdvanceRight` (Right). +/// +/// `edit_mode` is the editing mode to re-enter after advancing. The keymap +/// binding supplies this — the editing-mode keymap passes `editing` and the +/// records-editing keymap passes `records-editing`. The command itself +/// never inspects `ctx.mode`. #[derive(Debug)] pub struct CommitAndAdvance { pub key: CellKey, pub value: String, pub advance: AdvanceDir, pub cursor: CursorState, + pub edit_mode: AppMode, } impl Cmd for CommitAndAdvance { fn name(&self) -> &'static str { @@ -287,7 +322,9 @@ impl Cmd for CommitAndAdvance { } } } - effects.push(Box::new(effect::EnterEditAtCursor)); + effects.push(Box::new(effect::EnterEditAtCursor { + target_mode: self.edit_mode.clone(), + })); effects } } diff --git a/src/command/cmd/mode.rs b/src/command/cmd/mode.rs index 84ed870..1d735e3 100644 --- a/src/command/cmd/mode.rs +++ b/src/command/cmd/mode.rs @@ -10,21 +10,6 @@ mod tests { use crate::command::cmd::test_helpers::*; use crate::model::Model; - #[test] - fn enter_edit_mode_produces_editing_mode() { - let m = two_cat_model(); - let layout = make_layout(&m); - let reg = make_registry(); - let ctx = make_ctx(&m, &layout, ®); - let cmd = EnterEditMode { - initial_value: String::new(), - }; - let effects = cmd.execute(&ctx); - assert_eq!(effects.len(), 2); - let dbg = format!("{:?}", effects[1]); - assert!(dbg.contains("Editing"), "Expected Editing mode, got: {dbg}"); - } - #[test] fn enter_tile_select_with_categories() { let m = two_cat_model(); @@ -98,11 +83,80 @@ mod tests { let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); - let effects = EditOrDrill.execute(&ctx); + let effects = EditOrDrill { + edit_mode: AppMode::editing(), + } + .execute(&ctx); + assert_eq!(effects.len(), 2); let dbg = effects_debug(&effects); assert!(dbg.contains("Editing"), "Expected Editing mode, got: {dbg}"); } + /// EditOrDrill must trust its `edit_mode` parameter rather than checking + /// `ctx.mode` — the records-normal keymap supplies `records-editing`, + /// but the command itself never inspects the runtime mode. This is the + /// parallel of the (deleted) `enter_edit_mode_produces_editing_mode` + /// test for the records branch. + #[test] + fn edit_or_drill_passes_records_editing_mode_through() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + // Note: ctx.mode is still Normal here — the command must not look at it. + let ctx = make_ctx(&m, &layout, ®); + let effects = EditOrDrill { + edit_mode: AppMode::records_editing(), + } + .execute(&ctx); + assert_eq!(effects.len(), 2); + let dbg = effects_debug(&effects); + assert!( + dbg.contains("RecordsEditing"), + "Expected RecordsEditing mode, got: {dbg}" + ); + } + + /// `EnterEditAtCursorCmd` must hand its `target_mode` straight through + /// to the `EnterEditAtCursor` effect — the keymap (records `o` sequence + /// or commit-and-advance) decides; the command never inspects ctx. + #[test] + fn enter_edit_at_cursor_cmd_passes_target_mode_to_effect() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let ctx = make_ctx(&m, &layout, ®); + let effects = EnterEditAtCursorCmd { + target_mode: AppMode::records_editing(), + } + .execute(&ctx); + assert_eq!(effects.len(), 1); + let dbg = format!("{:?}", effects[0]); + assert!( + dbg.contains("RecordsEditing"), + "Expected RecordsEditing target_mode, got: {dbg}" + ); + } + + /// The edit branch pre-fills the `edit` buffer with the cell's current + /// display value so the user can modify rather than retype. + #[test] + fn edit_or_drill_pre_fills_edit_buffer_with_display_value() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.display_value = "42".to_string(); + let effects = EditOrDrill { + edit_mode: AppMode::editing(), + } + .execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + dbg.contains("SetBuffer") && dbg.contains("\"edit\"") && dbg.contains("\"42\""), + "Expected SetBuffer(\"edit\", \"42\"), got: {dbg}" + ); + } + #[test] fn enter_search_mode_sets_flag_and_clears_query() { let m = two_cat_model(); @@ -188,36 +242,18 @@ impl Cmd for SaveAndQuit { // ── Editing entry ─────────────────────────────────────────────────────── -/// Enter editing mode with an initial buffer value. -#[derive(Debug)] -pub struct EnterEditMode { - pub initial_value: String, -} -impl Cmd for EnterEditMode { - fn name(&self) -> &'static str { - "enter-edit-mode" - } - fn execute(&self, ctx: &CmdContext) -> Vec> { - let edit_mode = if ctx.mode.is_records() { - AppMode::records_editing() - } else { - AppMode::editing() - }; - vec![ - Box::new(effect::SetBuffer { - name: "edit".to_string(), - value: self.initial_value.clone(), - }), - effect::change_mode(edit_mode), - ] - } -} - /// Smart dispatch for i/a: if the cursor is on an aggregated pivot cell -/// (categories on `Axis::None`, no records mode), drill into it instead of -/// editing. Otherwise enter edit mode with the current displayed value. +/// (categories on `Axis::None` and the cell is not a synthetic records-mode +/// row), drill into it instead of editing. Otherwise pre-fill the edit +/// buffer with the displayed cell value and enter `edit_mode`. +/// +/// `edit_mode` is supplied by the keymap binding — the command itself is +/// mode-agnostic, so the records-normal keymap passes `records-editing` +/// while the normal keymap passes `editing`. #[derive(Debug)] -pub struct EditOrDrill; +pub struct EditOrDrill { + pub edit_mode: AppMode, +} impl Cmd for EditOrDrill { fn name(&self) -> &'static str { "edit-or-drill" @@ -232,7 +268,8 @@ impl Cmd for EditOrDrill { .map(|cat| cat.kind.is_regular()) .unwrap_or(false) }); - // In records mode (synthetic key), always edit directly — no drilling. + // Synthetic records-mode cells are never aggregated — edit directly. + // (This is a layout property, not a mode flag.) let is_synthetic = ctx.synthetic_record_at_cursor().is_some(); let is_aggregated = !is_synthetic && regular_none; if is_aggregated { @@ -241,23 +278,31 @@ impl Cmd for EditOrDrill { }; return DrillIntoCell { key }.execute(ctx); } - EnterEditMode { - initial_value: ctx.display_value.clone(), - } - .execute(ctx) + vec![ + Box::new(effect::SetBuffer { + name: "edit".to_string(), + value: ctx.display_value.clone(), + }), + effect::change_mode(self.edit_mode.clone()), + ] } } /// Thin command wrapper around the `EnterEditAtCursor` effect so it can -/// participate in `Binding::Sequence`. +/// participate in `Binding::Sequence`. `target_mode` is supplied as the +/// command argument by the keymap binding. #[derive(Debug)] -pub struct EnterEditAtCursorCmd; +pub struct EnterEditAtCursorCmd { + pub target_mode: AppMode, +} impl Cmd for EnterEditAtCursorCmd { fn name(&self) -> &'static str { "enter-edit-at-cursor" } fn execute(&self, _ctx: &CmdContext) -> Vec> { - vec![Box::new(effect::EnterEditAtCursor)] + vec![Box::new(effect::EnterEditAtCursor { + target_mode: self.target_mode.clone(), + })] } } diff --git a/src/command/cmd/registry.rs b/src/command/cmd/registry.rs index 13a727f..8f92a10 100644 --- a/src/command/cmd/registry.rs +++ b/src/command/cmd/registry.rs @@ -2,6 +2,27 @@ use crate::model::cell::CellKey; use crate::ui::app::AppMode; use crate::ui::effect::Panel; +/// Decode a mode-name string (as supplied by `enter-mode`/`edit-or-drill` +/// keymap bindings) into an `AppMode`. +fn parse_mode_name(s: &str) -> Result { + match s { + "normal" => Ok(AppMode::Normal), + "help" => Ok(AppMode::Help), + "formula-panel" => Ok(AppMode::FormulaPanel), + "category-panel" => Ok(AppMode::CategoryPanel), + "view-panel" => Ok(AppMode::ViewPanel), + "tile-select" => Ok(AppMode::TileSelect), + "command" => Ok(AppMode::command_mode()), + "category-add" => Ok(AppMode::category_add()), + "editing" => Ok(AppMode::editing()), + "records-normal" => Ok(AppMode::RecordsNormal), + "records-editing" => Ok(AppMode::records_editing()), + "formula-edit" => Ok(AppMode::formula_edit()), + "export-prompt" => Ok(AppMode::export_prompt()), + other => Err(format!("Unknown mode: {other}")), + } +} + use super::cell::*; use super::commit::*; use super::core::*; @@ -266,22 +287,16 @@ pub fn default_registry() -> CmdRegistry { r.register_nullary(|| Box::new(SaveAndQuit)); r.register_nullary(|| Box::new(SaveCmd)); r.register_nullary(|| Box::new(EnterSearchMode)); - r.register( - &EnterEditMode { - initial_value: String::new(), - }, - |args| { - let val = args.first().cloned().unwrap_or_default(); - Ok(Box::new(EnterEditMode { initial_value: val })) - }, - |_args, ctx| { - Ok(Box::new(EnterEditMode { - initial_value: ctx.display_value.clone(), - })) - }, - ); - r.register_nullary(|| Box::new(EditOrDrill)); - r.register_nullary(|| Box::new(EnterEditAtCursorCmd)); + r.register_pure(&NamedCmd("edit-or-drill"), |args| { + require_args("edit-or-drill", args, 1)?; + let edit_mode = parse_mode_name(&args[0])?; + Ok(Box::new(EditOrDrill { edit_mode })) + }); + r.register_pure(&NamedCmd("enter-edit-at-cursor"), |args| { + require_args("enter-edit-at-cursor", args, 1)?; + let target_mode = parse_mode_name(&args[0])?; + Ok(Box::new(EnterEditAtCursorCmd { target_mode })) + }); r.register_nullary(|| Box::new(EnterExportPrompt)); r.register_nullary(|| Box::new(EnterFormulaEdit)); r.register_nullary(|| Box::new(EnterTileSelect)); @@ -310,23 +325,7 @@ pub fn default_registry() -> CmdRegistry { ); r.register_pure(&NamedCmd("enter-mode"), |args| { require_args("enter-mode", args, 1)?; - let mode = match args[0].as_str() { - "normal" => AppMode::Normal, - "help" => AppMode::Help, - "formula-panel" => AppMode::FormulaPanel, - "category-panel" => AppMode::CategoryPanel, - "view-panel" => AppMode::ViewPanel, - "tile-select" => AppMode::TileSelect, - "command" => AppMode::command_mode(), - "category-add" => AppMode::category_add(), - "editing" => AppMode::editing(), - "records-normal" => AppMode::RecordsNormal, - "records-editing" => AppMode::records_editing(), - "formula-edit" => AppMode::formula_edit(), - "export-prompt" => AppMode::export_prompt(), - other => return Err(format!("Unknown mode: {other}")), - }; - Ok(Box::new(EnterMode(mode))) + Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) }); // ── Search ─────────────────────────────────────────────────────────── @@ -522,25 +521,33 @@ pub fn default_registry() -> CmdRegistry { r.register_nullary(|| Box::new(CommandModeBackspace)); // ── Commit ─────────────────────────────────────────────────────────── + // commit-cell-edit / commit-and-advance-right take a mode-name arg + // (e.g. "editing" or "records-editing") as args[0]. The keymap supplies + // it; the command never inspects ctx.mode. r.register( &CommitAndAdvance { key: CellKey::new(vec![]), value: String::new(), advance: AdvanceDir::Down, cursor: CursorState::default(), + edit_mode: AppMode::editing(), }, |args| { - if args.len() < 2 { - return Err("commit-cell-edit requires a value and coords".into()); + if args.len() < 3 { + return Err("commit-cell-edit requires a mode, value, and coords".into()); } + let edit_mode = parse_mode_name(&args[0])?; Ok(Box::new(CommitAndAdvance { - key: parse_cell_key_from_args(&args[1..]), - value: args[0].clone(), + key: parse_cell_key_from_args(&args[2..]), + value: args[1].clone(), advance: AdvanceDir::Down, cursor: CursorState::default(), + edit_mode, })) }, - |_args, ctx| { + |args, ctx| { + require_args("commit-cell-edit", 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 { @@ -548,6 +555,7 @@ pub fn default_registry() -> CmdRegistry { value, advance: AdvanceDir::Down, cursor: CursorState::from_ctx(ctx), + edit_mode, })) }, ); @@ -557,9 +565,12 @@ pub fn default_registry() -> CmdRegistry { value: String::new(), advance: AdvanceDir::Right, cursor: CursorState::default(), + edit_mode: AppMode::editing(), }, |_| Err("commit-and-advance-right requires context".into()), - |_args, ctx| { + |args, ctx| { + require_args("commit-and-advance-right", 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 { @@ -567,6 +578,7 @@ pub fn default_registry() -> CmdRegistry { value, advance: AdvanceDir::Right, cursor: CursorState::from_ctx(ctx), + edit_mode, })) }, ); diff --git a/src/ui/effect.rs b/src/ui/effect.rs index 872abde..346e081 100644 --- a/src/ui/effect.rs +++ b/src/ui/effect.rs @@ -123,10 +123,19 @@ impl Effect for RemoveFormula { /// Re-enter edit mode by reading the cell value at the current cursor. /// Used after commit+advance to continue data entry. +/// +/// `target_mode` is supplied by the caller (keymap binding via +/// `EnterEditAtCursorCmd`, or `CommitAndAdvance` from its own `edit_mode` +/// field). The effect itself never inspects `app.mode` — the mode is decided +/// statically by whoever invoked us. #[derive(Debug)] -pub struct EnterEditAtCursor; +pub struct EnterEditAtCursor { + pub target_mode: AppMode, +} impl Effect for EnterEditAtCursor { fn apply(&self, app: &mut App) { + // Layout may be stale relative to prior effects in this batch (e.g. + // AddRecordRow added a row); rebuild before reading display_value. app.rebuild_layout(); let ctx = app.cmd_context( crossterm::event::KeyCode::Null, @@ -135,11 +144,7 @@ impl Effect for EnterEditAtCursor { let value = ctx.display_value.clone(); drop(ctx); app.buffers.insert("edit".to_string(), value); - app.mode = if app.mode.is_records() { - AppMode::records_editing() - } else { - AppMode::editing() - }; + app.mode = self.target_mode.clone(); } } @@ -1284,6 +1289,41 @@ mod tests { assert_eq!(app.mode, AppMode::Help); } + /// `EnterEditAtCursor` must use its `target_mode` field, *not* whatever + /// `app.mode` happens to be when applied. Previous implementation + /// branched on `app.mode.is_records()` — the parameterized version + /// trusts the caller (keymap or composing command). + #[test] + fn enter_edit_at_cursor_uses_target_mode_not_app_mode() { + let mut app = test_app(); + // App starts in Normal mode — but caller has decided we want + // RecordsEditing (e.g. records-mode `o` sequence). + assert_eq!(app.mode, AppMode::Normal); + EnterEditAtCursor { + target_mode: AppMode::records_editing(), + } + .apply(&mut app); + assert!( + matches!(app.mode, AppMode::RecordsEditing { .. }), + "Expected RecordsEditing, got {:?}", + app.mode + ); + + // Same effect with editing target — should land in plain Editing + // even if app.mode was something else. + let mut app2 = test_app(); + app2.mode = AppMode::RecordsNormal; + EnterEditAtCursor { + target_mode: AppMode::editing(), + } + .apply(&mut app2); + assert!( + matches!(app2.mode, AppMode::Editing { .. }), + "Expected Editing, got {:?}", + app2.mode + ); + } + /// SetBuffer with empty value clears the buffer (used by clear-buffer command /// in keymap sequences after commit). #[test]