From 1086e4aab8e278d0804885f70271e1b292035a19 Mon Sep 17 00:00:00 2001 From: Eduardo Soares Date: Mon, 12 Oct 2026 19:44:27 -0300 Subject: [PATCH] #833: opened files grow non-custom rows to their tallest font --- crates/engine/src/io.rs | 31 ++++++++++++++++-- crates/engine/src/tests.rs | 65 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index bc7011dec..96746d913 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -56,7 +56,9 @@ pub fn open_bytes(name: &str, bytes: &[u8]) -> Result<(Workbook, Vec)> { /// quote the password. pub fn open_bytes_with_password(name: &str, bytes: &[u8], password: Option<&str>) -> Result<(Workbook, Vec)> { if gridcraft_xlsx::sniff(bytes) != gridcraft_xlsx::Format::Encrypted { - return open_plain(name, bytes); + let (mut wb, warnings) = open_plain(name, bytes)?; + fit_imported_rows(&mut wb); + return Ok((wb, warnings)); } // A scheme no password opens is said straight away, rather than after asking for one. let decrypt_error = |e: gridcraft_xlsx::DecryptError| EngineError::Other(decrypt_message(name, e)); @@ -68,11 +70,36 @@ pub fn open_bytes_with_password(name: &str, bytes: &[u8], password: Option<&str> if gridcraft_xlsx::sniff(&plain) == gridcraft_xlsx::Format::Encrypted { return Err(EngineError::Other(format!("We can't open '{name}': the encrypted file is damaged (it holds another encrypted file)."))); } - let (wb, mut warnings) = open_plain(name, &plain)?; + let (mut wb, mut warnings) = open_plain(name, &plain)?; + fit_imported_rows(&mut wb); warnings.insert(0, DECRYPTED_NOTICE.into()); Ok((wb, warnings)) } +/// Excel auto-fits rows to their tallest font when opening a file: rows without a user-set +/// height grow to fit their content. Rows the file made taller keep their height — our +/// line-height estimate can differ from Excel's, and shrinking files that already look +/// right would be a visible regression. Custom-height rows are left alone entirely. +fn fit_imported_rows(wb: &mut Workbook) { + for si in 0..wb.sheets.len() { + let Some(sh) = wb.sheet(si) else { continue }; + let mut rows: Vec = sh.cells.iter().map(|(c, _)| c.row).collect(); + rows.sort_unstable(); + rows.dedup(); + for row in rows { + let before = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); + crate::cmd::auto_row_height(wb, si, row); + let Some(before) = before else { continue }; + let after = wb.sheet(si).and_then(|sh| sh.rows.get(&row)).and_then(|i| i.size); + if after.is_none_or(|a| before > a + 0.5) + && let Some(sh) = wb.sheet_mut(si) + { + sh.rows.entry(row).or_default().size = Some(before); + } + } + } +} + /// What to tell the user when an encrypted workbook can't be decrypted. Never quotes the password. fn decrypt_message(name: &str, e: gridcraft_xlsx::DecryptError) -> String { match e { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 33fdc113e..49f67e8e8 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1007,6 +1007,71 @@ fn spill_references_roundtrip_through_xlsx() { assert_eq!((v(&s, "C1"), v(&s, "C2")), (Value::Number(10.0), Value::Number(4.0))); } +/// Fixture for the open-time row fit (issue #833): `row_info` = (size, custom, hidden) for +/// row 5 (index 4), `None` = no row entry at all. Cell A5 = "plain big bold", whole-cell +/// font 20pt. 11pt cell in A1 for contrast rows. Built with the model API, never through +/// commands — commands would auto-fit the rows — and exported with `write_xlsx`, which +/// omits `ht` for rows without a size: exactly the bug's input. +fn big_font_workbook(row_info: Option<(f32, bool, bool)>) -> Vec { + let mut wb = gridcraft_model::Workbook::new(); + let big = wb.styles.derive(gridcraft_model::StyleId::DEFAULT, |s| s.font.size = 20.0); + let sh = wb.sheet_mut(0).unwrap(); + sh.set_value(CellRef::new(0, 0), Value::text("small")); + sh.set_value(CellRef::new(4, 0), Value::text("plain big bold")); + sh.set_style(CellRef::new(4, 0), big); + if let Some((size, custom, hidden)) = row_info { + let e = sh.rows.entry(4).or_default(); + e.size = Some(size); + e.custom = custom; + e.hidden = hidden; + } + gridcraft_xlsx::write_xlsx(&wb).unwrap() +} + +#[test] // A — the issue's repro +fn open_grows_default_height_rows_to_their_font() { + let default_h = gridcraft_model::Workbook::new().active().unwrap().row_height(0); + let mut s = Session::new(); + s.execute("file.open", json!({"name": "bigfont.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(None))})).unwrap(); + let h = s.doc().unwrap().wb.active().unwrap().row_height(4); + assert!(h >= 30.0, "20pt cell must grow its default-height row on open, got {h}"); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(0), default_h, "11pt row stays default"); +} + +#[test] // B — Review Focus 1 +fn open_keeps_custom_height_rows() { + let mut s = Session::new(); + s.execute("file.open", json!({"name": "custom.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(Some((25.0, true, false))))})) + .unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), 25.0, "custom-height rows are left alone on open"); +} + +#[test] // C — Review Focus 2 +fn open_never_shrinks_taller_auto_rows() { + let mut s = Session::new(); + s.execute("file.open", json!({"name": "tall.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(Some((60.0, false, false))))})).unwrap(); + assert_eq!(s.doc().unwrap().wb.active().unwrap().row_height(4), 60.0, "open is grow-only: a 60px auto-height row keeps its height"); +} + +#[test] // D — Review Focus 5 +fn open_fitting_keeps_hidden_rows_hidden() { + let mut s = Session::new(); + s.execute("file.open", json!({"name": "hidden.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(Some((20.0, false, true))))})) + .unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + let info = sh.rows.get(&4).unwrap(); + assert!(info.size.is_none_or(|h| h >= 20.0), "hidden row grows or keeps its height, got {:?}", info.size); + assert!(info.hidden, "fitting a hidden row must not unhide it"); +} + +#[test] // E — Review Focus 4 +fn open_leaves_plain_workbooks_untouched() { + let mut s = Session::new(); + s.execute("file.open", json!({"name": "plain.xlsx", "base64": crate::io::base64_encode(&big_font_workbook(None))})).unwrap(); + let sh = s.doc().unwrap().wb.active().unwrap(); + assert!(sh.rows.get(&0).is_none_or(|i| i.size.is_none()), "11pt default rows get no height entry"); +} + #[test] fn tables_and_structured_refs() { let mut s = s();