From fbd672d5ede58c7b63ff253454a8c69eedbf4ac1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 6 Apr 2026 21:21:29 -0700 Subject: [PATCH] refactor!: unify records and pivot mode cell handling Refactor records mode to use synthetic CellKeys (_Index, _Dim) for all columns, allowing uniform handling of display values and edits across both pivot and records modes. - Introduce `synthetic_record_info` to extract metadata from synthetic keys. - Update `GridLayout::cell_key` to return synthetic keys in records mode. - Add `GridLayout::resolve_display` to handle value resolution for synthetic keys. - Replace `records_col` and `records_value` in `CmdContext` with a unified `display_value`. - Update `EditOrDrill` and `AddRecordRow` to use synthetic key detection. - Refactor `CommitCellEdit` to use a shared `commit_cell_value` helper. BREAKING CHANGE: CmdContext fields `records_col` and `records_value` are replaced by `display_value` . Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-31B-it-GGUF:UD-Q5_K_XL) --- src/command/cmd.rs | 89 ++++++++++++++++++++---------------------- src/ui/app.rs | 42 +++++++++++--------- src/ui/effect.rs | 10 +---- src/view/layout.rs | 96 +++++++++++++++++++++++++++++++++++----------- src/view/mod.rs | 2 +- 5 files changed, 141 insertions(+), 98 deletions(-) diff --git a/src/command/cmd.rs b/src/command/cmd.rs index b282f9f..999729a 100644 --- a/src/command/cmd.rs +++ b/src/command/cmd.rs @@ -41,12 +41,8 @@ pub struct CmdContext<'a> { /// View navigation stacks (for drill back/forward) pub view_back_stack: Vec, pub view_forward_stack: Vec, - /// Records-mode info (drill view). None for normal pivot views. - /// When Some, edits stage to drill_state.pending_edits. - pub records_col: Option, - /// The display value at the cursor in records mode (including any - /// pending edit override). None for normal pivot views. - pub records_value: Option, + /// 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, @@ -688,7 +684,11 @@ impl Cmd for EditOrDrill { .map(|cat| cat.kind.is_regular()) .unwrap_or(false) }); - let is_aggregated = ctx.records_col.is_none() && regular_none; + // In records mode (synthetic key), always edit directly — no drilling. + 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( @@ -697,18 +697,10 @@ impl Cmd for EditOrDrill { }; return DrillIntoCell { key }.execute(ctx); } - // Edit path: prefer records display value (includes pending edits), - // else the underlying cell's stored value. - let initial_value = if let Some(v) = &ctx.records_value { - v.clone() - } else { - ctx.cell_key - .as_ref() - .and_then(|k| ctx.model.get_cell(k).cloned()) - .map(|v| v.to_string()) - .unwrap_or_default() - }; - EnterEditMode { initial_value }.execute(ctx) + EnterEditMode { + initial_value: ctx.display_value.clone(), + } + .execute(ctx) } } @@ -721,7 +713,10 @@ impl Cmd for AddRecordRow { "add-record-row" } fn execute(&self, ctx: &CmdContext) -> Vec> { - if ctx.records_col.is_none() { + 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")]; } // Build a CellKey from the current page filters @@ -1928,14 +1923,34 @@ impl Cmd for PopChar { /// Commit a cell edit: set cell value, advance cursor, return to Normal. /// In records mode, stages the edit in drill_state.pending_edits instead of -/// writing directly to the model. +/// Commit a cell value: for synthetic records keys, stage in drill pending edits +/// or apply directly; for real keys, write to the model. +fn commit_cell_value(key: &CellKey, value: &str, effects: &mut Vec>) { + if let Some((record_idx, col_name)) = crate::view::synthetic_record_info(key) { + effects.push(Box::new(effect::SetDrillPendingEdit { + record_idx, + col_name, + new_value: value.to_string(), + })); + } else if value.is_empty() { + effects.push(Box::new(effect::ClearCell(key.clone()))); + effects.push(effect::mark_dirty()); + } else if let Ok(n) = value.parse::() { + effects.push(Box::new(effect::SetCell(key.clone(), CellValue::Number(n)))); + effects.push(effect::mark_dirty()); + } else { + effects.push(Box::new(effect::SetCell(key.clone(), CellValue::Text(value.to_string())))); + effects.push(effect::mark_dirty()); + } +} + +/// Commit a cell edit: set cell value, advance cursor, return to editing. +/// In records mode with drill, stages the edit in drill_state.pending_edits. +/// In records mode without drill or in pivot mode, writes directly to the model. #[derive(Debug)] pub struct CommitCellEdit { - pub key: crate::model::cell::CellKey, + pub key: CellKey, pub value: String, - /// Records-mode edit: (record_idx, column_name). When Some, stage in - /// pending_edits; otherwise write to the model directly. - pub records_edit: Option<(usize, String)>, } impl Cmd for CommitCellEdit { fn name(&self) -> &'static str { @@ -1944,29 +1959,7 @@ impl Cmd for CommitCellEdit { fn execute(&self, ctx: &CmdContext) -> Vec> { let mut effects: Vec> = Vec::new(); - if let Some((record_idx, col_name)) = &self.records_edit { - // Stage the edit in drill_state.pending_edits - effects.push(Box::new(effect::SetDrillPendingEdit { - record_idx: *record_idx, - col_name: col_name.clone(), - new_value: self.value.clone(), - })); - } else if self.value.is_empty() { - effects.push(Box::new(effect::ClearCell(self.key.clone()))); - effects.push(effect::mark_dirty()); - } else if let Ok(n) = self.value.parse::() { - effects.push(Box::new(effect::SetCell( - self.key.clone(), - CellValue::Number(n), - ))); - effects.push(effect::mark_dirty()); - } else { - effects.push(Box::new(effect::SetCell( - self.key.clone(), - CellValue::Text(self.value.clone()), - ))); - effects.push(effect::mark_dirty()); - } + commit_cell_value(&self.key, &self.value, &mut effects); // Advance cursor down (typewriter-style) and re-enter edit mode // at the new cell so the user can continue data entry. let adv = EnterAdvance { diff --git a/src/ui/app.rs b/src/ui/app.rs index 664964b..d06da3b 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -164,28 +164,34 @@ impl App { none_cats: layout.none_cats.clone(), view_back_stack: self.view_back_stack.clone(), view_forward_stack: self.view_forward_stack.clone(), - records_col: if layout.is_records_mode() { - Some(layout.col_label(sel_col)) - } else { - None + display_value: { + let key = layout.cell_key(sel_row, sel_col); + if let Some(k) = &key { + if let Some((idx, dim)) = crate::view::synthetic_record_info(k) { + // Synthetic records key: check pending drill edits first + self.drill_state.as_ref() + .and_then(|s| s.pending_edits.get(&(idx, dim)).cloned()) + .or_else(|| layout.resolve_display(k)) + .unwrap_or_default() + } else { + self.model.get_cell(k) + .map(|v| v.to_string()) + .unwrap_or_default() + } + } else { + String::new() + } }, - records_value: if layout.is_records_mode() { - // Check pending edits first, then fall back to original - let col_name = layout.col_label(sel_col); - let pending = self.drill_state.as_ref().and_then(|s| { - s.pending_edits.get(&(sel_row, col_name.clone())).cloned() - }); - pending.or_else(|| layout.records_display(sel_row, sel_col)) - } else { - None - }, - // Approximate visible rows/cols from terminal size. + // Approximate visible rows from terminal size. // Chrome: title(1) + border(2) + col_headers(n_col_levels) + separator(1) // + tile_bar(1) + status_bar(1) = ~8 rows of chrome. visible_rows: (self.term_height as usize).saturating_sub(8), - // Visible cols depends on column widths — use a rough estimate. - // The grid renderer does the precise calculation. - visible_cols: ((self.term_width as usize).saturating_sub(30) / 12).max(1), + visible_cols: { + let (fmt_comma, fmt_decimals) = parse_number_format(&view.number_format); + let col_widths = compute_col_widths(&self.model, &layout, fmt_comma, fmt_decimals); + let row_header_width = compute_row_header_width(&layout); + compute_visible_cols(&col_widths, row_header_width, self.term_width, view.col_offset) + }, expanded_cats: &self.expanded_cats, key_code: key, } diff --git a/src/ui/effect.rs b/src/ui/effect.rs index 7420466..464b0aa 100644 --- a/src/ui/effect.rs +++ b/src/ui/effect.rs @@ -97,15 +97,7 @@ pub struct EnterEditAtCursor; impl Effect for EnterEditAtCursor { fn apply(&self, app: &mut App) { let ctx = app.cmd_context(crossterm::event::KeyCode::Null, crossterm::event::KeyModifiers::NONE); - let value = if let Some(v) = &ctx.records_value { - v.clone() - } else { - ctx.cell_key - .as_ref() - .and_then(|k| ctx.model.get_cell(k).cloned()) - .map(|v| v.to_string()) - .unwrap_or_default() - }; + let value = ctx.display_value.clone(); drop(ctx); app.buffers.insert("edit".to_string(), value); app.mode = AppMode::Editing { diff --git a/src/view/layout.rs b/src/view/layout.rs index 7cf4066..d7c963c 100644 --- a/src/view/layout.rs +++ b/src/view/layout.rs @@ -2,6 +2,14 @@ use crate::model::cell::{CellKey, CellValue}; use crate::model::Model; use crate::view::{Axis, View}; +/// Extract (record_index, dim_name) from a synthetic records-mode CellKey. +/// Returns None for normal pivot-mode keys. +pub fn synthetic_record_info(key: &CellKey) -> Option<(usize, String)> { + let idx: usize = key.get("_Index")?.parse().ok()?; + let dim = key.get("_Dim")?.to_string(); + Some((idx, dim)) +} + /// One entry on a grid axis: either a visual group header or a data-item tuple. /// /// `GroupHeader` entries are always visible so the user can see the group label @@ -352,18 +360,36 @@ impl GridLayout { .unwrap_or_default() } + /// Resolve the display string for a synthetic records-mode CellKey. + /// Returns None for non-synthetic (pivot) keys. + pub fn resolve_display(&self, key: &CellKey) -> Option { + let (idx, dim) = synthetic_record_info(key)?; + let records = self.records.as_ref()?; + let (orig_key, value) = records.get(idx)?; + if dim == "Value" { + Some(value.to_string()) + } else { + Some(orig_key.get(&dim).unwrap_or("").to_string()) + } + } + /// Build the CellKey for the data cell at (row, col), including the active /// page-axis filter. Returns None if row or col is out of bounds. - /// In records mode: returns the real underlying CellKey when the column - /// is "Value" (editable); returns None for coord columns (read-only). + /// In records mode: returns a synthetic `(_Index, _Dim)` key for every column. pub fn cell_key(&self, row: usize, col: usize) -> Option { - if let Some(records) = &self.records { - // Records mode: only the Value column maps to a real, editable cell. - if self.col_label(col) == "Value" { - return records.get(row).map(|(k, _)| k.clone()); - } else { + if self.records.is_some() { + let records = self.records.as_ref().unwrap(); + if row >= records.len() { return None; } + let col_label = self.col_label(col); + if col_label.is_empty() { + return None; + } + return Some(CellKey::new(vec![ + ("_Index".to_string(), row.to_string()), + ("_Dim".to_string(), col_label), + ])); } let row_item = self .row_items @@ -527,7 +553,7 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec #[cfg(test)] mod tests { - use super::{AxisEntry, GridLayout}; + use super::{synthetic_record_info, AxisEntry, GridLayout}; use crate::model::cell::{CellKey, CellValue}; use crate::model::Model; use crate::view::Axis; @@ -592,40 +618,66 @@ mod tests { } #[test] - fn records_mode_cell_key_editable_for_value_column() { + fn records_mode_cell_key_returns_synthetic_for_all_columns() { let mut m = records_model(); let v = m.active_view_mut(); v.set_axis("_Index", Axis::Row); v.set_axis("_Dim", Axis::Column); let layout = GridLayout::new(&m, m.active_view()); assert!(layout.is_records_mode()); - // Find the "Value" column index let cols: Vec = (0..layout.col_count()).map(|i| layout.col_label(i)).collect(); + // All columns return synthetic keys let value_col = cols.iter().position(|c| c == "Value").unwrap(); - // cell_key should be Some for Value column - let key = layout.cell_key(0, value_col); - assert!(key.is_some(), "Value column should be editable"); - // cell_key should be None for coord columns + let key = layout.cell_key(0, value_col).unwrap(); + assert_eq!(key.get("_Index"), Some("0")); + assert_eq!(key.get("_Dim"), Some("Value")); + let region_col = cols.iter().position(|c| c == "Region").unwrap(); - assert!( - layout.cell_key(0, region_col).is_none(), - "Region column should not be editable" - ); + let key = layout.cell_key(0, region_col).unwrap(); + assert_eq!(key.get("_Index"), Some("0")); + assert_eq!(key.get("_Dim"), Some("Region")); } #[test] - fn records_mode_cell_key_maps_to_real_cell() { + fn records_mode_resolve_display_returns_values() { let mut m = records_model(); let v = m.active_view_mut(); v.set_axis("_Index", Axis::Row); v.set_axis("_Dim", Axis::Column); let layout = GridLayout::new(&m, m.active_view()); let cols: Vec = (0..layout.col_count()).map(|i| layout.col_label(i)).collect(); + + // Value column resolves to the cell value let value_col = cols.iter().position(|c| c == "Value").unwrap(); - // The CellKey at (0, Value) should look up a real cell value let key = layout.cell_key(0, value_col).unwrap(); - let val = m.evaluate(&key); - assert!(val.is_some(), "cell_key should resolve to a real cell"); + let display = layout.resolve_display(&key); + assert!(display.is_some(), "Value column should resolve"); + + // Category column resolves to the coordinate value + let region_col = cols.iter().position(|c| c == "Region").unwrap(); + let key = layout.cell_key(0, region_col).unwrap(); + let display = layout.resolve_display(&key).unwrap(); + assert!(!display.is_empty(), "Region column should resolve to a value"); + } + + #[test] + fn synthetic_record_info_returns_none_for_pivot_keys() { + let key = CellKey::new(vec![ + ("Region".to_string(), "East".to_string()), + ("Product".to_string(), "Shoes".to_string()), + ]); + assert!(synthetic_record_info(&key).is_none()); + } + + #[test] + fn synthetic_record_info_extracts_index_and_dim() { + let key = CellKey::new(vec![ + ("_Index".to_string(), "3".to_string()), + ("_Dim".to_string(), "Region".to_string()), + ]); + let (idx, dim) = synthetic_record_info(&key).unwrap(); + assert_eq!(idx, 3); + assert_eq!(dim, "Region"); } fn coord(pairs: &[(&str, &str)]) -> CellKey { diff --git a/src/view/mod.rs b/src/view/mod.rs index 710c870..1ac49c2 100644 --- a/src/view/mod.rs +++ b/src/view/mod.rs @@ -3,5 +3,5 @@ pub mod layout; pub mod types; pub use axis::Axis; -pub use layout::{AxisEntry, GridLayout}; +pub use layout::{synthetic_record_info, AxisEntry, GridLayout}; pub use types::View;