From 2c2228c2c26386b0197858879eeac195ca39839f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Hatcher?= Date: Sun, 27 Oct 2024 16:45:14 +0100 Subject: [PATCH] FIX: [App]: Borders done right --- base/src/test/user_model/test_border.rs | 226 ++++++++++++++++++++- base/src/test/user_model/test_paste_csv.rs | 48 +++++ base/src/user_model/common.rs | 132 +++++++++++- webapp/src/components/borderPicker.tsx | 51 +++-- webapp/src/components/toolbar.tsx | 4 +- webapp/src/locale/en_us.json | 16 +- 6 files changed, 442 insertions(+), 35 deletions(-) diff --git a/base/src/test/user_model/test_border.rs b/base/src/test/user_model/test_border.rs index 7437ce6..1c237bb 100644 --- a/base/src/test/user_model/test_border.rs +++ b/base/src/test/user_model/test_border.rs @@ -51,6 +51,84 @@ fn borders_all() { } } + // let's check the borders around + { + let row = 4; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: Some(border_item.clone()), + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let row = 9; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: Some(border_item.clone()), + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } + { + let column = 5; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: Some(border_item.clone()), + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let column = 9; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: Some(border_item.clone()), + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } + // Lets remove all of them: let border_area: BorderArea = serde_json::from_str( r##"{ @@ -63,8 +141,8 @@ fn borders_all() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); - for row in 5..9 { - for column in 6..9 { + for row in 4..10 { + for column in 5..10 { let style = model.get_cell_style(0, row, column).unwrap(); let expected_border = Border { diagonal_up: false, @@ -229,6 +307,84 @@ fn borders_outer() { }; assert_eq!(style.border, expected_border); } + + // let's check the borders around + { + let row = 4; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: Some(border_item.clone()), + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let row = 9; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: Some(border_item.clone()), + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } + { + let column = 5; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: Some(border_item.clone()), + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let column = 9; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: Some(border_item.clone()), + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } } #[test] @@ -275,6 +431,72 @@ fn borders_top() { assert_eq!(style.border, expected_border); } } + + // let's check the borders around + { + let row = 4; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let border_item = BorderItem { + style: BorderStyle::Thin, + color: Some("#FF5566".to_string()), + }; + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: Some(border_item.clone()), + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let row = 9; + for column in 6..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } + { + let column = 5; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + let column = 9; + for row in 5..9 { + let style = model.get_cell_style(0, row, column).unwrap(); + let expected_border = Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + } } #[test] diff --git a/base/src/test/user_model/test_paste_csv.rs b/base/src/test/user_model/test_paste_csv.rs index 24c9754..5c56f29 100644 --- a/base/src/test/user_model/test_paste_csv.rs +++ b/base/src/test/user_model/test_paste_csv.rs @@ -48,6 +48,54 @@ fn tsv_crlf_paste() { ); } +#[test] +fn cut_paste() { + let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); + model.set_user_input(0, 1, 1, "42").unwrap(); + model.set_user_input(0, 1, 2, "=A1*3+1").unwrap(); + + // set A1 bold + let range = Area { + sheet: 0, + row: 1, + column: 1, + width: 1, + height: 1, + }; + model.update_range_style(&range, "font.b", "true").unwrap(); + + model + .set_user_input(0, 2, 1, "A season of faith, \"perfection\"") + .unwrap(); + + // Select A1:B2 and copy + model.set_selected_range(1, 1, 2, 2).unwrap(); + let copy = model.copy_to_clipboard().unwrap(); + + model.set_selected_cell(4, 4).unwrap(); + + // paste in cell D4 (4, 4) + model + .paste_from_clipboard((1, 1, 2, 2), ©.data, true) + .unwrap(); + + assert_eq!(model.get_cell_content(0, 4, 4), Ok("42".to_string())); + assert_eq!(model.get_cell_content(0, 4, 5), Ok("=D4*3+1".to_string())); + assert_eq!( + model.get_formatted_cell_value(0, 4, 5), + Ok("127".to_string()) + ); + // cell D4 must be bold + let style_d4 = model.get_cell_style(0, 4, 4).unwrap(); + assert!(style_d4.font.b); + + // range A1:B2 must be empty + assert_eq!(model.get_cell_content(0, 1, 1), Ok("".to_string())); + assert_eq!(model.get_cell_content(0, 1, 2), Ok("".to_string())); + assert_eq!(model.get_cell_content(0, 2, 1), Ok("".to_string())); + assert_eq!(model.get_cell_content(0, 2, 2), Ok("".to_string())); +} + #[test] fn copy_paste_internal() { let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs index 80c39b6..3ed8d4e 100644 --- a/base/src/user_model/common.rs +++ b/base/src/user_model/common.rs @@ -7,7 +7,7 @@ use csv_sniffer::Sniffer; use serde::{Deserialize, Serialize}; use crate::{ - constants::{self, DEFAULT_ROW_HEIGHT}, + constants::{self, DEFAULT_ROW_HEIGHT, LAST_COLUMN, LAST_ROW}, expressions::{ types::{Area, CellReferenceIndex}, utils::{is_valid_column_number, is_valid_row}, @@ -41,7 +41,7 @@ pub struct Clipboard { pub(crate) range: (i32, i32, i32, i32), } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, PartialEq)] pub enum BorderType { All, Inner, @@ -796,7 +796,6 @@ impl UserModel { range: &Area, border_area: &BorderArea, ) -> Result<(), String> { - // FIXME: We need to set the border also in neighbouring cells. let sheet = range.sheet; let mut diff_list = Vec::new(); let last_row = range.row + range.height - 1; @@ -806,12 +805,6 @@ impl UserModel { let old_value = self.model.get_style_for_cell(sheet, row, column)?; let mut style = old_value.clone(); - // First remove all existing borders - style.border.top = None; - style.border.right = None; - style.border.bottom = None; - style.border.left = None; - match border_area.r#type { BorderType::All => { style.border.top = Some(border_area.item.clone()); @@ -868,7 +861,10 @@ impl UserModel { } } BorderType::None => { - // noop, we already removed all the borders + style.border.top = None; + style.border.right = None; + style.border.bottom = None; + style.border.left = None; } } @@ -882,6 +878,122 @@ impl UserModel { }); } } + // bottom of the cells above the first + if range.row > 1 + && [ + BorderType::Top, + BorderType::All, + BorderType::None, + BorderType::Outer, + ] + .contains(&border_area.r#type) + { + let row = range.row - 1; + for column in range.column..=last_column { + let old_value = self.model.get_style_for_cell(sheet, row, column)?; + let mut style = old_value.clone(); + if border_area.r#type == BorderType::None { + style.border.bottom = None; + } else { + style.border.bottom = Some(border_area.item.clone()); + } + self.model.set_cell_style(sheet, row, column, &style)?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column, + old_value: Box::new(old_value), + new_value: Box::new(style), + }); + } + } + // Cells to the right + if last_column < LAST_COLUMN + && [ + BorderType::Right, + BorderType::All, + BorderType::None, + BorderType::Outer, + ] + .contains(&border_area.r#type) + { + let column = last_column + 1; + for row in range.row..=last_row { + let old_value = self.model.get_style_for_cell(sheet, row, column)?; + let mut style = old_value.clone(); + if border_area.r#type == BorderType::None { + style.border.left = None; + } else { + style.border.left = Some(border_area.item.clone()); + } + self.model.set_cell_style(sheet, row, column, &style)?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column, + old_value: Box::new(old_value), + new_value: Box::new(style), + }); + } + } + // Cells bellow + if last_row < LAST_ROW + && [ + BorderType::Bottom, + BorderType::All, + BorderType::None, + BorderType::Outer, + ] + .contains(&border_area.r#type) + { + let row = last_row + 1; + for column in range.column..=last_column { + let old_value = self.model.get_style_for_cell(sheet, row, column)?; + let mut style = old_value.clone(); + if border_area.r#type == BorderType::None { + style.border.top = None; + } else { + style.border.top = Some(border_area.item.clone()); + } + self.model.set_cell_style(sheet, row, column, &style)?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column, + old_value: Box::new(old_value), + new_value: Box::new(style), + }); + } + } + // Cells to the left + if range.column > 1 + && [ + BorderType::Left, + BorderType::All, + BorderType::None, + BorderType::Outer, + ] + .contains(&border_area.r#type) + { + let column = range.column - 1; + for row in range.row..=last_row { + let old_value = self.model.get_style_for_cell(sheet, row, column)?; + let mut style = old_value.clone(); + if border_area.r#type == BorderType::None { + style.border.right = None; + } else { + style.border.right = Some(border_area.item.clone()); + } + self.model.set_cell_style(sheet, row, column, &style)?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column, + old_value: Box::new(old_value), + new_value: Box::new(style), + }); + } + } self.push_diff_list(diff_list); Ok(()) diff --git a/webapp/src/components/borderPicker.tsx b/webapp/src/components/borderPicker.tsx index 477d1c0..a54cb6b 100644 --- a/webapp/src/components/borderPicker.tsx +++ b/webapp/src/components/borderPicker.tsx @@ -7,7 +7,7 @@ import { PencilLine, } from "lucide-react"; import type React from "react"; -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { BorderBottomIcon, @@ -27,6 +27,7 @@ import ColorPicker from "./colorPicker"; type BorderPickerProps = { className?: string; onChange: (border: BorderOptions) => void; + onClose: () => void; anchorEl: React.RefObject; anchorOrigin?: PopoverOrigin; transformOrigin?: PopoverOrigin; @@ -36,25 +37,33 @@ type BorderPickerProps = { const BorderPicker = (properties: BorderPickerProps) => { const { t } = useTranslation(); - const [borderSelected, setBorderSelected] = useState(BorderType.None); + const [borderSelected, setBorderSelected] = useState(null); const [borderColor, setBorderColor] = useState("#000000"); const [borderStyle, setBorderStyle] = useState(BorderStyle.Thin); const [colorPickerOpen, setColorPickerOpen] = useState(false); const [stylePickerOpen, setStylePickerOpen] = useState(false); - const closePicker = (): void => { + + // biome-ignore lint/correctness/useExhaustiveDependencies: + useEffect(() => { + if (!borderSelected) { + return; + } properties.onChange({ color: borderColor, style: borderStyle, border: borderSelected, }); - }; + }, [borderColor, borderStyle, borderSelected]); + + const onClose = properties.onClose; + const borderColorButton = useRef(null); const borderStyleButton = useRef(null); return ( <> closePicker()} + onClose={onClose} anchorEl={properties.anchorEl.current} anchorOrigin={ properties.anchorOrigin || { vertical: "bottom", horizontal: "left" } @@ -71,13 +80,13 @@ const BorderPicker = (properties: BorderPickerProps) => { $pressed={borderSelected === BorderType.All} onClick={() => { if (borderSelected === BorderType.All) { - setBorderSelected(BorderType.None); + setBorderSelected(null); } else { setBorderSelected(BorderType.All); } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.all")} > @@ -86,13 +95,13 @@ const BorderPicker = (properties: BorderPickerProps) => { $pressed={borderSelected === BorderType.Inner} onClick={() => { if (borderSelected === BorderType.Inner) { - setBorderSelected(BorderType.None); + setBorderSelected(null); } else { setBorderSelected(BorderType.Inner); } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.inner")} > @@ -101,13 +110,13 @@ const BorderPicker = (properties: BorderPickerProps) => { $pressed={borderSelected === BorderType.CenterH} onClick={() => { if (borderSelected === BorderType.CenterH) { - setBorderSelected(BorderType.None); + setBorderSelected(null); } else { setBorderSelected(BorderType.CenterH); } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.horizontal")} > @@ -116,13 +125,13 @@ const BorderPicker = (properties: BorderPickerProps) => { $pressed={borderSelected === BorderType.CenterV} onClick={() => { if (borderSelected === BorderType.CenterV) { - setBorderSelected(BorderType.None); + setBorderSelected(null); } else { setBorderSelected(BorderType.CenterV); } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.vertical")} > @@ -137,7 +146,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.outer")} > @@ -154,7 +163,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.clear")} > @@ -169,7 +178,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.top")} > @@ -184,7 +193,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.right")} > @@ -199,7 +208,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.bottom")} > @@ -214,7 +223,7 @@ const BorderPicker = (properties: BorderPickerProps) => { } }} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.left")} > @@ -228,7 +237,7 @@ const BorderPicker = (properties: BorderPickerProps) => { $pressed={false} disabled={false} ref={borderColorButton} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.color")} > @@ -243,7 +252,7 @@ const BorderPicker = (properties: BorderPickerProps) => { type="button" $pressed={false} disabled={false} - title={t("workbook.toolbar.borders_button_title")} + title={t("toolbar.borders.style")} > diff --git a/webapp/src/components/toolbar.tsx b/webapp/src/components/toolbar.tsx index 51c6d51..b69f0d7 100644 --- a/webapp/src/components/toolbar.tsx +++ b/webapp/src/components/toolbar.tsx @@ -325,7 +325,7 @@ function Toolbar(properties: ToolbarProperties) { onClick={() => setBorderPickerOpen(true)} ref={borderButton} disabled={!canEdit} - title={t("toolbar.borders")} + title={t("toolbar.borders.title")} > @@ -369,6 +369,8 @@ function Toolbar(properties: ToolbarProperties) { { properties.onBorderChanged(border); + }} + onClose={() => { setBorderPickerOpen(false); }} anchorEl={borderButton} diff --git a/webapp/src/locale/en_us.json b/webapp/src/locale/en_us.json index 8fc0d19..5f2fd9d 100644 --- a/webapp/src/locale/en_us.json +++ b/webapp/src/locale/en_us.json @@ -15,7 +15,6 @@ "format_number": "Format number", "font_color": "Font color", "fill_color": "Fill color", - "borders": "Borders", "decimal_places_increase": "Increase decimal places", "decimal_places_decrease": "Decrease decimal places", "show_hide_grid_lines": "Show/hide grid lines", @@ -39,6 +38,21 @@ "currency_gbp_example": "£", "date_short_example": "09/24/2024", "date_long_example": "Tuesday, September 24, 2024" + }, + "borders": { + "title": "Borders", + "all": "All borders", + "inner": "Inner borders", + "outer": "Outer borders", + "top": "Top borders", + "bottom": "Bottom borders", + "clear": "Clear borders", + "left": "Left borders", + "right": "Right borders", + "horizontal": "Horizontal borders", + "vertical": "Vertical borders", + "color": "Border color", + "style": "Border style" } }, "num_fmt": {