From 74be62823d12d07fd766389397d87a0bc4cef74b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Hatcher?= <nicolas@theuniverse.today>
Date: Sat, 15 Feb 2025 10:35:39 +0100
Subject: [PATCH] FIX: Minor fixes and refactor

---
 base/src/user_model/border.rs | 101 ++++++++++++----------------------
 base/src/user_model/common.rs |  41 ++++++++------
 2 files changed, 58 insertions(+), 84 deletions(-)

diff --git a/base/src/user_model/border.rs b/base/src/user_model/border.rs
index f81ade1..d3d057a 100644
--- a/base/src/user_model/border.rs
+++ b/base/src/user_model/border.rs
@@ -1,7 +1,6 @@
 use crate::{
     constants::{LAST_COLUMN, LAST_ROW},
     expressions::types::Area,
-    types::Style,
 };
 
 use super::{
@@ -9,20 +8,21 @@ use super::{
 };
 
 impl UserModel {
-    fn get_new_border_value(
-        &self,
+    fn update_single_cell_border(
+        &mut self,
         border_area: &BorderArea,
-        old_value: &Option<Style>,
-        row: i32,
-        column: i32,
+        cell: (u32, i32, i32),
         range: (i32, i32, i32, i32),
-    ) -> Result<Style, String> {
-        let mut new_value = match old_value {
+        diff_list: &mut Vec<Diff>,
+    ) -> Result<(), String> {
+        let (sheet, row, column) = cell;
+        let (first_row, first_column, last_row, last_column) = range;
+
+        let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
+        let mut new_value = match &old_value {
             Some(value) => value.clone(),
             None => Default::default(),
         };
-        let (first_row, first_column, last_row, last_column) = range;
-
         match border_area.r#type {
             BorderType::All => {
                 new_value.border.top = Some(border_area.item.clone());
@@ -101,7 +101,15 @@ impl UserModel {
                 new_value.border.left = None;
             }
         }
-        Ok(new_value)
+        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(new_value),
+        });
+        Ok(())
     }
 
     fn set_rows_with_border(
@@ -188,23 +196,12 @@ impl UserModel {
                 .map(|row_data| row_data.keys().copied().collect())
                 .unwrap_or_default();
             for column in columns {
-                let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
-                let new_value = self.get_new_border_value(
+                self.update_single_cell_border(
                     border_area,
-                    &old_value,
-                    row,
-                    column,
+                    (sheet, row, column),
                     (first_row, 1, last_row, LAST_COLUMN),
+                    &mut diff_list,
                 )?;
-
-                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(new_value),
-                });
             }
 
             self.model.set_row_style(sheet, row, &new_value)?;
@@ -215,6 +212,7 @@ impl UserModel {
                 new_value: Box::new(new_value),
             });
         }
+        // TODO: We need to check the rows above and below. also any non empty cell in the rows above and below.
         self.push_diff_list(diff_list);
         Ok(())
     }
@@ -307,23 +305,12 @@ impl UserModel {
             for &row in &data_rows {
                 if let Some(data_row) = self.model.workbook.worksheet(sheet)?.sheet_data.get(&row) {
                     if data_row.get(&column).is_some() {
-                        let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
-                        let new_value = self.get_new_border_value(
+                        self.update_single_cell_border(
                             border_area,
-                            &old_value,
-                            row,
-                            column,
+                            (sheet, row, column),
                             (1, first_column, LAST_ROW, last_column),
+                            &mut diff_list,
                         )?;
-
-                        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(new_value),
-                        });
                     }
                 }
             }
@@ -331,23 +318,12 @@ impl UserModel {
             // We also need to overwrite those that have a row style
             for row_s in styled_rows.iter() {
                 let row = row_s.r;
-                let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
-                let new_value = self.get_new_border_value(
+                self.update_single_cell_border(
                     border_area,
-                    &old_value,
-                    row,
-                    column,
+                    (sheet, row, column),
                     (1, first_column, LAST_ROW, last_column),
+                    &mut diff_list,
                 )?;
-
-                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(new_value),
-                });
             }
 
             self.model.set_column_style(sheet, column, &new_value)?;
