From 6038cb2d8123341f4bf0c3f326e8068edd7f3338 Mon Sep 17 00:00:00 2001 From: Ed L Date: Tue, 24 Mar 2026 09:00:25 -0700 Subject: [PATCH] refactor: make active_view and axis_of infallible Both functions previously returned Option despite their invariants guaranteeing a value: active_view always names an existing view (maintained by new/switch_view/delete_view), and axis_of only returns None for categories never registered with the view (a programming error). Callers no longer need to handle the impossible None case, eliminating ~15 match/if-let Option guards across app.rs, dispatch.rs, grid.rs, tile_bar.rs, and category_panel.rs. Also adds Model::evaluate_f64 (returns 0.0 for empty cells) and collapses the double match-on-axis pattern in tile_bar/category_panel into a single axis_display(Axis) helper. Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 6 +++ src/command/dispatch.rs | 18 +++---- src/model/model.rs | 54 +++++++++++-------- src/ui/app.rs | 114 ++++++++++++++++----------------------- src/ui/category_panel.rs | 27 ++++------ src/ui/grid.rs | 70 ++++++++++-------------- src/ui/tile_bar.rs | 29 ++++------ src/view/layout.rs | 16 +++--- src/view/view.rs | 42 +++++++-------- 9 files changed, 168 insertions(+), 208 deletions(-) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..8665bbc --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,6 @@ +- Always use tests to demonstrate the existence of a bug before fixing the bug. + - If you suspect that a bug exists, use a test to demonstrate it first: + - prefer unit tests testing a small amount of code to integration or e2e tests +- Option<...> or Result<...> are fine but should not be present in the majority of the code. +- Similarly, code managing Box<...> or RC<...>, etc. for containers pointing to heap data should be split + from logic diff --git a/src/command/dispatch.rs b/src/command/dispatch.rs index e8d45c0..012e41b 100644 --- a/src/command/dispatch.rs +++ b/src/command/dispatch.rs @@ -104,24 +104,18 @@ pub fn dispatch(model: &mut Model, cmd: &Command) -> CommandResult { } Command::SetAxis { category, axis } => { - match model.active_view_mut() { - Some(view) => { view.set_axis(category, *axis); CommandResult::ok() } - None => CommandResult::err("No active view"), - } + model.active_view_mut().set_axis(category, *axis); + CommandResult::ok() } Command::SetPageSelection { category, item } => { - match model.active_view_mut() { - Some(view) => { view.set_page_selection(category, item); CommandResult::ok() } - None => CommandResult::err("No active view"), - } + model.active_view_mut().set_page_selection(category, item); + CommandResult::ok() } Command::ToggleGroup { category, group } => { - match model.active_view_mut() { - Some(view) => { view.toggle_group_collapse(category, group); CommandResult::ok() } - None => CommandResult::err("No active view"), - } + model.active_view_mut().toggle_group_collapse(category, group); + CommandResult::ok() } Command::Save { path } => { diff --git a/src/model/model.rs b/src/model/model.rs index a615e2e..74f89de 100644 --- a/src/model/model.rs +++ b/src/model/model.rs @@ -94,12 +94,14 @@ impl Model { &self.formulas } - pub fn active_view(&self) -> Option<&View> { + pub fn active_view(&self) -> &View { self.views.get(&self.active_view) + .expect("active_view always names an existing view") } - pub fn active_view_mut(&mut self) -> Option<&mut View> { + pub fn active_view_mut(&mut self) -> &mut View { self.views.get_mut(&self.active_view) + .expect("active_view always names an existing view") } pub fn create_view(&mut self, name: impl Into) -> &mut View { @@ -161,6 +163,11 @@ impl Model { self.data.get(key).cloned() } + /// Evaluate a key as a numeric value, returning 0.0 for empty/non-numeric cells. + pub fn evaluate_f64(&self, key: &CellKey) -> f64 { + self.evaluate(key).and_then(|v| v.as_f64()).unwrap_or(0.0) + } + fn eval_formula(&self, formula: &Formula, context: &CellKey) -> Option { use crate::formula::{Expr, AggFunc}; @@ -294,7 +301,8 @@ mod model_tests { #[test] fn new_model_has_default_view() { let m = Model::new("Test"); - assert!(m.active_view().is_some()); + // active_view() panics if missing; this test just ensures it doesn't panic + let _ = m.active_view(); } #[test] @@ -324,7 +332,8 @@ mod model_tests { fn add_category_notifies_existing_views() { let mut m = Model::new("Test"); m.add_category("Region").unwrap(); - assert_ne!(m.active_view().unwrap().axis_of("Region"), None); + // axis_of panics for unknown categories; not panicking here confirms it was registered + let _ = m.active_view().axis_of("Region"); } #[test] @@ -380,8 +389,9 @@ mod model_tests { m.add_category("Product").unwrap(); m.create_view("Secondary"); let v = m.views.get("Secondary").unwrap(); - assert_ne!(v.axis_of("Region"), None); - assert_ne!(v.axis_of("Product"), None); + // axis_of panics for unknown categories; not panicking confirms categories were registered + let _ = v.axis_of("Region"); + let _ = v.axis_of("Product"); } #[test] @@ -427,10 +437,10 @@ mod model_tests { m.add_category("Region").unwrap(); m.add_category("Product").unwrap(); m.add_category("Time").unwrap(); - let v = m.active_view().unwrap(); - assert_eq!(v.axis_of("Region"), Some(Axis::Row)); - assert_eq!(v.axis_of("Product"), Some(Axis::Column)); - assert_eq!(v.axis_of("Time"), Some(Axis::Page)); + let v = m.active_view(); + assert_eq!(v.axis_of("Region"), Axis::Row); + assert_eq!(v.axis_of("Product"), Axis::Column); + assert_eq!(v.axis_of("Time"), Axis::Page); } #[test] @@ -969,18 +979,19 @@ mod five_category { #[test] fn default_view_first_two_on_axes_rest_on_page() { let m = build_model(); - let v = m.active_view().unwrap(); - assert_eq!(v.axis_of("Region"), Some(Axis::Row)); - assert_eq!(v.axis_of("Product"), Some(Axis::Column)); - assert_eq!(v.axis_of("Channel"), Some(Axis::Page)); - assert_eq!(v.axis_of("Time"), Some(Axis::Page)); - assert_eq!(v.axis_of("Measure"), Some(Axis::Page)); + let v = m.active_view(); + assert_eq!(v.axis_of("Region"), Axis::Row); + assert_eq!(v.axis_of("Product"), Axis::Column); + assert_eq!(v.axis_of("Channel"), Axis::Page); + assert_eq!(v.axis_of("Time"), Axis::Page); + assert_eq!(v.axis_of("Measure"), Axis::Page); } #[test] fn rearranging_axes_does_not_affect_data() { let mut m = build_model(); - if let Some(v) = m.active_view_mut() { + { + let v = m.active_view_mut(); v.set_axis("Region", Axis::Page); v.set_axis("Product", Axis::Page); v.set_axis("Channel", Axis::Row); @@ -994,16 +1005,17 @@ mod five_category { fn two_views_have_independent_axis_assignments() { let mut m = build_model(); m.create_view("Pivot"); - if let Some(v) = m.views.get_mut("Pivot") { + { + let v = m.views.get_mut("Pivot").unwrap(); v.set_axis("Time", Axis::Row); v.set_axis("Channel", Axis::Column); v.set_axis("Region", Axis::Page); v.set_axis("Product", Axis::Page); v.set_axis("Measure", Axis::Page); } - assert_eq!(m.views.get("Default").unwrap().axis_of("Region"), Some(Axis::Row)); - assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), Some(Axis::Row)); - assert_eq!(m.views.get("Pivot").unwrap().axis_of("Channel"), Some(Axis::Column)); + assert_eq!(m.views.get("Default").unwrap().axis_of("Region"), Axis::Row); + assert_eq!(m.views.get("Pivot").unwrap().axis_of("Time"), Axis::Row); + assert_eq!(m.views.get("Pivot").unwrap().axis_of("Channel"), Axis::Column); } #[test] diff --git a/src/ui/app.rs b/src/ui/app.rs index 1185973..507e187 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -209,10 +209,9 @@ impl App { // 0 = first col, $ = last col (KeyCode::Char('0'), KeyModifiers::NONE) => { - if let Some(view) = self.model.active_view_mut() { - view.selected.1 = 0; - view.col_offset = 0; - } + let view = self.model.active_view_mut(); + view.selected.1 = 0; + view.col_offset = 0; } (KeyCode::Char('$'), _) => { self.jump_to_last_col(); } @@ -304,7 +303,8 @@ impl App { match (first, key.code) { // gg = first row ('g', KeyCode::Char('g')) => { - if let Some(view) = self.model.active_view_mut() { + { + let view = self.model.active_view_mut(); view.selected = (0, view.selected.1); view.row_offset = 0; } @@ -510,9 +510,7 @@ impl App { if rest.is_empty() { self.status_msg = "Usage: :set-format e.g. ,.0 ,.2 .4".to_string(); } else { - if let Some(view) = self.model.active_view_mut() { - view.number_format = rest.to_string(); - } + self.model.active_view_mut().number_format = rest.to_string(); self.status_msg = format!("Number format set to '{rest}'"); self.dirty = true; } @@ -636,9 +634,7 @@ impl App { } KeyCode::Enter | KeyCode::Char(' ') => { if let Some(cat_name) = cat_names.get(self.cat_panel_cursor) { - if let Some(view) = self.model.active_view_mut() { - view.cycle_axis(cat_name); - } + self.model.active_view_mut().cycle_axis(cat_name); } } // n — add a new category @@ -805,7 +801,7 @@ impl App { } KeyCode::Enter | KeyCode::Char(' ') => { if let Some(name) = cat_names.get(cat_idx) { - if let Some(view) = self.model.active_view_mut() { view.cycle_axis(name); } + self.model.active_view_mut().cycle_axis(name); self.dirty = true; } self.mode = AppMode::Normal; @@ -917,57 +913,42 @@ impl App { fn move_selection(&mut self, dr: i32, dc: i32) { let (row_max, col_max) = { - let view = match self.model.active_view() { Some(v) => v, None => return }; - let layout = GridLayout::new(&self.model, view); + let layout = GridLayout::new(&self.model, self.model.active_view()); (layout.row_count().saturating_sub(1), layout.col_count().saturating_sub(1)) }; - - if let Some(view) = self.model.active_view_mut() { - let (r, c) = view.selected; - let nr = (r as i32 + dr).clamp(0, row_max as i32) as usize; - let nc = (c as i32 + dc).clamp(0, col_max as i32) as usize; - view.selected = (nr, nc); - // Keep cursor in visible area (approximate viewport: 20 rows, 8 cols) - if nr < view.row_offset { view.row_offset = nr; } - if nr >= view.row_offset + 20 { view.row_offset = nr.saturating_sub(19); } - if nc < view.col_offset { view.col_offset = nc; } - if nc >= view.col_offset + 8 { view.col_offset = nc.saturating_sub(7); } - } + let view = self.model.active_view_mut(); + let (r, c) = view.selected; + let nr = (r as i32 + dr).clamp(0, row_max as i32) as usize; + let nc = (c as i32 + dc).clamp(0, col_max as i32) as usize; + view.selected = (nr, nc); + // Keep cursor in visible area (approximate viewport: 20 rows, 8 cols) + if nr < view.row_offset { view.row_offset = nr; } + if nr >= view.row_offset + 20 { view.row_offset = nr.saturating_sub(19); } + if nc < view.col_offset { view.col_offset = nc; } + if nc >= view.col_offset + 8 { view.col_offset = nc.saturating_sub(7); } } fn jump_to_last_row(&mut self) { - let count = { - let view = match self.model.active_view() { Some(v) => v, None => return }; - GridLayout::new(&self.model, view).row_count().saturating_sub(1) - }; - if let Some(view) = self.model.active_view_mut() { - view.selected.0 = count; - if count >= view.row_offset + 20 { view.row_offset = count.saturating_sub(19); } - } + let count = GridLayout::new(&self.model, self.model.active_view()).row_count().saturating_sub(1); + let view = self.model.active_view_mut(); + view.selected.0 = count; + if count >= view.row_offset + 20 { view.row_offset = count.saturating_sub(19); } } fn jump_to_last_col(&mut self) { - let count = { - let view = match self.model.active_view() { Some(v) => v, None => return }; - GridLayout::new(&self.model, view).col_count().saturating_sub(1) - }; - if let Some(view) = self.model.active_view_mut() { - view.selected.1 = count; - if count >= view.col_offset + 8 { view.col_offset = count.saturating_sub(7); } - } + let count = GridLayout::new(&self.model, self.model.active_view()).col_count().saturating_sub(1); + let view = self.model.active_view_mut(); + view.selected.1 = count; + if count >= view.col_offset + 8 { view.col_offset = count.saturating_sub(7); } } fn scroll_rows(&mut self, delta: i32) { - let row_max = { - let view = match self.model.active_view() { Some(v) => v, None => return }; - GridLayout::new(&self.model, view).row_count().saturating_sub(1) - }; - if let Some(view) = self.model.active_view_mut() { - let nr = (view.selected.0 as i32 + delta).clamp(0, row_max as i32) as usize; - view.selected.0 = nr; - if nr < view.row_offset { view.row_offset = nr; } - if nr >= view.row_offset + 20 { view.row_offset = nr.saturating_sub(19); } - } + let row_max = GridLayout::new(&self.model, self.model.active_view()).row_count().saturating_sub(1); + let view = self.model.active_view_mut(); + let nr = (view.selected.0 as i32 + delta).clamp(0, row_max as i32) as usize; + view.selected.0 = nr; + if nr < view.row_offset { view.row_offset = nr; } + if nr >= view.row_offset + 20 { view.row_offset = nr.saturating_sub(19); } } /// Navigate to the next (`forward=true`) or previous (`forward=false`) cell @@ -976,10 +957,7 @@ impl App { let query = self.search_query.to_lowercase(); if query.is_empty() { return; } - let view = match self.model.active_view() { - Some(v) => v, - None => return, - }; + let view = self.model.active_view(); let layout = GridLayout::new(&self.model, view); let (cur_row, cur_col) = view.selected; @@ -1022,7 +1000,8 @@ impl App { if let Some(flat) = target_flat { let ri = flat / total_cols; let ci = flat % total_cols; - if let Some(view) = self.model.active_view_mut() { + { + let view = self.model.active_view_mut(); view.selected = (ri, ci); // Adjust scroll offsets to keep cursor visible if ri < view.row_offset { view.row_offset = ri; } @@ -1038,15 +1017,14 @@ impl App { /// Gather (cat_name, items, current_idx) for all non-empty page categories. fn page_cat_data(&self) -> Vec<(String, Vec, usize)> { let page_cats: Vec = self.model.active_view() - .map(|v| v.categories_on(Axis::Page).into_iter().map(String::from).collect()) - .unwrap_or_default(); + .categories_on(Axis::Page).into_iter().map(String::from).collect(); page_cats.into_iter().filter_map(|cat| { let items: Vec = self.model.category(&cat) .map(|c| c.ordered_item_names().into_iter().map(String::from).collect()) .unwrap_or_default(); if items.is_empty() { return None; } let current = self.model.active_view() - .and_then(|v| v.page_selection(&cat)) + .page_selection(&cat) .map(String::from) .or_else(|| items.first().cloned()) .unwrap_or_default(); @@ -1066,10 +1044,9 @@ impl App { indices[i] += 1; if indices[i] >= data[i].1.len() { indices[i] = 0; } else { carry = false; } } - if let Some(view) = self.model.active_view_mut() { - for (i, (cat, items, _)) in data.iter().enumerate() { - view.set_page_selection(cat, &items[indices[i]]); - } + let view = self.model.active_view_mut(); + for (i, (cat, items, _)) in data.iter().enumerate() { + view.set_page_selection(cat, &items[indices[i]]); } } @@ -1088,17 +1065,16 @@ impl App { borrow = false; } } - if let Some(view) = self.model.active_view_mut() { - for (i, (cat, items, _)) in data.iter().enumerate() { - view.set_page_selection(cat, &items[indices[i]]); - } + let view = self.model.active_view_mut(); + for (i, (cat, items, _)) in data.iter().enumerate() { + view.set_page_selection(cat, &items[indices[i]]); } } // ── Cell key resolution ────────────────────────────────────────────────── pub fn selected_cell_key(&self) -> Option { - let view = self.model.active_view()?; + let view = self.model.active_view(); let (sel_row, sel_col) = view.selected; GridLayout::new(&self.model, view).cell_key(sel_row, sel_col) } diff --git a/src/ui/category_panel.rs b/src/ui/category_panel.rs index 3b6f33e..b76c3b6 100644 --- a/src/ui/category_panel.rs +++ b/src/ui/category_panel.rs @@ -9,6 +9,14 @@ use crate::model::Model; use crate::view::Axis; use crate::ui::app::AppMode; +fn axis_display(axis: Axis) -> (&'static str, Color) { + match axis { + Axis::Row => ("Row ↕", Color::Green), + Axis::Column => ("Col ↔", Color::Blue), + Axis::Page => ("Page ☰", Color::Magenta), + } +} + pub struct CategoryPanel<'a> { pub model: &'a Model, pub mode: &'a AppMode, @@ -44,10 +52,7 @@ impl<'a> Widget for CategoryPanel<'a> { let inner = block.inner(area); block.render(area, buf); - let view = match self.model.active_view() { - Some(v) => v, - None => return, - }; + let view = self.model.active_view(); let cat_names: Vec<&str> = self.model.category_names(); if cat_names.is_empty() { @@ -65,19 +70,7 @@ impl<'a> Widget for CategoryPanel<'a> { if i as u16 >= list_height { break; } let y = inner.y + i as u16; - let axis = view.axis_of(cat_name); - let axis_str = match axis { - Some(Axis::Row) => "Row ↕", - Some(Axis::Column) => "Col ↔", - Some(Axis::Page) => "Page ☰", - None => "none", - }; - let axis_color = match axis { - Some(Axis::Row) => Color::Green, - Some(Axis::Column) => Color::Blue, - Some(Axis::Page) => Color::Magenta, - None => Color::DarkGray, - }; + let (axis_str, axis_color) = axis_display(view.axis_of(cat_name)); let item_count = self.model.category(cat_name).map(|c| c.items.len()).unwrap_or(0); diff --git a/src/ui/grid.rs b/src/ui/grid.rs index 674dfb2..845f6d7 100644 --- a/src/ui/grid.rs +++ b/src/ui/grid.rs @@ -27,10 +27,7 @@ impl<'a> GridWidget<'a> { } fn render_grid(&self, area: Rect, buf: &mut Buffer) { - let view = match self.model.active_view() { - Some(v) => v, - None => return, - }; + let view = self.model.active_view(); let layout = GridLayout::new(self.model, view); let (sel_row, sel_col) = view.selected; @@ -97,9 +94,7 @@ impl<'a> GridWidget<'a> { }; let value = self.model.evaluate(&key); - let cell_str = value.as_ref() - .map(|v| format_value(v, fmt_comma, fmt_decimals)) - .unwrap_or_default(); + let cell_str = format_value(value.as_ref(), fmt_comma, fmt_decimals); let is_selected = ri == sel_row && ci == sel_col; let is_search_match = !self.search_query.is_empty() && cell_str.to_lowercase().contains(&self.search_query.to_lowercase()); @@ -151,7 +146,7 @@ impl<'a> GridWidget<'a> { if x >= area.x + area.width { break; } let total: f64 = (0..layout.row_count()) .filter_map(|ri| layout.cell_key(ri, ci)) - .map(|key| self.model.evaluate(&key).and_then(|v| v.as_f64()).unwrap_or(0.0)) + .map(|key| self.model.evaluate_f64(&key)) .sum(); let total_str = format_f64(total, fmt_comma, fmt_decimals); buf.set_string(x, y, @@ -176,34 +171,33 @@ impl<'a> Widget for GridWidget<'a> { block.render(area, buf); // Page axis bar - if let Some(view) = self.model.active_view() { - let layout = GridLayout::new(self.model, view); - if !layout.page_coords.is_empty() && inner.height > 0 { - let page_info: Vec = layout.page_coords.iter() - .map(|(cat, sel)| format!("{cat} = {sel}")) - .collect(); - let page_str = format!(" [{}] ", page_info.join(" | ")); - buf.set_string(inner.x, inner.y, - &page_str, - Style::default().fg(Color::Magenta)); + let layout = GridLayout::new(self.model, self.model.active_view()); + if !layout.page_coords.is_empty() && inner.height > 0 { + let page_info: Vec = layout.page_coords.iter() + .map(|(cat, sel)| format!("{cat} = {sel}")) + .collect(); + let page_str = format!(" [{}] ", page_info.join(" | ")); + buf.set_string(inner.x, inner.y, + &page_str, + Style::default().fg(Color::Magenta)); - let grid_area = Rect { - y: inner.y + 1, - height: inner.height.saturating_sub(1), - ..inner - }; - self.render_grid(grid_area, buf); - } else { - self.render_grid(inner, buf); - } + let grid_area = Rect { + y: inner.y + 1, + height: inner.height.saturating_sub(1), + ..inner + }; + self.render_grid(grid_area, buf); + } else { + self.render_grid(inner, buf); } } } -fn format_value(v: &CellValue, comma: bool, decimals: u8) -> String { +fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String { match v { - CellValue::Number(n) => format_f64(*n, comma, decimals), - CellValue::Text(s) => s.clone(), + Some(CellValue::Number(n)) => format_f64(*n, comma, decimals), + Some(CellValue::Text(s)) => s.clone(), + None => String::new(), } } @@ -395,9 +389,7 @@ mod tests { c.add_item("Alice"); c.add_item("Bob"); } - if let Some(v) = m.active_view_mut() { - v.set_page_selection("Payer", "Bob"); - } + m.active_view_mut().set_page_selection("Payer", "Bob"); let text = buf_text(&render(&m, 80, 24)); assert!(text.contains("Payer = Bob"), "expected 'Payer = Bob' in:\n{text}"); } @@ -451,9 +443,7 @@ mod tests { if let Some(c) = m.category_mut("Type") { c.add_item("Food"); c.add_item("Clothing"); } if let Some(c) = m.category_mut("Month") { c.add_item("Jan"); } if let Some(c) = m.category_mut("Recipient") { c.add_item("Alice"); c.add_item("Bob"); } - if let Some(v) = m.active_view_mut() { - v.set_axis("Recipient", crate::view::Axis::Row); - } + m.active_view_mut().set_axis("Recipient", crate::view::Axis::Row); let text = buf_text(&render(&m, 80, 24)); // Cross-product rows: Food/Alice, Food/Bob, Clothing/Alice, Clothing/Bob @@ -472,9 +462,7 @@ mod tests { if let Some(c) = m.category_mut("Type") { c.add_item("Food"); } if let Some(c) = m.category_mut("Month") { c.add_item("Jan"); } if let Some(c) = m.category_mut("Recipient") { c.add_item("Alice"); c.add_item("Bob"); } - if let Some(v) = m.active_view_mut() { - v.set_axis("Recipient", crate::view::Axis::Row); - } + m.active_view_mut().set_axis("Recipient", crate::view::Axis::Row); // Set data at the full 3-coordinate key m.set_cell( coord(&[("Month", "Jan"), ("Recipient", "Alice"), ("Type", "Food")]), @@ -493,9 +481,7 @@ mod tests { if let Some(c) = m.category_mut("Type") { c.add_item("Food"); } if let Some(c) = m.category_mut("Month") { c.add_item("Jan"); } if let Some(c) = m.category_mut("Year") { c.add_item("2024"); c.add_item("2025"); } - if let Some(v) = m.active_view_mut() { - v.set_axis("Year", crate::view::Axis::Column); - } + m.active_view_mut().set_axis("Year", crate::view::Axis::Column); let text = buf_text(&render(&m, 80, 24)); assert!(text.contains("Jan/2024"), "expected 'Jan/2024' in:\n{text}"); diff --git a/src/ui/tile_bar.rs b/src/ui/tile_bar.rs index a50dabc..a38c366 100644 --- a/src/ui/tile_bar.rs +++ b/src/ui/tile_bar.rs @@ -9,6 +9,14 @@ use crate::model::Model; use crate::view::Axis; use crate::ui::app::AppMode; +fn axis_display(axis: Axis) -> (&'static str, Color) { + match axis { + Axis::Row => ("↕", Color::Green), + Axis::Column => ("↔", Color::Blue), + Axis::Page => ("☰", Color::Magenta), + } +} + pub struct TileBar<'a> { pub model: &'a Model, pub mode: &'a AppMode, @@ -22,10 +30,7 @@ impl<'a> TileBar<'a> { impl<'a> Widget for TileBar<'a> { fn render(self, area: Rect, buf: &mut Buffer) { - let view = match self.model.active_view() { - Some(v) => v, - None => return, - }; + let view = self.model.active_view(); let selected_cat_idx = if let AppMode::TileSelect { cat_idx } = self.mode { Some(*cat_idx) @@ -39,26 +44,14 @@ impl<'a> Widget for TileBar<'a> { let cat_names: Vec<&str> = self.model.category_names(); for (i, cat_name) in cat_names.iter().enumerate() { - let axis = view.axis_of(cat_name); - let axis_symbol = match axis { - Some(Axis::Row) => "↕", - Some(Axis::Column) => "↔", - Some(Axis::Page) => "☰", - None => "─", - }; - + let (axis_symbol, axis_color) = axis_display(view.axis_of(cat_name)); let label = format!(" [{cat_name} {axis_symbol}] "); let is_selected = selected_cat_idx == Some(i); let style = if is_selected { Style::default().fg(Color::Black).bg(Color::Cyan).add_modifier(Modifier::BOLD) } else { - match axis { - Some(Axis::Row) => Style::default().fg(Color::Green), - Some(Axis::Column) => Style::default().fg(Color::Blue), - Some(Axis::Page) => Style::default().fg(Color::Magenta), - None => Style::default().fg(Color::DarkGray), - } + Style::default().fg(axis_color) }; if x + label.len() as u16 > area.x + area.width { break; } diff --git a/src/view/layout.rs b/src/view/layout.rs index 3dd5efe..33e7ff0 100644 --- a/src/view/layout.rs +++ b/src/view/layout.rs @@ -114,7 +114,7 @@ mod tests { #[test] fn row_and_col_counts_match_item_counts() { let m = two_cat_model(); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + let layout = GridLayout::new(&m, m.active_view()); assert_eq!(layout.row_count(), 2); // Food, Clothing assert_eq!(layout.col_count(), 2); // Jan, Feb } @@ -122,7 +122,7 @@ mod tests { #[test] fn cell_key_encodes_correct_coordinates() { let m = two_cat_model(); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + let layout = GridLayout::new(&m, m.active_view()); // row 0 = Food, col 1 = Feb let key = layout.cell_key(0, 1).unwrap(); assert_eq!(key, coord(&[("Month", "Feb"), ("Type", "Food")])); @@ -131,7 +131,7 @@ mod tests { #[test] fn cell_key_out_of_bounds_returns_none() { let m = two_cat_model(); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + let layout = GridLayout::new(&m, m.active_view()); assert!(layout.cell_key(99, 0).is_none()); assert!(layout.cell_key(0, 99).is_none()); } @@ -146,8 +146,8 @@ mod tests { m.category_mut("Month").unwrap().add_item("Jan"); m.category_mut("Region").unwrap().add_item("East"); m.category_mut("Region").unwrap().add_item("West"); - m.active_view_mut().unwrap().set_page_selection("Region", "West"); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + m.active_view_mut().set_page_selection("Region", "West"); + let layout = GridLayout::new(&m, m.active_view()); let key = layout.cell_key(0, 0).unwrap(); assert_eq!(key.get("Region"), Some("West")); } @@ -159,7 +159,7 @@ mod tests { coord(&[("Month", "Feb"), ("Type", "Clothing")]), CellValue::Number(42.0), ); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + let layout = GridLayout::new(&m, m.active_view()); // Clothing = row 1, Feb = col 1 let key = layout.cell_key(1, 1).unwrap(); assert_eq!(m.evaluate(&key), Some(CellValue::Number(42.0))); @@ -174,8 +174,8 @@ mod tests { m.category_mut("Type").unwrap().add_item("Food"); m.category_mut("Month").unwrap().add_item("Jan"); m.category_mut("Year").unwrap().add_item("2025"); - m.active_view_mut().unwrap().set_axis("Year", crate::view::Axis::Column); - let layout = GridLayout::new(&m, m.active_view().unwrap()); + m.active_view_mut().set_axis("Year", crate::view::Axis::Column); + let layout = GridLayout::new(&m, m.active_view()); assert_eq!(layout.col_label(0), "Jan/2025"); } } diff --git a/src/view/view.rs b/src/view/view.rs index f5340bb..5867451 100644 --- a/src/view/view.rs +++ b/src/view/view.rs @@ -61,8 +61,9 @@ impl View { } } - pub fn axis_of(&self, cat_name: &str) -> Option { - self.category_axes.get(cat_name).copied() + pub fn axis_of(&self, cat_name: &str) -> Axis { + *self.category_axes.get(cat_name) + .expect("axis_of called for category not registered with this view") } pub fn categories_on(&self, axis: Axis) -> Vec<&str> { @@ -114,9 +115,9 @@ impl View { /// Cycle axis for a category: Row → Column → Page → Row pub fn cycle_axis(&mut self, cat_name: &str) { let next = match self.axis_of(cat_name) { - Some(Axis::Row) | None => Axis::Column, - Some(Axis::Column) => Axis::Page, - Some(Axis::Page) => Axis::Row, + Axis::Row => Axis::Column, + Axis::Column => Axis::Page, + Axis::Page => Axis::Row, }; self.set_axis(cat_name, next); self.selected = (0, 0); @@ -139,27 +140,27 @@ mod tests { #[test] fn first_category_assigned_to_row() { let v = view_with_cats(&["Region"]); - assert_eq!(v.axis_of("Region"), Some(Axis::Row)); + assert_eq!(v.axis_of("Region"), Axis::Row); } #[test] fn second_category_assigned_to_column() { let v = view_with_cats(&["Region", "Product"]); - assert_eq!(v.axis_of("Product"), Some(Axis::Column)); + assert_eq!(v.axis_of("Product"), Axis::Column); } #[test] fn third_and_later_categories_assigned_to_page() { let v = view_with_cats(&["Region", "Product", "Time", "Scenario"]); - assert_eq!(v.axis_of("Time"), Some(Axis::Page)); - assert_eq!(v.axis_of("Scenario"), Some(Axis::Page)); + assert_eq!(v.axis_of("Time"), Axis::Page); + assert_eq!(v.axis_of("Scenario"), Axis::Page); } #[test] fn set_axis_changes_assignment() { let mut v = view_with_cats(&["Region", "Product"]); v.set_axis("Region", Axis::Column); - assert_eq!(v.axis_of("Region"), Some(Axis::Column)); + assert_eq!(v.axis_of("Region"), Axis::Column); } #[test] @@ -171,9 +172,10 @@ mod tests { } #[test] - fn axis_of_unknown_category_returns_none() { + #[should_panic(expected = "axis_of called for category not registered")] + fn axis_of_unknown_category_panics() { let v = View::new("Test"); - assert_eq!(v.axis_of("Ghost"), None); + v.axis_of("Ghost"); } #[test] @@ -208,7 +210,7 @@ mod tests { fn cycle_axis_row_to_column() { let mut v = view_with_cats(&["Region", "Product"]); v.cycle_axis("Region"); - assert_eq!(v.axis_of("Region"), Some(Axis::Column)); + assert_eq!(v.axis_of("Region"), Axis::Column); } #[test] @@ -216,14 +218,14 @@ mod tests { let mut v = view_with_cats(&["Region", "Product"]); v.set_axis("Product", Axis::Column); v.cycle_axis("Product"); - assert_eq!(v.axis_of("Product"), Some(Axis::Page)); + assert_eq!(v.axis_of("Product"), Axis::Page); } #[test] fn cycle_axis_page_to_row() { let mut v = view_with_cats(&["Region", "Product", "Time"]); v.cycle_axis("Time"); - assert_eq!(v.axis_of("Time"), Some(Axis::Row)); + assert_eq!(v.axis_of("Time"), Axis::Row); } #[test] @@ -259,9 +261,7 @@ mod prop_tests { for c in &cats { v.on_category_added(c); } for c in &cats { let axis = v.axis_of(c); - prop_assert!(axis.is_some(), - "category '{}' should be assigned after on_category_added", c); - let on_axis = v.categories_on(axis.unwrap()); + let on_axis = v.categories_on(axis); prop_assert!(on_axis.contains(&c.as_str()), "categories_on({:?}) should contain '{}'", axis, c); } @@ -287,9 +287,9 @@ mod prop_tests { fn on_category_added_idempotent(cats in unique_cat_names()) { let mut v = View::new("T"); for c in &cats { v.on_category_added(c); } - let axes_before: Vec> = cats.iter().map(|c| v.axis_of(c)).collect(); + let axes_before: Vec = cats.iter().map(|c| v.axis_of(c)).collect(); for c in &cats { v.on_category_added(c); } - let axes_after: Vec> = cats.iter().map(|c| v.axis_of(c)).collect(); + let axes_after: Vec = cats.iter().map(|c| v.axis_of(c)).collect(); prop_assert_eq!(axes_before, axes_after); } @@ -305,7 +305,7 @@ mod prop_tests { let idx = target_idx % cats.len(); let cat = &cats[idx]; v.set_axis(cat, axis); - prop_assert_eq!(v.axis_of(cat), Some(axis)); + prop_assert_eq!(v.axis_of(cat), axis); } /// After set_axis(cat, X), cat is NOT in categories_on(Y) for Y ≠ X