Merge branch 'main' into worktree-improvise-ewi-formula-crate

# Conflicts:
#	src/ui/app.rs
#	src/ui/effect.rs
#	src/view/layout.rs
This commit is contained in:
Edward Langley
2026-04-15 21:39:00 -07:00
13 changed files with 562 additions and 199 deletions

View File

@ -385,13 +385,13 @@ mod tests {
#[test] #[test]
fn parse_sum_with_inline_where_filter() { fn parse_sum_with_inline_where_filter() {
let f = parse_formula("EastTotal = SUM(Revenue WHERE Region = \"East\")", "Foo").unwrap(); 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!(
assert!(matches!(**inner, Expr::Ref(_))); &f.expr,
assert_eq!(filter.category, "Region"); Expr::Agg(AggFunc::Sum, inner, Some(filter))
assert_eq!(filter.item, "East"); if matches!(**inner, Expr::Ref(_))
} else { && filter.category == "Region"
panic!("Expected SUM with inline WHERE filter, got: {:?}", f.expr); && filter.item == "East"
} ));
} }
// ── Comparison operators ──────────────────────────────────────────── // ── Comparison operators ────────────────────────────────────────────
@ -467,21 +467,20 @@ mod tests {
fn parse_aggregate_name_without_parens_is_ref() { fn parse_aggregate_name_without_parens_is_ref() {
// "SUM" without parens should be treated as a reference, not a function // "SUM" without parens should be treated as a reference, not a function
let f = parse_formula("X = SUM + 1", "Cat").unwrap(); let f = parse_formula("X = SUM + 1", "Cat").unwrap();
assert!(matches!(f.expr, Expr::BinOp(BinOp::Add, _, _))); assert!(matches!(
if let Expr::BinOp(_, lhs, _) = &f.expr { &f.expr,
assert!(matches!(**lhs, Expr::Ref(_))); Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_))
} ));
} }
#[test] #[test]
fn parse_if_without_parens_is_ref() { fn parse_if_without_parens_is_ref() {
// "IF" without parens should be treated as a reference // "IF" without parens should be treated as a reference
let f = parse_formula("X = IF + 1", "Cat").unwrap(); let f = parse_formula("X = IF + 1", "Cat").unwrap();
if let Expr::BinOp(BinOp::Add, lhs, _) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(_))); &f.expr,
} else { Expr::BinOp(BinOp::Add, lhs, _) if matches!(**lhs, Expr::Ref(_))
panic!("Expected BinOp(Add), got: {:?}", f.expr); ));
}
} }
// ── Quoted string in WHERE ────────────────────────────────────────── // ── Quoted string in WHERE ──────────────────────────────────────────
@ -540,46 +539,46 @@ mod tests {
fn pipe_quoted_identifier_in_expression() { fn pipe_quoted_identifier_in_expression() {
let f = parse_formula("|Total Revenue| = |Base Revenue| + Bonus", "Foo").unwrap(); let f = parse_formula("|Total Revenue| = |Base Revenue| + Bonus", "Foo").unwrap();
assert_eq!(f.target, "|Total Revenue|"); assert_eq!(f.target, "|Total Revenue|");
if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue")); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Bonus")); Expr::BinOp(BinOp::Add, lhs, rhs)
} else { if matches!(**lhs, Expr::Ref(ref s) if s == "Base Revenue")
panic!("Expected Add, got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "Bonus")
} ));
} }
#[test] #[test]
fn pipe_quoted_keyword_as_identifier() { fn pipe_quoted_keyword_as_identifier() {
// A category named "WHERE" can be referenced with pipes // A category named "WHERE" can be referenced with pipes
let f = parse_formula("X = |WHERE| + |SUM|", "Cat").unwrap(); let f = parse_formula("X = |WHERE| + |SUM|", "Cat").unwrap();
if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(ref s) if s == "WHERE")); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "SUM")); Expr::BinOp(BinOp::Add, lhs, rhs)
} else { if matches!(**lhs, Expr::Ref(ref s) if s == "WHERE")
panic!("Expected Add, got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "SUM")
} ));
} }
#[test] #[test]
fn pipe_quoted_identifier_with_special_chars() { fn pipe_quoted_identifier_with_special_chars() {
// Pipes allow characters that would normally break tokenization // Pipes allow characters that would normally break tokenization
let f = parse_formula("X = |Revenue (USD)| + |Cost + Tax|", "Cat").unwrap(); let f = parse_formula("X = |Revenue (USD)| + |Cost + Tax|", "Cat").unwrap();
if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)")); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax")); Expr::BinOp(BinOp::Add, lhs, rhs)
} else { if matches!(**lhs, Expr::Ref(ref s) if s == "Revenue (USD)")
panic!("Expected Add, got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "Cost + Tax")
} ));
} }
#[test] #[test]
fn pipe_quoted_in_aggregate() { fn pipe_quoted_in_aggregate() {
let f = parse_formula("X = SUM(|Net Revenue|)", "Cat").unwrap(); let f = parse_formula("X = SUM(|Net Revenue|)", "Cat").unwrap();
if let Expr::Agg(AggFunc::Sum, inner, None) = &f.expr { assert!(matches!(
assert!(matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue")); &f.expr,
} else { Expr::Agg(AggFunc::Sum, inner, None)
panic!("Expected SUM aggregate, got: {:?}", f.expr); if matches!(**inner, Expr::Ref(ref s) if s == "Net Revenue")
} ));
} }
#[test] #[test]
@ -593,12 +592,11 @@ mod tests {
fn pipe_quoted_in_inline_where() { fn pipe_quoted_in_inline_where() {
let f = let f =
parse_formula("X = SUM(Revenue WHERE |Region Name| = |East Coast|)", "Foo").unwrap(); parse_formula("X = SUM(Revenue WHERE |Region Name| = |East Coast|)", "Foo").unwrap();
if let Expr::Agg(AggFunc::Sum, _, Some(filter)) = &f.expr { assert!(matches!(
assert_eq!(filter.category, "Region Name"); &f.expr,
assert_eq!(filter.item, "East Coast"); Expr::Agg(AggFunc::Sum, _, Some(filter))
} else { if filter.category == "Region Name" && filter.item == "East Coast"
panic!("Expected SUM with WHERE filter, got: {:?}", f.expr); ));
}
} }
// ── Pipe-quoted escape semantics ──────────────────────────────────── // ── Pipe-quoted escape semantics ────────────────────────────────────
@ -607,33 +605,21 @@ mod tests {
fn pipe_quoted_escape_literal_pipe() { fn pipe_quoted_escape_literal_pipe() {
// \| inside a pipe-quoted identifier is a literal pipe // \| inside a pipe-quoted identifier is a literal pipe
let f = parse_formula("X = |A\\|B|", "Cat").unwrap(); let f = parse_formula("X = |A\\|B|", "Cat").unwrap();
if let Expr::Ref(ref s) = f.expr { assert!(matches!(&f.expr, Expr::Ref(s) if s == "A|B"));
assert_eq!(s, "A|B");
} else {
panic!("Expected Ref, got: {:?}", f.expr);
}
} }
#[test] #[test]
fn pipe_quoted_escape_double_backslash() { fn pipe_quoted_escape_double_backslash() {
// \\ inside a pipe-quoted identifier is a literal backslash // \\ inside a pipe-quoted identifier is a literal backslash
let f = parse_formula("X = |A\\\\B|", "Cat").unwrap(); let f = parse_formula("X = |A\\\\B|", "Cat").unwrap();
if let Expr::Ref(ref s) = f.expr { assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\B"));
assert_eq!(s, "A\\B");
} else {
panic!("Expected Ref, got: {:?}", f.expr);
}
} }
#[test] #[test]
fn pipe_quoted_escape_newline() { fn pipe_quoted_escape_newline() {
// \n inside a pipe-quoted identifier is a literal newline // \n inside a pipe-quoted identifier is a literal newline
let f = parse_formula("X = |A\\nB|", "Cat").unwrap(); let f = parse_formula("X = |A\\nB|", "Cat").unwrap();
if let Expr::Ref(ref s) = f.expr { assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\nB"));
assert_eq!(s, "A\nB");
} else {
panic!("Expected Ref, got: {:?}", f.expr);
}
} }
#[test] #[test]
@ -642,11 +628,7 @@ mod tests {
// backslash-plus-character. The grammar's `"\\" ~ ANY` allows // backslash-plus-character. The grammar's `"\\" ~ ANY` allows
// any following character; we just don't interpret it. // any following character; we just don't interpret it.
let f = parse_formula("X = |A\\zB|", "Cat").unwrap(); let f = parse_formula("X = |A\\zB|", "Cat").unwrap();
if let Expr::Ref(ref s) = f.expr { assert!(matches!(&f.expr, Expr::Ref(s) if s == "A\\zB"));
assert_eq!(s, "A\\zB");
} else {
panic!("Expected Ref, got: {:?}", f.expr);
}
} }
// ── Operator precedence and associativity ────────────────────────── // ── Operator precedence and associativity ──────────────────────────
@ -655,66 +637,62 @@ mod tests {
fn mul_binds_tighter_than_add() { fn mul_binds_tighter_than_add() {
// `A + B * C` must parse as Add(A, Mul(B, C)) // `A + B * C` must parse as Add(A, Mul(B, C))
let f = parse_formula("X = A + B * C", "Cat").unwrap(); let f = parse_formula("X = A + B * C", "Cat").unwrap();
if let Expr::BinOp(BinOp::Add, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(_))); &f.expr,
assert!(matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _))); Expr::BinOp(BinOp::Add, lhs, rhs)
} else { if matches!(**lhs, Expr::Ref(_))
panic!("Expected Add with Mul on rhs, got: {:?}", f.expr); && matches!(**rhs, Expr::BinOp(BinOp::Mul, _, _))
} ));
} }
#[test] #[test]
fn pow_binds_tighter_than_mul() { fn pow_binds_tighter_than_mul() {
// `A * B ^ C` must parse as Mul(A, Pow(B, C)) // `A * B ^ C` must parse as Mul(A, Pow(B, C))
let f = parse_formula("X = A * B ^ C", "Cat").unwrap(); let f = parse_formula("X = A * B ^ C", "Cat").unwrap();
if let Expr::BinOp(BinOp::Mul, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::Ref(_))); &f.expr,
assert!(matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _))); Expr::BinOp(BinOp::Mul, lhs, rhs)
} else { if matches!(**lhs, Expr::Ref(_))
panic!("Expected Mul with Pow on rhs, got: {:?}", f.expr); && matches!(**rhs, Expr::BinOp(BinOp::Pow, _, _))
} ));
} }
#[test] #[test]
fn subtraction_is_left_associative() { fn subtraction_is_left_associative() {
// `A - B - C` must parse as Sub(Sub(A, B), C), not Sub(A, Sub(B, C)) // `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(); let f = parse_formula("X = A - B - C", "Cat").unwrap();
if let Expr::BinOp(BinOp::Sub, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _))); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C")); Expr::BinOp(BinOp::Sub, lhs, rhs)
} else { if matches!(**lhs, Expr::BinOp(BinOp::Sub, _, _))
panic!("Expected left-associative Sub, got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "C")
} ));
} }
#[test] #[test]
fn division_is_left_associative() { fn division_is_left_associative() {
// `A / B / C` must parse as Div(Div(A, B), C) // `A / B / C` must parse as Div(Div(A, B), C)
let f = parse_formula("X = A / B / C", "Cat").unwrap(); let f = parse_formula("X = A / B / C", "Cat").unwrap();
if let Expr::BinOp(BinOp::Div, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::BinOp(BinOp::Div, _, _))); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "C")); Expr::BinOp(BinOp::Div, lhs, rhs)
} else { if matches!(**lhs, Expr::BinOp(BinOp::Div, _, _))
panic!("Expected left-associative Div, got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "C")
} ));
} }
#[test] #[test]
fn unary_minus_before_pow() { fn unary_minus_before_pow() {
// `-A ^ B` — hand-rolled parser parses this as UnaryMinus(Ref(A)) // `-A ^ B` must parse as Pow(UnaryMinus(A), B). The `-` binds
// followed by nothing because pow is inside unary's recursion. // through `unary_minus = { "-" ~ primary }`, producing a unary
// My grammar: unary = unary_minus | primary, unary_minus = "-" ~ primary. // node that is then used as the pow_expr's base.
// 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).
let f = parse_formula("X = -A ^ B", "Cat").unwrap(); let f = parse_formula("X = -A ^ B", "Cat").unwrap();
if let Expr::BinOp(BinOp::Pow, lhs, rhs) = &f.expr { assert!(matches!(
assert!(matches!(**lhs, Expr::UnaryMinus(_))); &f.expr,
assert!(matches!(**rhs, Expr::Ref(ref s) if s == "B")); Expr::BinOp(BinOp::Pow, lhs, rhs)
} else { if matches!(**lhs, Expr::UnaryMinus(_))
panic!("Expected Pow(UnaryMinus, Ref), got: {:?}", f.expr); && matches!(**rhs, Expr::Ref(ref s) if s == "B")
} ));
} }
// ── Number literal variants ──────────────────────────────────────── // ── Number literal variants ────────────────────────────────────────
@ -767,11 +745,11 @@ mod tests {
fn nested_sum_aggregate() { fn nested_sum_aggregate() {
// Nested aggregates — outer SUM wrapping an inner SUM. // Nested aggregates — outer SUM wrapping an inner SUM.
let f = parse_formula("X = SUM(SUM(Revenue))", "Cat").unwrap(); let f = parse_formula("X = SUM(SUM(Revenue))", "Cat").unwrap();
if let Expr::Agg(AggFunc::Sum, outer_inner, None) = &f.expr { assert!(matches!(
assert!(matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None))); &f.expr,
} else { Expr::Agg(AggFunc::Sum, outer_inner, None)
panic!("Expected nested SUM aggregates, got: {:?}", f.expr); if matches!(**outer_inner, Expr::Agg(AggFunc::Sum, _, None))
} ));
} }
#[test] #[test]
@ -785,12 +763,12 @@ mod tests {
fn nested_if_expression() { fn nested_if_expression() {
// IF in the then-branch of another IF. // IF in the then-branch of another IF.
let f = parse_formula("X = IF(A > B, IF(C > D, 1, 2), 3)", "Cat").unwrap(); 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!(
assert!(matches!(**then_e, Expr::If(_, _, _))); &f.expr,
assert!(matches!(**else_e, Expr::Number(n) if n == 3.0)); Expr::If(_, then_e, else_e)
} else { if matches!(**then_e, Expr::If(_, _, _))
panic!("Expected outer IF, got: {:?}", f.expr); && matches!(**else_e, Expr::Number(n) if n == 3.0)
} ));
} }
// ── Whitespace tolerance ─────────────────────────────────────────── // ── Whitespace tolerance ───────────────────────────────────────────
@ -845,6 +823,34 @@ mod tests {
assert_eq!(f.target, "my_target-name"); 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] #[test]
fn pipe_quoted_target_preserves_pipes() { fn pipe_quoted_target_preserves_pipes() {
let f = parse_formula("|My Target| = A", "Cat").unwrap(); let f = parse_formula("|My Target| = A", "Cat").unwrap();
@ -907,13 +913,6 @@ mod generator {
match expr { match expr {
Expr::Str(s) => out.push_str(s), Expr::Str(s) => out.push_str(s),
Expr::Insens(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::Ident(name) => self.emit_ident(name, out, atomic),
Expr::Seq(a, b) => { Expr::Seq(a, b) => {
self.emit(a, out, atomic); self.emit(a, out, atomic);
@ -923,18 +922,19 @@ mod generator {
self.emit(b, out, atomic); self.emit(b, out, atomic);
} }
Expr::Choice(a, b) => { Expr::Choice(a, b) => {
let mut alts: Vec<&Expr> = vec![a.as_ref()]; // 50/50 between the two branches of each Choice.
let mut cur = b.as_ref(); // Nested Choices (used by rules with 3+ alternatives)
while let Expr::Choice(l, r) = cur { // recurse into themselves, so deeper branches are
alts.push(l.as_ref()); // chosen less often than shallower ones — good
cur = r.as_ref(); // 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) => { Expr::Opt(inner) => {
// ~66% chance of emitting // ~66% chance of emitting the optional branch.
if !self.pick().is_multiple_of(3) { if !self.pick().is_multiple_of(3) {
self.emit(inner, out, atomic); self.emit(inner, out, atomic);
} }
@ -949,20 +949,20 @@ mod generator {
} }
} }
Expr::RepOnce(inner) => { 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; let count = 1 + self.pick() % 3;
for i in 0..count { for _ in 0..count {
if i > 0 && !atomic {
out.push(' ');
}
self.emit(inner, out, atomic); self.emit(inner, out, atomic);
} }
} }
Expr::NegPred(_) | Expr::PosPred(_) => { // Lookaheads (NegPred, PosPred) don't emit output. Any
// Lookaheads don't emit output. // other Expr variant (Range, RepExact, Push, PeekSlice,
} // Skip, …) is unused by formula.pest and silently
_ => { // produces nothing; if the grammar starts using one,
// Skip unsupported expressions. // generated formulas will fail to parse and we'll know.
} _ => {}
} }
} }
@ -984,21 +984,31 @@ mod generator {
"NEWLINE" => out.push('\n'), "NEWLINE" => out.push('\n'),
"SOI" | "EOI" => {} "SOI" | "EOI" => {}
_ => { _ => {
if let Some((ty, inner)) = self.rules.get(name).cloned() { // Every Ident in formula.pest refers to a rule that
let inner_atomic = atomic // exists in the grammar file — if it doesn't, the
|| matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); // 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); self.emit(&inner, out, inner_atomic);
} }
} }
} }
}
pub fn generate(&mut self, rule_name: &str) -> String { pub fn generate(&mut self, rule_name: &str) -> String {
let mut out = String::new(); let mut out = String::new();
if let Some((ty, expr)) = self.rules.get(rule_name).cloned() { let (ty, expr) = self
.rules
.get(rule_name)
.cloned()
.expect("entry rule exists in formula.pest");
let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic); let atomic = matches!(ty, RuleType::Atomic | RuleType::CompoundAtomic);
self.emit(&expr, &mut out, atomic); self.emit(&expr, &mut out, atomic);
}
out out
} }
} }

