From b2386e1c2380bc4f8d40605d630cee91146cef66 Mon Sep 17 00:00:00 2001 From: Harrison O'Connor-Hoover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:45:41 -0400 Subject: [PATCH] Issue 30: preview border presets and clear shared edges --- ROADMAP.md | 1 + crates/engine/src/cmd/format.rs | 27 +++++- crates/engine/src/tests.rs | 75 ++++++++++++++++ crates/ui-egui/src/border_preview.rs | 82 +++++++++++++++++ crates/ui-egui/src/dialogs.rs | 7 +- crates/ui-egui/src/grid.rs | 2 +- crates/ui-egui/src/lib.rs | 1 + crates/ui-egui/src/ribbon.rs | 3 +- crates/ui-egui/tests/borders.rs | 129 +++++++++++++++++++++++++++ 9 files changed, 323 insertions(+), 4 deletions(-) create mode 100644 crates/ui-egui/src/border_preview.rs create mode 100644 crates/ui-egui/tests/borders.rs diff --git a/ROADMAP.md b/ROADMAP.md index 1b0cd34..4af7fb6 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -86,6 +86,7 @@ by open equivalents or left out, not cloned. formats, 47 cell styles, 60 table styles, conditional formatting (cell rules, text, dates, duplicates, top/bottom, averages, data bars, colour scales, icon sets, formulas), automatic row heights, autofit, hide/unhide, freeze panes. + Border presets include visual diagrams; Format Cells previews draft borders before applying. - Data: sort (multi-level, by colour, custom lists), AutoFilter (values, custom, top 10, average, colour), tables with totals rows, remove duplicates, text to columns, data validation with error alerts, grouping/outline, subtotals. diff --git a/crates/engine/src/cmd/format.rs b/crates/engine/src/cmd/format.rs index d6bc2ba..657fcfa 100644 --- a/crates/engine/src/cmd/format.rs +++ b/crates/engine/src/cmd/format.rs @@ -365,6 +365,9 @@ fn borders(s: &mut Session, p: &Json) -> Result { let base = border_style(str_param(p, "style").unwrap_or("thin")); let sheet = target_sheet(s, p)?; let ranges = target_ranges(s, p)?; + if s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.as_ref().is_some_and(|pr| !pr.format_cells)) { + return Err(EngineError::Other("The cell or chart you're trying to change is on a protected sheet.".into())); + } edit(s, |cx| { for r in &ranges { if r.count() > 2_000_000 { @@ -453,7 +456,29 @@ fn borders(s: &mut Session, p: &Json) -> Result { } }); cx.sheet_mut(sheet)?.set_style(c, new); - // Clear the neighbour's facing edge when removing borders, so it really goes. + if preset == "none" { + // A shared line can be stored on either cell. Selected cells are handled + // by this loop; remove only facing edges outside this range. + for (boundary, dr, dc) in [(top, -1, 0), (bottom, 1, 0), (left, 0, -1), (right, 0, 1)] { + if !boundary { + continue; + } + let Some(neighbor) = c.offset(dr, dc) else { continue }; + let st = cx.wb.sheet(sheet).map(|sh| sh.style_id(neighbor)).unwrap_or_default(); + let new = cx.wb.styles.derive(st, |s| { + let edge = match (dr, dc) { + (-1, _) => &mut s.borders.bottom, + (1, _) => &mut s.borders.top, + (_, -1) => &mut s.borders.right, + _ => &mut s.borders.left, + }; + *edge = BorderLine::default(); + }); + if new != st { + cx.sheet_mut(sheet)?.set_style(neighbor, new); + } + } + } } } Ok(Json::Null) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f3e36cd..1ae8471 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -39,6 +39,81 @@ fn undo_redo() { assert!(s.doc().unwrap().is_dirty()); } +#[test] +fn no_border_clears_only_shared_edges_and_undo_restores_them() { + use gridcraft_core::RangeRef; + use gridcraft_model::{BorderLine, Borders}; + + for (range, neighbors) in [ + ("B2", &[("B1", "bottom"), ("B3", "top"), ("A2", "right"), ("C2", "left")][..]), + ( + "B2:C3", + &[("B1", "bottom"), ("C1", "bottom"), ("B4", "top"), ("C4", "top"), ("A2", "right"), ("A3", "right"), ("D2", "left"), ("D3", "left")][..], + ), + ] { + let mut s = s(); + s.execute("range.setValues", json!({"range": "A1", "values": [[1, 2, 3, 4], [5, 6, 7, 8], [9, 10, 11, 12], [13, 14, 15, 16]]})).unwrap(); + s.execute("cell.set", json!({"cell": "D4", "input": "=SUM(A1:C1)"})).unwrap(); + s.execute("home.bold", json!({"range": "A1:D4", "on": true})).unwrap(); + for preset in ["all", "diagonalDown"] { + s.execute("home.borders", json!({"range": "A1:D4", "preset": preset, "style": "double", "color": "#CC3344"})).unwrap(); + } + let before: Vec<_> = RangeRef::parse("A1:D4").unwrap().iter().map(|c| s.execute("cell.get", json!({"cell": c.a1()})).unwrap()).collect(); + s.execute("home.borders", json!({"range": range, "preset": "none"})).unwrap(); + let selected = RangeRef::parse(range).unwrap(); + for original in &before { + let address = original["cell"].as_str().unwrap(); + let mut expected = original.clone(); + if selected.contains(CellRef::parse(address).unwrap()) { + expected["style"]["borders"] = json!(Borders::default()); + } else if let Some((_, edge)) = neighbors.iter().find(|(cell, _)| *cell == address) { + expected["style"]["borders"][*edge] = json!(BorderLine::default()); + } + assert_eq!(s.execute("cell.get", json!({"cell": address})).unwrap(), expected, "clearing {range}, cell {address}"); + } + s.execute("edit.undo", json!({})).unwrap(); + for original in before { + assert_eq!(s.execute("cell.get", json!({"cell": original["cell"]})).unwrap(), original); + } + } +} + +#[test] +fn no_border_handles_sheet_corners_without_creating_out_of_bounds_cells() { + for (range, corner, neighbors) in [ + ("A1:B2", "A1", [("B1", "left"), ("A2", "top")]), + ("XFC1048575:XFD1048576", "XFD1048576", [("XFC1048576", "right"), ("XFD1048575", "bottom")]), + ] { + let mut s = s(); + s.execute("home.borders", json!({"range": range, "preset": "all"})).unwrap(); + s.execute("home.borders", json!({"range": corner, "preset": "none"})).unwrap(); + for (neighbor, edge) in neighbors { + assert_eq!(s.execute("cell.get", json!({"cell": neighbor})).unwrap()["style"]["borders"][edge]["style"], "None"); + } + let sh = s.doc().unwrap().wb.active().unwrap(); + assert!(sh.cells.iter().all(|(c, _)| c.is_valid())); + assert!(sh.cells.iter().count() <= 4); + } +} + +#[test] +fn borders_respect_sheet_formatting_protection() { + let mut s = s(); + s.execute("home.borders", json!({"range": "A1:B2", "preset": "all"})).unwrap(); + s.execute("review.protectSheet", json!({})).unwrap(); + let before = s.doc().unwrap().wb.clone(); + let undo_count = s.doc().unwrap().undo.len(); + for preset in ["none", "left"] { + assert!(s.execute("home.borders", json!({"range": "A1", "preset": preset})).is_err()); + assert_eq!(s.doc().unwrap().wb.styles, before.styles); + assert_eq!(s.doc().unwrap().wb.active().unwrap().cells, before.active().unwrap().cells); + assert_eq!(s.doc().unwrap().undo.len(), undo_count); + } + s.execute("review.protectSheet", json!({"formatCells": true})).unwrap(); + s.execute("home.borders", json!({"range": "A1", "preset": "none"})).unwrap(); + assert_eq!(s.execute("cell.get", json!({"cell": "B1"})).unwrap()["style"]["borders"]["left"]["style"], "None"); +} + #[test] fn copy_paste_shifts_formulas() { let mut s = s(); diff --git a/crates/ui-egui/src/border_preview.rs b/crates/ui-egui/src/border_preview.rs new file mode 100644 index 0000000..11ff351 --- /dev/null +++ b/crates/ui-egui/src/border_preview.rs @@ -0,0 +1,82 @@ +//! Original border diagrams, painted with the same strokes as worksheet cells. + +use egui::{Align2, Color32, Rect, Response, Sense, Stroke, StrokeKind, Ui, pos2, vec2}; +use gridcraft_engine::model::{BorderStyle, Borders, Theme}; + +use crate::grid::paint_border; +use crate::theme::{self, Tokens}; + +/// A labelled menu button with a two-by-two diagram to distinguish inside edges +/// from the selection outline. Labels stay available to accessibility tools. +pub(crate) fn preset_button(ui: &mut Ui, label: &str, preset: &str) -> Response { + let response = ui.add(egui::Button::new("").frame(false).min_size(vec2(254.0, 26.0))); + response.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, ui.is_enabled(), label)); + let t = Tokens::get(ui.ctx()); + let rect = Rect::from_center_size(pos2(response.rect.left() + 16.0, response.rect.center().y), vec2(20.0, 16.0)); + let edges = [ + [rect.left_top(), rect.right_top()], + [rect.left_bottom(), rect.right_bottom()], + [rect.left_top(), rect.left_bottom()], + [rect.right_top(), rect.right_bottom()], + [rect.left_center(), rect.right_center()], + [rect.center_top(), rect.center_bottom()], + ]; + use BorderStyle::{Double, None, Thick, Thin}; + let styles = match preset { + "bottom" => [None, Thin, None, None, None, None], + "top" => [Thin, None, None, None, None, None], + "left" => [None, None, Thin, None, None, None], + "right" => [None, None, None, Thin, None, None], + "all" => [Thin; 6], + "outside" => [Thin, Thin, Thin, Thin, None, None], + "thickOutside" => [Thick, Thick, Thick, Thick, None, None], + "doubleBottom" => [None, Double, None, None, None, None], + "thickBottom" => [None, Thick, None, None, None, None], + "topBottom" => [Thin, Thin, None, None, None, None], + "topThickBottom" => [Thin, Thick, None, None, None, None], + "topDoubleBottom" => [Thin, Double, None, None, None, None], + "inside" => [None, None, None, None, Thin, Thin], + _ => [None; 6], + }; + for (points, style) in edges.into_iter().zip(styles) { + if style == None { + ui.painter().extend(egui::Shape::dotted_line(&points, t.text_disabled, 3.0, 0.45)); + } else { + paint_border(ui.painter(), points, style, t.text); + } + } + ui.painter().text(pos2(response.rect.left() + 36.0, response.rect.center().y), Align2::LEFT_CENTER, label, theme::ui_font(12.5), t.text); + response +} + +/// Preview only reads the dialog draft. A white worksheet surface keeps automatic +/// and explicit border colors faithful in both light and dark application themes. +pub(crate) fn sample(ui: &mut Ui, borders: &Borders, theme: &Theme) { + ui.add_space(8.0); + ui.label("Preview"); + let (area, response) = ui.allocate_exact_size(vec2(248.0, 104.0), Sense::hover()); + response.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Label, true, "Border preview")); + let t = Tokens::get(ui.ctx()); + ui.painter().rect_filled(area, 4.0, t.grid_bg); + ui.painter().rect_stroke(area, 4.0, Stroke::new(1.0, t.input_border), StrokeKind::Inside); + let rect = area.shrink2(vec2(30.0, 20.0)); + let edges = [ + (borders.top, [rect.left_top(), rect.right_top()]), + (borders.bottom, [rect.left_bottom(), rect.right_bottom()]), + (borders.left, [rect.left_top(), rect.left_bottom()]), + (borders.right, [rect.right_top(), rect.right_bottom()]), + (borders.diag_down, [rect.left_top(), rect.right_bottom()]), + (borders.diag_up, [rect.left_bottom(), rect.right_top()]), + ]; + for (line, points) in edges.iter().take(4) { + if line.is_none() { + ui.painter().extend(egui::Shape::dotted_line(points, Color32::from_gray(190), 4.0, 0.5)); + } + } + for (line, points) in edges { + if !line.is_none() { + let color = line.color.resolve(theme).map(theme::color32).unwrap_or(Color32::BLACK); + paint_border(ui.painter(), points, line.style, color); + } + } +} diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index b6c186d..d79254f 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1087,12 +1087,17 @@ fn format_cells(app: &mut SheetApp, ui: &mut egui::Ui, d: &mut Dialog, confirm: (BorderStyle::Dotted, "dotted"), (BorderStyle::Double, "double"), ] { - if ui.selectable_label(slot.style == s, label).clicked() { + let response = ui.selectable_label(slot.style == s, label); + response + .widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::SelectableLabel, ui.is_enabled(), format!("{n} {label}"))); + if response.clicked() { slot.style = s; } } }); } + let theme = app.session.active().map(|doc| &doc.wb.theme).cloned().unwrap_or_default(); + crate::border_preview::sample(ui, &st.borders, &theme); } "Fill" => { let colors = app.session.active().map(|doc| doc.wb.theme.colors).unwrap_or_default(); diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 4b326cc..5f861d8 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -769,7 +769,7 @@ pub fn filter_button_rect(geo: &Geo, sh: &Sheet, c: CellRef) -> Rect { Rect::from_min_size(pos2(rect.right() - s - 2.0, rect.bottom() - s - 2.0), vec2(s, s)) } -fn paint_border(p: &Painter, pts: [Pos2; 2], style: BorderStyle, col: Color32) { +pub(crate) fn paint_border(p: &Painter, pts: [Pos2; 2], style: BorderStyle, col: Color32) { let w = style.width(); let (a, b) = (pts[0], pts[1]); match style { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8eb1eca..86442bb 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -6,6 +6,7 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![forbid(unsafe_code)] +mod border_preview; pub mod chartview; pub mod control; pub mod credits; diff --git a/crates/ui-egui/src/ribbon.rs b/crates/ui-egui/src/ribbon.rs index 47c5432..825a8e7 100644 --- a/crates/ui-egui/src/ribbon.rs +++ b/crates/ui-egui/src/ribbon.rs @@ -352,6 +352,7 @@ fn home(app: &mut SheetApp, ui: &mut Ui) { if b { act(app, "home.borders", json!({"preset": app.grid.last_border.clone()})); } + ba.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, ui.is_enabled(), "Border presets")); egui::Popup::menu(&ba).show(|ui| { for (label, preset) in [ ("Bottom Border", "bottom"), @@ -369,7 +370,7 @@ fn home(app: &mut SheetApp, ui: &mut Ui) { ("Top and Double Bottom Border", "topDoubleBottom"), ("Inside Borders", "inside"), ] { - if ui.add(egui::Button::new(label).frame(false).min_size(vec2(220.0, 20.0))).clicked() { + if crate::border_preview::preset_button(ui, label, preset).clicked() { app.grid.last_border = preset.to_string(); act(app, "home.borders", json!({"preset": preset})); } diff --git a/crates/ui-egui/tests/borders.rs b/crates/ui-egui/tests/borders.rs new file mode 100644 index 0000000..af8a1a9 --- /dev/null +++ b/crates/ui-egui/tests/borders.rs @@ -0,0 +1,129 @@ +//! Border menu and dialog behavior through real egui controls. + +use egui::{Color32, Shape, vec2}; +use egui_kittest::kittest::Queryable; +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use gridcraft_engine::model::{BorderLine, BorderStyle, Borders, Color}; +use gridcraft_ui_egui::SheetApp; +use serde_json::json; + +fn harness() -> egui_kittest::Harness<'static, SheetApp> { + let mut session = Session::new(); + session.new_workbook(); + session.run("range.setValues", json!({"range": "A1", "values": [[1, 2], [3, 4]]})).unwrap(); + session.run("selection.set", json!({"range": "A1:B2"})).unwrap(); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1200.0, 800.0)).build_ui_state( + |ui, app: &mut SheetApp| { + app.logic(&ui.ctx().clone()); + app.ui(ui); + }, + SheetApp::new(session, Default::default()), + ); + h.run_steps(6); + h +} + +fn borders(h: &egui_kittest::Harness<'static, SheetApp>, cell: &str) -> Borders { + let doc = h.state().session.active().unwrap(); + let sheet = doc.wb.active().unwrap(); + doc.wb.styles.get(sheet.style_id(CellRef::parse(cell).unwrap())).borders +} + +#[test] +fn diagram_menu_applies_left_right_inside_and_outside_then_undoes() { + for label in ["Left Border", "Right Border", "Inside Borders", "Outside Borders"] { + let mut h = harness(); + h.get_by_label("Border presets").click(); + h.run_steps(6); + h.get_by_label(label).click(); + h.run_steps(6); + let a = borders(&h, "A1"); + let b = borders(&h, "B2"); + match label { + "Left Border" => { + assert_eq!(a.left.style, BorderStyle::Thin); + assert!(a.right.is_none() && b.left.is_none() && b.right.is_none()); + } + "Right Border" => { + assert_eq!(b.right.style, BorderStyle::Thin); + assert!(a.left.is_none() && a.right.is_none() && b.left.is_none()); + } + "Inside Borders" => { + assert_eq!(a.right.style, BorderStyle::Thin); + assert_eq!(a.bottom.style, BorderStyle::Thin); + assert_eq!(b.left.style, BorderStyle::Thin); + assert_eq!(b.top.style, BorderStyle::Thin); + assert!(a.left.is_none() && a.top.is_none() && b.right.is_none() && b.bottom.is_none()); + } + _ => { + assert_eq!(a.left.style, BorderStyle::Thin); + assert_eq!(a.top.style, BorderStyle::Thin); + assert_eq!(b.right.style, BorderStyle::Thin); + assert_eq!(b.bottom.style, BorderStyle::Thin); + assert!(a.right.is_none() && a.bottom.is_none() && b.left.is_none() && b.top.is_none()); + } + } + let sheet = h.state().session.active().unwrap().wb.active().unwrap(); + assert_eq!(sheet.value(CellRef::new(0, 0)), Value::Number(1.0)); + assert_eq!(sheet.value(CellRef::new(1, 1)), Value::Number(4.0)); + h.state_mut().run("edit.undo", json!({})).unwrap(); + assert_eq!(borders(&h, "A1"), Borders::default()); + assert_eq!(borders(&h, "B2"), Borders::default()); + } +} + +fn colored_borders() -> Borders { + let line = |color| BorderLine { style: BorderStyle::Thin, color: Color::Rgb(color) }; + Borders { + top: line(0xab192d), + bottom: line(0x174faa), + left: line(0x16864d), + right: line(0x985213), + diag_down: line(0x7c2ba2), + diag_up: line(0x248088), + } +} + +fn preview_lines(h: &egui_kittest::Harness<'static, SheetApp>, color: Color32) -> usize { + let preview = h.get_by_label("Border preview").rect(); + h.output() + .shapes + .iter() + .filter(|shape| match &shape.shape { + Shape::LineSegment { points, stroke } => stroke.color == color && points.iter().all(|p| preview.contains(*p)), + _ => false, + }) + .count() +} + +#[test] +fn border_preview_uses_the_colored_draft_until_ok_and_cancel_discards_it() { + let mut h = harness(); + let original = colored_borders(); + h.state_mut().run("home.formatCells", json!({"style": {"borders": original}})).unwrap(); + h.state_mut().open_dialog("formatCells", json!({"tab": "Border"})); + h.run_steps(6); + for line in [original.top, original.bottom, original.left, original.right, original.diag_down, original.diag_up] { + let [r, g, b] = line.color.resolve(&Default::default()).unwrap(); + assert_eq!(preview_lines(&h, Color32::from_rgb(r, g, b)), 1); + } + h.get_by_label("Top double").click(); + h.run_steps(6); + assert_eq!(borders(&h, "A1"), original); + assert_eq!(preview_lines(&h, Color32::from_rgb(0xab, 0x19, 0x2d)), 2); + h.get_by_label(" Cancel ").click(); + h.run_steps(6); + assert!(h.state().dialog.is_none()); + assert_eq!(borders(&h, "A1"), original); + h.state_mut().open_dialog("formatCells", json!({"tab": "Border"})); + h.run_steps(6); + h.get_by_label("Top double").click(); + h.run_steps(6); + h.get_by_label(" OK ").click(); + h.run_steps(6); + assert!(h.state().dialog.is_none()); + assert_eq!(borders(&h, "A1").top.style, BorderStyle::Double); + h.state_mut().run("edit.undo", json!({})).unwrap(); + assert_eq!(borders(&h, "A1"), original); +}