From 6a041fa75eb0c93740c8905431dd6e6ad4007894 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 7 Sep 2026 21:08:27 +0200 Subject: [PATCH] fix(odf): address a repeated sheet cell by position, not by index One element stands for every position a repeat covers, so an index cannot say which of them a handle means: `SheetCell::position()` answered with the anchor of the range, `Sheet::cell()` handed back the same handle throughout it, and `DocumentPath` named a cell the caller had not asked for. `sheet_cell(sheet, column, row)` now hands out `tag | ordinal | column | row` for a repeat, and the registry decodes it against the sheet's cell index. A cell standing for one position alone keeps its index, so nothing but a repeat pays for the indirection and the store stays a flat array. The decode is the shared `ElementRegistry::resolve_id`, identity for every engine that does not shadow it. `RegistryElementAdapter` navigates through `element_at`, so nothing else has to know, and `element_parent` drops the position on its own - a sheet is stored as a plain index. Because the decode goes through the index rather than the element it found, a handle follows the index. `extract_path` already spells a cell by position and `navigate_path` already goes back through `sheet_cell`, so a path names the right cell untouched. The position stops at the cell: one paragraph and one run stand for every position, so a path into a repeated cell still names the anchor. Cell style now resolves at the position the id carries rather than the payload's, since the anchor sits in another column and `table:default-cell-style-name` is a column's to state. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J325TWocZ4iXBjvKv2ZVZi --- CHANGELOG.md | 4 + src/odr/internal/common/element_registry.hpp | 12 ++- src/odr/internal/odf/AGENTS.md | 18 ++++- src/odr/internal/odf/odf_document.cpp | 23 +++--- src/odr/internal/odf/odf_element_registry.cpp | 18 +++++ src/odr/internal/odf/odf_element_registry.hpp | 78 +++++++++++++++++-- .../internal/odf/odf_sheet_repeat_test.cpp | 54 +++++++++++++ 7 files changed, 184 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 563d9eccf..42602ff5a 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 +- **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. + - `Sheet::set_cell` and `::clear_cell` write one cell of an `.ods` or an `.xlsx`, and `Document::is_editable` is true for both. An absent, repeated, covered, formula or richly marked-up cell refuses. diff --git a/src/odr/internal/common/element_registry.hpp b/src/odr/internal/common/element_registry.hpp index 0b428ead7..4e91a1d0a 100644 --- a/src/odr/internal/common/element_registry.hpp +++ b/src/odr/internal/common/element_registry.hpp @@ -104,9 +104,16 @@ class ElementRegistry { [[nodiscard]] std::size_t size() const noexcept { return m_elements.size(); } + /// The index @p id names; an engine whose ids are not all indices shadows it. + [[nodiscard]] static ElementIdentifier + resolve_id(const ElementIdentifier id) noexcept { + return id; + } + [[nodiscard]] auto &element_at(this auto &self, const ElementIdentifier id) { - self.check_element_id(id); - return self.m_elements[id - 1]; + const ElementIdentifier index = self.resolve_id(id); + self.check_element_id(index); + return self.m_elements[index - 1]; } void append_child(const ElementIdentifier parent_id, @@ -133,6 +140,7 @@ class ElementRegistry { /// Links @p child_id as the last child of the chain @p first_id / @p last_id /// - the element's own, or one of the secondary chains a payload holds. + /// Both ids are indices, not whatever @ref resolve_id accepts. void link_child(const ElementIdentifier parent_id, const ElementIdentifier child_id, Id &first_id, Id &last_id) { Element &child = element_at(child_id); diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index c55a926bf..770d0b430 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -62,8 +62,22 @@ over, and resolved with an upper bound — whether or not the cell has content. That last part is load-bearing: expanding a repeat per position let a 400-byte document ask for a `1048576 × 1024` grid of elements, both counts being legal repeats. Only non-empty cells get a real `sheet_cell` Element; empty ones are -recorded as ranges alone. Cells carry a `TablePosition` (the anchor of the -range, not each position it covers) + `is_repeated` flag. +recorded as ranges alone. The `SheetCell` payload carries the anchor's +`TablePosition` + an `is_repeated` flag. + +**A repeated cell is addressed by position, not by index.** One element for +many positions means an index cannot say which of them a handle means, so +`sheet_cell` hands out `tag | ordinal(15) | column(24) | row(24)` +(`positional_id`) for a repeat and the index itself otherwise. `resolve_id` +decodes it against the sheet's cell index — the shared +`ElementRegistry::resolve_id` hook, identity for everyone else — and +`RegistryElementAdapter` navigates through `element_at`, so nothing else needs +to know. + +Three consequences: `SheetCell::position()` and `DocumentPath` name the cell +asked for; a handle follows the index rather than the element it found, so a +run can be split under one; and children are **shared**, so the position stops +at the cell and a path to a run inside one names the anchor. The three containers are **sorted vectors, not maps**: parsing appends in document order, so the keys only grow, and a rb-tree node costs more than the 12 diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 3554c90a5..d6752b7f7 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -356,14 +356,7 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] ElementIdentifier sheet_cell(const ElementIdentifier element_id, const std::uint32_t column, const std::uint32_t row) const override { - const ElementRegistry::Sheet &sheet_registry = - m_registry->sheet_element_at(element_id); - if (const ElementRegistry::Sheet::Cell *sheet_cell = - sheet_registry.cell(column, row); - sheet_cell != nullptr) { - return sheet_cell->element_id; - } - return {}; + return m_registry->sheet_cell_id(element_id, column, row); } [[nodiscard]] ElementIdentifier sheet_first_shape(const ElementIdentifier element_id) const override { @@ -464,6 +457,10 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] TablePosition sheet_cell_position(const ElementIdentifier element_id) const override { + if (positional_id::holds(element_id)) { + return {positional_id::column_of(element_id), + positional_id::row_of(element_id)}; + } return m_registry->sheet_cell_element_at(element_id).position; } [[nodiscard]] bool @@ -1025,12 +1022,10 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] ResolvedStyle get_partial_style(const ElementIdentifier element_id) const { - if (const ElementRegistry::SheetCell *cell_registry = - m_registry->sheet_cell_element(element_id); - cell_registry != nullptr) { - const ElementIdentifier parent_id = element_parent(element_id); - return get_partial_cell_style(parent_id, element_id, - cell_registry->position); + if (m_registry->sheet_cell_element(element_id) != nullptr) { + // the id's position, not the anchor's: the column default is per column + return get_partial_cell_style(element_parent(element_id), element_id, + sheet_cell_position(element_id)); } if (const char *style_name = get_style_name(element_id); style_name != nullptr) { diff --git a/src/odr/internal/odf/odf_element_registry.cpp b/src/odr/internal/odf/odf_element_registry.cpp index caf7afebd..db9791d24 100644 --- a/src/odr/internal/odf/odf_element_registry.cpp +++ b/src/odr/internal/odf/odf_element_registry.cpp @@ -44,9 +44,27 @@ std::tuple(m_sheet_ids.size()); + m_sheet_ids.push_back(static_cast(element_id)); return {element_id, element, sheet}; } +ElementIdentifier +ElementRegistry::sheet_cell_id(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { + const Sheet &sheet = sheet_element_at(sheet_id); + const Sheet::Cell *cell = sheet.cell(column, row); + if (cell == nullptr || cell->element_id == null_element_id) { + return null_element_id; + } + if (!m_sheet_cells.at(cell->element_id).is_repeated) { + return cell->element_id; + } + const ElementIdentifier id = positional_id::make(sheet.ordinal, column, row); + return id != null_element_id ? id : cell->element_id; +} + std::tuple ElementRegistry::create_sheet_cell_element(const pugi::xml_node node, diff --git a/src/odr/internal/odf/odf_element_registry.hpp b/src/odr/internal/odf/odf_element_registry.hpp index e638ed5d4..236f0e9a0 100644 --- a/src/odr/internal/odf/odf_element_registry.hpp +++ b/src/odr/internal/odf/odf_element_registry.hpp @@ -25,6 +25,50 @@ struct RegistryElement final : ElementNode { pugi::xml_node node; }; +/// One element stands for every position a repeat covers ([ODF 1.2] 19.297, +/// 19.302), so the position rides in the id rather than an index. Resolved +/// through the sheet's cell index, so a handle follows it. +namespace positional_id { + +/// `tag | ordinal(15) | column(24) | row(24)`, the ordinal in document order. +constexpr ElementIdentifier tag = ElementIdentifier{1} << 63; +constexpr std::uint64_t column_shift = 24; +constexpr std::uint64_t ordinal_shift = 48; +constexpr std::uint64_t row_max = (std::uint64_t{1} << column_shift) - 1; +constexpr std::uint64_t column_max = + (std::uint64_t{1} << (ordinal_shift - column_shift)) - 1; +constexpr std::uint64_t ordinal_max = (std::uint64_t{1} << 15) - 1; + +/// Null where a field is too wide; the caller then keeps the index. +constexpr ElementIdentifier make(const std::uint32_t ordinal, + const std::uint32_t column, + const std::uint32_t row) noexcept { + if (ordinal > ordinal_max || column > column_max || row > row_max) { + return null_element_id; + } + return tag | static_cast(ordinal) << ordinal_shift | + static_cast(column) << column_shift | + static_cast(row); +} + +constexpr bool holds(const ElementIdentifier id) noexcept { + return (id & tag) != 0; +} + +constexpr std::uint32_t ordinal_of(const ElementIdentifier id) noexcept { + return static_cast(id >> ordinal_shift & ordinal_max); +} + +constexpr std::uint32_t column_of(const ElementIdentifier id) noexcept { + return static_cast(id >> column_shift & column_max); +} + +constexpr std::uint32_t row_of(const ElementIdentifier id) noexcept { + return static_cast(id & row_max); +} + +} // namespace positional_id + class ElementRegistry final : public internal::ElementRegistry { public: @@ -61,6 +105,9 @@ class ElementRegistry final TableDimensions dimensions; + /// Its place in document order, which is what a cell id names it by. + std::uint32_t ordinal{0}; + std::vector columns; std::vector rows; std::vector cells; @@ -112,29 +159,47 @@ class ElementRegistry final create_sheet_cell_element(pugi::xml_node node, const TablePosition &position, bool is_repeated); + /// Null where the position no longer holds a cell. + [[nodiscard]] ElementIdentifier resolve_id(const ElementIdentifier id) const { + if (!positional_id::holds(id)) { + return id; + } + const Sheet &sheet = + m_sheets.at(m_sheet_ids.at(positional_id::ordinal_of(id))); + const Sheet::Cell *cell = + sheet.cell(positional_id::column_of(id), positional_id::row_of(id)); + return cell != nullptr ? cell->element_id : null_element_id; + } + [[nodiscard]] auto &text_element_at(this auto &self, const ElementIdentifier id) { - return self.m_texts.at(id); + return self.m_texts.at(self.resolve_id(id)); } [[nodiscard]] auto &table_element_at(this auto &self, const ElementIdentifier id) { - return self.m_tables.at(id); + return self.m_tables.at(self.resolve_id(id)); } [[nodiscard]] auto &sheet_element_at(this auto &self, const ElementIdentifier id) { - return self.m_sheets.at(id); + 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(id); + return m_sheet_cells.at(resolve_id(id)); } [[nodiscard]] const SheetCell * sheet_cell_element(const ElementIdentifier id) const { - return m_sheet_cells.find(id); + return m_sheet_cells.find(resolve_id(id)); } + /// The id a handle for (@p column, @p row) carries - the index itself unless + /// the cell there is repeated. + [[nodiscard]] ElementIdentifier sheet_cell_id(ElementIdentifier sheet_id, + std::uint32_t column, + std::uint32_t row) const; + [[nodiscard]] ShapeType shape_type(ElementIdentifier id) const; void set_list_type(ElementIdentifier id, ListType type); @@ -148,6 +213,9 @@ class ElementRegistry final void append_sheet_cell(ElementIdentifier sheet_id, ElementIdentifier cell_id); private: + /// The sheets in document order, so a cell id can name one in 15 bits. + std::vector m_sheet_ids; + SortedSideTable m_texts; SortedSideTable m_tables; SortedSideTable m_sheets; diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp index fb41d315b..83a09c5e2 100644 --- a/test/src/internal/odf/odf_sheet_repeat_test.cpp +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -3,9 +3,11 @@ #include #include +#include #include #include #include +#include #include #include @@ -87,3 +89,55 @@ TEST(OdfSheetRepeat, a_repeated_cell_reads_at_every_position_it_covers) { } } } + +/// The one element cannot say which position a handle means, so the id does. +TEST(OdfSheetRepeat, a_repeated_cell_reports_the_position_it_was_asked_for) { + const std::shared_ptr held = + document_of(flat_sheet(repeated_rows(4, 3))); + const Sheet sheet = + (*odr::Document(held).root_element().children().begin()).as_sheet(); + + for (std::uint32_t row = 0; row < 4; ++row) { + for (std::uint32_t column = 0; column < 3; ++column) { + const TablePosition position = sheet.cell(column, row).position(); + EXPECT_EQ(position.column, column); + EXPECT_EQ(position.row, row); + } + } +} + +TEST(OdfSheetRepeat, two_positions_of_one_run_are_not_the_same_handle) { + const std::shared_ptr held = + document_of(flat_sheet(repeated_rows(4, 3))); + const Sheet sheet = + (*odr::Document(held).root_element().children().begin()).as_sheet(); + + EXPECT_NE(sheet.cell(0, 0), sheet.cell(2, 0)); + EXPECT_EQ(sheet.cell(2, 0), sheet.cell(2, 0)); +} + +/// `DocumentPath` spells a cell by position, so it names the one asked for. +TEST(OdfSheetRepeat, a_repeated_cell_round_trips_through_its_path) { + const std::shared_ptr held = + document_of(flat_sheet(repeated_rows(4, 3))); + const odr::Document document(held); + const Sheet sheet = (*document.root_element().children().begin()).as_sheet(); + + const SheetCell cell = sheet.cell(2, 1); + + EXPECT_EQ(document.root_element().navigate_path(cell.document_path()), cell); +} + +/// The position stops at the cell: one run stands for every position, so a +/// path into a repeated cell names the anchor. +TEST(OdfSheetRepeat, the_children_of_a_repeated_cell_are_shared) { + const std::shared_ptr held = + document_of(flat_sheet(repeated_rows(4, 3))); + const odr::Document document(held); + const Sheet sheet = (*document.root_element().children().begin()).as_sheet(); + + const Element text = + *(*sheet.cell(2, 1).children().begin()).children().begin(); + EXPECT_EQ(*(*sheet.cell(0, 0).children().begin()).children().begin(), text); + EXPECT_EQ(document.root_element().navigate_path(text.document_path()), text); +}