From 10fc7d7bd7e3052e18949d95c5e3ec74a5af0dba Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 22:05:50 +0200 Subject: [PATCH 1/2] fix: calculate spreadsheet content within preview limits --- CHANGELOG.md | 3 ++ src/odr/internal/odf/odf_document.cpp | 46 ++++++++----------- src/odr/internal/ooxml/spreadsheet/AGENTS.md | 6 +-- .../ooxml_spreadsheet_document.cpp | 32 +++++++++++-- .../internal/odf/odf_sheet_repeat_test.cpp | 39 ++++++++++++++++ .../ooxml/ooxml_spreadsheet_value_test.cpp | 34 ++++++++++++++ 6 files changed, 125 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23238f181..08e16fffe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Spreadsheet previews retain ODS cells at window boundaries, clip repeated + and merged cells, and trim XLSX output to populated cells inside the window. + - Filesystem copies preserve their source when copying onto itself and leave existing destinations intact when reading or writing fails. diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 73bbb9bfd..738470108 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -679,34 +679,28 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] TableDimensions sheet_content(const ElementIdentifier element_id, const std::optional range) const override { - const pugi::xml_node node = get_node(element_id); - + const TableDimensions limit = range.value_or( + TableDimensions(std::numeric_limits::max(), + std::numeric_limits::max())); TableDimensions result; - - TableCursor cursor; - for_each_table_row(node, [&](const pugi::xml_node row) { - const auto rows_repeated = - row.attribute("table:number-rows-repeated").as_uint(1); - cursor.add_row(rows_repeated); - - for (auto cell : row.children("table:table-cell")) { - const auto columns_repeated = - cell.attribute("table:number-columns-repeated").as_uint(1); - const auto colspan = - cell.attribute("table:number-columns-spanned").as_uint(1); - const auto rowspan = - cell.attribute("table:number-rows-spanned").as_uint(1); - cursor.add_cell(colspan, rowspan, columns_repeated); - - const std::uint32_t new_rows = cursor.row(); - const std::uint32_t new_cols = - std::max(result.columns, cursor.column()); - if (cell.first_child() && - (!range || (new_rows < range->rows && new_cols < range->columns))) { - result.rows = new_rows; - result.columns = new_cols; - } + for_each_cell_run(element_id, [&](const ElementRegistry::Sheet::Cell &cell, + const std::uint32_t row, + const TableDimensions &repeated) { + if (!cell.node.first_child() || row >= limit.rows || + cell.begin >= limit.columns) { + return; } + const std::uint32_t colspan = std::max( + 1u, cell.node.attribute("table:number-columns-spanned").as_uint(1)); + const std::uint32_t rowspan = std::max( + 1u, cell.node.attribute("table:number-rows-spanned").as_uint(1)); + const auto end_row = static_cast(std::min( + std::uint64_t{row} + repeated.rows + rowspan - 1, limit.rows)); + const auto end_column = + static_cast(std::min( + std::uint64_t{cell.end} + colspan - 1, limit.columns)); + result.rows = std::max(result.rows, end_row); + result.columns = std::max(result.columns, end_column); }); return result; diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index a73a06032..dc1552a86 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -104,10 +104,8 @@ the evaluator cannot compute. stale formulas, and saving after an edit invokes it. Unsupported and array formulas retain their cached results. Number formats supply displayed numbers, dates and times. -2. `sheet_content` ignores the requested range and returns the full - ``. -3. No `cellStyleXfs` inheritance. Borders render as `0.75pt solid` whatever +2. No `cellStyleXfs` inheritance. Borders render as `0.75pt solid` whatever the style. Cell `protection` is read and dropped. -4. `sheet_set_cell` writes numbers, strings, booleans, dates and times. `text_set_content` +3. `sheet_set_cell` writes numbers, strings, booleans, dates and times. `text_set_content` throws `UnsupportedOperation`. A `` is not modelled, so `link_href` is empty. Comments are not modelled. diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index f4ac4ed9d..ef5334f75 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -333,11 +334,32 @@ class ElementAdapter final : public AdapterBase { } [[nodiscard]] TableDimensions sheet_content(const ElementIdentifier element_id, - [[maybe_unused]] const std::optional range) - const override { - // TODO the range is ignored: this answers the whole `` rather - // than trimming to the populated cells inside it. - return sheet_dimensions(element_id); + const std::optional range) const override { + const TableDimensions limit = range.value_or( + TableDimensions(std::numeric_limits::max(), + std::numeric_limits::max())); + TableDimensions result; + for (const auto &[position, cell] : + m_registry->sheet_element_at(element_id).cells) { + if (!cell.node.first_child() || position.row >= limit.rows || + position.column >= limit.columns) { + continue; + } + const ElementRegistry::SheetCell &element = + m_registry->sheet_cell_element_at(cell.element_id); + if (element.is_covered) { + continue; + } + const auto end_row = static_cast(std::min( + std::uint64_t{position.row} + element.span.rows, limit.rows)); + const auto end_column = + static_cast(std::min( + std::uint64_t{position.column} + element.span.columns, + limit.columns)); + result.rows = std::max(result.rows, end_row); + result.columns = std::max(result.columns, end_column); + } + return result; } [[nodiscard]] ElementIdentifier sheet_cell(const ElementIdentifier element_id, const std::uint32_t column, diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp index 23a17bba0..2a816f082 100644 --- a/test/src/internal/odf/odf_sheet_repeat_test.cpp +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -159,3 +159,42 @@ TEST(OdfSheetRepeat, a_write_into_a_repeat_does_not_expand_it) { EXPECT_EQ(sheet.cell(512, 1023).value().text(), "x"); EXPECT_LT(document->element_registry().size(), 32); } + +TEST(OdfSheetRepeat, content_extent_clips_repeated_runs_at_the_window) { + const auto held = document_of(flat_sheet(repeated_rows(4, 3))); + const Sheet sheet = + odr::Document(held).root_element().first_child().as_sheet(); + + for (const TableDimensions limit : + {TableDimensions(4, 3), TableDimensions(2, 2), TableDimensions(1, 1)}) { + const TableDimensions content = sheet.content(limit); + EXPECT_EQ(content.rows, limit.rows); + EXPECT_EQ(content.columns, limit.columns); + } + EXPECT_EQ(sheet.content(TableDimensions(0, 3)).columns, 0); + EXPECT_EQ(sheet.content(TableDimensions(4, 0)).rows, 0); +} + +TEST(OdfSheetRepeat, content_extent_uses_indexed_positions_and_merged_spans) { + const auto held = document_of(flat_sheet( + R"()" + R"(x)" + R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"(outside)" + R"()")); + const Sheet sheet = + odr::Document(held).root_element().first_child().as_sheet(); + + const TableDimensions content = sheet.content(TableDimensions(5, 5)); + EXPECT_EQ(content.rows, 2); + EXPECT_EQ(content.columns, 3); + const TableDimensions blank = sheet.content(TableDimensions(5, 1)); + EXPECT_EQ(blank.rows, 0); + EXPECT_EQ(blank.columns, 0); +} diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp index 2677b74df..079329b30 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include @@ -331,3 +332,36 @@ TEST(OoxmlSpreadsheetValue, an_invalid_index_keeps_the_rest_of_the_sheet) { "", "zero")); EXPECT_EQ(first_sheet(document).cell(0, 0).value().text(), "zero"); } + +TEST(OoxmlSpreadsheetValue, content_extent_counts_only_cells_in_the_window) { + const Document document = + decode(workbook(R"(1)" + R"()" + R"(2)", + "", "", "", R"()")); + const Sheet sheet = first_sheet(document); + + for (const TableDimensions limit : + {TableDimensions(2, 2), TableDimensions(5, 5)}) { + const TableDimensions content = sheet.content(limit); + EXPECT_EQ(content.rows, 2); + EXPECT_EQ(content.columns, 2); + } + EXPECT_EQ(sheet.content(std::nullopt).rows, 10); + EXPECT_EQ(sheet.content(std::nullopt).columns, 10); + EXPECT_EQ(sheet.content(TableDimensions(1, 5)).columns, 0); + EXPECT_EQ(sheet.content(TableDimensions(5, 1)).rows, 0); +} + +TEST(OoxmlSpreadsheetValue, content_extent_clips_merged_cells_at_the_window) { + const Document document = + decode(workbook(R"(1)", + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_EQ(sheet.content(std::nullopt).rows, 4); + EXPECT_EQ(sheet.content(std::nullopt).columns, 5); + const TableDimensions content = sheet.content(TableDimensions(3, 3)); + EXPECT_EQ(content.rows, 3); + EXPECT_EQ(content.columns, 3); +} From 35b89a5449826cac924babd7d43bb5795662d785 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 22:20:34 +0200 Subject: [PATCH 2/2] fix(xlsx): keep empty cells with a border or fill in the preview Counting only cells with content cut off empty cells that still draw: Ordnerruecken.xlsx lost the row whose bottom borders close its spine boxes. An empty cell now counts where its style has a border or a visible fill, as the declared dimension covered it before. A check of every changed corpus page finds no trimmed cell with a border or fill. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MxyTMutqSUJRGfxA8CyzMc --- .../spreadsheet/ooxml_spreadsheet_document.cpp | 15 +++++++++++++-- .../ooxml/ooxml_spreadsheet_value_test.cpp | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index ef5334f75..3795b09ba 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -308,6 +308,13 @@ pugi::xml_node Document::create_styles_() { namespace { +/// Whether a cell draws something on its own: a border or a visible fill. +bool draws(const TableCellStyle &style) { + const DirectionalStyle &border = style.border; + return (style.background_color && style.background_color->alpha != 0) || + border.top || border.right || border.bottom || border.left; +} + using AdapterBase = internal::RegistryElementAdapter< ElementRegistry, abstract::SheetAdapter, abstract::SheetCellAdapter, abstract::LineBreakAdapter, abstract::ParagraphAdapter, @@ -341,8 +348,12 @@ class ElementAdapter final : public AdapterBase { TableDimensions result; for (const auto &[position, cell] : m_registry->sheet_element_at(element_id).cells) { - if (!cell.node.first_child() || position.row >= limit.rows || - position.column >= limit.columns) { + if (position.row >= limit.rows || position.column >= limit.columns) { + continue; + } + // an empty cell still counts where its border or fill draws something + if (!cell.node.first_child() && + !draws(sheet_cell_style(element_id, position.column, position.row))) { continue; } const ElementRegistry::SheetCell &element = diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp index 079329b30..b6dd5118c 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp @@ -365,3 +365,21 @@ TEST(OoxmlSpreadsheetValue, content_extent_clips_merged_cells_at_the_window) { EXPECT_EQ(content.rows, 3); EXPECT_EQ(content.columns, 3); } + +// An empty cell still draws its border, so the preview has to reach it. +TEST(OoxmlSpreadsheetValue, content_extent_counts_an_empty_cell_with_a_border) { + const Document document = decode(workbook( + R"(1)" + R"()" + R"()", + "", "", "", "", + R"()" + R"()" + R"()" + R"()" + R"()" + R"()")); + const TableDimensions content = first_sheet(document).content(std::nullopt); + EXPECT_EQ(content.rows, 4); + EXPECT_EQ(content.columns, 4); +}