fix(odf): address a repeated sheet cell by position, not by index

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.

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);

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

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) {

src/odr/internal/odf/odf_element_registry.cpp

@@ -44,9 +44,27 @@ std::tuple<ElementIdentifier, ElementRegistry::Element &,
 ElementRegistry::create_sheet_element(const pugi::xml_node node) {
   const auto &[element_id, element] = create_element(ElementType::sheet, node);
   Sheet &sheet = m_sheets.emplace(element_id, Sheet{});
+  sheet.ordinal = static_cast<std::uint32_t>(m_sheet_ids.size());
+  m_sheet_ids.push_back(static_cast<StoredId>(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<ElementIdentifier, ElementRegistry::Element &,
            ElementRegistry::SheetCell &>
 ElementRegistry::create_sheet_cell_element(const pugi::xml_node node,

src/odr/internal/odf/odf_element_registry.hpp

@@ -25,6 +25,50 @@ struct RegistryElement final : ElementNode<StoredId> {
   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<ElementIdentifier>(ordinal) << ordinal_shift |
+         static_cast<ElementIdentifier>(column) << column_shift |
+         static_cast<ElementIdentifier>(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<std::uint32_t>(id >> ordinal_shift & ordinal_max);
+}
+
+constexpr std::uint32_t column_of(const ElementIdentifier id) noexcept {
+  return static_cast<std::uint32_t>(id >> column_shift & column_max);
+}
+
+constexpr std::uint32_t row_of(const ElementIdentifier id) noexcept {
+  return static_cast<std::uint32_t>(id & row_max);
+}
+
+} // namespace positional_id
+
 class ElementRegistry final
     : public internal::ElementRegistry<RegistryElement, StoredId> {
 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<Column> columns;
     std::vector<Row> rows;
     std::vector<Cell> 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<StoredId> m_sheet_ids;
+
   SortedSideTable<Text, StoredId> m_texts;
   SortedSideTable<Table, StoredId> m_tables;
   SortedSideTable<Sheet, StoredId> m_sheets;

test/src/internal/odf/odf_sheet_repeat_test.cpp

@@ -3,9 +3,11 @@
 
 #include <odr/document.hpp>
 #include <odr/document_element.hpp>
+#include <odr/document_path.hpp>
 #include <odr/file.hpp>
 #include <odr/logger.hpp>
 #include <odr/table_dimension.hpp>
+#include <odr/table_position.hpp>
 
 #include <odr/internal/abstract/document.hpp>
 #include <odr/internal/abstract/file.hpp>
@@ -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<abstract::Document> 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<abstract::Document> 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<abstract::Document> 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<abstract::Document> 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);
+}