From b4349ff5daf92874f100e6ad6b1f3202713b0944 Mon Sep 17 00:00:00 2001 From: Brian Hung Date: Tue, 22 Jul 2025 17:25:34 -0700 Subject: [PATCH] fix diff generation and add test coverage --- base/src/test/user_model/mod.rs | 1 + .../user_model/test_batch_row_column_diff.rs | 154 ++++++++++++++++++ base/src/user_model/common.rs | 66 ++++++-- base/src/user_model/mod.rs | 2 +- 4 files changed, 206 insertions(+), 17 deletions(-) create mode 100644 base/src/test/user_model/test_batch_row_column_diff.rs diff --git a/base/src/test/user_model/mod.rs b/base/src/test/user_model/mod.rs index 658c1bd..89d3464 100644 --- a/base/src/test/user_model/mod.rs +++ b/base/src/test/user_model/mod.rs @@ -1,6 +1,7 @@ mod test_add_delete_sheets; mod test_autofill_columns; mod test_autofill_rows; +mod test_batch_row_column_diff; mod test_border; mod test_clear_cells; mod test_column_style; diff --git a/base/src/test/user_model/test_batch_row_column_diff.rs b/base/src/test/user_model/test_batch_row_column_diff.rs new file mode 100644 index 0000000..158954c --- /dev/null +++ b/base/src/test/user_model/test_batch_row_column_diff.rs @@ -0,0 +1,154 @@ +#![allow(clippy::unwrap_used)] + +use bitcode::decode; + +use crate::{ + test::util::new_empty_model, + user_model::history::{Diff, QueueDiffs}, + UserModel, +}; + +fn last_diff_list(model: &mut UserModel) -> Vec { + let bytes = model.flush_send_queue(); + let queue: Vec = decode(&bytes).unwrap(); + // Get the last operation's diff list + queue.last().unwrap().list.clone() +} + +#[test] +fn diff_invariant_insert_rows() { + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + assert!(model.insert_rows(0, 5, 3).is_ok()); + + let list = last_diff_list(&mut model); + assert_eq!(list.len(), 3); + for diff in list { + match diff { + Diff::InsertRow { sheet, row } => { + assert_eq!(sheet, 0); + assert_eq!(row, 5); + } + _ => panic!("Unexpected diff variant"), + } + } +} + +#[test] +fn diff_invariant_insert_columns() { + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + assert!(model.insert_columns(0, 2, 4).is_ok()); + let list = last_diff_list(&mut model); + assert_eq!(list.len(), 4); + for diff in list { + match diff { + Diff::InsertColumn { sheet, column } => { + assert_eq!(sheet, 0); + assert_eq!(column, 2); + } + _ => panic!("Unexpected diff variant"), + } + } +} + +#[test] +fn undo_redo_after_batch_delete() { + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + // Place values that will shift. + model.set_user_input(0, 20, 1, "A").unwrap(); + model.set_user_input(0, 1, 20, "B").unwrap(); + + // Fill the rows we are about to delete so the diff list can be produced + for r in 10..15 { + model.set_user_input(0, r, 1, "tmp").unwrap(); + } + + // Delete rows 10..14 and columns 5..8 + assert!(model.delete_rows(0, 10, 5).is_ok()); + assert!(model.delete_columns(0, 5, 4).is_ok()); + + // Verify shift + assert_eq!(model.get_formatted_cell_value(0, 15, 1).unwrap(), "A"); + assert_eq!(model.get_formatted_cell_value(0, 1, 16).unwrap(), "B"); + + // Undo + model.undo().unwrap(); // columns + model.undo().unwrap(); // rows + assert_eq!(model.get_formatted_cell_value(0, 20, 1).unwrap(), "A"); + assert_eq!(model.get_formatted_cell_value(0, 1, 20).unwrap(), "B"); + + // Redo + model.redo().unwrap(); // rows + model.redo().unwrap(); // columns + assert_eq!(model.get_formatted_cell_value(0, 15, 1).unwrap(), "A"); + assert_eq!(model.get_formatted_cell_value(0, 1, 16).unwrap(), "B"); +} + +#[test] +fn diff_order_delete_rows() { + // Verifies that delete diffs are generated bottom-to-top + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + // Populate rows to delete + for r in 5..10 { + model.set_user_input(0, r, 1, &r.to_string()).unwrap(); + } + + assert!(model.delete_rows(0, 5, 5).is_ok()); + + let list = last_diff_list(&mut model); + assert_eq!(list.len(), 5); + + // Diffs should be in reverse order: 9, 8, 7, 6, 5 + let mut expected_row = 9; + for diff in list { + match diff { + Diff::DeleteRow { row, .. } => { + assert_eq!(row, expected_row); + expected_row -= 1; + } + _ => panic!("Unexpected diff variant"), + } + } +} + +#[test] +fn batch_operations_with_formulas() { + // Verifies formulas update correctly after batch ops + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + model.set_user_input(0, 1, 1, "10").unwrap(); + model.set_user_input(0, 5, 1, "=A1*2").unwrap(); // Will become A3 after insert + + assert!(model.insert_rows(0, 2, 2).is_ok()); + + // Formula should now reference A1 (unchanged) but be in row 7 + assert_eq!(model.get_formatted_cell_value(0, 7, 1).unwrap(), "20"); + assert_eq!(model.get_cell_content(0, 7, 1).unwrap(), "=A1*2"); + + // Undo and verify formula is back at original position + model.undo().unwrap(); + assert_eq!(model.get_formatted_cell_value(0, 5, 1).unwrap(), "20"); +} + +#[test] +fn edge_case_single_operation() { + // Single row/column operations should still work correctly + let base = new_empty_model(); + let mut model = UserModel::from_model(base); + + assert!(model.insert_rows(0, 1, 1).is_ok()); + let list = last_diff_list(&mut model); + assert_eq!(list.len(), 1); + + assert!(model.insert_columns(0, 1, 1).is_ok()); + let list = last_diff_list(&mut model); + assert_eq!(list.len(), 1); +} diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs index 0ab2bdd..b19ae5b 100644 --- a/base/src/user_model/common.rs +++ b/base/src/user_model/common.rs @@ -858,14 +858,23 @@ impl UserModel { Ok(()) } - /// Inserts several rows at once + /// Inserts `row_count` blank rows starting at `row` (both 0-based). + /// + /// Parameters + /// * `sheet` – worksheet index. + /// * `row` – first row to insert. + /// * `row_count` – number of rows (> 0). + /// + /// History: the method pushes `row_count` [`crate::user_model::history::Diff::InsertRow`] + /// items **all using the same `row` index**. Replaying those diffs (undo / redo) + /// is therefore immune to the row-shifts that happen after each individual + /// insertion. + /// + /// See also [`Model::insert_rows`]. pub fn insert_rows(&mut self, sheet: u32, row: i32, row_count: i32) -> Result<(), String> { let mut diff_list = Vec::new(); - for r in 0..row_count { - diff_list.push(Diff::InsertRow { - sheet, - row: row + r, - }); + for _ in 0..row_count { + diff_list.push(Diff::InsertRow { sheet, row }); } self.push_diff_list(diff_list); self.model.insert_rows(sheet, row, row_count)?; @@ -873,7 +882,18 @@ impl UserModel { Ok(()) } - /// Inserts several columns at once + /// Inserts `column_count` blank columns starting at `column` (0-based). + /// + /// Parameters + /// * `sheet` – worksheet index. + /// * `column` – first column to insert. + /// * `column_count` – number of columns (> 0). + /// + /// History: pushes one [`crate::user_model::history::Diff::InsertColumn`] + /// per inserted column, all with the same `column` value, preventing index + /// drift when the diffs are reapplied. + /// + /// See also [`Model::insert_columns`]. pub fn insert_columns( &mut self, sheet: u32, @@ -881,11 +901,8 @@ impl UserModel { column_count: i32, ) -> Result<(), String> { let mut diff_list = Vec::new(); - for c in 0..column_count { - diff_list.push(Diff::InsertColumn { - sheet, - column: column + c, - }); + for _ in 0..column_count { + diff_list.push(Diff::InsertColumn { sheet, column }); } self.push_diff_list(diff_list); self.model.insert_columns(sheet, column, column_count)?; @@ -893,12 +910,20 @@ impl UserModel { Ok(()) } - /// Deletes several rows at once + /// Deletes `row_count` rows starting at `row`. + /// + /// History: a [`crate::user_model::history::Diff::DeleteRow`] is created for + /// each row, ordered **bottom → top**. Undo therefore recreates rows from + /// top → bottom and redo removes them bottom → top, avoiding index drift. + /// + /// See also [`Model::delete_rows`]. pub fn delete_rows(&mut self, sheet: u32, row: i32, row_count: i32) -> Result<(), String> { let diff_list = { let worksheet = self.model.workbook.worksheet(sheet)?; let mut diff_list = Vec::new(); - for r in row..row + row_count { + // Collect diffs from bottom to top so that `undo` re-inserts rows + // in the correct order (top to bottom). + for r in (row..row + row_count).rev() { let mut row_data = None; for rd in &worksheet.rows { if rd.r == r { @@ -927,7 +952,13 @@ impl UserModel { Ok(()) } - /// Deletes several columns at once + /// Deletes `column_count` columns starting at `column`. + /// + /// History: pushes one [`crate::user_model::history::Diff::DeleteColumn`] + /// per column, **right → left**, so replaying the list is always safe with + /// respect to index shifts. + /// + /// See also [`Model::delete_columns`]. pub fn delete_columns( &mut self, sheet: u32, @@ -937,7 +968,8 @@ impl UserModel { let diff_list = { let worksheet = self.model.workbook.worksheet(sheet)?; let mut diff_list = Vec::new(); - for c in column..column + column_count { + // Iterate from right-to-left so `undo` restores from left-to-right. + for c in (column..column + column_count).rev() { if !is_valid_column_number(c) { return Err(format!("Column number '{c}' is not valid.")); } @@ -2497,4 +2529,6 @@ mod tests { assert_eq!(horizontal(&format!("{a}")), Ok(a)); } } + + // (batch diff tests moved to separate file) } diff --git a/base/src/user_model/mod.rs b/base/src/user_model/mod.rs index 06cd85c..5d44a46 100644 --- a/base/src/user_model/mod.rs +++ b/base/src/user_model/mod.rs @@ -3,7 +3,7 @@ mod border; mod border_utils; mod common; -mod history; +pub(crate) mod history; mod ui; pub use common::UserModel;