From 0df132d5c286ed12adf9f19d349dc58754a1da41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Hatcher?= Date: Mon, 11 Nov 2024 18:43:50 +0100 Subject: [PATCH] FIX: Work undone We are not going to follow this path, but I will leave this commit as part of the git history --- base/src/test/user_model/test_border.rs | 128 ++++++- base/src/types.rs | 2 +- base/src/user_model/border_utils.rs | 143 ++++++++ base/src/user_model/common.rs | 333 ++++-------------- base/src/user_model/mod.rs | 1 + .../src/components/useKeyboardNavigation.ts | 8 + 6 files changed, 348 insertions(+), 267 deletions(-) create mode 100644 base/src/user_model/border_utils.rs diff --git a/base/src/test/user_model/test_border.rs b/base/src/test/user_model/test_border.rs index 1c237bb..6814b90 100644 --- a/base/src/test/user_model/test_border.rs +++ b/base/src/test/user_model/test_border.rs @@ -1,11 +1,111 @@ #![allow(clippy::unwrap_used)] use crate::{ + constants::{LAST_COLUMN, LAST_ROW}, expressions::{types::Area, utils::number_to_column}, types::{Border, BorderItem, BorderStyle}, BorderArea, UserModel, }; +// checks there are no borders in the sheet +#[track_caller] +fn check_no_borders(model: &UserModel) { + let workbook = &model.model.workbook; + for ws in &workbook.worksheets { + for data_row in ws.sheet_data.values() { + for cell in data_row.values() { + let style_index = cell.get_style(); + let style = workbook.styles.get_style(style_index).unwrap(); + assert_eq!( + style.border, + Border { + diagonal_up: false, + diagonal_down: false, + left: None, + right: None, + top: None, + bottom: None, + diagonal: None + } + ) + } + } + } +} + +// checks that all the borders are consistent +#[track_caller] +fn check_borders(model: &UserModel) { + let workbook = &model.model.workbook; + for (sheet_index, ws) in workbook.worksheets.iter().enumerate() { + let sheet = sheet_index as u32; + for (&row, data_row) in &ws.sheet_data { + for (&column, cell) in data_row { + let style_index = cell.get_style(); + let style = workbook.styles.get_style(style_index).unwrap(); + // Top border: + if let Some(top_border) = style.border.top { + if row > 1 { + let top_cell_style = model.get_cell_style(sheet, row - 1, column).unwrap(); + assert_eq!( + Some(top_border), + top_cell_style.border.bottom, + "(Top). Sheet: {}, row: {}, column: {}", + sheet, + row, + column + ); + } + } + // Border to the right + if let Some(right_border) = style.border.right { + if column < LAST_COLUMN { + let right_cell_style = + model.get_cell_style(sheet, row, column + 1).unwrap(); + assert_eq!( + Some(right_border), + right_cell_style.border.left, + "(Right). Sheet: {}, row: {}, column: {}", + sheet, + row, + column + ); + } + } + // Bottom border: + if let Some(bottom_border) = style.border.bottom { + if row < LAST_ROW { + let bottom_cell_style = + model.get_cell_style(sheet, row + 1, column).unwrap(); + assert_eq!( + Some(bottom_border), + bottom_cell_style.border.top, + "(Bottom). Sheet: {}, row: {}, column: {}", + sheet, + row, + column + ); + } + } + // Left Border + if let Some(left_border) = style.border.left { + if column > 1 { + let left_cell_style = model.get_cell_style(sheet, row, column - 1).unwrap(); + assert_eq!( + Some(left_border), + left_cell_style.border.right, + "(Left). Sheet: {}, row: {}, column: {}", + sheet, + row, + column + ); + } + } + } + } + } +} + #[test] fn borders_all() { let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); @@ -129,6 +229,8 @@ fn borders_all() { } } + check_borders(&model); + // Lets remove all of them: let border_area: BorderArea = serde_json::from_str( r##"{ @@ -156,11 +258,14 @@ fn borders_all() { assert_eq!(style.border, expected_border); } } + + check_borders(&model); } #[test] fn borders_inner() { let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); + check_borders(&model); // We set an outer border in cells F5:H9 let range = &Area { sheet: 0, @@ -183,6 +288,7 @@ fn borders_inner() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); // The inner part all have borders for row in 6..8 { for column in 7..8 { @@ -269,6 +375,7 @@ fn borders_outer() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); { // We check the border on F5 let style = model.get_cell_style(0, 5, 6).unwrap(); @@ -412,6 +519,7 @@ fn borders_top() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); for row in 5..9 { for column in 6..9 { let style = model.get_cell_style(0, row, column).unwrap(); @@ -419,13 +527,18 @@ fn borders_top() { style: BorderStyle::Thin, color: Some("#FF5566".to_string()), }; + let bottom = if row == 8 { + None + } else { + Some(border_item.clone()) + }; let expected_border = Border { diagonal_up: false, diagonal_down: false, left: None, right: None, top: Some(border_item.clone()), - bottom: None, + bottom, diagonal: None, }; assert_eq!(style.border, expected_border); @@ -497,6 +610,8 @@ fn borders_top() { assert_eq!(style.border, expected_border); } } + assert!(model.undo().is_ok()); + check_no_borders(&model); } #[test] @@ -524,6 +639,7 @@ fn borders_right() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); for row in 5..9 { for column in 6..9 { let style = model.get_cell_style(0, row, column).unwrap(); @@ -570,6 +686,7 @@ fn borders_bottom() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); for row in 5..9 { for column in 6..9 { let style = model.get_cell_style(0, row, column).unwrap(); @@ -577,12 +694,18 @@ fn borders_bottom() { style: BorderStyle::Thin, color: Some("#FF5566".to_string()), }; + // The top will also have a value for all but the first one + let top = if row == 5 { + None + } else { + Some(border_item.clone()) + }; let expected_border = Border { diagonal_up: false, diagonal_down: false, left: None, right: None, - top: None, + top, bottom: Some(border_item.clone()), diagonal: None, }; @@ -616,6 +739,7 @@ fn borders_left() { ) .unwrap(); model.set_area_with_border(range, &border_area).unwrap(); + check_borders(&model); for row in 5..9 { for column in 6..9 { let style = model.get_cell_style(0, row, column).unwrap(); diff --git a/base/src/types.rs b/base/src/types.rs index b62cdb5..c4f85fc 100644 --- a/base/src/types.rs +++ b/base/src/types.rs @@ -578,7 +578,7 @@ impl Default for CellStyles { } } -#[derive(Serialize, Deserialize, Encode, Decode, Debug, PartialEq, Eq, Clone)] +#[derive(Serialize, Deserialize, Encode, Decode, Debug, PartialEq, Eq, PartialOrd, Clone)] #[serde(rename_all = "lowercase")] pub enum BorderStyle { Thin, diff --git a/base/src/user_model/border_utils.rs b/base/src/user_model/border_utils.rs new file mode 100644 index 0000000..09ec666 --- /dev/null +++ b/base/src/user_model/border_utils.rs @@ -0,0 +1,143 @@ +use crate::types::BorderItem; + +fn parse_color(s: &str) -> Option<(u8, u8, u8)> { + let s = s.trim_start_matches('#'); + match s.len() { + 6 => { + let r = u8::from_str_radix(&s[0..2], 16).ok()?; + let g = u8::from_str_radix(&s[2..4], 16).ok()?; + let b = u8::from_str_radix(&s[4..6], 16).ok()?; + Some((r, g, b)) + } + 3 => { + let r = u8::from_str_radix(&s[0..1], 16).ok()?; + let g = u8::from_str_radix(&s[1..2], 16).ok()?; + let b = u8::from_str_radix(&s[2..3], 16).ok()?; + // Expand single hex digits to full bytes + Some((r * 17, g * 17, b * 17)) + } + _ => None, + } +} + +fn compute_luminance(r: u8, g: u8, b: u8) -> f64 { + // Normalize RGB values to [0, 1] + let r = r as f64 / 255.0; + let g = g as f64 / 255.0; + let b = b as f64 / 255.0; + // Calculate luminance using the Rec. 601 formula + 0.299 * r + 0.587 * g + 0.114 * b +} + +fn is_max_color(a: &str, b: &str) -> bool { + let (ar, ag, ab) = match parse_color(a) { + Some(rgb) => rgb, + None => return false, // Invalid color format for 'a' + }; + + let (br, bg, bb) = match parse_color(b) { + Some(rgb) => rgb, + None => return false, // Invalid color format for 'b' + }; + + let luminance_a = compute_luminance(ar, ag, ab); + let luminance_b = compute_luminance(br, bg, bb); + + // 'b' is heavier if its luminance is less than 'a's luminance + luminance_b < luminance_a +} + +pub(crate) fn is_max_border(a: &Option, b: &Option) -> bool { + match (a, b) { + (_, None) => false, + (None, Some(_)) => true, + (Some(item_a), Some(item_b)) => { + if item_a.style < item_b.style { + return true; + } else if item_a.style > item_b.style { + return false; + } + match (&item_a.color, &item_b.color) { + (_, None) => false, + (None, Some(_)) => true, + (Some(color_a), Some(color_b)) => is_max_color(color_a, color_b), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_basic_colors() { + // Black vs White + assert!(is_max_color("#FFFFFF", "#000000")); + assert!(!is_max_color("#000000", "#FFFFFF")); + + // Red vs Dark Red + assert!(is_max_color("#FF0000", "#800000")); + assert!(!is_max_color("#800000", "#FF0000")); + + // Green vs Dark Green + assert!(is_max_color("#00FF00", "#008000")); + assert!(!is_max_color("#008000", "#00FF00")); + + // Blue vs Dark Blue + assert!(is_max_color("#0000FF", "#000080")); + assert!(!is_max_color("#000080", "#0000FF")); + } + + #[test] + fn test_same_color() { + // Comparing the same color should return false + assert!(!is_max_color("#123456", "#123456")); + } + + #[test] + fn test_shorthand_colors() { + // Shorthand notation (#RGB) + assert!(is_max_color("#FFF", "#000")); + assert!(is_max_color("#F00", "#800")); + assert!(is_max_color("#0F0", "#080")); + assert!(is_max_color("#00F", "#008")); + assert!(!is_max_color("#ABC", "#123")); + } + + #[test] + fn test_invalid_colors() { + // Invalid color formats + assert!(!is_max_color("#GGGGGG", "#000000")); + assert!(!is_max_color("#12345", "#000000")); + assert!(!is_max_color("#1234567", "#000000")); + assert!(!is_max_color("123456", "#000000")); + assert!(!is_max_color("#123456", "#XYZ")); + assert!(!is_max_color("#", "#000000")); + } + + #[test] + fn test_edge_cases() { + // Colors with minimal luminance difference + assert!(!is_max_color("#000000", "#010101")); + assert!(!is_max_color("#FEFEFE", "#FFFFFF")); + assert!(!is_max_color("#7F7F7F", "#808080")); + } + + #[test] + fn test_luminance_ordering() { + // Colors with known luminance differences + assert!(is_max_color("#CCCCCC", "#333333")); // Light gray vs Day + assert!(is_max_color("#FFFF00", "#808000")); // Yellow ve + assert!(is_max_color("#FF00FF", "#800080")); // Magenta vle + } + + #[test] + fn test_borderline_cases() { + // Testing colors with equal luminance + assert!(!is_max_color("#777777", "#777777")); + + // Testing black against near-black + assert!(!is_max_color("#000000", "#010000")); + } +} diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs index 57c12af..bd012b2 100644 --- a/base/src/user_model/common.rs +++ b/base/src/user_model/common.rs @@ -22,6 +22,8 @@ use crate::{ use crate::user_model::history::{ ColumnData, Diff, DiffList, DiffType, History, QueueDiffs, RowData, }; + +use super::border_utils::is_max_border; /// Data for the clipboard pub type ClipboardData = HashMap>; @@ -785,7 +787,9 @@ impl UserModel { Ok(()) } - /// Sets the border + /// Sets the border in an area of cells. + /// When setting the border we need to check if the adjacent cells have a "heavier" border + /// If that is the case we need to change it pub fn set_area_with_border( &mut self, range: &Area, @@ -835,9 +839,13 @@ impl UserModel { style.border.right = Some(border_area.item.clone()); } } - BorderType::Top => style.border.top = Some(border_area.item.clone()), + BorderType::Top => { + style.border.top = Some(border_area.item.clone()); + } BorderType::Right => style.border.right = Some(border_area.item.clone()), - BorderType::Bottom => style.border.bottom = Some(border_area.item.clone()), + BorderType::Bottom => { + style.border.bottom = Some(border_area.item.clone()); + } BorderType::Left => style.border.left = Some(border_area.item.clone()), BorderType::CenterH => { if row != range.row { @@ -873,122 +881,6 @@ 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(()) @@ -1096,7 +988,62 @@ impl UserModel { /// * [Model::get_style_for_cell] #[inline] pub fn get_cell_style(&self, sheet: u32, row: i32, column: i32) -> Result { - self.model.get_style_for_cell(sheet, row, column) + let mut style = self.model.get_style_for_cell(sheet, row, column)?; + + // We need to check if the adjacent cells have a "heavier" border + let border_top = if row > 1 { + self.model + .get_style_for_cell(sheet, row - 1, column)? + .border + .bottom + } else { + None + }; + + let border_right = if column < LAST_COLUMN { + self.model + .get_style_for_cell(sheet, row, column + 1)? + .border + .left + } else { + None + }; + + let border_bottom = if row < LAST_ROW { + self.model + .get_style_for_cell(sheet, row + 1, column)? + .border + .top + } else { + None + }; + + let border_left = if column > 1 { + self.model + .get_style_for_cell(sheet, row, column - 1)? + .border + .right + } else { + None + }; + + if is_max_border(&style.border.top, &border_top) { + style.border.top = border_top; + } + + if is_max_border(&style.border.right, &border_right) { + style.border.right = border_right; + } + + if is_max_border(&style.border.bottom, &border_bottom) { + style.border.bottom = border_bottom; + } + + if is_max_border(&style.border.left, &border_left) { + style.border.left = border_left; + } + + Ok(style) } /// Fills the cells from `source_area` until `to_row`. @@ -1176,42 +1123,6 @@ impl UserModel { let new_style = self.model.get_style_for_cell(sheet, source_row, column)?; self.model.set_cell_style(sheet, row, column, &new_style)?; - if column == column1 && column > 1 { - // Fix the right border of the cell to the left - let old_left_style = self.model.get_style_for_cell(sheet, row, column - 1)?; - if old_left_style.border.right != new_style.border.left { - let mut new_left_style = old_left_style.clone(); - new_left_style.border.right = new_style.border.left.clone(); - self.model - .set_cell_style(sheet, row, column - 1, &new_left_style)?; - // Add the diff - diff_list.push(Diff::SetCellStyle { - sheet, - row, - column: column - 1, - old_value: Box::new(old_left_style), - new_value: Box::new(new_left_style), - }); - } - } else if column == column1 + width1 - 1 && column < LAST_COLUMN { - // Fix the left border of the cell to the right - let old_right_style = self.model.get_style_for_cell(sheet, row, column + 1)?; - if old_right_style.border.left != new_style.border.right { - let mut new_right_style = old_right_style.clone(); - new_right_style.border.left = new_style.border.right.clone(); - self.model - .set_cell_style(sheet, row, column + 1, &new_right_style)?; - // Add the diff - diff_list.push(Diff::SetCellStyle { - sheet, - row, - column: column + 1, - old_value: Box::new(old_right_style), - new_value: Box::new(new_right_style), - }); - } - } - // Add the diffs diff_list.push(Diff::SetCellStyle { sheet, @@ -1310,43 +1221,6 @@ impl UserModel { .set_user_input(sheet, row, column, target_value.to_string())?; let new_style = self.model.get_style_for_cell(sheet, row, source_column)?; - - if row == first_row && row > 1 { - // Fix the lower border of the upper cell - let old_upper_style = self.model.get_style_for_cell(sheet, row - 1, column)?; - if old_upper_style.border.bottom != new_style.border.top { - let mut new_upper_style = old_upper_style.clone(); - new_upper_style.border.bottom = new_style.border.top.clone(); - self.model - .set_cell_style(sheet, row - 1, column, &new_upper_style)?; - // Add the diffs - diff_list.push(Diff::SetCellStyle { - sheet, - row: row - 1, - column, - old_value: Box::new(old_upper_style), - new_value: Box::new(new_upper_style), - }); - } - } else if row == last_row && row < LAST_ROW { - // Fix the upper border of the lower cell - let old_lower_style = self.model.get_style_for_cell(sheet, row + 1, column)?; - if old_lower_style.border.top != new_style.border.bottom { - let mut new_lower_style = old_lower_style.clone(); - new_lower_style.border.top = new_style.border.bottom.clone(); - self.model - .set_cell_style(sheet, row + 1, column, &new_lower_style)?; - // Add the diffs - diff_list.push(Diff::SetCellStyle { - sheet, - row: row + 1, - column, - old_value: Box::new(old_lower_style), - new_value: Box::new(new_lower_style), - }); - } - } - // Compute the new style and set it self.model.set_cell_style(sheet, row, column, &new_style)?; @@ -1359,6 +1233,7 @@ impl UserModel { old_value: Box::new(old_style), new_value: Box::new(new_style), }); + diff_list.push(Diff::SetCellValue { sheet, row, @@ -1509,76 +1384,6 @@ impl UserModel { .model .get_style_for_cell(sheet, target_row, target_column)?; - let new_style = value.style.clone(); - - // Fix borders - if target_row == source_first_row && target_row > 1 { - // Bottom border of the top cell - let old_top_style = - self.model - .get_style_for_cell(sheet, target_row - 1, target_column)?; - if new_style.border.top != old_top_style.border.bottom { - let mut new_top_style = old_top_style.clone(); - new_top_style.border.bottom = new_style.border.top.clone(); - diff_list.push(Diff::SetCellStyle { - sheet, - row: target_row - 1, - column: target_column, - old_value: Box::new(old_top_style), - new_value: Box::new(new_top_style), - }); - } - } else if target_row == source_last_row && target_row < LAST_ROW { - // Top border of the lower cell - let old_bottom_style = - self.model - .get_style_for_cell(sheet, target_row + 1, target_column)?; - if new_style.border.bottom != old_bottom_style.border.top { - let mut new_top_style = old_bottom_style.clone(); - new_top_style.border.top = new_style.border.bottom.clone(); - diff_list.push(Diff::SetCellStyle { - sheet, - row: target_row + 1, - column: target_column, - old_value: Box::new(old_bottom_style), - new_value: Box::new(new_top_style), - }); - } - } - if target_column == source_first_column && target_column > 1 { - // Right border of the cell to the left - let old_left_style = - self.model - .get_style_for_cell(sheet, target_row, target_column - 1)?; - if new_style.border.left != old_left_style.border.bottom { - let mut new_left_style = old_left_style.clone(); - new_left_style.border.right = new_style.border.left.clone(); - diff_list.push(Diff::SetCellStyle { - sheet, - row: target_row, - column: target_column - 1, - old_value: Box::new(old_left_style), - new_value: Box::new(new_left_style), - }); - } - } else if target_column == source_last_column && target_column < LAST_COLUMN { - // Left border of the cell to the right - let old_right_style = - self.model - .get_style_for_cell(sheet, target_row, target_column + 1)?; - if new_style.border.right != old_right_style.border.left { - let mut new_right_style = old_right_style.clone(); - new_right_style.border.left = new_style.border.right.clone(); - diff_list.push(Diff::SetCellStyle { - sheet, - row: target_row, - column: target_column + 1, - old_value: Box::new(old_right_style), - new_value: Box::new(new_right_style), - }); - } - } - self.model .set_user_input(sheet, target_row, target_column, new_value.clone())?; self.model @@ -1610,7 +1415,7 @@ impl UserModel { .worksheet(sheet)? .cell(row, column) .cloned(); - // TODO: also clear the styles and adjacent borders + diff_list.push(Diff::CellClearContents { sheet, row, diff --git a/base/src/user_model/mod.rs b/base/src/user_model/mod.rs index 66815c2..b5d0851 100644 --- a/base/src/user_model/mod.rs +++ b/base/src/user_model/mod.rs @@ -1,5 +1,6 @@ #![deny(missing_docs)] +mod border_utils; mod common; mod history; mod ui; diff --git a/webapp/src/components/useKeyboardNavigation.ts b/webapp/src/components/useKeyboardNavigation.ts index 47c46b3..478607d 100644 --- a/webapp/src/components/useKeyboardNavigation.ts +++ b/webapp/src/components/useKeyboardNavigation.ts @@ -108,11 +108,19 @@ const useKeyboardNavigation = ( break; } + case "a": { + // TODO: Area selection. CTRL+A should select "continuous" area around the selection, + // if it does exist then whole sheet is selected. + event.stopPropagation(); + event.preventDefault(); + break; + } // No default } if (isNavigationKey(key)) { // Ctrl+Arrows, Ctrl+Home/End options.onNavigationToEdge(key); + // navigate_to_edge_in_direction event.stopPropagation(); event.preventDefault(); }