diff --git a/src/formats/sheet/mod.rs b/src/formats/sheet/mod.rs index d217cd1..f2188be 100644 --- a/src/formats/sheet/mod.rs +++ b/src/formats/sheet/mod.rs @@ -2,6 +2,7 @@ use crate::error::ConvertError; use crate::model::{Block, Cell, Document, GridBuilder, Inline, TableKind}; +use crate::package::limits; use crate::shared::text::clean_text; use calamine::{Data, Dimensions, Reader, Sheets, open_workbook_auto_from_rs}; use std::collections::{HashMap, HashSet}; @@ -44,26 +45,54 @@ pub fn parse(bytes: &[u8]) -> Result { // Merged regions in range-relative coordinates: the top-left cell // becomes a spanning origin, the other positions are covered. let start = range.start().unwrap_or((0, 0)); - let (height, width) = (range.height(), range.width()); + let (start_r, start_c) = (start.0 as u64, start.1 as u64); + let (mut height, mut width) = (range.height() as u64, range.width() as u64); + let regions = merged.get(name.as_str()).map(Vec::as_slice).unwrap_or_default(); + // A merge anchored inside the used range declares its covered cells + // as real extent even when those cells hold no data: grow the grid to + // the merge's far boundary instead of clipping the span to the + // populated range. Growth is capped at the expansion budget, so an + // oversized region keeps the clipped behavior below rather than + // forcing a huge grid. + let mut preserve_extent = false; + if !regions.is_empty() { + let mut far_rows = height; + let mut far_cols = width; + for d in regions { + let (ar, ac) = (d.start.0 as u64, d.start.1 as u64); + if ar < start_r || ac < start_c || ar >= start_r + height || ac >= start_c + width { + continue; + } + far_rows = far_rows.max((d.end.0 as u64 + 1).saturating_sub(start_r)); + far_cols = far_cols.max((d.end.1 as u64 + 1).saturating_sub(start_c)); + } + if (far_rows != height || far_cols != width) + && far_rows.saturating_mul(far_cols) <= limits::MAX_EXPANSION + { + height = far_rows; + width = far_cols; + preserve_extent = true; + } + } let mut origins: HashMap<(usize, usize), (u32, u32)> = HashMap::new(); let mut covered: HashSet<(usize, usize)> = HashSet::new(); - for d in merged.get(name.as_str()).map(Vec::as_slice).unwrap_or_default() { - // Intersect the absolute merged region with the used range first: - // a region wholly above or left of the range must not saturate - // onto relative (0,0), and positions outside the range are never + for d in regions { + // Intersect the absolute merged region with the effective grid + // first: a region wholly above or left of it must not saturate + // onto relative (0,0), and positions outside the grid are never // materialized (a crafted region list must not force insertions // beyond the cells that actually exist). let (row0, col0) = (d.start.0.max(start.0), d.start.1.max(start.1)); - let row_end = (d.end.0 as u64 + 1).min(start.0 as u64 + height as u64); - let col_end = (d.end.1 as u64 + 1).min(start.1 as u64 + width as u64); + let row_end = (d.end.0 as u64 + 1).min(start_r + height); + let col_end = (d.end.1 as u64 + 1).min(start_c + width); if (row0 as u64) >= row_end || (col0 as u64) >= col_end { continue; } // Translate the non-empty intersection to range-relative form. let r0 = (row0 - start.0) as usize; let c0 = (col0 - start.1) as usize; - let r1 = (row_end - start.0 as u64) as usize; - let c1 = (col_end - start.1 as u64) as usize; + let r1 = (row_end - start_r) as usize; + let c1 = (col_end - start_c) as usize; if r1 - r0 == 1 && c1 - c0 == 1 { continue; } @@ -77,14 +106,22 @@ pub fn parse(bytes: &[u8]) -> Result { } } let mut builder = GridBuilder::new(); - for (r, row) in range.rows().enumerate() { + if preserve_extent { + builder.keep_covered_extent(); + } + for r in 0..height as usize { builder.next_row(); - for (c, data) in row.iter().enumerate() { + for c in 0..width as usize { if covered.contains(&(r, c)) { builder.covered(); continue; } - let text = format_data(data); + // Absolute position; cells the grid grew into beyond the + // populated range read as None and become empty cells. + let text = match range.get_value((start.0 + r as u32, start.1 + c as u32)) { + Some(data) => format_data(data), + None => String::new(), + }; let cell = if text.is_empty() { Cell::default() } else { @@ -211,9 +248,31 @@ mod tests { /// Minimal xlsx with a used range at D11:E12 and the given merged region. fn xlsx_with_merge(merge_ref: &str) -> Vec { - let sheet = format!( - r#"xyzw"# - ); + zip_xlsx(&xlsx_sheet_xml( + &[("11", &[("D11", "x"), ("E11", "y")]), ("12", &[("D12", "z"), ("E12", "w")])], + merge_ref, + )) + } + + /// `rows` groups (row number, cells) as `(cell reference, value)` pairs. + fn xlsx_sheet_xml(rows: &[(&str, &[(&str, &str)])], merge_ref: &str) -> String { + let mut data = String::new(); + for (rn, cells) in rows { + data.push_str(&format!(r#""#)); + for (cell, value) in *cells { + data.push_str(&format!( + r#"{value}"# + )); + } + data.push_str(""); + } + format!( + r#"{data}"# + ) + } + + /// Package one worksheet into a single-sheet workbook. + fn zip_xlsx(sheet: &str) -> Vec { let parts: &[(&str, &str)] = &[ ( "[Content_Types].xml", @@ -268,6 +327,74 @@ mod tests { assert_eq!(covered_count(&doc), 0, "out-of-range merge must not cover cells"); } + fn origin(doc: &Document, row: usize, col: usize) -> crate::model::Cell { + let Some(Block::Table(t)) = doc.blocks.first() else { + panic!("expected a table, got {:?}", doc.blocks.first()); + }; + match &t.grid[row][col] { + crate::model::CellSlot::Origin(cell) => cell.clone(), + crate::model::CellSlot::Covered { .. } => { + panic!("expected an origin at ({row},{col})") + } + } + } + + #[test] + fn merge_spanning_past_the_populated_range_is_preserved() { + // Issue #8: the only populated cell (F1) anchors a merge that extends + // beyond the populated range; the span must not collapse to 1x1. + let doc = parse(&zip_xlsx(&xlsx_sheet_xml(&[("1", &[("F1", "Merged heading")])], "F1:O3"))) + .unwrap(); + let Some(Block::Table(t)) = doc.blocks.first() else { + panic!("expected a table, got {:?}", doc.blocks.first()); + }; + assert_eq!(t.grid.len(), 3, "grid must cover the merge rows"); + assert_eq!(t.grid[0].len(), 10, "grid must cover the merge columns"); + let origin = origin(&doc, 0, 0); + assert_eq!(origin.row_span, 3); + assert_eq!(origin.col_span, 10); + assert_eq!(covered_count(&doc), 3 * 10 - 1); + } + + #[test] + fn merge_past_the_range_keeps_content_below() { + // A merge anchored at F1 extends past the populated range while data + // continues below it; the merge keeps its span and the data survives. + let doc = parse(&zip_xlsx(&xlsx_sheet_xml( + &[("1", &[("F1", "Merged heading")]), ("4", &[("F4", "a"), ("G4", "b")])], + "F1:O3", + ))) + .unwrap(); + let Some(Block::Table(t)) = doc.blocks.first() else { + panic!("expected a table, got {:?}", doc.blocks.first()); + }; + assert_eq!(t.grid.len(), 4); + assert_eq!(t.grid[0].len(), 10); + let origin = origin(&doc, 0, 0); + assert_eq!(origin.row_span, 3); + assert_eq!(origin.col_span, 10); + assert_eq!(covered_count(&doc), 3 * 10 - 1); + } + + #[test] + fn oversized_merge_falls_back_to_the_populated_range() { + // A merge whose materialized area would blow the expansion budget is + // clipped to the populated range rather than forcing a huge grid. + let doc = parse(&zip_xlsx(&xlsx_sheet_xml( + &[("1", &[("F1", "Merged heading")])], + "F1:XFD1048576", + ))) + .unwrap(); + let Some(Block::Table(t)) = doc.blocks.first() else { + panic!("expected a table, got {:?}", doc.blocks.first()); + }; + assert_eq!(t.grid.len(), 1); + assert_eq!(t.grid[0].len(), 1); + let origin = origin(&doc, 0, 0); + assert_eq!((origin.row_span, origin.col_span), (1, 1)); + assert_eq!(covered_count(&doc), 0); + } + #[test] fn string_cells_are_not_trimmed() { assert_eq!(format_data(&Data::String(" padded ".into())), " padded "); diff --git a/src/model/table.rs b/src/model/table.rs index 42f16ca..11f5ebf 100644 --- a/src/model/table.rs +++ b/src/model/table.rs @@ -120,6 +120,9 @@ pub struct GridBuilder { /// [`limits::MAX_EXPANSION`] *before* any per-position work so a tiny /// document carrying a huge span cannot force unbounded insertions. expansion: u64, + /// When set, [`GridBuilder::finish`] keeps trailing rows that consist + /// only of covered positions instead of trimming them as filler. + keep_covered_extent: bool, } impl GridBuilder { @@ -220,6 +223,15 @@ impl GridBuilder { } } + /// Preserve trailing rows that consist only of covered positions, rather + /// than trimming them as filler. Callers whose covered positions are a + /// source-declared extent - spreadsheet merges cover real cells even + /// when those cells are empty - opt in so `finish` does not collapse the + /// region down to its origin row. + pub fn keep_covered_extent(&mut self) { + self.keep_covered_extent = true; + } + pub fn finish(mut self, kind: TableKind) -> Table { // Materialize every pending covered position in surviving rows, // including tails behind a gap (a short row under a span at a later @@ -244,11 +256,12 @@ impl GridBuilder { self.grid[row].push(CellSlot::Covered { origin_row, origin_col }); } } - // Drop trailing all-empty rows. + // Drop trailing all-empty rows. Covered slots count as filler, unless + // the caller opted to preserve a source-declared covered extent. while self.grid.last().is_some_and(|r| { r.iter().all(|s| match s { CellSlot::Origin(c) => c.is_empty(), - CellSlot::Covered { .. } => true, + CellSlot::Covered { .. } => !self.keep_covered_extent, }) }) { self.grid.pop(); @@ -481,4 +494,42 @@ mod tests { let t = b.finish(TableKind::Data); assert_eq!(widths(&t), vec![1]); } + + #[test] + fn covered_extent_rows_survive_when_preserved() { + // A merge's covered rows are real structure: with the flag set they + // are kept, and the origin keeps its full span. + let mut b = GridBuilder::new(); + b.keep_covered_extent(); + b.next_row(); + b.place(spanning_text("title", 2, 3)).unwrap(); + b.next_row(); + assert!(b.covered()); + assert!(b.covered()); + b.next_row(); + assert!(b.covered()); + assert!(b.covered()); + let t = b.finish(TableKind::Data); + assert_eq!(t.grid.len(), 3); + assert!(matches!(&t.grid[0][0], CellSlot::Origin(c) if c.row_span == 3 && c.col_span == 2)); + assert_spans_backed(&t); + } + + #[test] + fn covered_extent_rows_trim_without_the_flag() { + // The default stays conservative: phantom covered rows are filler and + // get trimmed, clamping the span to what actually remains. + let mut b = GridBuilder::new(); + b.next_row(); + b.place(spanning_text("title", 2, 3)).unwrap(); + b.next_row(); + assert!(b.covered()); + assert!(b.covered()); + b.next_row(); + assert!(b.covered()); + assert!(b.covered()); + let t = b.finish(TableKind::Data); + assert_eq!(t.grid.len(), 1); + assert!(matches!(&t.grid[0][0], CellSlot::Origin(c) if c.row_span == 1)); + } } diff --git a/tests/fixtures/xlsx/handmade-merge-overhang.xlsx b/tests/fixtures/xlsx/handmade-merge-overhang.xlsx new file mode 100644 index 0000000..e9c681a Binary files /dev/null and b/tests/fixtures/xlsx/handmade-merge-overhang.xlsx differ diff --git a/tests/snapshots.rs b/tests/snapshots.rs index 477a1ad..f7190e1 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -143,6 +143,31 @@ fn docx_ole_payload_wins_over_its_preview_image() { assert_eq!(ole.bytes, b"DOCX-OLE-PAYLOAD".repeat(4)); } +/// A merge anchored in a sheet's only populated cell and extending past the +/// populated range must be retained in the document model with its full +/// span (issue #8). +#[test] +fn xlsx_merge_overhang_is_retained() { + let path = fixture_root().join("xlsx").join("handmade-merge-overhang.xlsx"); + let bytes = std::fs::read(&path).unwrap(); + let doc = anydoc::to_document(&bytes, anydoc::Format::Excel).unwrap(); + let table = doc + .blocks + .iter() + .find_map(|b| match b { + anydoc::model::Block::Table(t) => Some(t), + _ => None, + }) + .expect("sheet converts to a table"); + assert_eq!(table.grid.len(), 6, "grid covers the six used rows"); + assert_eq!(table.grid[0].len(), 10, "grid covers the merge's ten columns"); + let cell = match &table.grid[0][0] { + anydoc::model::CellSlot::Origin(cell) => cell, + anydoc::model::CellSlot::Covered { .. } => panic!("expected an origin at (0,0)"), + }; + assert_eq!((cell.row_span, cell.col_span), (3, 10)); +} + /// Repeated references to one part must neither re-decompress against the /// archive budget nor duplicate the retained asset (S12). #[test] diff --git a/tests/snapshots/snapshots__xlsx__handmade-merge-overhang.xlsx.snap b/tests/snapshots/snapshots__xlsx__handmade-merge-overhang.xlsx.snap new file mode 100644 index 0000000..55c86f9 --- /dev/null +++ b/tests/snapshots/snapshots__xlsx__handmade-merge-overhang.xlsx.snap @@ -0,0 +1,12 @@ +--- +source: tests/snapshots.rs +expression: output +--- +| | | +| --- | --- | +| Merged heading | | +| | | +| | | +| Metric | Value | +| Height | 120 | +| Weight | 34 |