From b7e5115a8ec1208f949b150d14dd7319bb79b18e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 6 Apr 2026 21:56:47 -0700 Subject: [PATCH] refactor(view): unify cell handling for records and pivot modes Unifies cell text retrieval and formatting across pivot and records modes. Introduces `GridLayout::display_text` to centralize how cell content is resolved, reducing duplication in `GridWidget` and `export_csv`. Moves formatting logic from `src/ui/grid.rs` to a new dedicated `src/format.rs` module to improve reusability. Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-31B-it-GGUF:UD-Q5_K_XL) --- src/format.rs | 50 ++++++++++++++ src/main.rs | 1 + src/persistence/mod.rs | 12 +--- src/ui/grid.rs | 147 +++++++++-------------------------------- src/view/layout.rs | 33 ++++++--- 5 files changed, 109 insertions(+), 134 deletions(-) create mode 100644 src/format.rs diff --git a/src/format.rs b/src/format.rs new file mode 100644 index 0000000..ed31509 --- /dev/null +++ b/src/format.rs @@ -0,0 +1,50 @@ +use crate::model::cell::CellValue; + +/// Format a CellValue for display with number formatting options. +pub fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String { + match v { + Some(CellValue::Number(n)) => format_f64(*n, comma, decimals), + Some(CellValue::Text(s)) => s.clone(), + None => String::new(), + } +} + +/// Parse a number format string like ",.0" into (use_commas, decimal_places). +pub fn parse_number_format(fmt: &str) -> (bool, u8) { + let comma = fmt.contains(','); + let decimals = fmt + .rfind('.') + .and_then(|i| fmt[i + 1..].parse::().ok()) + .unwrap_or(0); + (comma, decimals) +} + +/// Format an f64 with optional comma grouping and decimal places. +pub fn format_f64(n: f64, comma: bool, decimals: u8) -> String { + let formatted = format!("{:.prec$}", n, prec = decimals as usize); + if !comma { + return formatted; + } + let (int_part, dec_part) = if let Some(dot) = formatted.find('.') { + (&formatted[..dot], Some(&formatted[dot..])) + } else { + (&formatted[..], None) + }; + let is_neg = int_part.starts_with('-'); + let digits = if is_neg { &int_part[1..] } else { int_part }; + let mut result = String::new(); + for (idx, c) in digits.chars().rev().enumerate() { + if idx > 0 && idx % 3 == 0 { + result.push(','); + } + result.push(c); + } + if is_neg { + result.push('-'); + } + let mut out: String = result.chars().rev().collect(); + if let Some(dec) = dec_part { + out.push_str(dec); + } + out +} diff --git a/src/main.rs b/src/main.rs index b1d1adf..642f343 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,6 @@ mod command; mod draw; +mod format; mod formula; mod import; mod model; diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index 2912448..4052e29 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -458,17 +458,7 @@ pub fn export_csv(model: &Model, view_name: &str, path: &Path) -> Result<()> { out.push(','); } let row_values: Vec = (0..layout.col_count()) - .map(|ci| { - if layout.is_records_mode() { - layout.records_display(ri, ci).unwrap_or_default() - } else { - layout - .cell_key(ri, ci) - .and_then(|key| model.evaluate_aggregated(&key, &layout.none_cats)) - .map(|v| v.to_string()) - .unwrap_or_default() - } - }) + .map(|ci| layout.display_text(model, ri, ci, false, 0)) .collect(); out.push_str(&row_values.join(",")); out.push('\n'); diff --git a/src/ui/grid.rs b/src/ui/grid.rs index a021ec6..486a68d 100644 --- a/src/ui/grid.rs +++ b/src/ui/grid.rs @@ -6,7 +6,6 @@ use ratatui::{ }; use unicode_width::UnicodeWidthStr; -use crate::model::cell::CellValue; use crate::model::Model; use crate::ui::app::AppMode; use crate::view::{AxisEntry, GridLayout}; @@ -46,18 +45,6 @@ impl<'a> GridWidget<'a> { } } - /// In records mode, get the display text for (row, col): pending edit if - /// staged, otherwise the underlying record's value for that column. - fn records_cell_text(&self, layout: &GridLayout, row: usize, col: usize) -> String { - let col_name = layout.col_label(col); - let pending = self - .drill_state - .and_then(|s| s.pending_edits.get(&(row, col_name.clone())).cloned()); - pending - .or_else(|| layout.records_display(row, col)) - .unwrap_or_default() - } - fn render_grid(&self, area: Rect, buf: &mut Buffer) { let view = self.model.active_view(); @@ -71,21 +58,7 @@ impl<'a> GridWidget<'a> { let n_col_levels = layout.col_cats.len().max(1); let n_row_levels = layout.row_cats.len().max(1); - let mut col_widths = compute_col_widths(self.model, &layout, fmt_comma, fmt_decimals); - // Records mode: also measure cell text widths (needs drill_state) - if layout.is_records_mode() { - let n = layout.col_count(); - for ri in 0..layout.row_count() { - for (ci, wref) in col_widths.iter_mut().enumerate().take(n) { - let s = self.records_cell_text(&layout, ri, ci); - let w = s.width() as u16; - let needed = (w + 1).max(MIN_COL_WIDTH).min(MAX_COL_WIDTH); - if needed > *wref { - *wref = needed; - } - } - } - } + let col_widths = compute_col_widths(self.model, &layout, fmt_comma, fmt_decimals); // ── Adaptive row header widths ─────────────────────────────── let data_row_items: Vec<&Vec> = layout @@ -377,23 +350,15 @@ impl<'a> GridWidget<'a> { } let cw = col_w_at(ci) as usize; - let (cell_str, value) = if layout.is_records_mode() { - let s = self.records_cell_text(&layout, ri, ci); - // In records mode the value is a string, not aggregated - let v = if !s.is_empty() { - Some(crate::model::cell::CellValue::Text(s.clone())) - } else { - None - }; - (s, v) + // Check pending drill edits first, then use display_text + let cell_str = if let Some(ds) = self.drill_state { + let col_name = layout.col_label(ci); + ds.pending_edits + .get(&(ri, col_name)) + .cloned() + .unwrap_or_else(|| layout.display_text(self.model, ri, ci, fmt_comma, fmt_decimals)) } else { - let key = match layout.cell_key(ri, ci) { - Some(k) => k, - None => continue, - }; - let value = self.model.evaluate_aggregated(&key, &layout.none_cats); - let s = format_value(value.as_ref(), fmt_comma, fmt_decimals); - (s, value) + layout.display_text(self.model, ri, ci, fmt_comma, fmt_decimals) }; let is_selected = ri == sel_row && ci == sel_col; let is_search_match = !self.search_query.is_empty() @@ -420,13 +385,13 @@ impl<'a> GridWidget<'a> { } else if is_search_match { Style::default().fg(Color::Black).bg(Color::Yellow) } else if is_sel_row { - let fg = if value.is_none() { + let fg = if cell_str.is_empty() { Color::DarkGray } else { Color::White }; Style::default().fg(fg).bg(ROW_HIGHLIGHT_BG) - } else if value.is_none() { + } else if cell_str.is_empty() { Style::default().fg(Color::DarkGray) } else { Style::default() @@ -582,31 +547,27 @@ pub fn compute_col_widths(model: &Model, layout: &GridLayout, fmt_comma: bool, f } } } - if !layout.is_records_mode() { - for ri in 0..layout.row_count() { - for (ci, wref) in widths.iter_mut().enumerate().take(n) { - if let Some(key) = layout.cell_key(ri, ci) { - let value = model.evaluate_aggregated(&key, &layout.none_cats); - let s = format_value(value.as_ref(), fmt_comma, fmt_decimals); - let w = s.width() as u16; - if w > *wref { - *wref = w; - } - } + // Measure cell content widths (works for both pivot and records modes) + for ri in 0..layout.row_count() { + for (ci, wref) in widths.iter_mut().enumerate().take(n) { + let s = layout.display_text(model, ri, ci, fmt_comma, fmt_decimals); + let w = s.width() as u16; + if w > *wref { + *wref = w; } } - // Measure total row (column sums) — totals can be wider than any single cell - if layout.row_count() > 0 { - for (ci, wref) in widths.iter_mut().enumerate().take(n) { - let total: f64 = (0..layout.row_count()) - .filter_map(|ri| layout.cell_key(ri, ci)) - .map(|key| model.evaluate_aggregated_f64(&key, &layout.none_cats)) - .sum(); - let s = format_f64(total, fmt_comma, fmt_decimals); - let w = s.width() as u16; - if w > *wref { - *wref = w; - } + } + // Measure total row (column sums) — pivot mode only + if !layout.is_records_mode() && layout.row_count() > 0 { + for (ci, wref) in widths.iter_mut().enumerate().take(n) { + let total: f64 = (0..layout.row_count()) + .filter_map(|ri| layout.cell_key(ri, ci)) + .map(|key| model.evaluate_aggregated_f64(&key, &layout.none_cats)) + .sum(); + let s = format_f64(total, fmt_comma, fmt_decimals); + let w = s.width() as u16; + if w > *wref { + *wref = w; } } } @@ -661,52 +622,8 @@ pub fn compute_visible_cols(col_widths: &[u16], row_header_width: u16, term_widt count.max(1) } -fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String { - match v { - Some(CellValue::Number(n)) => format_f64(*n, comma, decimals), - Some(CellValue::Text(s)) => s.clone(), - None => String::new(), - } -} - -pub fn parse_number_format(fmt: &str) -> (bool, u8) { - let comma = fmt.contains(','); - let decimals = fmt - .rfind('.') - .and_then(|i| fmt[i + 1..].parse::().ok()) - .unwrap_or(0); - (comma, decimals) -} - -pub fn format_f64(n: f64, comma: bool, decimals: u8) -> String { - let formatted = format!("{:.prec$}", n, prec = decimals as usize); - if !comma { - return formatted; - } - // Split integer and decimal parts - let (int_part, dec_part) = if let Some(dot) = formatted.find('.') { - (&formatted[..dot], Some(&formatted[dot..])) - } else { - (&formatted[..], None) - }; - let is_neg = int_part.starts_with('-'); - let digits = if is_neg { &int_part[1..] } else { int_part }; - let mut result = String::new(); - for (idx, c) in digits.chars().rev().enumerate() { - if idx > 0 && idx % 3 == 0 { - result.push(','); - } - result.push(c); - } - if is_neg { - result.push('-'); - } - let mut out: String = result.chars().rev().collect(); - if let Some(dec) = dec_part { - out.push_str(dec); - } - out -} +// Re-export shared formatting functions +pub use crate::format::{format_f64, parse_number_format}; fn truncate(s: &str, max_width: usize) -> String { let w = s.width(); diff --git a/src/view/layout.rs b/src/view/layout.rs index d7c963c..ce26fd6 100644 --- a/src/view/layout.rs +++ b/src/view/layout.rs @@ -228,14 +228,10 @@ impl GridLayout { let mut has_value = vec![vec![false; cc]; rc]; for ri in 0..rc { for ci in 0..cc { - has_value[ri][ci] = if self.is_records_mode() { - let s = self.records_display(ri, ci).unwrap_or_default(); - !s.is_empty() - } else { - self.cell_key(ri, ci) - .and_then(|k| model.evaluate_aggregated(&k, &self.none_cats)) - .is_some() - }; + has_value[ri][ci] = self + .cell_key(ri, ci) + .and_then(|k| model.evaluate_aggregated(&k, &self.none_cats)) + .is_some(); } } @@ -373,6 +369,27 @@ impl GridLayout { } } + /// Unified display text for a cell at (row, col). Handles both pivot and + /// records modes. In pivot mode, evaluates and formats the cell value. + /// In records mode, resolves via the frozen records snapshot. + pub fn display_text( + &self, + model: &Model, + row: usize, + col: usize, + fmt_comma: bool, + fmt_decimals: u8, + ) -> String { + if self.is_records_mode() { + self.records_display(row, col).unwrap_or_default() + } else { + self.cell_key(row, col) + .and_then(|key| model.evaluate_aggregated(&key, &self.none_cats)) + .map(|v| crate::format::format_value(Some(&v), fmt_comma, fmt_decimals)) + .unwrap_or_default() + } + } + /// 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 a synthetic `(_Index, _Dim)` key for every column.