From 58372a8d8afbc943d739d4d7380831916994a8a6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 6 Apr 2026 23:18:39 -0700 Subject: [PATCH] refactor(cmd): delegate cell information to GridLayout in CmdContext Refactor CmdContext to use GridLayout for cell-related information instead of storing redundant fields. This simplifies the context structure and ensures commands always use the most up-to-date layout information. - Add layout field to CmdContext - Remove redundant row_count, col_count, cell_key, and none_cats fields - Implement helper methods on CmdContext to delegate to layout - Update all command implementations and tests to use the new methods Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-26B-A4B-it-GGUF:UD-Q5_K_XL) --- src/command/cmd.rs | 177 ++++++++++++++++++++++++++------------------- 1 file changed, 104 insertions(+), 73 deletions(-) diff --git a/src/command/cmd.rs b/src/command/cmd.rs index e0cbc57..ff23a60 100644 --- a/src/command/cmd.rs +++ b/src/command/cmd.rs @@ -12,6 +12,7 @@ use crate::view::{Axis, AxisEntry, GridLayout}; /// Read-only context available to commands for decision-making. pub struct CmdContext<'a> { pub model: &'a Model, + pub layout: &'a GridLayout, pub mode: &'a AppMode, pub selected: (usize, usize), pub row_offset: usize, @@ -31,20 +32,12 @@ pub struct CmdContext<'a> { pub tile_cat_idx: usize, /// Named text buffers pub buffers: &'a HashMap, - /// Pre-resolved cell key at the cursor position (None if out of bounds) - pub cell_key: Option, - /// Grid dimensions (so commands don't need GridLayout) - pub row_count: usize, - pub col_count: usize, - /// Categories on Axis::None — aggregated away in the current view - pub none_cats: Vec, /// View navigation stacks (for drill back/forward) - pub view_back_stack: Vec, - pub view_forward_stack: Vec, + pub view_back_stack: &'a [String], + pub view_forward_stack: &'a [String], /// Display value at the cursor — works uniformly for pivot and records mode. pub display_value: String, /// How many data rows/cols fit on screen (for viewport scrolling). - /// Defaults to generous fallbacks when unknown. pub visible_rows: usize, pub visible_cols: usize, /// Expanded categories in the tree panel @@ -53,6 +46,21 @@ pub struct CmdContext<'a> { pub key_code: KeyCode, } +impl<'a> CmdContext<'a> { + pub fn cell_key(&self) -> Option { + self.layout.cell_key(self.selected.0, self.selected.1) + } + pub fn row_count(&self) -> usize { + self.layout.row_count() + } + pub fn col_count(&self) -> usize { + self.layout.col_count() + } + pub fn none_cats(&self) -> &[String] { + &self.layout.none_cats + } +} + impl<'a> CmdContext<'a> { /// Resolve the category panel tree entry at the current cursor. pub fn cat_tree_entry(&self) -> Option { @@ -247,8 +255,8 @@ impl CursorState { Self { row: ctx.selected.0, col: ctx.selected.1, - row_count: ctx.row_count, - col_count: ctx.col_count, + row_count: ctx.row_count(), + col_count: ctx.col_count(), row_offset: ctx.row_offset, col_offset: ctx.col_offset, visible_rows: ctx.visible_rows, @@ -443,7 +451,7 @@ impl Cmd for SaveAndQuit { // ── Cell operations ────────────────────────────────────────────────────────── // All cell commands take an explicit CellKey. The interactive spec fills it -// from ctx.cell_key; the parser fills it from Cat/Item coordinate args. +// from ctx.cell_key(); the parser fills it from Cat/Item coordinate args. /// Clear a cell. #[derive(Debug)] @@ -472,8 +480,7 @@ impl Cmd for YankCell { "yank" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let layout = GridLayout::new(ctx.model, ctx.model.active_view()); - let value = ctx.model.evaluate_aggregated(&self.key, &layout.none_cats); + let value = ctx.model.evaluate_aggregated(&self.key, ctx.none_cats()); vec![ Box::new(effect::SetYanked(value)), effect::set_status("Yanked"), @@ -649,22 +656,22 @@ impl Cmd for EditOrDrill { // Only consider regular (non-virtual, non-label) categories on None // as true aggregation. Virtuals like _Index/_Dim are always None in // pivot mode and don't imply aggregation. - let regular_none = ctx.none_cats.iter().any(|c| { + let regular_none = ctx.none_cats().iter().any(|c| { ctx.model .category(c) .map(|cat| cat.kind.is_regular()) .unwrap_or(false) }); // In records mode (synthetic key), always edit directly — no drilling. - let is_synthetic = ctx.cell_key.as_ref() + let is_synthetic = ctx + .cell_key() + .as_ref() .and_then(|k| crate::view::synthetic_record_info(k)) .is_some(); let is_aggregated = !is_synthetic && regular_none; if is_aggregated { - let Some(key) = ctx.cell_key.clone() else { - return vec![effect::set_status( - "cannot drill — no cell at cursor", - )]; + let Some(key) = ctx.cell_key().clone() else { + return vec![effect::set_status("cannot drill — no cell at cursor")]; }; return DrillIntoCell { key }.execute(ctx); } @@ -684,11 +691,15 @@ impl Cmd for AddRecordRow { "add-record-row" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let is_records = ctx.cell_key.as_ref() + let is_records = ctx + .cell_key() + .as_ref() .and_then(|k| crate::view::synthetic_record_info(k)) .is_some(); if !is_records { - return vec![effect::set_status("add-record-row only works in records mode")]; + return vec![effect::set_status( + "add-record-row only works in records mode", + )]; } // Build a CellKey from the current page filters let view = ctx.model.active_view(); @@ -725,11 +736,15 @@ impl Cmd for OpenRecordRow { "open-record-row" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let is_records = ctx.cell_key.as_ref() + let is_records = ctx + .cell_key() + .as_ref() .and_then(|k| crate::view::synthetic_record_info(k)) .is_some(); if !is_records { - return vec![effect::set_status("open-record-row only works in records mode")]; + return vec![effect::set_status( + "open-record-row only works in records mode", + )]; } let mut effects = AddRecordRow.execute(ctx); effects.push(Box::new(effect::EnterEditAtCursor)); @@ -790,10 +805,9 @@ impl Cmd for SearchNavigate { return vec![]; } - let layout = GridLayout::new(ctx.model, ctx.model.active_view()); let (cur_row, cur_col) = ctx.selected; - let total_rows = layout.row_count().max(1); - let total_cols = layout.col_count().max(1); + let total_rows = ctx.row_count().max(1); + let total_cols = ctx.col_count().max(1); let total = total_rows * total_cols; let cur_flat = cur_row * total_cols + cur_col; @@ -801,11 +815,11 @@ impl Cmd for SearchNavigate { .filter(|&flat| { let ri = flat / total_cols; let ci = flat % total_cols; - let key = match layout.cell_key(ri, ci) { + let key = match ctx.layout.cell_key(ri, ci) { Some(k) => k, None => return false, }; - let s = match ctx.model.evaluate_aggregated(&key, &layout.none_cats) { + let s = match ctx.model.evaluate_aggregated(&key, ctx.none_cats()) { Some(CellValue::Number(n)) => format!("{n}"), Some(CellValue::Text(t)) => t, None => String::new(), @@ -1004,9 +1018,8 @@ impl Cmd for ToggleGroupUnderCursor { "toggle-group-under-cursor" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let layout = GridLayout::new(ctx.model, ctx.model.active_view()); let sel_row = ctx.selected.0; - let Some((cat, group)) = layout.row_group_for(sel_row) else { + let Some((cat, group)) = ctx.layout.row_group_for(sel_row) else { return vec![]; }; vec![ @@ -1027,9 +1040,8 @@ impl Cmd for ToggleColGroupUnderCursor { "toggle-col-group-under-cursor" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let layout = GridLayout::new(ctx.model, ctx.model.active_view()); let sel_col = ctx.selected.1; - let Some((cat, group)) = layout.col_group_for(sel_col) else { + let Some((cat, group)) = ctx.layout.col_group_for(sel_col) else { return vec![]; }; // After toggling, col_count may shrink — clamp selection @@ -1053,12 +1065,12 @@ impl Cmd for HideSelectedRowItem { "hide-selected-row-item" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let layout = GridLayout::new(ctx.model, ctx.model.active_view()); - let Some(cat_name) = layout.row_cats.first().cloned() else { + let Some(cat_name) = ctx.layout.row_cats.first().cloned() else { return vec![]; }; let sel_row = ctx.selected.0; - let Some(items) = layout + let Some(items) = ctx + .layout .row_items .iter() .filter_map(|e| { @@ -1185,7 +1197,7 @@ impl Cmd for DrillIntoCell { // Previously-aggregated categories (none_cats) stay on Axis::None so // they don't filter records; they'll appear as columns in records mode. // Skip virtual categories — we already set _Index/_Dim above. - for cat in &ctx.none_cats { + for cat in ctx.none_cats() { if fixed_cats.contains(cat) || cat.starts_with('_') { continue; } @@ -2444,7 +2456,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |_args, ctx| { - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(ClearCellCommand { key })) }, ); @@ -2592,12 +2604,14 @@ pub fn default_registry() -> CmdRegistry { })) }, |_args, ctx| { - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(YankCell { key })) }, ); r.register( - &PasteCell { key: CellKey::new(vec![]) }, + &PasteCell { + key: CellKey::new(vec![]), + }, |args| { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); @@ -2607,11 +2621,11 @@ pub fn default_registry() -> CmdRegistry { })) }, |_args, ctx| { - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(PasteCell { key })) }, ); - // clear-cell is registered above (unified: ctx.cell_key or explicit coords) + // clear-cell is registered above (unified: ctx.cell_key() or explicit coords) // ── View / page ────────────────────────────────────────────────────── r.register_nullary(|| Box::new(TransposeAxes)); @@ -2658,7 +2672,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |_args, ctx| { - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(DrillIntoCell { key })) }, ); @@ -2885,7 +2899,7 @@ pub fn default_registry() -> CmdRegistry { }, |_args, ctx| { let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(CommitCellEdit { key, value })) }, ); @@ -2898,7 +2912,7 @@ pub fn default_registry() -> CmdRegistry { |_| Err("commit-and-advance-right requires context".into()), |_args, ctx| { let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key.clone().ok_or("no cell at cursor")?; + let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; Ok(Box::new(CommitAndAdvanceRight { key, value, @@ -2948,12 +2962,16 @@ mod tests { static EMPTY_EXPANDED: std::sync::LazyLock> = std::sync::LazyLock::new(std::collections::HashSet::new); - fn make_ctx(model: &Model) -> CmdContext<'_> { + fn make_layout(model: &Model) -> GridLayout { + GridLayout::new(model, model.active_view()) + } + + fn make_ctx<'a>(model: &'a Model, layout: &'a GridLayout) -> CmdContext<'a> { let view = model.active_view(); - let layout = GridLayout::new(model, view); let (sr, sc) = view.selected; CmdContext { model, + layout, mode: &AppMode::Normal, selected: view.selected, row_offset: view.row_offset, @@ -2970,9 +2988,8 @@ mod tests { view_panel_cursor: 0, tile_cat_idx: 0, buffers: &EMPTY_BUFFERS, - none_cats: layout.none_cats.clone(), - view_back_stack: Vec::new(), - view_forward_stack: Vec::new(), + view_back_stack: &[], + view_forward_stack: &[], display_value: { let key = layout.cell_key(sr, sc); key.as_ref() @@ -2980,9 +2997,6 @@ mod tests { .map(|v| v.to_string()) .unwrap_or_default() }, - cell_key: layout.cell_key(sr, sc), - row_count: layout.row_count(), - col_count: layout.col_count(), visible_rows: 20, visible_cols: 8, expanded_cats: &EMPTY_EXPANDED, @@ -3004,7 +3018,8 @@ mod tests { #[test] fn move_selection_down_produces_set_selected() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = MoveSelection { dr: 1, dc: 0, @@ -3018,7 +3033,8 @@ mod tests { #[test] fn move_selection_clamps_to_bounds() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); // Try to move way past the end let cmd = MoveSelection { dr: 100, @@ -3034,7 +3050,8 @@ mod tests { let m = two_cat_model(); let mut bufs = HashMap::new(); bufs.insert("command".to_string(), "q".to_string()); - let mut ctx = make_ctx(&m); + let layout = make_layout(&m); + let mut ctx = make_ctx(&m, &layout); ctx.dirty = true; ctx.buffers = &bufs; let cmd = ExecuteCommand; @@ -3048,7 +3065,8 @@ mod tests { let m = two_cat_model(); let mut bufs = HashMap::new(); bufs.insert("command".to_string(), "q".to_string()); - let mut ctx = make_ctx(&m); + let layout = make_layout(&m); + let mut ctx = make_ctx(&m, &layout); ctx.buffers = &bufs; let cmd = ExecuteCommand; let effects = cmd.execute(&ctx); @@ -3067,9 +3085,10 @@ mod tests { ("Month".to_string(), "Jan".to_string()), ]); m.set_cell(key, CellValue::Number(42.0)); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = ClearCellCommand { - key: ctx.cell_key.clone().unwrap(), + key: ctx.cell_key().clone().unwrap(), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); // ClearCell + MarkDirty @@ -3083,9 +3102,10 @@ mod tests { ("Month".to_string(), "Jan".to_string()), ]); m.set_cell(key, CellValue::Number(99.0)); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = YankCell { - key: ctx.cell_key.clone().unwrap(), + key: ctx.cell_key().clone().unwrap(), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); // SetYanked + SetStatus @@ -3094,7 +3114,8 @@ mod tests { #[test] fn toggle_panel_open_and_focus() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, open: true, @@ -3112,7 +3133,8 @@ mod tests { #[test] fn toggle_panel_close_and_unfocus() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, open: false, @@ -3125,7 +3147,8 @@ mod tests { #[test] fn enter_advance_moves_down() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = EnterAdvance { cursor: CursorState::from_ctx(&ctx), }; @@ -3141,7 +3164,8 @@ mod tests { #[test] fn search_navigate_with_empty_query_returns_nothing() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = SearchNavigate(true); let effects = cmd.execute(&ctx); assert!(effects.is_empty()); @@ -3150,7 +3174,8 @@ mod tests { #[test] fn enter_edit_mode_produces_editing_mode() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = EnterEditMode { initial_value: String::new(), }; @@ -3163,7 +3188,8 @@ mod tests { #[test] fn enter_tile_select_with_categories() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = EnterTileSelect; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); // SetTileCatIdx + ChangeMode @@ -3179,7 +3205,8 @@ mod tests { // Models always have virtual categories (_Index, _Dim), so tile // select always has something to operate on. let m = Model::new("Empty"); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = EnterTileSelect; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); // SetTileCatIdx + ChangeMode @@ -3188,7 +3215,8 @@ mod tests { #[test] fn toggle_group_under_cursor_returns_empty_without_groups() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = ToggleGroupUnderCursor; let effects = cmd.execute(&ctx); // No groups defined, so nothing to toggle @@ -3198,7 +3226,8 @@ mod tests { #[test] fn search_or_category_add_without_query_opens_category_add() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = SearchOrCategoryAdd; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); // SetPanelOpen + ChangeMode @@ -3212,7 +3241,8 @@ mod tests { #[test] fn cycle_panel_focus_with_no_panels_open() { let m = two_cat_model(); - let ctx = make_ctx(&m); + let layout = make_layout(&m); + let ctx = make_ctx(&m, &layout); let cmd = CyclePanelFocus { formula_open: false, category_open: false, @@ -3225,7 +3255,8 @@ mod tests { #[test] fn cycle_panel_focus_with_formula_panel_open() { let m = two_cat_model(); - let mut ctx = make_ctx(&m); + let layout = make_layout(&m); + let mut ctx = make_ctx(&m, &layout); ctx.formula_panel_open = true; let cmd = CyclePanelFocus { formula_open: true,