View File

@ -3,6 +3,7 @@ use crate::ui::app::AppMode;
use crate::ui::effect::{self, Effect}; use crate::ui::effect::{self, Effect};
use super::core::{Cmd, CmdContext}; use super::core::{Cmd, CmdContext};
use super::grid::AddRecordRow;
use super::navigation::{CursorState, EnterAdvance, viewport_effects}; use super::navigation::{CursorState, EnterAdvance, viewport_effects};
#[cfg(test)] #[cfg(test)]
@ -259,6 +260,21 @@ impl Cmd for CommitAndAdvance {
} }
AdvanceDir::Right => { AdvanceDir::Right => {
let col_max = self.cursor.col_count.saturating_sub(1); let col_max = self.cursor.col_count.saturating_sub(1);
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); let nc = (self.cursor.col + 1).min(col_max);
effects.extend(viewport_effects( effects.extend(viewport_effects(
self.cursor.row, self.cursor.row,
@ -270,6 +286,7 @@ impl Cmd for CommitAndAdvance {
)); ));
} }
} }
}
effects.push(Box::new(effect::EnterEditAtCursor)); effects.push(Box::new(effect::EnterEditAtCursor));
effects effects
} }

View File

@ -41,8 +41,8 @@ pub struct CmdContext<'a> {
/// Named text buffers /// Named text buffers
pub buffers: &'a HashMap<String, String>, pub buffers: &'a HashMap<String, String>,
/// View navigation stacks (for drill back/forward) /// View navigation stacks (for drill back/forward)
pub view_back_stack: &'a [String], pub view_back_stack: &'a [crate::ui::app::ViewFrame],
pub view_forward_stack: &'a [String], pub view_forward_stack: &'a [crate::ui::app::ViewFrame],
/// Whether the app currently has an active drill snapshot. /// Whether the app currently has an active drill snapshot.
pub has_drill_state: bool, pub has_drill_state: bool,
/// Display value at the cursor — works uniformly for pivot and records mode. /// Display value at the cursor — works uniformly for pivot and records mode.

