From 8f951d461f655b2babb1c91cdcf14cc1c9e390a2 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 7 Sep 2026 21:53:01 +0200 Subject: [PATCH] feat(odf): write a repeated sheet cell by cutting the run `Sheet::set_cell` refused a repeated cell: one element stands for every position the repeat covers, so a write would have hit all of them. `split_repeat` cuts the run instead. The `table:table-row` and the `table:table-cell` are copied around the position written, the original node staying as the one written so its element and children survive, and `reindex_sheet` rebuilds the sheet's position index off the dom - a cell node keeping the element it already carries, a copied one getting a fresh one. The positions around it read as they did. The cost follows the row the write touched, not the grid the repeat claims: a write into a `1048576 x 1024` repeat leaves the registry under 32 elements, which is the expansion #776 removed and a test pins. The formula and markup refusals now run against the cell before the split, so a refused write leaves the run uncut. `only_text_run` splits in two: `holds_one_run` decides the refusal without writing anything, `text_run_of` creates the run an empty paragraph needs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J325TWocZ4iXBjvKv2ZVZi --- CHANGELOG.md | 4 + src/odr/internal/odf/AGENTS.md | 16 ++- src/odr/internal/odf/README.md | 3 +- src/odr/internal/odf/odf_document.cpp | 128 +++++++++++++----- src/odr/internal/odf/odf_element_registry.hpp | 6 +- src/odr/internal/odf/odf_parser.cpp | 118 ++++++++++------ src/odr/internal/odf/odf_parser.hpp | 4 + .../internal/odf/odf_sheet_repeat_test.cpp | 19 +++ .../src/internal/odf/odf_sheet_write_test.cpp | 101 +++++++++++++- 9 files changed, 315 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42602ff5a..13535e621 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- `Sheet::set_cell` writes a repeated `.ods` cell: the run is cut into the + position written and the parts around it, which keep the value they had. + Only a cell the file states no element for still refuses. + - **Fix**: a repeated `.ods` cell answers for the position it was looked up at rather than the anchor of its range, so `SheetCell::position()`, `Sheet::cell()` and `DocumentPath` all name the cell that was asked for. diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 770d0b430..ee9182a73 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -187,10 +187,18 @@ The structural/foundational gaps, roughly by value: `office:value-type`/`office:value` *and* the `text:p` under the cell — the file states the value and shows a rendering of it, and setting one without the other leaves it contradicting itself. It writes through the cell's - single text run, so a cell that is absent, repeated, holding a formula, or - holding richer markup than one plain paragraph refuses instead. Splitting a - repeat, which is what would let an absent or repeated cell be written, is - the next step in [`spreadsheet-editing.md`](../../../../docs/design/spreadsheet-editing.md). + single text run, so a cell holding a formula or richer markup than one plain + paragraph refuses, as does one the file states no element for. + + A **repeated** cell is written by cutting the run: `split_repeat` copies the + `table:table-row` and the `table:table-cell` around the position and leaves + the original node as the one written, so its element and children survive. + `reindex_sheet` then rebuilds the sheet's position index off the dom, a cell + node keeping the element it already carries. Both refusals are decided + before any of that, so a refused write leaves the run uncut. Two costs: + cutting a repeated row copies every cell in it, so the elements grow with + the row rather than with the repeat; and the reindex walks the row nodes, + which a repeat collapses, so it is bounded by the dom rather than the grid. 3. **Save never re-encrypts**, and refuses rather than dropping the encryption: a document decrypted from a password-protected package reports `is_savable(false) == false` and every `save` overload throws diff --git a/src/odr/internal/odf/README.md b/src/odr/internal/odf/README.md index 2421b8b4c..ebfdeb5d7 100644 --- a/src/odr/internal/odf/README.md +++ b/src/odr/internal/odf/README.md @@ -119,7 +119,8 @@ Roughly ordered by importance. not evaluated) - [x] edit - [x] cell values (number, string, cleared) - - [ ] a cell that is absent or repeated (the run has to be split first) + - [x] a repeated cell, by cutting the run around the position written + - [ ] a cell the file states no element for - [ ] a formula cell, and a cell of richer markup than one plain paragraph ### Presentation documents (`.odp`) diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index d6752b7f7..615ff66d9 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -367,27 +368,28 @@ class ElementAdapter final : public AdapterBase { void sheet_set_cell(const ElementIdentifier element_id, const std::uint32_t column, const std::uint32_t row, const CellValue &value) const override { - const ElementRegistry::Sheet &sheet = - m_registry->sheet_element_at(element_id); - const ElementRegistry::Sheet::Cell *cell = sheet.cell(column, row); + const ElementRegistry::Sheet::Cell *cell = + m_registry->sheet_element_at(element_id).cell(column, row); if (cell == nullptr || cell->element_id == null_element_id) { throw UnsupportedOperation(); // an empty cell is written as no element } - const ElementIdentifier cell_id = cell->element_id; - if (m_registry->sheet_cell_element_at(cell_id).is_repeated) { - throw UnsupportedOperation(); - } + ElementIdentifier cell_id = cell->element_id; - pugi::xml_node node = get_node(cell_id); - if (node.attribute("table:formula")) { + // both refusals are decided on the run, before the split writes anything + if (get_node(cell_id).attribute("table:formula")) { throw UnsupportedOperation(); // its dependants would go stale } - - const ElementIdentifier text_id = only_text_run(cell_id); - if (text_id == null_element_id) { + if (!holds_one_run(cell_id)) { throw UnsupportedOperation(); } - text_set_content(text_id, value.has_text() ? value.text() : ""); + + if (m_registry->sheet_cell_element_at(cell_id).is_repeated) { + cell_id = split_repeat(element_id, column, row); // `cell` is stale after + } + + pugi::xml_node node = get_node(cell_id); + text_set_content(text_run_of(cell_id), + value.has_text() ? value.text() : ""); static constexpr std::array stated = { "office:value-type", "office:value", "office:boolean-value", @@ -884,31 +886,93 @@ class ElementAdapter final : public AdapterBase { return m_registry->element_at(element_id).node; } - /// The single text run under a cell of one plain paragraph, created where - /// that paragraph is empty. Null where the markup is richer than that, which - /// a write then keeps rather than throws away. - [[nodiscard]] ElementIdentifier - only_text_run(const ElementIdentifier cell_id) const { + /// The attribute where @p node repeats at all, none where it covers one. + static void set_repeat(pugi::xml_node node, const char *attribute, + const std::uint32_t repeated) { + node.remove_attribute(attribute); + if (repeated > 1) { + node.append_attribute(attribute).set_value(repeated); + } + } + + /// Cuts the run [@p begin, @p end) so @p at stands alone, copying the parts + /// around it. @p node stays as the one at @p at, so its element survives. + static void split_run(pugi::xml_node node, const char *attribute, + const std::uint32_t begin, const std::uint32_t end, + const std::uint32_t at) { + if (end > at + 1) { + set_repeat(node.parent().insert_copy_after(node, node), attribute, + end - at - 1); + } + if (at > begin) { + set_repeat(node.parent().insert_copy_before(node, node), attribute, + at - begin); + } + set_repeat(node, attribute, 1); + } + + /// Gives (@p column, @p row) a cell of its own, splitting the row and the + /// cell run it is one position of. Reindexes: every pointer read before is + /// stale. + [[nodiscard]] ElementIdentifier split_repeat(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { + const ElementRegistry::Sheet &sheet = + m_registry->sheet_element_at(sheet_id); + + const ElementRegistry::Sheet::Row *row_entry = sheet.row(row); + const std::size_t row_index = row_entry - sheet.rows.data(); + const std::uint32_t row_begin = + row_index == 0 ? 0 : sheet.rows[row_index - 1].end; + + const std::span cells = + sheet.row_cells(*row_entry); + const ElementRegistry::Sheet::Cell *cell_entry = sheet.cell(column, row); + const std::size_t cell_index = cell_entry - cells.data(); + const std::uint32_t cell_begin = + cell_index == 0 ? 0 : cells[cell_index - 1].end; + + // the row first: the cell keeps its node, so its own run is unmoved + split_run(row_entry->node, "table:number-rows-repeated", row_begin, + row_entry->end, row); + split_run(cell_entry->node, "table:number-columns-repeated", cell_begin, + cell_entry->end, column); + + reindex_sheet(*m_registry, sheet_id); + + return m_registry->sheet_element_at(sheet_id).cell(column, row)->element_id; + } + + /// Whether a write can go through the cell: one plain paragraph of one run + /// at most. Richer markup is kept rather than overwritten. + [[nodiscard]] bool holds_one_run(const ElementIdentifier cell_id) const { const ElementIdentifier paragraph_id = element_first_child(cell_id); if (paragraph_id == null_element_id || element_next_sibling(paragraph_id) != null_element_id || element_type(paragraph_id) != ElementType::paragraph) { - return null_element_id; + return false; } const ElementIdentifier text_id = element_first_child(paragraph_id); - if (text_id == null_element_id) { - const pugi::xml_node text_node = - get_node(paragraph_id).append_child(pugi::xml_node_type::node_pcdata); - const auto &[new_id, unused1, unused2] = - m_registry->create_text_element(text_node, text_node); - m_registry->append_child(paragraph_id, new_id); - return new_id; - } - if (element_next_sibling(text_id) != null_element_id || - element_type(text_id) != ElementType::text) { - return null_element_id; - } - return text_id; + return text_id == null_element_id || + (element_next_sibling(text_id) == null_element_id && + element_type(text_id) == ElementType::text); + } + + /// That run, created where the paragraph is empty - what a cleared cell is. + /// @ref holds_one_run has to pass. + [[nodiscard]] ElementIdentifier + text_run_of(const ElementIdentifier cell_id) const { + const ElementIdentifier paragraph_id = element_first_child(cell_id); + if (const ElementIdentifier text_id = element_first_child(paragraph_id); + text_id != null_element_id) { + return text_id; + } + const pugi::xml_node text_node = + get_node(paragraph_id).append_child(pugi::xml_node_type::node_pcdata); + const auto &[new_id, unused1, unused2] = + m_registry->create_text_element(text_node, text_node); + m_registry->append_child(paragraph_id, new_id); + return new_id; } /// The image's base64 bytes where the markup carries them itself. diff --git a/src/odr/internal/odf/odf_element_registry.hpp b/src/odr/internal/odf/odf_element_registry.hpp index 236f0e9a0..37bfba352 100644 --- a/src/odr/internal/odf/odf_element_registry.hpp +++ b/src/odr/internal/odf/odf_element_registry.hpp @@ -184,9 +184,9 @@ class ElementRegistry final return self.m_sheets.at(self.resolve_id(id)); } - [[nodiscard]] const SheetCell & - sheet_cell_element_at(const ElementIdentifier id) const { - return m_sheet_cells.at(resolve_id(id)); + [[nodiscard]] auto &sheet_cell_element_at(this auto &self, + const ElementIdentifier id) { + return self.m_sheet_cells.at(self.resolve_id(id)); } [[nodiscard]] const SheetCell * diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index 48c301734..8fa7993cd 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -159,6 +159,64 @@ bool is_cell_empty(const pugi::xml_node cell_node) { cell_node.attribute("table:number-rows-spanned").as_uint(1) <= 1; } +/// The element a cell node already carries, so a reindex keeps it. +using SheetCellElements = std::unordered_map; + +/// Builds @p sheet's row and cell index off its dom. A cell node in +/// @p existing keeps that element; one that is not gets a fresh one. +void index_sheet_rows(ElementRegistry ®istry, + const ElementIdentifier sheet_id, + ElementRegistry::Sheet &sheet, const pugi::xml_node node, + const SheetCellElements &existing) { + TableCursor cursor; + + for_each_table_row(node, [&](const pugi::xml_node row_node) { + const std::uint32_t rows_repeated = + row_node.attribute("table:number-rows-repeated").as_uint(1); + + sheet.register_row(cursor.row(), rows_repeated, row_node); + + // TODO covered cells + for (const pugi::xml_node cell_node : + row_node.children("table:table-cell")) { + const std::uint32_t columns_repeated = + cell_node.attribute("table:number-columns-repeated").as_uint(1); + const std::uint32_t colspan = + cell_node.attribute("table:number-columns-spanned").as_uint(1); + const std::uint32_t rowspan = + cell_node.attribute("table:number-rows-spanned").as_uint(1); + const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; + + ElementIdentifier cell_id = null_element_id; + const auto kept = existing.find(cell_node.internal_object()); + if (kept != std::end(existing)) { + cell_id = kept->second; + ElementRegistry::SheetCell &cell = + registry.sheet_cell_element_at(cell_id); + cell.position = cursor.position(); + cell.is_repeated = is_repeated; + } else if (!is_cell_empty(cell_node)) { + const auto &[id, unused1, unused2] = registry.create_sheet_cell_element( + cell_node, cursor.position(), is_repeated); + cell_id = id; + registry.append_sheet_cell(sheet_id, cell_id); + parse_any_element_children(registry, cell_id, cell_node); + } + + sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, + rows_repeated, cell_node, cell_id); + + cursor.add_cell(colspan, rowspan, columns_repeated); + } + + // TODO a rowspan out of a repeated row is dropped - `add_row` clears the + // cursor's pending ranges for a repeat > 1 + cursor.add_row(rows_repeated); + }); + + sheet.dimensions.rows = cursor.row(); +} + /// One entry per row and per cell node at most - repeats collapse onto one. void reserve_sheet(ElementRegistry::Sheet &sheet, const pugi::xml_node node) { std::size_t rows = 0; @@ -199,47 +257,7 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { sheet.dimensions.columns = cursor.column(); cursor = {}; - for_each_table_row(node, [&](const pugi::xml_node row_node) { - const std::uint32_t rows_repeated = - row_node.attribute("table:number-rows-repeated").as_uint(1); - - sheet.register_row(cursor.row(), rows_repeated, row_node); - - // TODO covered cells - for (const pugi::xml_node cell_node : - row_node.children("table:table-cell")) { - const std::uint32_t columns_repeated = - cell_node.attribute("table:number-columns-repeated").as_uint(1); - const std::uint32_t colspan = - cell_node.attribute("table:number-columns-spanned").as_uint(1); - const std::uint32_t rowspan = - cell_node.attribute("table:number-rows-spanned").as_uint(1); - const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; - - ElementIdentifier cell_id = null_element_id; - if (!is_cell_empty(cell_node)) { - const auto &[id, unused1, unused2] = registry.create_sheet_cell_element( - cell_node, cursor.position(), is_repeated); - cell_id = id; - registry.append_sheet_cell(element_id, cell_id); - } - - sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, - rows_repeated, cell_node, cell_id); - - if (cell_id != null_element_id) { - parse_any_element_children(registry, cell_id, cell_node); - } - - cursor.add_cell(colspan, rowspan, columns_repeated); - } - - // TODO a rowspan out of a repeated row is dropped - `add_row` clears the - // cursor's pending ranges for a repeat > 1 - cursor.add_row(rows_repeated); - }); - - sheet.dimensions.rows = cursor.row(); + index_sheet_rows(registry, element_id, sheet, node, {}); for (const pugi::xml_node shape_node : node.child("table:shapes").children()) { @@ -413,4 +431,22 @@ ElementIdentifier odf::parse_tree(ElementRegistry ®istry, return root; } +void odf::reindex_sheet(ElementRegistry ®istry, + const ElementIdentifier sheet_id) { + ElementRegistry::Sheet &sheet = registry.sheet_element_at(sheet_id); + + SheetCellElements existing; + existing.reserve(sheet.cells.size()); + for (const ElementRegistry::Sheet::Cell &cell : sheet.cells) { + if (cell.element_id != null_element_id) { + existing.emplace(cell.node.internal_object(), cell.element_id); + } + } + + sheet.rows.clear(); + sheet.cells.clear(); + index_sheet_rows(registry, sheet_id, sheet, + registry.element_at(sheet_id).node, existing); +} + } // namespace odr::internal diff --git a/src/odr/internal/odf/odf_parser.hpp b/src/odr/internal/odf/odf_parser.hpp index 41c6244c8..f40043042 100644 --- a/src/odr/internal/odf/odf_parser.hpp +++ b/src/odr/internal/odf/odf_parser.hpp @@ -11,4 +11,8 @@ class ElementRegistry; ElementIdentifier parse_tree(ElementRegistry ®istry, pugi::xml_node node); +/// Rebuilds the row and cell index of @p sheet_id off its dom, for a write +/// that split a repeat. A cell node keeps the element it already carries. +void reindex_sheet(ElementRegistry ®istry, ElementIdentifier sheet_id); + } // namespace odr::internal::odf diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp index 83a09c5e2..8c5f1dce2 100644 --- a/test/src/internal/odf/odf_sheet_repeat_test.cpp +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -141,3 +141,22 @@ TEST(OdfSheetRepeat, the_children_of_a_repeated_cell_are_shared) { EXPECT_EQ(*(*sheet.cell(0, 0).children().begin()).children().begin(), text); EXPECT_EQ(document.root_element().navigate_path(text.document_path()), text); } + +/// A write cuts the run into three rather than expanding it, so what it costs +/// follows the row it touched and not the grid the repeat claims. +TEST(OdfSheetRepeat, a_write_into_a_repeat_does_not_expand_it) { + const std::string source = flat_sheet(repeated_rows(1048576, 1024)); + const std::shared_ptr held = document_of(source); + const auto *document = dynamic_cast(held.get()); + ASSERT_NE(document, nullptr); + + const odr::Document public_document(held); + const Sheet sheet = + (*public_document.root_element().children().begin()).as_sheet(); + sheet.set_cell(512, 1024, CellValue("y")); + + EXPECT_EQ(sheet.cell(512, 1024).value().text(), "y"); + EXPECT_EQ(sheet.cell(511, 1024).value().text(), "x"); + EXPECT_EQ(sheet.cell(512, 1023).value().text(), "x"); + EXPECT_LT(document->element_registry().size(), 32); +} diff --git a/test/src/internal/odf/odf_sheet_write_test.cpp b/test/src/internal/odf/odf_sheet_write_test.cpp index c164c9e64..906cca005 100644 --- a/test/src/internal/odf/odf_sheet_write_test.cpp +++ b/test/src/internal/odf/odf_sheet_write_test.cpp @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include #include @@ -93,14 +95,107 @@ TEST(OdfSheetWrite, a_cleared_cell_states_nothing) { EXPECT_EQ(sheet.cell(0, 0).value().text(), ""); } -/// One element stands for every cell of the run, so a write hits all of them. -TEST(OdfSheetWrite, a_repeated_cell_refuses_to_be_written) { +/// One element stands for every cell of the run, so a write cuts it in three. +TEST(OdfSheetWrite, a_repeated_cell_is_split_by_a_write) { const Document document = document_of(flat_sheet( R"(x)")); const Sheet sheet = first_sheet(document); - EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); + sheet.set_cell(2, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "x"); + EXPECT_EQ(sheet.cell(1, 0).value().text(), "x"); + EXPECT_EQ(sheet.cell(2, 0).value().text(), "y"); + EXPECT_EQ(sheet.cell(3, 0).value().text(), "x"); +} + +TEST(OdfSheetWrite, a_repeat_is_split_at_either_end_too) { + const Document document = document_of(flat_sheet( + R"(x)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("a")); + sheet.set_cell(2, 0, CellValue("c")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_EQ(sheet.cell(1, 0).value().text(), "x"); + EXPECT_EQ(sheet.cell(2, 0).value().text(), "c"); +} + +/// The refusals are decided on the run, so one leaves it uncut. +TEST(OdfSheetWrite, a_refused_write_leaves_a_repeat_uncut) { + const Document document = document_of(flat_sheet( + R"(7)")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(2, 0, CellValue("y")), UnsupportedOperation); + + std::ostringstream saved; + document.save(saved); + EXPECT_NE(saved.str().find(R"(table:number-columns-repeated="4")"), + std::string::npos); +} + +/// A repeated row is cut the same way, so only the row written changes. +TEST(OdfSheetWrite, a_repeated_row_is_split_by_a_write) { + const Document document = document_of( + R"()" + R"()" + R"()" + R"()" + R"()" + R"(x)" + R"()" + R"()"); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(1, 1, CellValue("y")); + + EXPECT_EQ(sheet.cell(1, 1).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 1).value().text(), "x"); + for (const std::uint32_t row : {0u, 2u}) { + EXPECT_EQ(sheet.cell(0, row).value().text(), "x") << row; + EXPECT_EQ(sheet.cell(1, row).value().text(), "x") << row; + } + EXPECT_EQ(sheet.dimensions().rows, 3); +} + +/// The id names a position, so the handle still means its cell once the run +/// is cut under it. +TEST(OdfSheetWrite, a_handle_taken_before_a_split_shows_the_write) { + const Document document = document_of(flat_sheet( + R"(x)")); + const Sheet sheet = first_sheet(document); + + const SheetCell held = sheet.cell(2, 0); + sheet.set_cell(2, 0, CellValue("y")); + + EXPECT_EQ(held.value().text(), "y"); + EXPECT_EQ(held.position().column, 2); +} + +TEST(OdfSheetWrite, a_split_sheet_saves_and_reopens) { + const Document document = document_of(flat_sheet( + R"(x)")); + first_sheet(document).set_cell(2, 0, CellValue(41.5, "41.5")); + + std::ostringstream saved; + document.save(saved); + const Document reopened = document_of(saved.str()); + const Sheet sheet = first_sheet(reopened); + + EXPECT_EQ(sheet.cell(1, 0).value().text(), "x"); + ASSERT_TRUE(sheet.cell(2, 0).value().has_number()); + EXPECT_DOUBLE_EQ(sheet.cell(2, 0).value().number(), 41.5); + EXPECT_EQ(sheet.cell(3, 0).value().text(), "x"); } TEST(OdfSheetWrite, a_formula_cell_refuses_to_be_written) {