diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs index 460d0ef..654d220 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs @@ -31,16 +31,3 @@ pub use sheet::Sheet; pub use style::{BorderEdge, BorderTarget, CellAlign, CellStyle, Color, NumberFormat}; pub use undo::{Change, ChangeSet}; pub use workbook_api::{Workbook, WorkbookCommand}; - -#[cfg(test)] -pub(crate) mod test_disk { - use std::sync::{Mutex, MutexGuard}; - - static LOCK: Mutex<()> = Mutex::new(()); - - /// Serialize tests that touch the shared `generated/` save files, so - /// parallel `cargo test` workers cannot clobber each other's fixtures. - pub(crate) fn lock() -> MutexGuard<'static, ()> { - LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) - } -} diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs index 7b9946a..a2ed84e 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs @@ -197,7 +197,6 @@ mod tests { /// clobbering another test's fixture. #[test] fn the_legacy_file_is_read_when_the_current_one_is_missing() { - let _disk = crate::test_disk::lock(); let current = sheet_generated_file_path(); let legacy = sheet_generated_legacy_file_path(); let saved_current = fs::read_to_string(¤t).ok(); diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs index 10124b2..044cf5b 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs @@ -368,46 +368,4 @@ mod tests { // =AZ10 with 0,+1 → =BA10 assert_eq!(adjust_formula_refs("=AZ10", 0, 1), "=BA10"); } - - // --- write_col_letters (no-alloc variant used by the grid draw loop) --- - - #[test] - fn write_col_letters_matches_col_letters() { - for i in [0u32, 1, 25, 26, 27, 51, 52, 701, 702, 18277] { - let mut buf = String::from("leftover"); - write_col_letters(i, &mut buf); - assert_eq!(buf, col_letters(i), "write_col_letters({i}) diverged"); - } - // The buffer is cleared on entry, so a shorter label leaves no - // stale tail from the previous call. - let mut buf = String::new(); - write_col_letters(702, &mut buf); // "AAA" - write_col_letters(0, &mut buf); // "A" - assert_eq!(buf, "A"); - } - - // --- adjust_formula_refs: digit-bearing names and overflow guards --- - - #[test] - fn adjust_refs_function_name_with_digits() { - // LOG10 has digits in its identifier; it must not be treated as a - // cell reference. - assert_eq!(adjust_formula_refs("=LOG10(100)", 1, 1), "=LOG10(100)"); - } - - #[test] - fn adjust_refs_overflowing_column_is_left_alone() { - // An 8-letter column exceeds u32 in `col_str_to_index`, so the - // scanner hits the overflow fallback. With a zero delta the output - // is verbatim, proving the fallback copies rather than mangles. - assert_eq!(adjust_formula_refs("=AAAAAAAA1", 0, 0), "=AAAAAAAA1"); - } - - #[test] - fn adjust_refs_overflowing_row_is_left_alone() { - // A 30-digit row does not fit i64, so the scanner copies it verbatim. - let row = "9".repeat(30); - let formula = format!("=A{}", row); - assert_eq!(adjust_formula_refs(&formula, 1, 1), formula); - } } diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs index f6fd8a9..0cb4a68 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs @@ -144,15 +144,18 @@ impl Workbook { } /// Load from the default save path (`generated/current.workbook.tsv`). - /// Falls back to the legacy single-sheet format if the workbook file + /// Falls back to legacy single-sheet format if the workbook file /// doesn't exist. pub fn load() -> Option { - // `deserialize_workbook` already migrates the legacy V1 formats - // (single-sheet `#MP_SHEET_V2` and the older CSV) into a one-sheet - // workbook, so there is no separate legacy branch to take here. let saved = load_saved_spreadsheet_state()?; - let (sheets, active) = deserialize_workbook(&saved)?; - Some(Self { sheets, active }) + if let Some((sheets, active)) = deserialize_workbook(&saved) { + return Some(Self { sheets, active }); + } + if is_legacy_workbook_payload(&saved) { + Some(migrate_legacy_workbook(&saved)) + } else { + None + } } /// Save to the default save path. @@ -492,6 +495,22 @@ impl Workbook { } } +fn is_legacy_workbook_payload(serialized: &str) -> bool { + let first = serialized.lines().next().unwrap_or_default().trim(); + first == "#MP_SHEET_V2" || first.contains(',') +} + +fn migrate_legacy_workbook(serialized: &str) -> Workbook { + let mut data = SpreadsheetData::default(); + data.deserialize(serialized); + data.undo_stack.clear(); + data.redo_stack.clear(); + Workbook { + sheets: vec![Sheet::new("Sheet 1", data)], + active: 0, + } +} + impl Default for Workbook { fn default() -> Self { Self::new() @@ -673,6 +692,23 @@ mod tests { assert_eq!(wb.get_raw(0, 0), "test"); } + #[test] + fn legacy_payload_detection_rejects_arbitrary_text() { + assert!(is_legacy_workbook_payload("#MP_SHEET_V2\n")); + assert!(is_legacy_workbook_payload("0,0,legacy\n")); + assert!(!is_legacy_workbook_payload("not a workbook")); + } + + #[test] + fn legacy_payload_migrates_to_one_sheet() { + let mut data = SpreadsheetData::default(); + data.set_cell(0, 0, "legacy"); + let migrated = migrate_legacy_workbook(&data.serialize()); + assert_eq!(migrated.sheet_count(), 1); + assert_eq!(migrated.get_raw(0, 0), "legacy"); + assert_eq!(migrated.active_sheet(), 0); + } + #[test] fn workbook_try_deserialize_reports_invalid_input() { let mut wb = Workbook::new(); @@ -937,190 +973,4 @@ mod tests { wb.set_active_sheet(1); assert_eq!(wb.get_display_value(0, 0), "20"); } - - // ── Previously-uncovered branches ─────────────────────────────────── - - /// An empty sheet list cannot build a workbook, so `with_sheets_active` - /// falls back to a fresh single-sheet workbook. - #[test] - fn with_sheets_active_empty_defaults_to_one_sheet() { - let wb = Workbook::with_sheets_active(vec![], 7); - assert_eq!(wb.sheet_count(), 1); - assert_eq!(wb.active_sheet(), 0); - } - - /// Setting an out-of-range active index is a no-op, not a panic. - #[test] - fn set_active_sheet_out_of_range_is_noop() { - let mut wb = Workbook::new(); - wb.set_active_sheet(99); - assert_eq!(wb.active_sheet(), 0); - } - - /// Removing a sheet *before* the active one shifts the active index - /// down so it keeps pointing at the same sheet. - #[test] - fn remove_sheet_before_active_shifts_active_down() { - let mut wb = Workbook::new(); - wb.add_sheet("B"); - wb.add_sheet("C"); - wb.add_sheet("D"); // ["Sheet 1", "B", "C", "D"] - wb.set_active_sheet(2); // C - let removed = wb.remove_sheet(0); // remove "Sheet 1" (before C) - assert!(removed.is_some()); - assert_eq!(wb.sheet_count(), 3); - assert_eq!(wb.active_sheet(), 1); // shifted down, still C - assert_eq!(wb.active_sheet_name(), "C"); - } - - /// Removing the active sheet (or one after it) clamps the active index - /// to the new last sheet. - #[test] - fn remove_sheet_at_or_after_active_clamps_to_end() { - let mut wb = Workbook::new(); - wb.add_sheet("B"); - wb.add_sheet("C"); // ["Sheet 1", "B", "C"] - wb.set_active_sheet(2); // C (last) - let removed = wb.remove_sheet(1); // remove B; active 2 >= new len 2 - assert!(removed.is_some()); - assert_eq!(wb.sheet_count(), 2); - assert_eq!(wb.active_sheet(), 1); // clamped to the new last sheet - assert_eq!(wb.active_sheet_name(), "C"); - } - - /// `apply` forwards the remaining style commands to the active sheet: - /// italic, underline, number format, and background. - #[test] - fn apply_covers_italic_underline_number_format_and_background() { - let mut wb = Workbook::new(); - wb.apply_batch([ - WorkbookCommand::ToggleItalic { row: 0, col: 0 }, - WorkbookCommand::ToggleUnderline { row: 0, col: 0 }, - WorkbookCommand::SetNumberFormat { - row: 0, - col: 0, - format: NumberFormat::Currency, - }, - WorkbookCommand::SetBackground { - row: 0, - col: 0, - color: Some(Color::new(1.0, 0.0, 0.0, 1.0)), - }, - ]); - let cell = wb - .active_sheet_data() - .cells - .get(&CellId::new(0, 0)) - .unwrap(); - assert!(cell.style.italic); - assert!(cell.style.underline); - assert_eq!(cell.style.number_format, NumberFormat::Currency); - assert_eq!(cell.style.bg_color, Some(Color::new(1.0, 0.0, 0.0, 1.0))); - } - - /// `apply`'s SetBorders arm: a per-edge target writes each requested - /// edge, and `BorderTarget::None` clears all four. - #[test] - fn apply_set_borders_covers_clear_and_per_edge() { - let mut wb = Workbook::new(); - let edge = BorderEdge { - color: Color::new(0.0, 0.0, 1.0, 1.0), - width: 2.0, - }; - wb.apply(WorkbookCommand::SetBorders { - row: 0, - col: 0, - target: BorderTarget::All, - edge, - top: true, - bottom: true, - left: true, - right: true, - }); - let cell = wb - .active_sheet_data() - .cells - .get(&CellId::new(0, 0)) - .unwrap(); - assert_eq!(cell.style.border_top, Some(edge)); - assert_eq!(cell.style.border_bottom, Some(edge)); - assert_eq!(cell.style.border_left, Some(edge)); - assert_eq!(cell.style.border_right, Some(edge)); - - wb.apply(WorkbookCommand::SetBorders { - row: 0, - col: 0, - target: BorderTarget::None, - edge, - top: false, - bottom: false, - left: false, - right: false, - }); - let cell = wb - .active_sheet_data() - .cells - .get(&CellId::new(0, 0)) - .unwrap(); - assert_eq!(cell.style.border_top, None); - assert_eq!(cell.style.border_bottom, None); - assert_eq!(cell.style.border_left, None); - assert_eq!(cell.style.border_right, None); - } - - /// `Workbook::default()` is a fresh single-sheet workbook. - #[test] - fn workbook_default_is_a_fresh_single_sheet() { - let wb = Workbook::default(); - assert_eq!(wb.sheet_count(), 1); - assert_eq!(wb.active_sheet_name(), "Sheet 1"); - } - - /// `save()` / `load()` round-trip through the default path, a legacy - /// single-sheet payload still loads, and a non-workbook payload yields - /// `None`. - /// - /// Runs single-threaded against the shared default filenames via - /// `crate::test_disk::lock()`, and saves/restores whatever was there - /// before — the same convention as the persistence module's disk tests. - #[test] - fn workbook_save_load_legacy_and_reject_via_default_path() { - let _disk = crate::test_disk::lock(); - let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("generated"); - let current = dir.join("current.workbook.tsv"); - let saved_current = std::fs::read_to_string(¤t).ok(); - - // Round-trip through the default path. - let mut wb = Workbook::new(); - wb.set_cell(0, 0, "disk"); - wb.set_cell(0, 1, "=A1&\"!\""); - wb.save().expect("save failed"); - let loaded = Workbook::load().expect("load failed"); - assert_eq!(loaded.sheet_count(), 1); - assert_eq!(loaded.active_sheet(), 0); - assert_eq!(loaded.get_raw(0, 0), "disk"); - assert_eq!(loaded.get_display_value(0, 1), "disk!"); - - // A legacy single-sheet payload still loads as a one-sheet workbook. - let mut legacy_data = SpreadsheetData::default(); - legacy_data.set_cell(0, 0, "legacy"); - std::fs::write(¤t, legacy_data.serialize()).expect("write legacy"); - let migrated = Workbook::load().expect("legacy load failed"); - assert_eq!(migrated.sheet_count(), 1); - assert_eq!(migrated.get_raw(0, 0), "legacy"); - - // A non-workbook payload yields None. - std::fs::write(¤t, "not a workbook\n").expect("write garbage"); - assert!(Workbook::load().is_none()); - - // Restore whatever was there before. - match saved_current { - Some(text) => { - std::fs::write(¤t, text).ok(); - } - None => { - std::fs::remove_file(¤t).ok(); - } - } - } }