View File

@ -1,4 +1,5 @@
use crate::model::cell::CellValue; use crate::model::cell::CellValue;
use crate::ui::app::AppMode;
use crate::ui::effect::{self, Effect}; use crate::ui::effect::{self, Effect};
use crate::view::AxisEntry; use crate::view::AxisEntry;
@ -67,7 +68,10 @@ mod tests {
let m = two_cat_model(); let m = two_cat_model();
let layout = make_layout(&m); let layout = make_layout(&m);
let reg = make_registry(); 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, &reg); let mut ctx = make_ctx(&m, &layout, &reg);
ctx.view_forward_stack = &fwd_stack; ctx.view_forward_stack = &fwd_stack;
let cmd = ViewNavigate { forward: true }; let cmd = ViewNavigate { forward: true };
@ -84,7 +88,10 @@ mod tests {
let m = two_cat_model(); let m = two_cat_model();
let layout = make_layout(&m); let layout = make_layout(&m);
let reg = make_registry(); 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, &reg); let mut ctx = make_ctx(&m, &layout, &reg);
ctx.view_back_stack = &back_stack; ctx.view_back_stack = &back_stack;
let cmd = ViewNavigate { forward: false }; let cmd = ViewNavigate { forward: false };
@ -408,6 +415,8 @@ impl Cmd for ToggleRecordsMode {
let mut effects: Vec<Box<dyn Effect>> = Vec::new(); let mut effects: Vec<Box<dyn Effect>> = Vec::new();
let records_name = "_Records".to_string(); let records_name = "_Records".to_string();
effects.push(Box::new(effect::SortData));
// Create (or replace) a _Records view and switch to it // Create (or replace) a _Records view and switch to it
effects.push(Box::new(effect::CreateView(records_name.clone()))); effects.push(Box::new(effect::CreateView(records_name.clone())));
effects.push(Box::new(effect::SwitchView(records_name))); 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.push(effect::set_status("Records mode"));
effects effects
} }

