From 411c5de59bcead2dfa4542e33b1ab2bb736a4a58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Hatcher?= Date: Sat, 23 Nov 2024 20:02:57 +0100 Subject: [PATCH] FIX: Another round a borders Only test are missing(!) --- base/src/test/user_model/test_border.rs | 138 ++++++++++- base/src/user_model/border_utils.rs | 48 ++-- base/src/user_model/common.rs | 299 +++++++++++++++++++++--- 3 files changed, 415 insertions(+), 70 deletions(-) diff --git a/base/src/test/user_model/test_border.rs b/base/src/test/user_model/test_border.rs index 6814b90..7ea7d64 100644 --- a/base/src/test/user_model/test_border.rs +++ b/base/src/test/user_model/test_border.rs @@ -639,19 +639,29 @@ 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 { + for column in 6..10 { let style = model.get_cell_style(0, row, column).unwrap(); let border_item = BorderItem { style: BorderStyle::Thin, color: Some("#FF5566".to_string()), }; + let left = if column == 6 { + None + } else { + Some(border_item.clone()) + }; + let right = if column == 9 { + None + } else { + Some(border_item.clone()) + }; let expected_border = Border { diagonal_up: false, diagonal_down: false, - left: None, - right: Some(border_item.clone()), + left, + right, top: None, bottom: None, diagonal: None, @@ -739,19 +749,29 @@ 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 { + for column 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 left = if column == 5 { + None + } else { + Some(border_item.clone()) + }; + let right = if column == 8 { + None + } else { + Some(border_item.clone()) + }; let expected_border = Border { diagonal_up: false, diagonal_down: false, - left: Some(border_item.clone()), - right: None, + left, + right, top: None, bottom: None, diagonal: None, @@ -760,3 +780,105 @@ fn borders_left() { } } } + +#[test] +fn heavier_side() { + let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); + // We set an outer border in cells F5: + let range = &Area { + sheet: 0, + row: 5, + column: 6, + width: 1, + height: 1, + }; + let border_area: BorderArea = serde_json::from_str( + r##"{ + "item": { + "style": "thin", + "color": "#FF5566" + }, + "type": "All" + }"##, + ) + .unwrap(); + model.set_area_with_border(range, &border_area).unwrap(); + + // Get adjacent cells + { + // F4 + let style = model.get_cell_style(0, 4, 6).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), + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + { + // G5 + let style = model.get_cell_style(0, 5, 7).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), + right: None, + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + { + // F6 + let style = model.get_cell_style(0, 6, 6).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), + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } + { + // E5 + let style = model.get_cell_style(0, 5, 5).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), + top: None, + bottom: None, + diagonal: None, + }; + assert_eq!(style.border, expected_border); + } +} diff --git a/base/src/user_model/border_utils.rs b/base/src/user_model/border_utils.rs index 09ec666..d9ce28d 100644 --- a/base/src/user_model/border_utils.rs +++ b/base/src/user_model/border_utils.rs @@ -47,7 +47,8 @@ fn is_max_color(a: &str, b: &str) -> bool { luminance_b < luminance_a } -pub(crate) fn is_max_border(a: &Option, b: &Option) -> bool { +/// Is border b "heavier" than a? +pub(crate) fn is_max_border(a: Option<&BorderItem>, b: Option<&BorderItem>) -> bool { match (a, b) { (_, None) => false, (None, Some(_)) => true, @@ -69,9 +70,23 @@ pub(crate) fn is_max_border(a: &Option, b: &Option) -> b #[cfg(test)] mod tests { use super::*; + use crate::types::BorderStyle; #[test] - fn test_basic_colors() { + fn compare_borders() { + let b = BorderItem { + style: BorderStyle::Thin, + color: Some("#FFF".to_string()), + }; + // Some border *always* beats no border + assert!(is_max_border(None, Some(&b))); + + // No border is beaten by some border + assert!(!is_max_border(Some(&b), None)); + } + + #[test] + fn basic_colors() { // Black vs White assert!(is_max_color("#FFFFFF", "#000000")); assert!(!is_max_color("#000000", "#FFFFFF")); @@ -90,34 +105,13 @@ mod tests { } #[test] - fn test_same_color() { + fn 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() { + fn edge_cases() { // Colors with minimal luminance difference assert!(!is_max_color("#000000", "#010101")); assert!(!is_max_color("#FEFEFE", "#FFFFFF")); @@ -125,7 +119,7 @@ mod tests { } #[test] - fn test_luminance_ordering() { + fn 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 @@ -133,7 +127,7 @@ mod tests { } #[test] - fn test_borderline_cases() { + fn borderline_cases() { // Testing colors with equal luminance assert!(!is_max_color("#777777", "#777777")); diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs index bd012b2..7a3a69b 100644 --- a/base/src/user_model/common.rs +++ b/base/src/user_model/common.rs @@ -802,86 +802,312 @@ impl UserModel { for row in range.row..=last_row { 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(); + let mut new_value = old_value.clone(); match border_area.r#type { BorderType::All => { - style.border.top = Some(border_area.item.clone()); - style.border.right = Some(border_area.item.clone()); - style.border.bottom = Some(border_area.item.clone()); - style.border.left = Some(border_area.item.clone()); + new_value.border.top = Some(border_area.item.clone()); + new_value.border.right = Some(border_area.item.clone()); + new_value.border.bottom = Some(border_area.item.clone()); + new_value.border.left = Some(border_area.item.clone()); } BorderType::Inner => { if row != range.row { - style.border.top = Some(border_area.item.clone()); + new_value.border.top = Some(border_area.item.clone()); } if row != last_row { - style.border.bottom = Some(border_area.item.clone()); + new_value.border.bottom = Some(border_area.item.clone()); } if column != range.column { - style.border.left = Some(border_area.item.clone()); + new_value.border.left = Some(border_area.item.clone()); } if column != last_column { - style.border.right = Some(border_area.item.clone()); + new_value.border.right = Some(border_area.item.clone()); } } BorderType::Outer => { if row == range.row { - style.border.top = Some(border_area.item.clone()); + new_value.border.top = Some(border_area.item.clone()); } if row == last_row { - style.border.bottom = Some(border_area.item.clone()); + new_value.border.bottom = Some(border_area.item.clone()); } if column == range.column { - style.border.left = Some(border_area.item.clone()); + new_value.border.left = Some(border_area.item.clone()); } if column == last_column { - style.border.right = Some(border_area.item.clone()); + new_value.border.right = Some(border_area.item.clone()); } } BorderType::Top => { - style.border.top = Some(border_area.item.clone()); + new_value.border.top = Some(border_area.item.clone()); + // Check if the cell above has a "heavier" bottom border + if row > 1 { + let old_value_cell_above = + self.model.get_style_for_cell(sheet, row - 1, column)?; + if is_max_border( + Some(&border_area.item), + old_value_cell_above.border.bottom.as_ref(), + ) { + let mut new_value_cell_above = old_value_cell_above.clone(); + if border_area.r#type == BorderType::None { + new_value_cell_above.border.bottom = None; + } else { + new_value_cell_above.border.bottom = + Some(border_area.item.clone()); + } + self.model.set_cell_style( + sheet, + row - 1, + column, + &new_value_cell_above, + )?; + diff_list.push(Diff::SetCellStyle { + sheet, + row: row - 1, + column, + old_value: Box::new(old_value_cell_above), + new_value: Box::new(new_value_cell_above), + }); + } + } + } + BorderType::Right => { + new_value.border.right = Some(border_area.item.clone()); + // Check if the cell to the right has a "heavier" left border + if column < LAST_COLUMN { + let old_value_cell_right = + self.model.get_style_for_cell(sheet, row, column + 1)?; + if is_max_border( + Some(&border_area.item), + old_value_cell_right.border.left.as_ref(), + ) { + let mut new_value_cell_right = old_value_cell_right.clone(); + if border_area.r#type == BorderType::None { + new_value_cell_right.border.left = None; + } else { + new_value_cell_right.border.left = + Some(border_area.item.clone()); + } + self.model.set_cell_style( + sheet, + row, + column + 1, + &new_value_cell_right, + )?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column: column + 1, + old_value: Box::new(old_value_cell_right), + new_value: Box::new(new_value_cell_right), + }); + } + } } - BorderType::Right => style.border.right = Some(border_area.item.clone()), BorderType::Bottom => { - style.border.bottom = Some(border_area.item.clone()); + new_value.border.bottom = Some(border_area.item.clone()); + // Check if the cell bellow has a "heavier" top border + if row < LAST_ROW { + let old_value_cell_below = + self.model.get_style_for_cell(sheet, row + 1, column)?; + if is_max_border( + Some(&border_area.item), + old_value_cell_below.border.top.as_ref(), + ) { + let mut new_value_cell_below = old_value_cell_below.clone(); + if border_area.r#type == BorderType::None { + new_value_cell_below.border.top = None; + } else { + new_value_cell_below.border.top = + Some(border_area.item.clone()); + } + self.model.set_cell_style( + sheet, + row + 1, + column, + &new_value_cell_below, + )?; + diff_list.push(Diff::SetCellStyle { + sheet, + row: row + 1, + column, + old_value: Box::new(old_value_cell_below), + new_value: Box::new(new_value_cell_below), + }); + } + } + } + BorderType::Left => { + new_value.border.left = Some(border_area.item.clone()); + // Check if the cell to the left has a "heavier" right border + if column > 1 { + let old_value_cell_left = + self.model.get_style_for_cell(sheet, row, column - 1)?; + if is_max_border( + Some(&border_area.item), + old_value_cell_left.border.right.as_ref(), + ) { + let mut new_value_cell_left = old_value_cell_left.clone(); + if border_area.r#type == BorderType::None { + new_value_cell_left.border.right = None; + } else { + new_value_cell_left.border.right = + Some(border_area.item.clone()); + } + self.model.set_cell_style( + sheet, + row, + column - 1, + &new_value_cell_left, + )?; + diff_list.push(Diff::SetCellStyle { + sheet, + row, + column: column - 1, + old_value: Box::new(old_value_cell_left), + new_value: Box::new(new_value_cell_left), + }); + } + } } - BorderType::Left => style.border.left = Some(border_area.item.clone()), BorderType::CenterH => { if row != range.row { - style.border.top = Some(border_area.item.clone()); + new_value.border.top = Some(border_area.item.clone()); } if row != last_row { - style.border.bottom = Some(border_area.item.clone()); + new_value.border.bottom = Some(border_area.item.clone()); } } BorderType::CenterV => { if column != range.column { - style.border.left = Some(border_area.item.clone()); + new_value.border.left = Some(border_area.item.clone()); } if column != last_column { - style.border.right = Some(border_area.item.clone()); + new_value.border.right = Some(border_area.item.clone()); } } BorderType::None => { - style.border.top = None; - style.border.right = None; - style.border.bottom = None; - style.border.left = None; + new_value.border.top = None; + new_value.border.right = None; + new_value.border.bottom = None; + new_value.border.left = None; } } - self.model.set_cell_style(sheet, row, column, &style)?; + self.model.set_cell_style(sheet, row, column, &new_value)?; diff_list.push(Diff::SetCellStyle { sheet, row, column, old_value: Box::new(old_value), - new_value: Box::new(style), + new_value: Box::new(new_value), }); } } + // bottom of the cells above the first + if range.row > 1 + && [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)?; + if is_max_border(Some(&border_area.item), old_value.border.bottom.as_ref()) { + 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::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)?; + // If the border in the adjacent cell is "heavier" we change it + if is_max_border(Some(&border_area.item), old_value.border.left.as_ref()) { + 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::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)?; + if is_max_border(Some(&border_area.item), old_value.border.top.as_ref()) { + 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::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)?; + if is_max_border(Some(&border_area.item), old_value.border.right.as_ref()) { + 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(()) } @@ -984,13 +1210,16 @@ impl UserModel { /// Returns the style for a cell /// + /// Cells share a border, so the left border of B1 is the right border of A1 + /// In the object structure the borders of the cells might be difference, + /// We always pick the "heaviest" border. + /// /// See also: /// * [Model::get_style_for_cell] - #[inline] pub fn get_cell_style(&self, sheet: u32, row: i32, column: i32) -> Result { let mut style = self.model.get_style_for_cell(sheet, row, column)?; - // We need to check if the adjacent cells have a "heavier" border + // 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)? @@ -1027,19 +1256,19 @@ impl UserModel { None }; - if is_max_border(&style.border.top, &border_top) { + if is_max_border(style.border.top.as_ref(), border_top.as_ref()) { style.border.top = border_top; } - if is_max_border(&style.border.right, &border_right) { + if is_max_border(style.border.right.as_ref(), border_right.as_ref()) { style.border.right = border_right; } - if is_max_border(&style.border.bottom, &border_bottom) { + if is_max_border(style.border.bottom.as_ref(), border_bottom.as_ref()) { style.border.bottom = border_bottom; } - if is_max_border(&style.border.left, &border_left) { + if is_max_border(style.border.left.as_ref(), border_left.as_ref()) { style.border.left = border_left; } @@ -1291,7 +1520,7 @@ impl UserModel { for column in column_start..=column_end { let text = self.get_formatted_cell_value(sheet, row, column)?; let content = self.get_cell_content(sheet, row, column)?; - let style = self.get_cell_style(sheet, row, column)?; + let style = self.model.get_style_for_cell(sheet, row, column)?; data_row.insert( column, ClipboardCell { @@ -1505,7 +1734,7 @@ impl UserModel { fn apply_undo_diff_list(&mut self, diff_list: &DiffList) -> Result<(), String> { let mut needs_evaluation = false; - for diff in diff_list { + for diff in diff_list.iter().rev() { match diff { Diff::SetCellValue { sheet,