From e3f0ae8a1c8ed3f653dca241519bd2b9ebd7e9fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 25 Sep 2026 21:44:52 -0400 Subject: [PATCH] T14 review fixes: numbers sort before words, a removed filter takes its field, sort rows take free columns, grids fit their cursor, land on merges and keep Tab, formulas guard long references and count only numbers --- TASKS.md | 18 ++-- src/tables/builders.rs | 70 +++++++++++++- src/tables/formula.rs | 99 ++++++++++++++----- src/tables/grid.rs | 87 ++++++++--------- src/tables/gridkeys.rs | 126 +++++++++++++++++++++++-- src/tables/rules.rs | 46 ++++++++- src/tables/tests.rs | 210 +++++++++++++++++++++++++++++++++++++++-- 7 files changed, 550 insertions(+), 106 deletions(-) diff --git a/TASKS.md b/TASKS.md index 0faaba8..d9f43b8 100644 --- a/TASKS.md +++ b/TASKS.md @@ -44,11 +44,11 @@ Per-component lines live in `tasks/`. Tags there: - [x] T11d Motion · Effects - [x] T12a Data Display · People & numbers - [x] T12b Data Display · Records -- [ ] T12c Data Display · Media +- [x] T12c Data Display · Media - [x] T12d Data Display · Codes & measures -- [ ] T13a Lists & Trees · Lists -- [ ] T13b Lists & Trees · Trees -- [ ] T13c Lists & Trees · Files +- [x] T13a Lists & Trees · Lists +- [x] T13b Lists & Trees · Trees +- [x] T13c Lists & Trees · Files - [ ] T14a Tables · Core - [ ] T14b Tables · Columns & rows - [ ] T14c Tables · Grids @@ -192,11 +192,15 @@ Per-component lines live in `tasks/`. Tags there: | T12b | 2 | PASS | | | T12c | 1 | FAIL | the gallery kept an open index past a shrunk list, and the lightbox asserted on it | | T12c | 2 | FAIL | emptying the list removed the open lightbox without handing focus back | +| T12c | 3 | PASS | | | T12d | 1 | FAIL | Code 128 took a set-switch character, a usage bar's float sum tripped its assert, and a dense QR floored its modules to nothing | | T12d | 2 | PASS | | | T13a | 1 | FAIL | Down and Cmd-A picked disabled rows, and Enter opened them | +| T13a | 2 | PASS | | | T13b | 1 | FAIL | Down stuck on a loading row and never reached the nodes past it | +| T13b | 2 | PASS | | | T13c | 1 | FAIL | the cursor kept its index through a re-sort, so Enter opened another file | -| T14a | 1 | — | pending: codex still logged out | -| T14b | 1 | — | pending: codex still logged out | -| T14c | 1 | — | pending review | +| T13c | 2 | PASS | | +| T14a | 1 | FAIL | numbers and words compared out of order in a mixed column, and the sort panicked | +| T14b | 1 | FAIL | a removed filter rule left its typing to the next rule, and two sort rows could take one column | +| T14c | 1 | FAIL | a shrunk grid kept its cursor outside, long column letters overflowed, presses landed in merged cells, Tab left the grid, and ranges counted text and blanks | diff --git a/src/tables/builders.rs b/src/tables/builders.rs index d8f3dc5..2bddf54 100644 --- a/src/tables/builders.rs +++ b/src/tables/builders.rs @@ -20,7 +20,7 @@ type OnSorts = Rc; /// The newest rules and hearer, for value fields whose edits come later. type Latest = Rc, bool, Option)>>; -/// A builder's value fields, one per rule, and what their edits report to. +/// A builder's value fields, one per rule in the rules' order, and what their edits report to. #[derive(Default)] struct Fields { fields: Vec<(Entity, Subscription)>, @@ -34,6 +34,19 @@ fn choices(columns: &[(SharedString, SharedString)]) -> Vec { .collect() } +/// The columns a sort row may take: its own, and those no other row holds. +fn open( + columns: &[(SharedString, SharedString)], + keys: &[SortKey], + own: &SharedString, +) -> Vec<(SharedString, SharedString)> { + columns + .iter() + .filter(|(key, _)| key == own || !keys.iter().any(|sort| sort.column == *key)) + .cloned() + .collect() +} + /// Filter rules to build: each a column, a test and a value, and a switch between keeping rows that pass all and rows that pass any. The owner keeps the rules and hands them to a `DataTable`. #[derive(IntoElement)] pub struct FilterBuilder { @@ -86,13 +99,19 @@ impl RenderOnce for FilterBuilder { let latest = fields.read(cx).latest.clone(); *latest.borrow_mut() = (self.rules.clone(), self.any, self.on_change.clone()); while fields.read(cx).fields.len() < self.rules.len() { - let ix = fields.read(cx).fields.len(); let field = cx.new(|cx| TextInput::new(window, cx)); - let latest = latest.clone(); + let (latest, owner) = (latest.clone(), fields.downgrade()); let heard = window.subscribe(&field, cx, move |field, event, window, cx| { if !matches!(event, InputEvent::Changed) { return; } + let Some(ix) = owner.upgrade().and_then(|owner| { + let held = &owner.read(cx).fields; + held.iter() + .position(|(held, _)| held.entity_id() == field.entity_id()) + }) else { + return; + }; let (mut rules, any, on_change) = latest.borrow().clone(); let Some(rule) = rules.get_mut(ix) else { return; @@ -141,7 +160,8 @@ impl RenderOnce for FilterBuilder { .iter() .enumerate() .map(|(ix, rule)| { - let (column, test, remove) = (emit.clone(), emit.clone(), emit.clone()); + let (column, test, remove, held) = + (emit.clone(), emit.clone(), emit.clone(), fields.clone()); let (for_column, for_test, for_remove) = (rules.clone(), rules.clone(), rules.clone()); div() @@ -189,6 +209,7 @@ impl RenderOnce for FilterBuilder { .on_click(move |_, window, cx| { let mut next = for_remove.clone(); next.remove(ix); + held.update(cx, |held, _| drop(held.fields.remove(ix))); remove(next, any, window, cx) }), ) @@ -259,8 +280,15 @@ impl SortBuilder { } } + /// The keys, each on its own column. pub fn keys(mut self, keys: impl IntoIterator) -> Self { self.keys = keys.into_iter().collect(); + assert!( + self.keys.iter().enumerate().all(|(ix, key)| self.keys[..ix] + .iter() + .all(|before| before.column != key.column)), + "sort keys take a column each" + ); self } @@ -318,7 +346,7 @@ impl RenderOnce for SortBuilder { div().w(theme.label_width()).child( Select::new( (id.clone(), format!("sort-column-{ix}")), - choices(&self.columns), + choices(&open(&self.columns, &keys, &sort.column)), ) .selected(sort.column.clone()) .size(ControlSize::Sm) @@ -381,3 +409,35 @@ impl RenderOnce for SortBuilder { })) } } + +#[cfg(test)] +mod tests { + use gpui::SharedString; + + use super::{SortKey, open}; + + #[test] + fn a_sort_row_offers_its_own_column_and_the_unused_ones() { + let columns: Vec<(SharedString, SharedString)> = ["a", "b", "c"] + .map(|key| (key.into(), key.to_uppercase().into())) + .to_vec(); + let keys = [ + SortKey { + column: "a".into(), + rising: true, + }, + SortKey { + column: "b".into(), + rising: false, + }, + ]; + let offered = |own: &'static str| { + open(&columns, &keys, &own.into()) + .into_iter() + .map(|(key, _)| key.to_string()) + .collect::>() + }; + assert_eq!(offered("a"), ["a", "c"]); + assert_eq!(offered("b"), ["b", "c"]); + } +} diff --git a/src/tables/formula.rs b/src/tables/formula.rs index 9e33da1..dbee264 100644 --- a/src/tables/formula.rs +++ b/src/tables/formula.rs @@ -20,6 +20,9 @@ static REFERENCE: LazyLock = /// How deep formulas may lean on each other before a loop is assumed. const DEPTH: usize = 64; +/// The most cells a range may cover. +const RANGE_CELLS: usize = 10_000; + /// A column's letters: A to Z, then AA. pub(crate) fn letters(mut col: usize) -> String { let mut out = Vec::new(); @@ -39,50 +42,71 @@ pub(crate) fn name(row: usize, col: usize) -> String { format!("{}{}", letters(col), row + 1) } -/// The row and column an A1 name points to, counted from zero. +/// The row and column an A1 name points to, counted from zero; none when it is too long to count. pub(crate) fn place(text: &str) -> Option<(usize, usize)> { let caps = REFERENCE .captures(text) .filter(|caps| caps.get(0).is_some_and(|all| all.as_str() == text))?; - let col = caps[1].bytes().fold(0usize, |col, letter| { - col * 26 + usize::from(letter - b'A' + 1) - }) - 1; + let col = caps[1].bytes().try_fold(0usize, |col, letter| { + col.checked_mul(26)? + .checked_add(usize::from(letter - b'A' + 1)) + })? - 1; let row = caps[2].parse::().ok()?.checked_sub(1)?; Some((row, col)) } -/// Every cell name in a rectangle, row by row. -fn span(from: &str, to: &str) -> Result, String> { +/// Every cell in a rectangle, row by row. +fn span(from: &str, to: &str) -> Result, String> { let ((r0, c0), (r1, c1)) = ( place(from).ok_or("a bad cell")?, place(to).ok_or("a bad cell")?, ); let (rows, cols) = (r0.min(r1)..=r0.max(r1), c0.min(c1)..=c0.max(c1)); + let covered = (r0.abs_diff(r1) + 1).saturating_mul(c0.abs_diff(c1) + 1); + if covered > RANGE_CELLS { + return Err(format!("{from}:{to} covers more than {RANGE_CELLS} cells")); + } Ok(rows - .flat_map(|row| cols.clone().map(move |col| name(row, col))) + .flat_map(|row| cols.clone().map(move |col| (row, col))) .collect()) } -/// A formula with its ranges written out and its functions in the evaluator's words. -fn expand(source: &str) -> Result { +/// A range function over the numbers in its range; text and blanks are left out, as in a spreadsheet. +fn aggregate(function: &str, numbers: &[f64]) -> Result { + let sum: f64 = numbers.iter().sum(); + let least = || numbers.iter().copied().reduce(f64::min).unwrap_or(0.0); + let most = || numbers.iter().copied().reduce(f64::max).unwrap_or(0.0); + match function.to_uppercase().as_str() { + "SUM" => Ok(sum), + "AVERAGE" | "AVG" if numbers.is_empty() => Err("no numbers to average".into()), + "AVERAGE" | "AVG" => Ok(sum / numbers.len() as f64), + "MIN" => Ok(least()), + "MAX" => Ok(most()), + "COUNT" => Ok(numbers.len() as f64), + other => Err(format!("{other} takes no range")), + } +} + +/// A formula with each range function worked out and its other functions in the evaluator's words. +fn expand( + cells: &dyn Fn(usize, usize) -> SharedString, + source: &str, + depth: usize, +) -> Result { let mut failure = None; let written = RANGE.replace_all(source, |caps: &Captures| { - let cells = match span(&caps[2], &caps[3]) { - Ok(cells) => cells, + let worked = span(&caps[2], &caps[3]).and_then(|places| { + let values = places + .iter() + .map(|(row, col)| value(cells, *row, *col, depth + 1)) + .collect::, _>>()?; + let numbers: Vec = values.into_iter().flatten().collect(); + aggregate(&caps[1], &numbers) + }); + match worked { + Ok(number) => format!("({number})"), Err(error) => { - failure = Some(error); - return String::new(); - } - }; - let (sum, count) = (cells.join("+"), cells.len()); - match caps[1].to_uppercase().as_str() { - "SUM" => format!("({sum})"), - "AVERAGE" | "AVG" => format!("(({sum})/{count})"), - "MIN" => format!("min({})", cells.join(",")), - "MAX" => format!("max({})", cells.join(",")), - "COUNT" => count.to_string(), - other => { - failure = Some(format!("{other} takes no range")); + failure.get_or_insert(error); String::new() } } @@ -119,7 +143,7 @@ fn formula( if depth > DEPTH { return Err("a loop".into()); } - let expanded = expand(source)?; + let expanded = expand(cells, source, depth)?; let mut variables: Vec<(SharedString, f64)> = Vec::new(); for caps in REFERENCE.captures_iter(&expanded) { let reference = caps[0].to_string(); @@ -262,6 +286,31 @@ mod tests { ); } + #[test] + fn range_functions_read_only_the_numbers_in_their_range() { + let cells = sheet(&[&[ + "8", + "text", + "", + "=COUNT(A1:C1)", + "=AVERAGE(A1:C1)", + "=MIN(A1:C1)", + ]]); + assert_eq!(shown(&cells, 0, 3), "1"); + assert_eq!(shown(&cells, 0, 4), "8"); + assert_eq!(shown(&cells, 0, 5), "8"); + let blank = sheet(&[&["", "=AVERAGE(A1:A1)"]]); + assert_eq!(shown(&blank, 0, 1), "#ERR", "nothing to average"); + } + + #[test] + fn a_reference_too_long_to_count_fails_as_a_formula() { + assert_eq!(place("ZZZZZZZZZZZZZZZZZZZZ1"), None); + let cells = sheet(&[&["=ZZZZZZZZZZZZZZZZZZZZ1", "=SUM(A1:ZZZZ99999)"]]); + assert_eq!(shown(&cells, 0, 0), "#ERR"); + assert_eq!(shown(&cells, 0, 1), "#ERR", "a range too wide to walk"); + } + #[test] fn fills_step_numbers_shift_formulas_and_repeat_text() { let texts = |list: &[&'static str]| { diff --git a/src/tables/grid.rs b/src/tables/grid.rs index 3f10488..943532e 100644 --- a/src/tables/grid.rs +++ b/src/tables/grid.rs @@ -6,9 +6,10 @@ use gpui::{ SharedString, StatefulInteractiveElement, Styled, Window, canvas, div, point, prelude::*, }; -use super::gridkeys::{begin, fill, keys}; +use super::gridkeys::{Metrics, across, begin, fill, fit, keys, land}; use crate::{ forms::{Editing, Input}, + primitives::{FocusNext, FocusPrev}, theme::{ActiveTheme, ControlSize, TextSize}, }; @@ -49,34 +50,6 @@ pub(crate) struct Face { pub on_change: Option, } -/// Sizes in pixels: a cell, a header, the row labels, and the frozen stretch. -#[derive(Clone, Copy)] -pub(crate) struct Metrics { - pub wide: Pixels, - pub tall: Pixels, - pub label: Pixels, - pub frozen: Point, -} - -impl Metrics { - /// The cell under a point within the grid, if any. - pub(crate) fn cell_at( - &self, - at: Point, - offset: Point, - face: (usize, usize), - ) -> Option<(usize, usize)> { - let (x, y) = (at.x - self.label, at.y - self.tall); - if x < Pixels::ZERO || y < Pixels::ZERO { - return None; - } - let x = if x < self.frozen.x { x } else { x + offset.x }; - let y = if y < self.frozen.y { y } else { y + offset.y }; - let (col, row) = ((x / self.wide) as usize, (y / self.tall) as usize); - (row < face.0 && col < face.1).then_some((row, col)) - } -} - /// A drag of a grid's fill handle, named by its owner. pub(crate) struct FillDrag { owner: EntityId, @@ -96,7 +69,13 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) -> let sheet: Entity = window.use_keyed_state((id.clone(), "sheet"), cx, |_, _| Sheet::default()); let editor = window.use_keyed_state((id.clone(), "editor"), cx, |_, _| Editing::default()); - sheet.update(cx, |sheet, _| sheet.rows = face.rows); + let shrunk = sheet.update(cx, |sheet, _| { + sheet.rows = face.rows; + fit(sheet, (face.rows, face.cols)) + }); + if shrunk { + editor.update(cx, |editing, cx| editing.finish(true, window, cx)); + } if sheet.read(cx).editing && editor.read(cx).field().is_none() { sheet.update(cx, |sheet, _| sheet.editing = false); window.focus(&focus); @@ -104,25 +83,7 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) -> let theme = cx.theme(); let colors = theme.colors.clone(); let rem = window.rem_size(); - let metrics = Metrics { - wide: theme.grid_column().to_pixels(rem), - tall: theme - .table_row(crate::theme::Density::Compact) - .to_pixels(rem), - label: if face.numbered { - theme.control_height(ControlSize::Lg).to_pixels(rem) * 1.25 - } else { - Pixels::ZERO - }, - frozen: Point::default(), - }; - let metrics = Metrics { - frozen: point( - metrics.wide * face.frozen.1 as f32, - metrics.tall * face.frozen.0 as f32, - ), - ..metrics - }; + let metrics = Metrics::new(&face, window, cx); let (rows, cols) = (face.rows, face.cols); let (offset, bounds, editing, fill_to) = { let sheet = sheet.read(cx); @@ -348,6 +309,12 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) -> let (raw, on_change, begin_with) = (face.raw.clone(), face.on_change.clone(), editor.clone()); let (fill_raw, fill_hears) = (face.raw.clone(), face.on_change.clone()); let numbered = face.numbered; + let (merges, frozen) = (Rc::new(face.merges.clone()), face.frozen); + let (hovered_merges, stepped, stepped_back) = ( + merges.clone(), + (sheet.clone(), editor.clone()), + (sheet.clone(), editor.clone()), + ); let keyed = keys(face, metrics, sheet.clone(), editor.clone()); let focus_on_press = focus.clone(); let text_size = theme.text_size(TextSize::Sm); @@ -361,12 +328,33 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) -> .border_1() .border_color(colors.border) .on_key_down(keyed) + .capture_action(move |_: &FocusNext, window, cx| { + across( + &stepped.0, + &stepped.1, + (face_cols, false), + (metrics, frozen), + window, + cx, + ) + }) + .capture_action(move |_: &FocusPrev, window, cx| { + across( + &stepped_back.0, + &stepped_back.1, + (face_cols, true), + (metrics, frozen), + window, + cx, + ) + }) .on_mouse_down(MouseButton::Left, move |event, window, cx| { let at = event.position - pressed.read(cx).bounds.origin; let Some(cell) = metrics.cell_at(at, pressed.read(cx).offset, (face_rows, face_cols)) else { return; }; + let cell = land(&merges, cell); window.focus(&focus_on_press); pressed.update(cx, |sheet, cx| { sheet.cursor = cell; @@ -394,6 +382,7 @@ pub(crate) fn grid(face: Face, base: Div, window: &mut Window, cx: &mut App) -> } let at = event.position - moved.read(cx).bounds.origin; if let Some(cell) = metrics.cell_at(at, moved.read(cx).offset, (face_rows, face_cols)) { + let cell = land(&hovered_merges, cell); moved.update(cx, |sheet, cx| { if sheet.cursor != cell && !sheet.editing { sheet.cursor = cell; diff --git a/src/tables/gridkeys.rs b/src/tables/gridkeys.rs index fc3d044..09932be 100644 --- a/src/tables/gridkeys.rs +++ b/src/tables/gridkeys.rs @@ -1,12 +1,115 @@ use std::rc::Rc; -use gpui::{App, ClipboardItem, Entity, KeyDownEvent, Pixels, SharedString, Window}; +use gpui::{App, ClipboardItem, Entity, KeyDownEvent, Pixels, Point, SharedString, Window, point}; use super::{ formula::filled, - grid::{Face, Metrics, OnCells, Sheet, Text, range}, + grid::{Face, Merge, OnCells, Sheet, Text, range}, }; -use crate::forms::Editing; +use crate::{ + forms::Editing, + theme::{ActiveTheme, ControlSize, Density}, +}; + +/// Sizes in pixels: a cell, a header, the row labels, and the frozen stretch. +#[derive(Clone, Copy)] +pub(crate) struct Metrics { + pub wide: Pixels, + pub tall: Pixels, + pub label: Pixels, + pub frozen: Point, +} + +impl Metrics { + /// A face's sizes at the window's rem. + pub(crate) fn new(face: &Face, window: &Window, cx: &App) -> Self { + let theme = cx.theme(); + let rem = window.rem_size(); + let (wide, tall) = ( + theme.grid_column().to_pixels(rem), + theme.table_row(Density::Compact).to_pixels(rem), + ); + Self { + wide, + tall, + label: if face.numbered { + theme.control_height(ControlSize::Lg).to_pixels(rem) * 1.25 + } else { + Pixels::ZERO + }, + frozen: point(wide * face.frozen.1 as f32, tall * face.frozen.0 as f32), + } + } + + /// The cell under a point within the grid, if any. + pub(crate) fn cell_at( + &self, + at: Point, + offset: Point, + face: (usize, usize), + ) -> Option<(usize, usize)> { + let (x, y) = (at.x - self.label, at.y - self.tall); + if x < Pixels::ZERO || y < Pixels::ZERO { + return None; + } + let x = if x < self.frozen.x { x } else { x + offset.x }; + let y = if y < self.frozen.y { y } else { y + offset.y }; + let (col, row) = ((x / self.wide) as usize, (y / self.tall) as usize); + (row < face.0 && col < face.1).then_some((row, col)) + } +} + +/// The cell a place stands for: a merge's first cell for any cell it covers. +pub(crate) fn land(merges: &[Merge], (row, col): (usize, usize)) -> (usize, usize) { + merges + .iter() + .find(|merge| { + (merge.from.0..=merge.to.0).contains(&row) && (merge.from.1..=merge.to.1).contains(&col) + }) + .map_or((row, col), |merge| merge.from) +} + +/// Pulls the cursor, its anchor and a fill back inside a grid of `size`; whether anything moved. +pub(crate) fn fit(sheet: &mut Sheet, size: (usize, usize)) -> bool { + let last = (size.0.saturating_sub(1), size.1.saturating_sub(1)); + let inside = |(row, col): (usize, usize)| (row.min(last.0), col.min(last.1)); + let (cursor, anchor) = (inside(sheet.cursor), inside(sheet.anchor)); + let moved = (cursor, anchor) != (sheet.cursor, sheet.anchor); + if moved { + log::info!("grid: now {size:?}; the cursor moved to {cursor:?}"); + (sheet.cursor, sheet.anchor, sheet.fill) = (cursor, anchor, None); + } + moved +} + +/// Tab and Shift-Tab: keep an open edit, then step across, staying inside the grid. +pub(crate) fn across( + sheet: &Entity, + editor: &Entity, + (cols, back): (usize, bool), + (metrics, frozen): (Metrics, (usize, usize)), + window: &mut Window, + cx: &mut App, +) { + cx.stop_propagation(); + let (row, col) = sheet.read(cx).cursor; + if sheet.read(cx).editing { + editor.update(cx, |editing, cx| editing.finish(false, window, cx)); + } + let to = ( + row, + if back { + col.saturating_sub(1) + } else { + (col + 1).min(cols.saturating_sub(1)) + }, + ); + sheet.update(cx, |sheet, cx| { + (sheet.cursor, sheet.anchor) = (to, to); + reveal(sheet, metrics, frozen); + cx.notify(); + }); +} /// Scrolls just enough that the cursor shows, past the frozen rows and columns. pub(crate) fn reveal(sheet: &mut Sheet, metrics: Metrics, frozen: (usize, usize)) { @@ -100,7 +203,7 @@ pub(crate) fn pasted( .collect() } -/// The key handler: arrows walk and Shift stretches the range, Tab and Enter step, typing or F2 edits, Delete clears, Cmd-C and Cmd-V copy and paste. +/// The key handler: arrows walk, stepping over merges, and Shift stretches the range; Enter steps down, typing or F2 edits, Delete clears, Cmd-C and Cmd-V copy and paste. pub(crate) fn keys( face: Face, metrics: Metrics, @@ -118,20 +221,23 @@ pub(crate) fn keys( let stroke = &event.keystroke; let held = &stroke.modifiers; let (row, col) = sheet.read(cx).cursor; + let far = face + .merges + .iter() + .find(|merge| merge.from == (row, col)) + .map_or((row, col), |merge| merge.to); let to = match stroke.key.as_str() { "up" => Some((row.saturating_sub(1), col)), - "down" | "enter" => Some(((row + 1).min(rows - 1), col)), + "down" | "enter" => Some(((far.0 + 1).min(rows - 1), col)), "left" => Some((row, col.saturating_sub(1))), - "right" => Some((row, (col + 1).min(cols - 1))), - "tab" if held.shift => Some((row, col.saturating_sub(1))), - "tab" => Some((row, (col + 1).min(cols - 1))), + "right" => Some((row, (far.1 + 1).min(cols - 1))), "home" => Some((row, 0)), "end" => Some((row, cols - 1)), _ => None, }; - if let Some(to) = to { + if let Some(to) = to.map(|to| land(&face.merges, to)) { cx.stop_propagation(); - let stretch = held.shift && !matches!(stroke.key.as_str(), "tab" | "enter"); + let stretch = held.shift && stroke.key != "enter"; sheet.update(cx, |sheet, cx| { sheet.cursor = to; if !stretch { diff --git a/src/tables/rules.rs b/src/tables/rules.rs index 93a9757..e6c57e3 100644 --- a/src/tables/rules.rs +++ b/src/tables/rules.rs @@ -112,15 +112,17 @@ pub(crate) fn kept( .collect() } -/// `order` sorted by each key in turn, the first deciding most; numbers by value, the rest by their words. +/// `order` sorted by each key in turn, the first deciding most; numbers by value and before the rest, the rest by their words. pub(crate) fn sorted_by(rows: &[Row], mut order: Vec, keys: &[(usize, bool)]) -> Vec { order.sort_by(|a, b| { keys.iter().fold(Ordering::Equal, |so_far, (col, rising)| { so_far.then_with(|| { let (a, b) = (&rows[*a].cells[*col], &rows[*b].cells[*col]); let by = match (a.number(), b.number()) { - (Some(a), Some(b)) => a.partial_cmp(&b).unwrap_or(Ordering::Equal), - _ => a.words().to_lowercase().cmp(&b.words().to_lowercase()), + (Some(a), Some(b)) => a.total_cmp(&b), + (Some(_), None) => Ordering::Less, + (None, Some(_)) => Ordering::Greater, + (None, None) => a.words().to_lowercase().cmp(&b.words().to_lowercase()), }; if *rising { by } else { by.reverse() } }) @@ -273,6 +275,44 @@ mod tests { ); } + #[test] + fn numbers_sort_before_words_in_a_mixed_column() { + let cells: [Cell; 6] = [ + "11".into(), + 10.0.into(), + "apple".into(), + 2.0.into(), + Cell::Empty, + "Banana".into(), + ]; + let rows: Vec = cells + .into_iter() + .enumerate() + .map(|(ix, cell)| Row::new(ix.to_string(), [cell])) + .collect(); + let rising = sorted_by(&rows, (0..rows.len()).collect(), &[(0, true)]); + assert_eq!( + rising, + [3, 1, 4, 0, 2, 5], + "2, 10, then the words: empty, 11, apple, banana" + ); + let many: Vec = (0..64) + .map(|ix| { + Row::new( + ix.to_string(), + [if ix % 3 == 0 { + Cell::from(format!("{ix}")) + } else { + Cell::from(ix as f64) + }], + ) + }) + .collect(); + let order = sorted_by(&many, (0..64).rev().collect(), &[(0, true)]); + let numbers = order.iter().take_while(|ix| *ix % 3 != 0).count(); + assert_eq!(numbers, 42, "every number comes before every word"); + } + #[test] fn csv_quotes_what_needs_it() { let columns = vec![Column::new("a", "Name"), Column::new("b", "Note")]; diff --git a/src/tables/tests.rs b/src/tables/tests.rs index 6bca722..efd64a1 100644 --- a/src/tables/tests.rs +++ b/src/tables/tests.rs @@ -1,12 +1,19 @@ -use std::{cell::RefCell, rc::Rc}; - -use gpui::{ - Context, IntoElement, Modifiers, ParentElement, Render, SharedString, Styled, TestAppContext, - VisualTestContext, Window, point, px, +use std::{ + cell::{Cell as Count, RefCell}, + collections::HashMap, + rc::Rc, }; -use super::{Column, DataTable, Row}; -use crate::theme::Theme; +use gpui::{ + Context, FocusHandle, IntoElement, KeyBinding, Modifiers, ParentElement, Render, SharedString, + Styled, TestAppContext, VisualTestContext, Window, div, point, px, +}; + +use super::{Column, DataTable, FilterBuilder, FilterRule, Row}; +use crate::{ + primitives::{FocusNext, FocusPrev, FocusScope}, + theme::Theme, +}; /// Five selectable rows, 36 tall under a 36 tall header; it keeps what the table reports. struct Picks(Rc>>); @@ -188,6 +195,12 @@ struct Grid(Rc>>); impl Render for Grid { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + self.render_grid() + } +} + +impl Grid { + fn render_grid(&self) -> impl IntoElement + use<> { let heard = self.0.clone(); let rows: Vec> = (0..3) .map(|row| vec![format!("a{row}").into(), format!("b{row}").into()]) @@ -230,3 +243,186 @@ fn typing_edits_enter_keeps_and_backspace_clears_a_range(cx: &mut TestAppContext "enter moved down; the range clears" ); } + +/// Filter rules the owner keeps, 600 across, rebuilt from what the builder reports. +struct Filters(Rc>>); + +impl Render for Filters { + fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { + let (rules, store, view) = (self.0.borrow().clone(), self.0.clone(), cx.entity()); + div().w(px(600.0)).child( + FilterBuilder::new("filters", [("name", "Name"), ("region", "Region")]) + .rules(rules, false) + .on_change(move |rules, _, _, cx| { + *store.borrow_mut() = rules.to_vec(); + view.update(cx, |_, cx| cx.notify()); + }), + ) + } +} + +#[gpui::test] +fn removing_the_rule_being_typed_leaves_the_next_rule_its_own_value(cx: &mut TestAppContext) { + cx.update(|cx| { + Theme::init(cx); + crate::forms::bind_keys(cx); + }); + let rule = |column: &'static str, value: &'static str| FilterRule { + column: column.into(), + value: value.into(), + ..FilterRule::default() + }; + let rules = Rc::new(RefCell::new(vec![ + rule("name", "one"), + rule("region", "keep"), + ])); + let seen = rules.clone(); + let (_, cx) = cx.add_window_view(|_, _| Filters(seen)); + settle(cx); + cx.simulate_click(point(px(330.0), px(12.0)), Modifiers::none()); + settle(cx); + cx.simulate_input("typed"); + settle(cx); + assert_eq!( + rules.borrow()[0].value.as_ref(), + "onetyped", + "the first field took the typing" + ); + cx.simulate_click(point(px(588.0), px(12.0)), Modifiers::none()); + settle(cx); + cx.simulate_input("X"); + settle(cx); + assert_eq!( + *rules.borrow(), + [rule("region", "keep")], + "the removed rule's draft went with it" + ); +} + +/// A two-column grid whose row count the test sets. +struct Shrinking(Rc>); + +impl Render for Shrinking { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let rows: Vec> = (0..self.0.get()) + .map(|row| vec![format!("a{row}").into(), format!("b{row}").into()]) + .collect(); + super::DataGrid::new("shrink", ["A", "B"], rows) + .w(px(300.0)) + .h(px(200.0)) + } +} + +fn grid_keys(cx: &mut TestAppContext) { + cx.update(|cx| { + Theme::init(cx); + crate::forms::bind_keys(cx); + cx.bind_keys([ + KeyBinding::new("tab", FocusNext, None), + KeyBinding::new("shift-tab", FocusPrev, None), + ]); + }); +} + +#[gpui::test] +fn a_grid_that_shrinks_keeps_its_cursor_inside(cx: &mut TestAppContext) { + grid_keys(cx); + let count = Rc::new(Count::new(3)); + let (view, cx) = cx.add_window_view(|_, _| Shrinking(count.clone())); + settle(cx); + cx.simulate_click(point(px(146.0), px(98.0)), Modifiers::none()); + settle(cx); + count.set(1); + view.update(cx, |_, cx| cx.notify()); + settle(cx); + cx.simulate_keystrokes("cmd-c"); + settle(cx); + let copied = cx.update(|_, cx| cx.read_from_clipboard().and_then(|item| item.text())); + assert_eq!( + copied.as_deref(), + Some("b0"), + "the cursor moved to the last row left" + ); +} + +/// A sheet with A1:C1 merged, 4 across, that keeps every edit it hears. +struct Merged(Rc>>); + +impl Render for Merged { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let heard = self.0.clone(); + let cells: HashMap<(usize, usize), SharedString> = + [((0, 0), "Title".into())].into_iter().collect(); + super::Spreadsheet::new("merged", 3, 4) + .cells(cells) + .merge((0, 0), (0, 2)) + .on_change(move |edits, _, _| { + heard.borrow_mut().extend( + edits + .iter() + .map(|(row, col, text)| (*row, *col, text.to_string())), + ) + }) + .w(px(500.0)) + .h(px(200.0)) + } +} + +#[gpui::test] +fn a_merge_answers_for_every_cell_it_covers(cx: &mut TestAppContext) { + grid_keys(cx); + let heard = Rc::new(RefCell::new(Vec::new())); + let seen = heard.clone(); + let (_, cx) = cx.add_window_view(|_, _| Merged(seen)); + settle(cx); + cx.simulate_click(point(px(146.0), px(42.0)), Modifiers::none()); + settle(cx); + cx.simulate_keystrokes("7 enter"); + settle(cx); + cx.simulate_keystrokes("up right 8 enter"); + settle(cx); + assert_eq!( + *heard.borrow(), + [(0, 0, "7".to_string()), (0, 3, "8".to_string())], + "a press inside lands on the merge; right steps past it" + ); +} + +/// A grid inside a focus scope, as apps hold it, that keeps every edit it hears. +struct Scoped(FocusHandle, Rc>>); + +impl Render for Scoped { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + FocusScope::new(&self.0) + .size_full() + .child(Grid(self.1.clone()).render_grid()) + } +} + +#[gpui::test] +fn tab_steps_across_the_grid_and_keeps_an_edit(cx: &mut TestAppContext) { + grid_keys(cx); + let heard = Rc::new(RefCell::new(Vec::new())); + let seen = heard.clone(); + let (_, cx) = cx.add_window_view(|_, cx| Scoped(cx.focus_handle(), seen)); + settle(cx); + cx.simulate_click(point(px(48.0), px(42.0)), Modifiers::none()); + settle(cx); + cx.simulate_keystrokes("tab 7 enter"); + settle(cx); + cx.simulate_click(point(px(48.0), px(70.0)), Modifiers::none()); + settle(cx); + cx.simulate_keystrokes("5 tab"); + settle(cx); + cx.simulate_keystrokes("6 enter shift-tab"); + settle(cx); + assert_eq!( + *heard.borrow(), + [ + (0, 1, "7".to_string()), + (1, 0, "5".to_string()), + (1, 1, "6".to_string()) + ], + "tab moves right, and keeps an open edit first" + ); +}