View File

@ -197,13 +197,18 @@ impl Cmd for EnterEditMode {
fn name(&self) -> &'static str { fn name(&self) -> &'static str {
"enter-edit-mode" "enter-edit-mode"
} }
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> { fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let edit_mode = if ctx.mode.is_records() {
AppMode::records_editing()
} else {
AppMode::editing()
};
vec![ vec![
Box::new(effect::SetBuffer { Box::new(effect::SetBuffer {
name: "edit".to_string(), name: "edit".to_string(),
value: self.initial_value.clone(), value: self.initial_value.clone(),
}), }),
effect::change_mode(AppMode::editing()), effect::change_mode(edit_mode),
] ]
} }
} }

View File

@ -320,6 +320,8 @@ pub fn default_registry() -> CmdRegistry {
"command" => AppMode::command_mode(), "command" => AppMode::command_mode(),
"category-add" => AppMode::category_add(), "category-add" => AppMode::category_add(),
"editing" => AppMode::editing(), "editing" => AppMode::editing(),
"records-normal" => AppMode::RecordsNormal,
"records-editing" => AppMode::records_editing(),
"formula-edit" => AppMode::formula_edit(), "formula-edit" => AppMode::formula_edit(),
"export-prompt" => AppMode::export_prompt(), "export-prompt" => AppMode::export_prompt(),
other => return Err(format!("Unknown mode: {other}")), other => return Err(format!("Unknown mode: {other}")),

View File

@ -61,6 +61,8 @@ pub enum ModeKey {
CommandMode, CommandMode,
SearchMode, SearchMode,
ImportWizard, ImportWizard,
RecordsNormal,
RecordsEditing,
} }
impl ModeKey { impl ModeKey {
@ -80,6 +82,8 @@ impl ModeKey {
AppMode::ExportPrompt { .. } => Some(ModeKey::ExportPrompt), AppMode::ExportPrompt { .. } => Some(ModeKey::ExportPrompt),
AppMode::CommandMode { .. } => Some(ModeKey::CommandMode), AppMode::CommandMode { .. } => Some(ModeKey::CommandMode),
AppMode::ImportWizard => Some(ModeKey::ImportWizard), AppMode::ImportWizard => Some(ModeKey::ImportWizard),
AppMode::RecordsNormal => Some(ModeKey::RecordsNormal),
AppMode::RecordsEditing { .. } => Some(ModeKey::RecordsEditing),
_ => None, _ => None,
} }
} }
@ -450,14 +454,9 @@ impl KeymapSet {
normal.bind(KeyCode::Char('z'), none, "toggle-group-under-cursor"); normal.bind(KeyCode::Char('z'), none, "toggle-group-under-cursor");
normal.bind(KeyCode::Char('H'), none, "hide-selected-row-item"); 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, "drill-into-cell");
normal.bind(KeyCode::Char('<'), none, "view-back"); 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 // Records mode toggle and prune toggle
normal.bind(KeyCode::Char('R'), none, "toggle-records-mode"); normal.bind(KeyCode::Char('R'), none, "toggle-records-mode");
@ -484,7 +483,17 @@ impl KeymapSet {
z_map.bind(KeyCode::Char('Z'), none, "wq"); z_map.bind(KeyCode::Char('Z'), none, "wq");
normal.bind_prefix(KeyCode::Char('Z'), none, Arc::new(z_map)); 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 ──────────────────────────────────────────────────── // ── Help mode ────────────────────────────────────────────────────
let mut help = Keymap::new(); let mut help = Keymap::new();
@ -754,7 +763,20 @@ impl KeymapSet {
); );
ed.bind_args(KeyCode::Backspace, none, "pop-char", vec!["edit".into()]); ed.bind_args(KeyCode::Backspace, none, "pop-char", vec!["edit".into()]);
ed.bind_any_char("append-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 ───────────────────────────────────────────────── // ── Formula edit ─────────────────────────────────────────────────
let mut fe = Keymap::new(); let mut fe = Keymap::new();
@ -1106,6 +1128,8 @@ mod tests {
ModeKey::CommandMode, ModeKey::CommandMode,
ModeKey::SearchMode, ModeKey::SearchMode,
ModeKey::ImportWizard, ModeKey::ImportWizard,
ModeKey::RecordsNormal,
ModeKey::RecordsEditing,
]; ];
for mode in &expected_modes { for mode in &expected_modes {
assert!( assert!(

View File

@ -132,12 +132,16 @@ fn mode_name(mode: &AppMode) -> &'static str {
AppMode::CommandMode { .. } => "COMMAND", AppMode::CommandMode { .. } => "COMMAND",
AppMode::Help => "HELP", AppMode::Help => "HELP",
AppMode::Quit => "QUIT", AppMode::Quit => "QUIT",
AppMode::RecordsNormal => "RECORDS",
AppMode::RecordsEditing { .. } => "RECORDS INSERT",
} }
} }
fn mode_style(mode: &AppMode) -> Style { fn mode_style(mode: &AppMode) -> Style {
match mode { 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::CommandMode { .. } => Style::default().fg(Color::Black).bg(Color::Yellow),
AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta), AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta),
_ => Style::default().fg(Color::Black).bg(Color::DarkGray), _ => Style::default().fg(Color::Black).bg(Color::DarkGray),

View File

@ -1,3 +1,4 @@
use indexmap::IndexMap;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use std::collections::{HashMap, HashSet}; 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. /// to implement the `Serialize`-as-string requirement for JSON object keys.
#[derive(Debug, Clone, Default)] #[derive(Debug, Clone, Default)]
pub struct DataStore { pub struct DataStore {
/// Primary storage — interned keys for O(1) hash/compare. /// Primary storage — interned keys, insertion-ordered so records mode
cells: HashMap<InternedKey, CellValue>, /// can display rows in the order they were entered.
cells: IndexMap<InternedKey, CellValue>,
/// String interner — all category/item names are interned here. /// String interner — all category/item names are interned here.
pub symbols: SymbolTable, pub symbols: SymbolTable,
/// Secondary index: interned (category, item) → set of interned keys. /// 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) { pub fn set(&mut self, key: CellKey, value: CellValue) {
let ikey = self.intern_key(&key); let ikey = self.intern_key(&key);
// Update index for each coordinate pair // Update index for each coordinate pair
@ -193,7 +215,7 @@ impl DataStore {
let Some(ikey) = self.lookup_key(key) else { let Some(ikey) = self.lookup_key(key) else {
return; return;
}; };
if self.cells.remove(&ikey).is_some() { if self.cells.shift_remove(&ikey).is_some() {
for pair in &ikey.0 { for pair in &ikey.0 {
if let Some(set) = self.index.get_mut(pair) { if let Some(set) = self.index.get_mut(pair) {
set.remove(&ikey); set.remove(&ikey);

View File

@ -20,6 +20,13 @@ use crate::ui::grid::{
use crate::view::GridLayout; use crate::view::GridLayout;
use crate::workbook::Workbook; 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 /// Drill-down state: frozen record snapshot + pending edits that have not
/// yet been applied to the model. /// yet been applied to the model.
#[derive(Debug, Clone, Default)] #[derive(Debug, Clone, Default)]
@ -72,6 +79,12 @@ pub enum AppMode {
}, },
Help, Help,
Quit, 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 { impl AppMode {
@ -79,6 +92,7 @@ impl AppMode {
pub fn minibuffer(&self) -> Option<&MinibufferConfig> { pub fn minibuffer(&self) -> Option<&MinibufferConfig> {
match self { match self {
Self::Editing { minibuf, .. } Self::Editing { minibuf, .. }
| Self::RecordsEditing { minibuf, .. }
| Self::FormulaEdit { minibuf, .. } | Self::FormulaEdit { minibuf, .. }
| Self::CommandMode { minibuf, .. } | Self::CommandMode { minibuf, .. }
| Self::CategoryAdd { 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 { pub fn editing() -> Self {
Self::Editing { Self::Editing {
minibuf: MinibufferConfig { 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 { pub fn formula_edit() -> Self {
Self::FormulaEdit { Self::FormulaEdit {
minibuf: MinibufferConfig { minibuf: MinibufferConfig {
@ -173,9 +207,9 @@ pub struct App {
pub tile_cat_idx: usize, pub tile_cat_idx: usize,
/// View navigation history: views visited before the current one. /// View navigation history: views visited before the current one.
/// Pushed on SwitchView, popped by `<` (back). /// Pushed on SwitchView, popped by `<` (back).
pub view_back_stack: Vec<String>, pub view_back_stack: Vec<ViewFrame>,
/// Views that were "back-ed" from, available for forward navigation (`>`). /// Views that were "back-ed" from, available for forward navigation (`>`).
pub view_forward_stack: Vec<String>, pub view_forward_stack: Vec<ViewFrame>,
/// Frozen records list for the drill view. When present, this is the /// Frozen records list for the drill view. When present, this is the
/// snapshot that records-mode layouts iterate — records don't disappear /// snapshot that records-mode layouts iterate — records don't disappear
/// when filters would change. Pending edits are stored alongside and /// when filters would change. Pending edits are stored alongside and
@ -389,7 +423,12 @@ impl App {
AppMode::Normal => { AppMode::Normal => {
"hjkl:nav i:edit R:records P:prune F/C/V:panels T:tiles [:]:page >:drill ::cmd" "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::FormulaPanel => "n:new d:delete jk:nav Esc:back",
AppMode::FormulaEdit { .. } => "Enter:save Esc:cancel — type: Name = expression", AppMode::FormulaEdit { .. } => "Enter:save Esc:cancel — type: Name = expression",
AppMode::CategoryPanel => { AppMode::CategoryPanel => {
@ -710,6 +749,43 @@ mod tests {
} }
/// Regression: pressing `o` in an empty records view should create the /// 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. /// first synthetic row instead of only entering edit mode on empty space.
#[test] #[test]
fn add_record_row_in_empty_records_view_creates_first_row() { 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" "o should create the first record row in an empty records view"
); );
assert!( assert!(
matches!(app.mode, AppMode::Editing { .. }), app.mode.is_editing(),
"o should leave the app in edit mode, got {:?}", "o should leave the app in edit mode, got {:?}",
app.mode app.mode
); );
@ -771,14 +847,107 @@ mod tests {
.unwrap(); .unwrap();
assert_eq!( assert_eq!(
app.workbook.model.get_cell(&CellKey::new(vec![ app.workbook.model.get_cell(&CellKey::new(vec![(
("_Measure".to_string(), "Rev".to_string()), "_Measure".to_string(),
])), "Rev".to_string(),
)])),
Some(&crate::model::cell::CellValue::Number(5.0)), Some(&crate::model::cell::CellValue::Number(5.0)),
"editing a synthetic row in plain records mode should write the value" "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<String> = (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 /// Drill-view edits should stay staged in drill state until the user
/// navigates back, at which point ApplyAndClearDrill writes them through. /// navigates back, at which point ApplyAndClearDrill writes them through.
#[test] #[test]

View File

@ -4,7 +4,7 @@ use std::path::PathBuf;
use crate::model::cell::{CellKey, CellValue}; use crate::model::cell::{CellKey, CellValue};
use crate::view::Axis; 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"; 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)] #[derive(Debug)]
pub struct SetCell(pub CellKey, pub CellValue); pub struct SetCell(pub CellKey, pub CellValue);
impl Effect for SetCell { impl Effect for SetCell {
@ -128,7 +136,11 @@ impl Effect for EnterEditAtCursor {
let value = ctx.display_value.clone(); let value = ctx.display_value.clone();
drop(ctx); drop(ctx);
app.buffers.insert("edit".to_string(), value); 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) { fn apply(&self, app: &mut App) {
let current = app.workbook.active_view.clone(); let current = app.workbook.active_view.clone();
if current != self.0 { 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(); app.view_forward_stack.clear();
} }
let _ = app.workbook.switch_view(&self.0); let _ = app.workbook.switch_view(&self.0);
@ -206,10 +221,14 @@ impl Effect for SwitchView {
pub struct ViewBack; pub struct ViewBack;
impl Effect for ViewBack { impl Effect for ViewBack {
fn apply(&self, app: &mut App) { 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(); let current = app.workbook.active_view.clone();
app.view_forward_stack.push(current); app.view_forward_stack.push(ViewFrame {
let _ = app.workbook.switch_view(&prev); 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; pub struct ViewForward;
impl Effect for ViewForward { impl Effect for ViewForward {
fn apply(&self, app: &mut App) { 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(); let current = app.workbook.active_view.clone();
app.view_back_stack.push(current); app.view_back_stack.push(ViewFrame {
let _ = app.workbook.switch_view(&next); 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); SwitchView("View 2".to_string()).apply(&mut app);
assert_eq!(app.workbook.active_view.as_str(), "View 2"); 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 // Forward stack should be cleared
assert!(app.view_forward_stack.is_empty()); assert!(app.view_forward_stack.is_empty());
} }
@ -1159,13 +1183,15 @@ mod tests {
// Go back // Go back
ViewBack.apply(&mut app); ViewBack.apply(&mut app);
assert_eq!(app.workbook.active_view.as_str(), "Default"); 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()); assert!(app.view_back_stack.is_empty());
// Go forward // Go forward
ViewForward.apply(&mut app); ViewForward.apply(&mut app);
assert_eq!(app.workbook.active_view.as_str(), "View 2"); 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()); assert!(app.view_forward_stack.is_empty());
} }

View File

@ -429,7 +429,7 @@ impl<'a> GridWidget<'a> {
} }
// Edit indicator // 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 buffer = self.buffers.get("edit").map(|s| s.as_str()).unwrap_or("");
let edit_x = col_x_at(sel_col); let edit_x = col_x_at(sel_col);

View File

@ -132,14 +132,12 @@ impl GridLayout {
page_coords: Vec<(String, String)>, page_coords: Vec<(String, String)>,
none_cats: Vec<String>, none_cats: Vec<String>,
) -> Self { ) -> Self {
let mut records: Vec<(CellKey, CellValue)> = model let records: Vec<(CellKey, CellValue)> = model
.data .data
.matching_cells(&page_coords) .matching_cells(&page_coords)
.into_iter() .into_iter()
.map(|(k, v)| (k, v.clone())) .map(|(k, v)| (k, v.clone()))
.collect(); .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 // Synthesize row items: one per record, labeled with its index
let row_items: Vec<AxisEntry> = (0..records.len()) let row_items: Vec<AxisEntry> = (0..records.len())
@ -152,7 +150,10 @@ impl GridLayout {
.into_iter() .into_iter()
.filter(|c| { .filter(|c| {
let kind = model.category(c).map(|cat| cat.kind); 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) .map(String::from)
.collect(); .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 { fn coord(pairs: &[(&str, &str)]) -> CellKey {
CellKey::new( CellKey::new(
pairs pairs