From 42d869e4c2ba4e8035204e3932063de8ea091970 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 7 Apr 2026 09:29:45 -0700 Subject: [PATCH] refactor(ui): integrate centralized layout and display logic Update UI components and view layout to use the new centralized layout and display logic. - Update `CategoryPanel` to remove redundant title text. - Update `ViewPanel` to remove redundant title text. - Refactor `Effect` implementations to use `display_value` and `Rc` for records. - Update `GridWidget` to use the centralized `layout` and `display_text` . - Refactor `GridLayout` to support synthetic keys for records mode and unified display. - Update `view` module to re-export `synthetic_record_info` . Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-26B-A4B-it-GGUF:UD-Q5_K_XL) --- src/ui/category_panel.rs | 2 +- src/ui/effect.rs | 22 ++-- src/ui/grid.rs | 240 +++++++++++++++++++-------------------- src/ui/view_panel.rs | 2 +- src/view/layout.rs | 142 +++++++++++++++++------ src/view/mod.rs | 2 +- 6 files changed, 237 insertions(+), 173 deletions(-) diff --git a/src/ui/category_panel.rs b/src/ui/category_panel.rs index 687e67d..be75eb8 100644 --- a/src/ui/category_panel.rs +++ b/src/ui/category_panel.rs @@ -49,7 +49,7 @@ impl<'a> Widget for CategoryPanel<'a> { let is_active = matches!(self.mode, AppMode::CategoryPanel) || is_item_add || is_cat_add; let (border_color, title) = if is_active { - (Color::Cyan, " Categories n:new d:del Space:axis ") + (Color::Cyan, " Categories ") } else { (Color::DarkGray, " Categories ") }; diff --git a/src/ui/effect.rs b/src/ui/effect.rs index 7420466..a4f26f3 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 { @@ -406,7 +398,7 @@ pub struct StartDrill(pub Vec<(CellKey, CellValue)>); impl Effect for StartDrill { fn apply(&self, app: &mut App) { app.drill_state = Some(super::app::DrillState { - records: self.0.clone(), + records: std::rc::Rc::new(self.0.clone()), pending_edits: std::collections::HashMap::new(), }); } @@ -838,6 +830,16 @@ pub enum Panel { View, } +impl Panel { + pub fn mode(self) -> AppMode { + match self { + Panel::Formula => AppMode::FormulaPanel, + Panel::Category => AppMode::CategoryPanel, + Panel::View => AppMode::ViewPanel, + } + } +} + impl Effect for SetPanelOpen { fn apply(&self, app: &mut App) { match self.panel { diff --git a/src/ui/grid.rs b/src/ui/grid.rs index b97a23e..4725d07 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}; @@ -23,6 +22,7 @@ const GROUP_COLLAPSED: &str = "▶"; pub struct GridWidget<'a> { pub model: &'a Model, + pub layout: &'a GridLayout, pub mode: &'a AppMode, pub search_query: &'a str, pub buffers: &'a std::collections::HashMap, @@ -32,6 +32,7 @@ pub struct GridWidget<'a> { impl<'a> GridWidget<'a> { pub fn new( model: &'a Model, + layout: &'a GridLayout, mode: &'a AppMode, search_query: &'a str, buffers: &'a std::collections::HashMap, @@ -39,6 +40,7 @@ impl<'a> GridWidget<'a> { ) -> Self { Self { model, + layout, mode, search_query, buffers, @@ -46,23 +48,9 @@ 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(); - - let frozen = self.drill_state.map(|s| s.records.clone()); - let layout = GridLayout::with_frozen_records(self.model, view, frozen); + let layout = self.layout; let (sel_row, sel_col) = view.selected; let row_offset = view.row_offset; let col_offset = view.col_offset; @@ -71,56 +59,9 @@ impl<'a> GridWidget<'a> { let n_col_levels = layout.col_cats.len().max(1); let n_row_levels = layout.row_cats.len().max(1); - // ── Adaptive column widths ──────────────────────────────────── - // Size each column to fit its widest content (header + cell values) - // plus 1 char gap. Minimum MIN_COL_WIDTH, capped at MAX_COL_WIDTH. - let col_widths: Vec = { - let n = layout.col_count(); - let mut widths = vec![0u16; n]; - // Measure column header labels - for ci in 0..n { - let header = layout.col_label(ci); - let w = header.width() as u16; - if w > widths[ci] { - widths[ci] = w; - } - } - // Measure cell content - if layout.is_records_mode() { - for ri in 0..layout.row_count() { - for (ci, wref) in widths.iter_mut().enumerate().take(n) { - let s = self.records_cell_text(&layout, ri, ci); - let w = s.width() as u16; - if w > *wref { - *wref = w; - } - } - } - } else { - // Pivot mode: measure formatted cell values - 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 = - self.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; - } - } - } - } - } - // +1 for gap between columns - widths - .into_iter() - .map(|w| (w + 1).max(MIN_COL_WIDTH).min(MAX_COL_WIDTH)) - .collect() - }; + let col_widths = compute_col_widths(self.model, &layout, fmt_comma, fmt_decimals); // ── Adaptive row header widths ─────────────────────────────── - // Measure the widest label at each row-header level. let data_row_items: Vec<&Vec> = layout .row_items .iter() @@ -410,23 +351,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() @@ -453,13 +386,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() @@ -588,53 +521,111 @@ impl<'a> Widget for GridWidget<'a> { } } -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(','); +/// Compute adaptive column widths for pivot mode (header labels + cell values). +/// Header widths use the widest *individual* level label (not the joined +/// multi-level string), matching how the grid renderer draws each level on +/// its own row with repeat-suppression. +pub fn compute_col_widths(model: &Model, layout: &GridLayout, fmt_comma: bool, fmt_decimals: u8) -> Vec { + let n = layout.col_count(); + let mut widths = vec![0u16; n]; + // Measure individual header level labels + let data_col_items: Vec<&Vec> = layout + .col_items + .iter() + .filter_map(|e| { + if let AxisEntry::DataItem(v) = e { + Some(v) + } else { + None + } + }) + .collect(); + for (ci, wref) in widths.iter_mut().enumerate().take(n) { + if let Some(levels) = data_col_items.get(ci) { + let max_level_w = levels.iter().map(|s| s.width() as u16).max().unwrap_or(0); + if max_level_w > *wref { + *wref = max_level_w; + } } - result.push(c); } - if is_neg { - result.push('-'); + // 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; + } + } } - let mut out: String = result.chars().rev().collect(); - if let Some(dec) = dec_part { - out.push_str(dec); + // 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; + } + } } - out + widths + .into_iter() + .map(|w| (w + 1).max(MIN_COL_WIDTH).min(MAX_COL_WIDTH)) + .collect() } +/// Compute the total row header width from the layout's row items. +pub fn compute_row_header_width(layout: &GridLayout) -> u16 { + let n_row_levels = layout.row_cats.len().max(1); + let data_row_items: Vec<&Vec> = layout + .row_items + .iter() + .filter_map(|e| { + if let AxisEntry::DataItem(v) = e { + Some(v) + } else { + None + } + }) + .collect(); + let sub_widths: Vec = (0..n_row_levels) + .map(|d| { + let max_label = data_row_items + .iter() + .filter_map(|v| v.get(d)) + .map(|s| s.width() as u16) + .max() + .unwrap_or(0); + (max_label + 1).max(MIN_ROW_HEADER_W).min(MAX_ROW_HEADER_W) + }) + .collect(); + sub_widths.iter().sum() +} + +/// Count how many columns fit starting from `col_offset` given the available width. +pub fn compute_visible_cols(col_widths: &[u16], row_header_width: u16, term_width: u16, col_offset: usize) -> usize { + // Account for grid border (2 chars) + let data_area_width = term_width.saturating_sub(2).saturating_sub(row_header_width); + let mut acc = 0u16; + let mut count = 0usize; + for ci in col_offset..col_widths.len() { + let w = col_widths[ci]; + if acc + w > data_area_width { + break; + } + acc += w; + count += 1; + } + count.max(1) +} + +// 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(); if w <= max_width { @@ -674,7 +665,8 @@ mod tests { let area = Rect::new(0, 0, width, height); let mut buf = Buffer::empty(area); let bufs = std::collections::HashMap::new(); - GridWidget::new(model, &AppMode::Normal, "", &bufs, None).render(area, &mut buf); + let layout = GridLayout::new(model, model.active_view()); + GridWidget::new(model, &layout, &AppMode::Normal, "", &bufs, None).render(area, &mut buf); buf } diff --git a/src/ui/view_panel.rs b/src/ui/view_panel.rs index e087ddb..123eb97 100644 --- a/src/ui/view_panel.rs +++ b/src/ui/view_panel.rs @@ -36,7 +36,7 @@ impl<'a> Widget for ViewPanel<'a> { let block = Block::default() .borders(Borders::ALL) .border_style(border_style) - .title(" Views [Enter] switch [n]ew [d]elete "); + .title(" Views "); let inner = block.inner(area); block.render(area, buf); diff --git a/src/view/layout.rs b/src/view/layout.rs index 7cf4066..2d5b7b5 100644 --- a/src/view/layout.rs +++ b/src/view/layout.rs @@ -1,7 +1,17 @@ +use std::rc::Rc; + 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 @@ -30,8 +40,8 @@ pub struct GridLayout { /// Categories on `Axis::None` — hidden, implicitly aggregated. pub none_cats: Vec, /// In records mode: the filtered cell list, one per row. - /// None for normal pivot views. - pub records: Option>, + /// None for normal pivot views. Rc for cheap sharing. + pub records: Option>>, } impl GridLayout { @@ -40,12 +50,11 @@ impl GridLayout { pub fn with_frozen_records( model: &Model, view: &View, - frozen_records: Option>, + frozen_records: Option>>, ) -> Self { let mut layout = Self::new(model, view); if layout.is_records_mode() { if let Some(records) = frozen_records { - // Re-build with the frozen records instead let row_items: Vec = (0..records.len()) .map(|i| AxisEntry::DataItem(vec![i.to_string()])) .collect(); @@ -175,7 +184,7 @@ impl GridLayout { row_items, col_items, none_cats, - records: Some(records), + records: Some(Rc::new(records)), } } @@ -220,14 +229,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(); } } @@ -297,7 +302,7 @@ impl GridLayout { .map(|i| AxisEntry::DataItem(vec![i.to_string()])) .collect(); self.row_items = new_row_items; - self.records = Some(new_records); + self.records = Some(Rc::new(new_records)); } } @@ -352,18 +357,57 @@ 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()) + } + } + + /// 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 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 +571,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 +636,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;