@@ -358,6 +334,8 @@ impl UserModel {
                 new_value: Box::new(new_value),
             });
         }
+        // We need to check the borders of the column to the left and the column to the right
+        // We also need to check every non-empty cell in the columns to the left and right
         self.push_diff_list(diff_list);
         Ok(())
     }
@@ -388,23 +366,12 @@ impl UserModel {
         let mut diff_list = Vec::new();
         for row in first_row..=last_row {
             for column in first_column..=last_column {
-                let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
-                let new_value = self.get_new_border_value(
+                self.update_single_cell_border(
                     border_area,
-                    &old_value,
-                    row,
-                    column,
+                    (sheet, row, column),
                     (first_row, first_column, last_row, last_column),
+                    &mut diff_list,
                 )?;
-
-                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(new_value),
-                });
             }
         }
 
diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs
index 936ab1a..a98b064 100644
--- a/base/src/user_model/common.rs
+++ b/base/src/user_model/common.rs
@@ -1081,7 +1081,7 @@ impl UserModel {
         Ok(())
     }
 
-    // Updates the style of a cell
+    // Updates the style of a cell, adding the new style to the diff list
     fn update_single_cell_style(
         &mut self,
         sheet: u32,
@@ -1093,9 +1093,10 @@ impl UserModel {
     ) -> Result<(), String> {
         // This is the value in the cell itself
         let old_value = self.model.get_cell_style_or_none(sheet, row, column)?;
-        // This takes into account row or column styles
-        let style = self.get_cell_style(sheet, row, column)?;
-        let new_style = update_style(&style, style_path, value)?;
+
+        // This takes into account row or column styles. If none of those are present, it will return the default style
+        let old_style = self.get_cell_style(sheet, row, column)?;
+        let new_style = update_style(&old_style, style_path, value)?;
         self.model.set_cell_style(sheet, row, column, &new_style)?;
         diff_list.push(Diff::SetCellStyle {
             sheet,
@@ -1118,19 +1119,19 @@ impl UserModel {
     ) -> Result<(), String> {
         let sheet = range.sheet;
         let mut diff_list = Vec::new();
-        // We need all the rows in the column to update the style
-        // NB: This is too much, this is all the rows that have values
-        let data_rows: Vec<i32> = self
-            .model
-            .workbook
-            .worksheet(sheet)?
-            .sheet_data
-            .keys()
-            .copied()
-            .collect();
-        let styled_rows = &self.model.workbook.worksheet(sheet)?.rows.clone();
         if range.row == 1 && range.height == LAST_ROW {
             // Full columns
+            let styled_rows = &self.model.workbook.worksheet(sheet)?.rows.clone();
+            // We need all the rows in the column to update the style
+            // NB: This is too much, this is all the rows that have values
+            let data_rows: Vec<i32> = self
+                .model
+                .workbook
+                .worksheet(sheet)?
+                .sheet_data
+                .keys()
+                .copied()
+                .collect();
             for column in range.column..range.column + range.width {
                 // we set the style of the full column
                 let old_style = self.model.get_column_style(sheet, column)?;
@@ -1154,6 +1155,7 @@ impl UserModel {
                         self.model.workbook.worksheet(sheet)?.sheet_data.get(&row)
                     {
                         if data_row.get(&column).is_some() {
+                            // If the cell has non empty content it will always have some style
                             self.update_single_cell_style(
                                 sheet,
                                 row,
@@ -1167,9 +1169,14 @@ impl UserModel {
                 }
                 // We need to update the styles in all cells that have a row style
                 for row_s in styled_rows.iter() {
+                    let row = row_s.r;
+                    if data_rows.contains(&row) {
+                        // Skip if the row has data
+                        continue;
+                    }
                     self.update_single_cell_style(
                         sheet,
-                        row_s.r,
+                        row,
                         column,
                         style_path,
                         value,
@@ -1201,7 +1208,7 @@ impl UserModel {
                     )?;
                 }
 
-                // We need to go through all the cells that have a column style and merge them
+                // We need to go through all the cells that have a column style and merge the styles
                 for col in styled_columns.iter() {
                     for column in col.min..col.max + 1 {
                         self.update_single_cell_style(