diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 51cb101..b76506f 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -385,13 +385,13 @@ mod tests { #[test] fn parse_sum_with_inline_where_filter() { let f = parse_formula("EastTotal = SUM(Revenue WHERE Region = \"East\")", "Foo").unwrap(); - if let Expr::Agg(AggFunc::Sum, inner, Some(filter)) = &f.expr { - assert!(matches!(**inner, Expr::Ref(_))); - assert_eq!(filter.category, "Region"); - assert_eq!(filter.item, "East"); - } else { - panic!("Expected SUM with inline WHERE filter, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::Agg(AggFunc::Sum, inner, Some(filter)) + if matches!(**inner, Expr::Ref(_)) + && filter.category == "Region" + && filter.item == "East" + )); } // ── Comparison operators ──────────────────────────────────────────── @@ -467,21 +467,20 @@ mod tests { fn parse_aggregate_name_without_parens_is_ref() { // "SUM" without parens should be treated as a reference, not a function let f = parse_formula("X = SUM + 1", "Cat").unwrap(); - assert!(matches!(f.expr, Expr::BinOp(BinOp::Add, _, _))); - if let Expr::BinOp(_, lhs, _) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(_))); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_)) + )); } #[test] fn parse_if_without_parens_is_ref() { // "IF" without parens should be treated as a reference let f = parse_formula("X = IF + 1", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Add, lhs, _) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(_))); - } else { - panic!("Expected BinOp(Add), got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_)) + )); } // ── Quoted string in WHERE ────────────────────────────────────────── @@ -540,46 +539,46 @@ mod tests { fn pipe_quoted_identifier_in_expression() { let f = parse_formula("|Total Revenue| = |Base Revenue| + Bonus", "Foo").unwrap(); assert_eq!(f.target, "|Total Revenue|"); - if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue")); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Bonus")); - } else { - panic!("Expected Add, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, rhs) + if matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue") + && matches!(**rhs, Expr::Ref(ref s) if s == "Bonus") + )); } #[test] fn pipe_quoted_keyword_as_identifier() { // A category named "WHERE" can be referenced with pipes let f = parse_formula("X = |WHERE| + |SUM|", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(ref s) if s == "WHERE")); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "SUM")); - } else { - panic!("Expected Add, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, rhs) + if matches!(**lhs, Expr::Ref(ref s) if s == "WHERE") + && matches!(**rhs, Expr::Ref(ref s) if s == "SUM") + )); } #[test] fn pipe_quoted_identifier_with_special_chars() { // Pipes allow characters that would normally break tokenization let f = parse_formula("X = |Revenue (USD)| + |Cost + Tax|", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)")); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax")); - } else { - panic!("Expected Add, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, rhs) + if matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)") + && matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax") + )); } #[test] fn pipe_quoted_in_aggregate() { let f = parse_formula("X = SUM(|Net Revenue|)", "Cat").unwrap(); - if let Expr::Agg(AggFunc::Sum, inner, None) = &f.expr { - assert!(matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue")); - } else { - panic!("Expected SUM aggregate, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::Agg(AggFunc::Sum, inner, None) + if matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue") + )); } #[test] @@ -593,12 +592,11 @@ mod tests { fn pipe_quoted_in_inline_where() { let f = parse_formula("X = SUM(Revenue WHERE |Region Name| = |East Coast|)", "Foo").unwrap(); - if let Expr::Agg(AggFunc::Sum, _, Some(filter)) = &f.expr { - assert_eq!(filter.category, "Region Name"); - assert_eq!(filter.item, "East Coast"); - } else { - panic!("Expected SUM with WHERE filter, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::Agg(AggFunc::Sum, _, Some(filter)) + if filter.category == "Region Name" && filter.item == "East Coast" + )); } // ── Pipe-quoted escape semantics ──────────────────────────────────── @@ -607,33 +605,21 @@ mod tests { fn pipe_quoted_escape_literal_pipe() { // \| inside a pipe-quoted identifier is a literal pipe let f = parse_formula("X = |A\\|B|", "Cat").unwrap(); - if let Expr::Ref(ref s) = f.expr { - assert_eq!(s, "A|B"); - } else { - panic!("Expected Ref, got: {:?}", f.expr); - } + assert!(matches!(&f.expr, Expr::Ref(s) if s == "A|B")); } #[test] fn pipe_quoted_escape_double_backslash() { // \\ inside a pipe-quoted identifier is a literal backslash let f = parse_formula("X = |A\\\\B|", "Cat").unwrap(); - if let Expr::Ref(ref s) = f.expr { - assert_eq!(s, "A\\B"); - } else { - panic!("Expected Ref, got: {:?}", f.expr); - } + assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\B")); } #[test] fn pipe_quoted_escape_newline() { // \n inside a pipe-quoted identifier is a literal newline let f = parse_formula("X = |A\\nB|", "Cat").unwrap(); - if let Expr::Ref(ref s) = f.expr { - assert_eq!(s, "A\nB"); - } else { - panic!("Expected Ref, got: {:?}", f.expr); - } + assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\nB")); } #[test] @@ -642,11 +628,7 @@ mod tests { // backslash-plus-character. The grammar's `"\\" ~ ANY` allows // any following character; we just don't interpret it. let f = parse_formula("X = |A\\zB|", "Cat").unwrap(); - if let Expr::Ref(ref s) = f.expr { - assert_eq!(s, "A\\zB"); - } else { - panic!("Expected Ref, got: {:?}", f.expr); - } + assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\zB")); } // ── Operator precedence and associativity ────────────────────────── @@ -655,66 +637,62 @@ mod tests { fn mul_binds_tighter_than_add() { // `A + B * C` must parse as Add(A, Mul(B, C)) let f = parse_formula("X = A + B * C", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(_))); - assert!(matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _))); - } else { - panic!("Expected Add with Mul on rhs, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, rhs) + if matches!(**lhs, Expr::Ref(_)) + && matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _)) + )); } #[test] fn pow_binds_tighter_than_mul() { // `A * B ^ C` must parse as Mul(A, Pow(B, C)) let f = parse_formula("X = A * B ^ C", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Mul, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::Ref(_))); - assert!(matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _))); - } else { - panic!("Expected Mul with Pow on rhs, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Mul, lhs, rhs) + if matches!(**lhs, Expr::Ref(_)) + && matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _)) + )); } #[test] fn subtraction_is_left_associative() { // `A - B - C` must parse as Sub(Sub(A, B), C), not Sub(A, Sub(B, C)) let f = parse_formula("X = A - B - C", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Sub, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _))); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C")); - } else { - panic!("Expected left-associative Sub, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Sub, lhs, rhs) + if matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _)) + && matches!(**rhs, Expr::Ref(ref s) if s == "C") + )); } #[test] fn division_is_left_associative() { // `A / B / C` must parse as Div(Div(A, B), C) let f = parse_formula("X = A / B / C", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Div, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::BinOp(BinOp::Div, _, _))); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C")); - } else { - panic!("Expected left-associative Div, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Div, lhs, rhs) + if matches!(**lhs, Expr::BinOp(BinOp::Div, _, _)) + && matches!(**rhs, Expr::Ref(ref s) if s == "C") + )); } #[test] fn unary_minus_before_pow() { - // `-A ^ B` — hand-rolled parser parses this as UnaryMinus(Ref(A)) - // followed by nothing because pow is inside unary's recursion. - // My grammar: unary = unary_minus | primary, unary_minus = "-" ~ primary. - // So `-A` is UnaryMinus(Ref(A)); `^B` can't attach because pow_expr - // sits OUTSIDE unary. Result: pow_expr(unary_minus(A)) — no pow - // because the optional pow_op doesn't have another unary to bind to. - // Verify: `-A ^ B` parses to BinOp(Pow, UnaryMinus(A), B). + // `-A ^ B` must parse as Pow(UnaryMinus(A), B). The `-` binds + // through `unary_minus = { "-" ~ primary }`, producing a unary + // node that is then used as the pow_expr's base. let f = parse_formula("X = -A ^ B", "Cat").unwrap(); - if let Expr::BinOp(BinOp::Pow, lhs, rhs) = &f.expr { - assert!(matches!(**lhs, Expr::UnaryMinus(_))); - assert!(matches!(**rhs, Expr::Ref(ref s) if s == "B")); - } else { - panic!("Expected Pow(UnaryMinus, Ref), got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Pow, lhs, rhs) + if matches!(**lhs, Expr::UnaryMinus(_)) + && matches!(**rhs, Expr::Ref(ref s) if s == "B") + )); } // ── Number literal variants ──────────────────────────────────────── @@ -767,11 +745,11 @@ mod tests { fn nested_sum_aggregate() { // Nested aggregates — outer SUM wrapping an inner SUM. let f = parse_formula("X = SUM(SUM(Revenue))", "Cat").unwrap(); - if let Expr::Agg(AggFunc::Sum, outer_inner, None) = &f.expr { - assert!(matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None))); - } else { - panic!("Expected nested SUM aggregates, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::Agg(AggFunc::Sum, outer_inner, None) + if matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None)) + )); } #[test] @@ -785,12 +763,12 @@ mod tests { fn nested_if_expression() { // IF in the then-branch of another IF. let f = parse_formula("X = IF(A > B, IF(C > D, 1, 2), 3)", "Cat").unwrap(); - if let Expr::If(_, then_e, else_e) = &f.expr { - assert!(matches!(**then_e, Expr::If(_, _, _))); - assert!(matches!(**else_e, Expr::Number(n) if n == 3.0)); - } else { - panic!("Expected outer IF, got: {:?}", f.expr); - } + assert!(matches!( + &f.expr, + Expr::If(_, then_e, else_e) + if matches!(**then_e, Expr::If(_, _, _)) + && matches!(**else_e, Expr::Number(n) if n == 3.0) + )); } // ── Whitespace tolerance ─────────────────────────────────────────── @@ -845,6 +823,34 @@ mod tests { assert_eq!(f.target, "my_target-name"); } + /// Regression: the hand-rolled tokenizer didn't allow `-` inside bare + /// identifiers, so references to persistence-legal names like + /// `east-coast` were silently parsed as `east - coast` (a subtraction + /// of two unrelated Refs). The pest grammar mirrors improv.pest's + /// `bare_name`, which does allow `-`, so a hyphenated name now parses + /// as a single identifier. + #[test] + fn hyphen_in_bare_identifier_reference() { + let f = parse_formula("Total = east-coast + west-coast", "Cat").unwrap(); + assert!(matches!( + &f.expr, + Expr::BinOp(BinOp::Add, lhs, rhs) + if matches!(**lhs, Expr::Ref(ref s) if s == "east-coast") + && matches!(**rhs, Expr::Ref(ref s) if s == "west-coast") + )); + } + + #[test] + fn hyphen_in_bare_identifier_inside_aggregate() { + // Same fix, but inside a SUM() to exercise the aggregate path as well. + let f = parse_formula("X = SUM(east-coast)", "Cat").unwrap(); + assert!(matches!( + &f.expr, + Expr::Agg(AggFunc::Sum, inner, None) + if matches!(**inner, Expr::Ref(ref s) if s == "east-coast") + )); + } + #[test] fn pipe_quoted_target_preserves_pipes() { let f = parse_formula("|My Target| = A", "Cat").unwrap(); @@ -907,13 +913,6 @@ mod generator { match expr { Expr::Str(s) => out.push_str(s), Expr::Insens(s) => out.push_str(s), - Expr::Range(lo, hi) => { - let lo = lo.chars().next().unwrap() as u32; - let hi = hi.chars().next().unwrap() as u32; - let range = hi - lo + 1; - let ch = char::from_u32(lo + (self.pick() as u32 % range)).unwrap(); - out.push(ch); - } Expr::Ident(name) => self.emit_ident(name, out, atomic), Expr::Seq(a, b) => { self.emit(a, out, atomic); @@ -923,18 +922,19 @@ mod generator { self.emit(b, out, atomic); } Expr::Choice(a, b) => { - let mut alts: Vec<&Expr> = vec![a.as_ref()]; - let mut cur = b.as_ref(); - while let Expr::Choice(l, r) = cur { - alts.push(l.as_ref()); - cur = r.as_ref(); + // 50/50 between the two branches of each Choice. + // Nested Choices (used by rules with 3+ alternatives) + // recurse into themselves, so deeper branches are + // chosen less often than shallower ones — good + // enough for the random-generation use case. + if self.pick().is_multiple_of(2) { + self.emit(a, out, atomic); + } else { + self.emit(b, out, atomic); } - alts.push(cur); - let idx = self.pick() as usize % alts.len(); - self.emit(alts[idx], out, atomic); } Expr::Opt(inner) => { - // ~66% chance of emitting + // ~66% chance of emitting the optional branch. if !self.pick().is_multiple_of(3) { self.emit(inner, out, atomic); } @@ -949,20 +949,20 @@ mod generator { } } Expr::RepOnce(inner) => { + // formula.pest only uses `+` inside atomic rules + // (ASCII_DIGIT+), so inner repetitions never need + // whitespace separation. let count = 1 + self.pick() % 3; - for i in 0..count { - if i > 0 && !atomic { - out.push(' '); - } + for _ in 0..count { self.emit(inner, out, atomic); } } - Expr::NegPred(_) | Expr::PosPred(_) => { - // Lookaheads don't emit output. - } - _ => { - // Skip unsupported expressions. - } + // Lookaheads (NegPred, PosPred) don't emit output. Any + // other Expr variant (Range, RepExact, Push, PeekSlice, + // Skip, …) is unused by formula.pest and silently + // produces nothing; if the grammar starts using one, + // generated formulas will fail to parse and we'll know. + _ => {} } } @@ -984,21 +984,31 @@ mod generator { "NEWLINE" => out.push('\n'), "SOI" | "EOI" => {} _ => { - if let Some((ty, inner)) = self.rules.get(name).cloned() { - let inner_atomic = atomic - || matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); - self.emit(&inner, out, inner_atomic); - } + // Every Ident in formula.pest refers to a rule that + // exists in the grammar file — if it doesn't, the + // grammar itself failed to compile and we wouldn't be + // running tests, so `expect` here is a bug marker. + let (ty, inner) = self + .rules + .get(name) + .cloned() + .expect("rule referenced by grammar exists"); + let inner_atomic = + atomic || matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); + self.emit(&inner, out, inner_atomic); } } } pub fn generate(&mut self, rule_name: &str) -> String { let mut out = String::new(); - if let Some((ty, expr)) = self.rules.get(rule_name).cloned() { - let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); - self.emit(&expr, &mut out, atomic); - } + let (ty, expr) = self + .rules + .get(rule_name) + .cloned() + .expect("entry rule exists in formula.pest"); + let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); + self.emit(&expr, &mut out, atomic); out } } diff --git a/src/command/cmd/commit.rs b/src/command/cmd/commit.rs index 80a3cd8..1b166cf 100644 --- a/src/command/cmd/commit.rs +++ b/src/command/cmd/commit.rs @@ -3,6 +3,7 @@ use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; use super::core::{Cmd, CmdContext}; +use super::grid::AddRecordRow; use super::navigation::{CursorState, EnterAdvance, viewport_effects}; #[cfg(test)] @@ -259,15 +260,31 @@ impl Cmd for CommitAndAdvance { } AdvanceDir::Right => { let col_max = self.cursor.col_count.saturating_sub(1); - let nc = (self.cursor.col + 1).min(col_max); - effects.extend(viewport_effects( - self.cursor.row, - nc, - self.cursor.row_offset, - self.cursor.col_offset, - self.cursor.visible_rows, - self.cursor.visible_cols, - )); + let row_max = self.cursor.row_count.saturating_sub(1); + let at_bottom_right = self.cursor.row >= row_max && self.cursor.col >= col_max; + + if at_bottom_right && ctx.is_records_mode() { + let add = AddRecordRow; + effects.extend(add.execute(ctx)); + effects.extend(viewport_effects( + self.cursor.row + 1, + 0, + self.cursor.row_offset, + self.cursor.col_offset, + self.cursor.visible_rows, + self.cursor.visible_cols, + )); + } else { + let nc = (self.cursor.col + 1).min(col_max); + effects.extend(viewport_effects( + self.cursor.row, + nc, + self.cursor.row_offset, + self.cursor.col_offset, + self.cursor.visible_rows, + self.cursor.visible_cols, + )); + } } } effects.push(Box::new(effect::EnterEditAtCursor)); diff --git a/src/command/cmd/core.rs b/src/command/cmd/core.rs index 4134f70..a7d4f22 100644 --- a/src/command/cmd/core.rs +++ b/src/command/cmd/core.rs @@ -41,8 +41,8 @@ pub struct CmdContext<'a> { /// Named text buffers pub buffers: &'a HashMap, /// View navigation stacks (for drill back/forward) - pub view_back_stack: &'a [String], - pub view_forward_stack: &'a [String], + pub view_back_stack: &'a [crate::ui::app::ViewFrame], + pub view_forward_stack: &'a [crate::ui::app::ViewFrame], /// Whether the app currently has an active drill snapshot. pub has_drill_state: bool, /// Display value at the cursor — works uniformly for pivot and records mode. diff --git a/src/command/cmd/grid.rs b/src/command/cmd/grid.rs index 74976e3..d0e2b8e 100644 --- a/src/command/cmd/grid.rs +++ b/src/command/cmd/grid.rs @@ -1,4 +1,5 @@ use crate::model::cell::CellValue; +use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; use crate::view::AxisEntry; @@ -67,7 +68,10 @@ mod tests { let m = two_cat_model(); let layout = make_layout(&m); let reg = make_registry(); - let fwd_stack = vec!["View 2".to_string()]; + let fwd_stack = vec![crate::ui::app::ViewFrame { + view_name: "View 2".to_string(), + mode: crate::ui::app::AppMode::Normal, + }]; let mut ctx = make_ctx(&m, &layout, ®); ctx.view_forward_stack = &fwd_stack; let cmd = ViewNavigate { forward: true }; @@ -84,7 +88,10 @@ mod tests { let m = two_cat_model(); let layout = make_layout(&m); let reg = make_registry(); - let back_stack = vec!["Default".to_string()]; + let back_stack = vec![crate::ui::app::ViewFrame { + view_name: "Default".to_string(), + mode: crate::ui::app::AppMode::Normal, + }]; let mut ctx = make_ctx(&m, &layout, ®); ctx.view_back_stack = &back_stack; let cmd = ViewNavigate { forward: false }; @@ -408,6 +415,8 @@ impl Cmd for ToggleRecordsMode { let mut effects: Vec> = Vec::new(); let records_name = "_Records".to_string(); + effects.push(Box::new(effect::SortData)); + // Create (or replace) a _Records view and switch to it effects.push(Box::new(effect::CreateView(records_name.clone()))); effects.push(Box::new(effect::SwitchView(records_name))); @@ -429,6 +438,7 @@ impl Cmd for ToggleRecordsMode { })); } } + effects.push(effect::change_mode(AppMode::RecordsNormal)); effects.push(effect::set_status("Records mode")); effects } diff --git a/src/command/cmd/mode.rs b/src/command/cmd/mode.rs index 6719f7b..4c900b0 100644 --- a/src/command/cmd/mode.rs +++ b/src/command/cmd/mode.rs @@ -197,13 +197,18 @@ impl Cmd for EnterEditMode { fn name(&self) -> &'static str { "enter-edit-mode" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { + let edit_mode = if ctx.mode.is_records() { + AppMode::records_editing() + } else { + AppMode::editing() + }; vec![ Box::new(effect::SetBuffer { name: "edit".to_string(), value: self.initial_value.clone(), }), - effect::change_mode(AppMode::editing()), + effect::change_mode(edit_mode), ] } } diff --git a/src/command/cmd/registry.rs b/src/command/cmd/registry.rs index d4d2fa4..bf726e0 100644 --- a/src/command/cmd/registry.rs +++ b/src/command/cmd/registry.rs @@ -320,6 +320,8 @@ pub fn default_registry() -> CmdRegistry { "command" => AppMode::command_mode(), "category-add" => AppMode::category_add(), "editing" => AppMode::editing(), + "records-normal" => AppMode::RecordsNormal, + "records-editing" => AppMode::records_editing(), "formula-edit" => AppMode::formula_edit(), "export-prompt" => AppMode::export_prompt(), other => return Err(format!("Unknown mode: {other}")), diff --git a/src/command/keymap.rs b/src/command/keymap.rs index d7bdac5..a0276e3 100644 --- a/src/command/keymap.rs +++ b/src/command/keymap.rs @@ -61,6 +61,8 @@ pub enum ModeKey { CommandMode, SearchMode, ImportWizard, + RecordsNormal, + RecordsEditing, } impl ModeKey { @@ -80,6 +82,8 @@ impl ModeKey { AppMode::ExportPrompt { .. } => Some(ModeKey::ExportPrompt), AppMode::CommandMode { .. } => Some(ModeKey::CommandMode), AppMode::ImportWizard => Some(ModeKey::ImportWizard), + AppMode::RecordsNormal => Some(ModeKey::RecordsNormal), + AppMode::RecordsEditing { .. } => Some(ModeKey::RecordsEditing), _ => None, } } @@ -450,14 +454,9 @@ impl KeymapSet { normal.bind(KeyCode::Char('z'), none, "toggle-group-under-cursor"); normal.bind(KeyCode::Char('H'), none, "hide-selected-row-item"); - // Drill into aggregated cell / view history / add row + // Drill into aggregated cell / view history normal.bind(KeyCode::Char('>'), none, "drill-into-cell"); normal.bind(KeyCode::Char('<'), none, "view-back"); - normal.bind_seq( - KeyCode::Char('o'), - none, - vec![("add-record-row", vec![]), ("enter-edit-at-cursor", vec![])], - ); // Records mode toggle and prune toggle normal.bind(KeyCode::Char('R'), none, "toggle-records-mode"); @@ -484,7 +483,17 @@ impl KeymapSet { z_map.bind(KeyCode::Char('Z'), none, "wq"); normal.bind_prefix(KeyCode::Char('Z'), none, Arc::new(z_map)); - set.insert(ModeKey::Normal, Arc::new(normal)); + let normal = Arc::new(normal); + set.insert(ModeKey::Normal, normal.clone()); + + // ── Records normal mode (inherits from normal) ──────────────────── + let mut rn = Keymap::with_parent(normal); + rn.bind_seq( + KeyCode::Char('o'), + none, + vec![("add-record-row", vec![]), ("enter-edit-at-cursor", vec![])], + ); + set.insert(ModeKey::RecordsNormal, Arc::new(rn)); // ── Help mode ──────────────────────────────────────────────────── let mut help = Keymap::new(); @@ -754,7 +763,20 @@ impl KeymapSet { ); ed.bind_args(KeyCode::Backspace, none, "pop-char", vec!["edit".into()]); ed.bind_any_char("append-char", vec!["edit".into()]); - set.insert(ModeKey::Editing, Arc::new(ed)); + let ed = Arc::new(ed); + set.insert(ModeKey::Editing, ed.clone()); + + // ── Records editing mode (inherits from editing) ────────────────── + let mut re = Keymap::with_parent(ed); + re.bind_seq( + KeyCode::Esc, + none, + vec![ + ("clear-buffer", vec!["edit".into()]), + ("enter-mode", vec!["records-normal".into()]), + ], + ); + set.insert(ModeKey::RecordsEditing, Arc::new(re)); // ── Formula edit ───────────────────────────────────────────────── let mut fe = Keymap::new(); @@ -1106,6 +1128,8 @@ mod tests { ModeKey::CommandMode, ModeKey::SearchMode, ModeKey::ImportWizard, + ModeKey::RecordsNormal, + ModeKey::RecordsEditing, ]; for mode in &expected_modes { assert!( diff --git a/src/draw.rs b/src/draw.rs index 3ec504c..a12dc03 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -132,12 +132,16 @@ fn mode_name(mode: &AppMode) -> &'static str { AppMode::CommandMode { .. } => "COMMAND", AppMode::Help => "HELP", AppMode::Quit => "QUIT", + AppMode::RecordsNormal => "RECORDS", + AppMode::RecordsEditing { .. } => "RECORDS INSERT", } } fn mode_style(mode: &AppMode) -> Style { match mode { - AppMode::Editing { .. } => Style::default().fg(Color::Black).bg(Color::Green), + AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => { + Style::default().fg(Color::Black).bg(Color::Green) + } AppMode::CommandMode { .. } => Style::default().fg(Color::Black).bg(Color::Yellow), AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta), _ => Style::default().fg(Color::Black).bg(Color::DarkGray), diff --git a/src/model/cell.rs b/src/model/cell.rs index c74cd39..063251c 100644 --- a/src/model/cell.rs +++ b/src/model/cell.rs @@ -1,3 +1,4 @@ +use indexmap::IndexMap; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; @@ -104,8 +105,9 @@ pub struct InternedKey(pub Vec<(Symbol, Symbol)>); /// to implement the `Serialize`-as-string requirement for JSON object keys. #[derive(Debug, Clone, Default)] pub struct DataStore { - /// Primary storage — interned keys for O(1) hash/compare. - cells: HashMap, + /// Primary storage — interned keys, insertion-ordered so records mode + /// can display rows in the order they were entered. + cells: IndexMap, /// String interner — all category/item names are interned here. pub symbols: SymbolTable, /// Secondary index: interned (category, item) → set of interned keys. @@ -160,6 +162,26 @@ impl DataStore { ) } + /// Sort cells by their CellKey for deterministic display order. + /// Call once on entry into records mode so existing data is ordered; + /// subsequent inserts append at the end. + pub fn sort_by_key(&mut self) { + let symbols = &self.symbols; + self.cells.sort_by(|a, _, b, _| { + let resolve = |k: &InternedKey| -> Vec<(String, String)> { + k.0.iter() + .map(|(c, i)| { + ( + symbols.resolve(*c).to_string(), + symbols.resolve(*i).to_string(), + ) + }) + .collect() + }; + resolve(a).cmp(&resolve(b)) + }); + } + pub fn set(&mut self, key: CellKey, value: CellValue) { let ikey = self.intern_key(&key); // Update index for each coordinate pair @@ -193,7 +215,7 @@ impl DataStore { let Some(ikey) = self.lookup_key(key) else { return; }; - if self.cells.remove(&ikey).is_some() { + if self.cells.shift_remove(&ikey).is_some() { for pair in &ikey.0 { if let Some(set) = self.index.get_mut(pair) { set.remove(&ikey); diff --git a/src/ui/app.rs b/src/ui/app.rs index 35ed666..a21593d 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -20,6 +20,13 @@ use crate::ui::grid::{ use crate::view::GridLayout; use crate::workbook::Workbook; +/// A saved view+mode pair for the navigation stack. +#[derive(Debug, Clone, PartialEq)] +pub struct ViewFrame { + pub view_name: String, + pub mode: AppMode, +} + /// Drill-down state: frozen record snapshot + pending edits that have not /// yet been applied to the model. #[derive(Debug, Clone, Default)] @@ -72,6 +79,12 @@ pub enum AppMode { }, Help, Quit, + /// Records-mode normal: inherits from Normal with records-specific bindings. + RecordsNormal, + /// Records-mode editing: inherits from Editing with boundary-aware Tab/Enter. + RecordsEditing { + minibuf: MinibufferConfig, + }, } impl AppMode { @@ -79,6 +92,7 @@ impl AppMode { pub fn minibuffer(&self) -> Option<&MinibufferConfig> { match self { Self::Editing { minibuf, .. } + | Self::RecordsEditing { minibuf, .. } | Self::FormulaEdit { minibuf, .. } | Self::CommandMode { minibuf, .. } | Self::CategoryAdd { minibuf, .. } @@ -88,6 +102,11 @@ impl AppMode { } } + /// True for any cell-editing mode (normal or records). + pub fn is_editing(&self) -> bool { + matches!(self, Self::Editing { .. } | Self::RecordsEditing { .. }) + } + pub fn editing() -> Self { Self::Editing { minibuf: MinibufferConfig { @@ -98,6 +117,21 @@ impl AppMode { } } + pub fn records_editing() -> Self { + Self::RecordsEditing { + minibuf: MinibufferConfig { + buffer_key: "edit", + prompt: "edit: ".into(), + color: Color::Green, + }, + } + } + + /// True when this mode is a records-mode variant. + pub fn is_records(&self) -> bool { + matches!(self, Self::RecordsNormal | Self::RecordsEditing { .. }) + } + pub fn formula_edit() -> Self { Self::FormulaEdit { minibuf: MinibufferConfig { @@ -173,9 +207,9 @@ pub struct App { pub tile_cat_idx: usize, /// View navigation history: views visited before the current one. /// Pushed on SwitchView, popped by `<` (back). - pub view_back_stack: Vec, + pub view_back_stack: Vec, /// Views that were "back-ed" from, available for forward navigation (`>`). - pub view_forward_stack: Vec, + pub view_forward_stack: Vec, /// Frozen records list for the drill view. When present, this is the /// snapshot that records-mode layouts iterate — records don't disappear /// when filters would change. Pending edits are stored alongside and @@ -389,7 +423,12 @@ impl App { AppMode::Normal => { "hjkl:nav i:edit R:records P:prune F/C/V:panels T:tiles [:]:page >:drill ::cmd" } - AppMode::Editing { .. } => "Enter:commit Tab:commit+right Esc:cancel", + AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => { + "Enter:commit Tab:commit+right Esc:cancel" + } + AppMode::RecordsNormal => { + "hjkl:nav i:edit o:add-row R:pivot P:prune <:back ::cmd" + } AppMode::FormulaPanel => "n:new d:delete jk:nav Esc:back", AppMode::FormulaEdit { .. } => "Enter:save Esc:cancel — type: Name = expression", AppMode::CategoryPanel => { @@ -710,6 +749,43 @@ mod tests { } /// Regression: pressing `o` in an empty records view should create the + /// Pressing R to enter records mode should sort existing data by CellKey + /// so display order is deterministic regardless of insertion order. + #[test] + fn entering_records_mode_sorts_existing_data() { + use crate::model::cell::{CellKey, CellValue}; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.model.category_mut("Region").unwrap().add_item("North"); + wb.model.category_mut("Region").unwrap().add_item("East"); + // Insert in reverse-alphabetical order + wb.model.set_cell( + CellKey::new(vec![("Region".into(), "North".into())]), + CellValue::Number(1.0), + ); + wb.model.set_cell( + CellKey::new(vec![("Region".into(), "East".into())]), + CellValue::Number(2.0), + ); + let mut app = App::new(wb, None); + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.layout.is_records_mode()); + let region_col = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Region") + .unwrap(); + let row0 = app.layout.records_display(0, region_col).unwrap(); + let row1 = app.layout.records_display(1, region_col).unwrap(); + assert_eq!( + row0, "East", + "R should sort existing data: first row should be East" + ); + assert_eq!( + row1, "North", + "R should sort existing data: second row should be North" + ); + } + /// first synthetic row instead of only entering edit mode on empty space. #[test] fn add_record_row_in_empty_records_view_creates_first_row() { @@ -732,7 +808,7 @@ mod tests { "o should create the first record row in an empty records view" ); assert!( - matches!(app.mode, AppMode::Editing { .. }), + app.mode.is_editing(), "o should leave the app in edit mode, got {:?}", app.mode ); @@ -771,14 +847,107 @@ mod tests { .unwrap(); assert_eq!( - app.workbook.model.get_cell(&CellKey::new(vec![ - ("_Measure".to_string(), "Rev".to_string()), - ])), + app.workbook.model.get_cell(&CellKey::new(vec![( + "_Measure".to_string(), + "Rev".to_string(), + )])), Some(&crate::model::cell::CellValue::Number(5.0)), "editing a synthetic row in plain records mode should write the value" ); } + /// Build a records-mode app with two data rows for testing Tab/Enter + /// behavior at boundaries. Row 0 has _Measure=meas2, row 1 has _Measure=meas1. + fn records_model_with_two_rows() -> App { + use crate::model::cell::{CellKey, CellValue}; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.model.category_mut("Region").unwrap().add_item("North"); + wb.model.category_mut("_Measure").unwrap().add_item("meas1"); + wb.model.category_mut("_Measure").unwrap().add_item("meas2"); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "meas2".into()), + ]), + CellValue::Number(10.0), + ); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "meas1".into()), + ]), + CellValue::Number(20.0), + ); + let mut app = App::new(wb, None); + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + assert!( + app.layout.is_records_mode(), + "setup: should be records mode" + ); + assert_eq!(app.layout.row_count(), 2, "setup: should have 2 records"); + let cols: Vec = (0..app.layout.col_count()) + .map(|i| app.layout.col_label(i)) + .collect(); + assert!( + cols.contains(&"Region".to_string()), + "setup: should have Region column; got {:?}", + cols + ); + assert!( + cols.contains(&"_Measure".to_string()), + "setup: should have _Measure column; got {:?}", + cols + ); + assert_eq!( + cols.last().unwrap(), + "Value", + "setup: Value must be last column; got {:?}", + cols + ); + app + } + + /// improvise-hmu: TAB on the bottom-right cell of records view should + /// insert a new record below and move to the first cell of the new row + /// in edit mode. + #[test] + fn tab_on_bottom_right_of_records_inserts_below() { + let mut app = records_model_with_two_rows(); + let initial_rows = app.layout.row_count(); + assert!(initial_rows >= 1, "setup: need at least 1 record"); + + let last_row = initial_rows - 1; + let last_col = app.layout.col_count() - 1; + app.workbook.active_view_mut().selected = (last_row, last_col); + + // Enter edit mode on the bottom-right cell + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.mode.is_editing(), "setup: should be editing"); + + // TAB should commit, insert below, move to first cell of new row + app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + .unwrap(); + + assert_eq!( + app.layout.row_count(), + initial_rows + 1, + "TAB on bottom-right should insert a record below" + ); + assert_eq!( + app.workbook.active_view().selected, + (initial_rows, 0), + "TAB should move to first cell of the new row" + ); + assert!( + app.mode.is_editing(), + "should enter edit mode on the new cell, got {:?}", + app.mode + ); + } + /// Drill-view edits should stay staged in drill state until the user /// navigates back, at which point ApplyAndClearDrill writes them through. #[test] diff --git a/src/ui/effect.rs b/src/ui/effect.rs index ca60fb1..eb40c9d 100644 --- a/src/ui/effect.rs +++ b/src/ui/effect.rs @@ -4,7 +4,7 @@ use std::path::PathBuf; use crate::model::cell::{CellKey, CellValue}; use crate::view::Axis; -use super::app::{App, AppMode}; +use super::app::{App, AppMode, ViewFrame}; pub(crate) const RECORD_COORDS_CANNOT_BE_EMPTY: &str = "Record coordinates cannot be empty"; @@ -59,6 +59,14 @@ impl Effect for AddItemInGroup { } } +#[derive(Debug)] +pub struct SortData; +impl Effect for SortData { + fn apply(&self, app: &mut App) { + app.workbook.model.data.sort_by_key(); + } +} + #[derive(Debug)] pub struct SetCell(pub CellKey, pub CellValue); impl Effect for SetCell { @@ -128,7 +136,11 @@ impl Effect for EnterEditAtCursor { let value = ctx.display_value.clone(); drop(ctx); app.buffers.insert("edit".to_string(), value); - app.mode = AppMode::editing(); + app.mode = if app.mode.is_records() { + AppMode::records_editing() + } else { + AppMode::editing() + }; } } @@ -194,7 +206,10 @@ impl Effect for SwitchView { fn apply(&self, app: &mut App) { let current = app.workbook.active_view.clone(); if current != self.0 { - app.view_back_stack.push(current); + app.view_back_stack.push(ViewFrame { + view_name: current, + mode: app.mode.clone(), + }); app.view_forward_stack.clear(); } let _ = app.workbook.switch_view(&self.0); @@ -206,10 +221,14 @@ impl Effect for SwitchView { pub struct ViewBack; impl Effect for ViewBack { fn apply(&self, app: &mut App) { - if let Some(prev) = app.view_back_stack.pop() { + if let Some(frame) = app.view_back_stack.pop() { let current = app.workbook.active_view.clone(); - app.view_forward_stack.push(current); - let _ = app.workbook.switch_view(&prev); + app.view_forward_stack.push(ViewFrame { + view_name: current, + mode: app.mode.clone(), + }); + let _ = app.workbook.switch_view(&frame.view_name); + app.mode = frame.mode; } } } @@ -219,10 +238,14 @@ impl Effect for ViewBack { pub struct ViewForward; impl Effect for ViewForward { fn apply(&self, app: &mut App) { - if let Some(next) = app.view_forward_stack.pop() { + if let Some(frame) = app.view_forward_stack.pop() { let current = app.workbook.active_view.clone(); - app.view_back_stack.push(current); - let _ = app.workbook.switch_view(&next); + app.view_back_stack.push(ViewFrame { + view_name: current, + mode: app.mode.clone(), + }); + let _ = app.workbook.switch_view(&frame.view_name); + app.mode = frame.mode; } } } @@ -1137,7 +1160,8 @@ mod tests { SwitchView("View 2".to_string()).apply(&mut app); assert_eq!(app.workbook.active_view.as_str(), "View 2"); - assert_eq!(app.view_back_stack, vec!["Default".to_string()]); + assert_eq!(app.view_back_stack.len(), 1); + assert_eq!(app.view_back_stack[0].view_name, "Default"); // Forward stack should be cleared assert!(app.view_forward_stack.is_empty()); } @@ -1159,13 +1183,15 @@ mod tests { // Go back ViewBack.apply(&mut app); assert_eq!(app.workbook.active_view.as_str(), "Default"); - assert_eq!(app.view_forward_stack, vec!["View 2".to_string()]); + assert_eq!(app.view_forward_stack.len(), 1); + assert_eq!(app.view_forward_stack[0].view_name, "View 2"); assert!(app.view_back_stack.is_empty()); // Go forward ViewForward.apply(&mut app); assert_eq!(app.workbook.active_view.as_str(), "View 2"); - assert_eq!(app.view_back_stack, vec!["Default".to_string()]); + assert_eq!(app.view_back_stack.len(), 1); + assert_eq!(app.view_back_stack[0].view_name, "Default"); assert!(app.view_forward_stack.is_empty()); } diff --git a/src/ui/grid.rs b/src/ui/grid.rs index 04a7485..a657f98 100644 --- a/src/ui/grid.rs +++ b/src/ui/grid.rs @@ -429,7 +429,7 @@ impl<'a> GridWidget<'a> { } // Edit indicator - if matches!(self.mode, AppMode::Editing { .. }) && ri == sel_row { + if self.mode.is_editing() && ri == sel_row { { let buffer = self.buffers.get("edit").map(|s| s.as_str()).unwrap_or(""); let edit_x = col_x_at(sel_col); diff --git a/src/view/layout.rs b/src/view/layout.rs index 9ed6be4..4b3fe1c 100644 --- a/src/view/layout.rs +++ b/src/view/layout.rs @@ -132,14 +132,12 @@ impl GridLayout { page_coords: Vec<(String, String)>, none_cats: Vec, ) -> Self { - let mut records: Vec<(CellKey, CellValue)> = model + let records: Vec<(CellKey, CellValue)> = model .data .matching_cells(&page_coords) .into_iter() .map(|(k, v)| (k, v.clone())) .collect(); - // Sort for deterministic ordering - records.sort_by(|a, b| a.0.0.cmp(&b.0.0)); // Synthesize row items: one per record, labeled with its index let row_items: Vec = (0..records.len()) @@ -152,7 +150,10 @@ impl GridLayout { .into_iter() .filter(|c| { let kind = model.category(c).map(|cat| cat.kind); - !matches!(kind, Some(CategoryKind::VirtualIndex | CategoryKind::VirtualDim)) + !matches!( + kind, + Some(CategoryKind::VirtualIndex | CategoryKind::VirtualDim) + ) }) .map(String::from) .collect(); @@ -732,6 +733,79 @@ mod tests { ); } + /// On initial entry into records mode, rows should be in a deterministic + /// (CellKey-sorted) order regardless of insertion order. + #[test] + fn records_mode_initial_layout_is_sorted() { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.model.category_mut("Region").unwrap().add_item("North"); + wb.model.category_mut("Region").unwrap().add_item("East"); + // Insert East before North — opposite of alphabetical + wb.model.set_cell( + CellKey::new(vec![("Region".into(), "East".into())]), + CellValue::Number(2.0), + ); + wb.model.set_cell( + CellKey::new(vec![("Region".into(), "North".into())]), + CellValue::Number(1.0), + ); + // Sort the store before entering records mode, as ToggleRecordsMode would + wb.model.data.sort_by_key(); + let v = wb.active_view_mut(); + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + let layout = GridLayout::new(&wb.model, wb.active_view()); + assert_eq!(layout.row_count(), 2); + let region_col = (0..layout.col_count()) + .find(|&c| layout.col_label(c) == "Region") + .unwrap(); + let row0_region = layout.records_display(0, region_col).unwrap(); + let row1_region = layout.records_display(1, region_col).unwrap(); + assert_eq!( + row0_region, "East", + "first row should be East (alphabetical)" + ); + assert_eq!( + row1_region, "North", + "second row should be North (alphabetical)" + ); + } + + /// New records added after initial layout should appear at the bottom, + /// not re-sorted into the middle by CellKey order. + #[test] + fn records_mode_new_record_appends_at_bottom() { + let mut wb = records_workbook(); + let v = wb.active_view_mut(); + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + let layout = GridLayout::new(&wb.model, wb.active_view()); + assert_eq!(layout.row_count(), 2); + let first_value = layout.records_display(0, layout.col_count() - 1).unwrap(); + + // Add a record whose CellKey sorts BEFORE the existing ones + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "AAA".into()), + ("_Measure".into(), "Cost".into()), + ]), + CellValue::Number(999.0), + ); + let layout2 = GridLayout::new(&wb.model, wb.active_view()); + assert_eq!(layout2.row_count(), 3); + // First row should still be the same record — new one appends at bottom + let first_value2 = layout2.records_display(0, layout2.col_count() - 1).unwrap(); + assert_eq!( + first_value, first_value2, + "first row should be unchanged after adding a new record; \ + new record should append at bottom, not re-sort" + ); + // New record should be the last row + let last_value = layout2.records_display(2, layout2.col_count() - 1).unwrap(); + assert_eq!(last_value, "999", "new record should be the last row"); + } + fn coord(pairs: &[(&str, &str)]) -> CellKey { CellKey::new( pairs