Merge pull request #106 from harrisonoconnorhover/codex/border-previews

Preview border presets and fix shared-edge removal

ROADMAP.md

@@ -87,6 +87,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.

crates/engine/src/cmd/format.rs

@@ -365,6 +365,9 @@ fn borders(s: &mut Session, p: &Json) -> Result<Json> {
     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<Json> {
                     }
                 });
                 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)

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();

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);
+        }
+    }
+}

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();

crates/ui-egui/src/grid.rs

@@ -775,7 +775,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 {

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;

crates/ui-egui/src/ribbon.rs

@@ -379,6 +379,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"),
@@ -396,7 +397,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}));
                     }

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);
+}