feat(spreadsheet): state the cell an xlsx file spells none of

CHANGELOG.md

@@ -16,6 +16,16 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- An `.xlsx` cell the file states no `c` for is written: the write states the
+  `c` in its row in column order, the `row` in `sheetData` in row order where
+  the file states none, and widens `dimension` to hold the new cell. A position
+  a merge covers still refuses, whether or not the file states a `c` for it.
+
+- **Fix**: a cell in the last row or column of a merged range reports
+  `SheetCell::is_covered` as true. `TableRange::contains` left the end of the
+  range out, so an `.xlsx` merge too big to walk cell by cell marked one row
+  and one column too few.
+
 - A `.ods` sheet grows to the position a write names. Past the last cell of a
   row, past the last row, or past both, the write states the rows and the empty
   cells it takes to reach it and declares the columns the sheet stops before,

docs/design/spreadsheet-editing.md

@@ -1,6 +1,6 @@
 # Spreadsheet editing design
 
-Status: **steps 0, 1 and 2.1 landed; 2.2 and 2.3 are next.** This
+Status: **steps 0, 1, 2.1 and 2.2 landed; 2.3 is next.** This
 records why spreadsheet editing is staged the way it is, what the code already
 gives us, and the order the steps go in. It is a plan, not a record — update it
 as steps land.
@@ -44,7 +44,7 @@ results go stale the moment an input changes.
 | ODS sheet growth | `odf_document.cpp::grow_to_cell` | A write past the last row or the last cell of a row appends both, and declares the columns (step 2.1, landed) |
 | XLSX edit | `sheet_set_cell` | Writes a cell value (step 0.2, landed); `text_set_content` is still a no-op |
 | XLSX save | `ooxml_spreadsheet_document.cpp::save` | Writes back the worksheets and `workbook.xml`, copies the rest (step 0.2, landed) |
-| XLSX cells | `Sheet.cells` `(col,row) → {node, id}` map | Off-tree; an empty position has no `<c>` node |
+| XLSX cells | `Sheet.cells` `(col,row) → {node, id}` map | Off-tree; a position the file states no `<c>` for is written by `insert_cell` (step 2.2, landed) |
 | Cell value | `SheetCellAdapter` | `sheet_cell_value` reads the number and the formula (step 0.1, landed); `sheet_cell_value_type` stays the cheap question the renderer asks. Dates, booleans and errors still report `string` |
 | Number formats | — | Not parsed in either engine. ODS shows the producer's cached `text:p`; XLSX shows the raw `<v>` (a date is its serial) |
 | Formulas | `sheet_cell_value` | The expression is read and handed out as a string (step 0.1, landed); nothing parses or evaluates it. XLSX shows the cached `<v>`, ODS the cached `text:p`. `xls` and `numbers` drop the expression at parse time |
@@ -409,8 +409,12 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`.
    name a cell it rendered. A write past what LibreOffice holds (1024 columns,
    1048576 rows) saves a valid package that LibreOffice then drops the cell
    from.
-2. XLSX: insert `<c r="…">` in column order into its `<row>`, create the
-   `<row>` in row order, grow `<dimension ref>`.
+2. **Landed.** XLSX: `insert_cell` states the `<c r="…">` in its `<row>` in
+   column order, the `<row>` in `sheetData` in row order where the file states
+   none, and widens `<dimension ref>` around the new cell. The map is keyed by
+   position, so the insert is local and nothing is reindexed. A position a
+   merge covers refuses before any of it, because Excel ignores what a covered
+   `c` holds.
 3. Rich cells: replace with one plain paragraph, keeping the cell style. The
    `rich` lock stays on a cell with a link or a line break; it goes for
    several runs of the same paragraph.
@@ -488,9 +492,6 @@ Ordered by value over cost; all in step 0 or 1.
   translate time from the neighbours; the browser has to redo it for the
   edited row. Without it an edit into a blank cell shows the left neighbour's
   overflow painting across the new text.
-- **A position the engine cannot write yet** — an `.xlsx` cell with no `<c>` —
-  carries no lock, so the page takes the edit and `Document::edit` throws it
-  back at the host. Step 2.2 closes it; until then, the page says so.
 - **Sheets past the cut** (`spreadsheet_limit`, `spreadsheet_cell_limit`) are
   not in the page and cannot be edited; the mode should say so where a view
   reports a `sheet_cut`.

src/odr/internal/common/table_range.cpp

@@ -28,8 +28,8 @@ std::string TableRange::to_string() const noexcept {
 }
 
 bool TableRange::contains(const TablePosition &position) const noexcept {
-  return m_from.column <= position.column && m_to.column > position.column &&
-         m_from.row <= position.row && m_to.row > position.row;
+  return m_from.column <= position.column && m_to.column >= position.column &&
+         m_from.row <= position.row && m_to.row >= position.row;
 }
 
 } // namespace odr::internal

src/odr/internal/common/table_range.hpp

@@ -16,6 +16,7 @@ class TableRange final {
   [[nodiscard]] const TablePosition &to() const noexcept;
   [[nodiscard]] std::string to_string() const noexcept;
 
+  /// Closed: @ref to is the last position of the range, and it is contained.
   [[nodiscard]] bool contains(const TablePosition &position) const noexcept;
 
 private:

src/odr/internal/ooxml/spreadsheet/AGENTS.md

@@ -54,8 +54,17 @@ it wrote; the elements that read the old children keep their ids and stop being
 reachable, which is the tombstoning the editing design asks for. A shared
 string is **never** written back into `sharedStrings.xml` — every other cell
 indexing that entry would change with it — so the cell becomes
-`t="inlineStr"`. Three cells refuse rather than lose something: one the file
-writes no `c` for, a covered one, and one holding an `f`.
+`t="inlineStr"`. Two cells refuse rather than lose something: a covered one and
+one holding an `f`.
+
+A position the file writes no `c` for is **stated** rather than refused.
+`insert_cell` puts the `c` in its row in column order and, where the file
+states no row either, the `row` in `sheetData` in row order, then widens
+`dimension` around the new cell. Nothing is reindexed: the cell map is keyed by
+position, so an insert touches one entry. A position a merge covers refuses
+first, because Excel ignores what a covered `c` holds. That check reads
+`mergeCells` again rather than the parsed flags, which only exist for a cell
+the file states.
 
 **`save` writes back the parts it can have changed** — every worksheet and
 `workbook.xml` — and byte-copies the rest, as `ooxml/text` does for
@@ -84,6 +93,6 @@ Coverage is in [`README.md`](README.md). Foundational gaps, roughly by value:
 3. **No named/master cell-style inheritance** (`cellStyleXfs` loaded but unused);
    borders rendered as `0.75pt solid` regardless of actual style (`// TODO thin
    only`); cell protection unhandled.
-4. **Writing is one cell value.** `sheet_set_cell` writes a number or a string
-   into a cell the file already spells; `text_set_content` is still a no-op
-   stub. Links and comments/annotations not modelled.
+4. **Writing is one cell value.** `sheet_set_cell` writes a number or a string,
+   into a cell the file spells or one it states; `text_set_content` is still a
+   no-op stub. Links and comments/annotations not modelled.

src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp

@@ -7,6 +7,7 @@
 #include <odr/internal/abstract/filesystem.hpp>
 #include <odr/internal/common/element_adapter.hpp>
 #include <odr/internal/common/file.hpp>
+#include <odr/internal/common/table_range.hpp>
 #include <odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.hpp>
 #include <odr/internal/util/number_util.hpp>
 #include <odr/internal/xml/xml_util.hpp>
@@ -218,19 +219,23 @@ class ElementAdapter final : public AdapterBase {
     const ElementRegistry::Sheet &sheet =
         m_registry->sheet_element_at(element_id);
     const ElementRegistry::Sheet::Cell *cell = sheet.cell(column, row);
-    if (cell == nullptr || cell->element_id == null_element_id) {
-      throw UnsupportedOperation(); // the file spells no `c` here
-    }
-    const ElementIdentifier cell_id = cell->element_id;
-    if (m_registry->sheet_cell_element_at(cell_id).is_covered) {
-      throw UnsupportedOperation(); // the anchor of the merge answers for it
-    }
 
-    pugi::xml_node node = cell->node;
-    if (node.child("f")) {
-      throw UnsupportedOperation(); // its dependants would go stale
+    ElementIdentifier cell_id = null_element_id;
+    if (cell == nullptr) {
+      // the file spells no `c` here, so there is nothing to refuse
+      cell_id = insert_cell(element_id, column, row);
+    } else {
+      cell_id = cell->element_id;
+      if (m_registry->sheet_cell_element_at(cell_id).is_covered) {
+        throw UnsupportedOperation(); // the anchor of the merge answers for it
+      }
+      if (cell->node.child("f")) {
+        throw UnsupportedOperation(); // its dependants would go stale
+      }
     }
 
+    pugi::xml_node node = get_node(cell_id);
+
     // the elements over the old children keep their ids and stop being
     // reachable
     while (const pugi::xml_node child = node.first_child()) {
@@ -498,6 +503,116 @@ class ElementAdapter final : public AdapterBase {
     return m_registry->element_at(element_id).node;
   }
 
+  /// A new `row` in @p sheet_data, before the first one past @p row: 18.3.1.80
+  /// states them in row order.
+  static pugi::xml_node insert_row_node(pugi::xml_node sheet_data,
+                                        const std::uint32_t row) {
+    for (const pugi::xml_node child : sheet_data.children("row")) {
+      if (child.attribute("r").as_uint() > row + 1) {
+        return sheet_data.insert_child_before("row", child);
+      }
+    }
+    return sheet_data.append_child("row");
+  }
+
+  /// A new `c` in @p row_node, before the first one past @p column: 18.3.1.73
+  /// states them in column order.
+  static pugi::xml_node insert_cell_node(pugi::xml_node row_node,
+                                         const std::uint32_t column) {
+    for (const pugi::xml_node child : row_node.children("c")) {
+      if (TablePosition(child.attribute("r").value()).column > column) {
+        return row_node.insert_child_before("c", child);
+      }
+    }
+    return row_node.append_child("c");
+  }
+
+  /// Widens the sheet's extent and its `dimension` (18.3.1.35) to hold
+  /// @p position.
+  static void grow_dimension(const pugi::xml_node sheet_node,
+                             ElementRegistry::Sheet &sheet,
+                             const TablePosition &position) {
+    if (position.column < sheet.dimensions.columns &&
+        position.row < sheet.dimensions.rows) {
+      return;
+    }
+    sheet.dimensions.columns =
+        std::max(sheet.dimensions.columns, position.column + 1);
+    sheet.dimensions.rows = std::max(sheet.dimensions.rows, position.row + 1);
+
+    pugi::xml_attribute ref = sheet_node.child("dimension").attribute("ref");
+    if (!ref) {
+      return; // it is optional, and a reader without it takes the cells
+    }
+    const std::string value = ref.value();
+    const TablePosition stated = value.find(':') == std::string::npos
+                                     ? TablePosition(value)
+                                     : TableRange(value).from();
+    const TableRange grown(
+        TablePosition(std::min(stated.column, position.column),
+                      std::min(stated.row, position.row)),
+        TablePosition(sheet.dimensions.columns - 1, sheet.dimensions.rows - 1));
+    ref.set_value(grown.to_string().c_str());
+  }
+
+  /// Whether a merge covers @p position without anchoring it - Excel ignores
+  /// what a covered `c` holds.
+  static bool is_covered_by_merge(const pugi::xml_node sheet_node,
+                                  const TablePosition &position) {
+    for (const pugi::xml_node merge_node :
+         sheet_node.child("mergeCells").children("mergeCell")) {
+      const std::string ref = merge_node.attribute("ref").value();
+      if (ref.find(':') == std::string::npos) {
+        continue;
+      }
+      const TableRange range(ref);
+      if (range.contains(position) && !(position == range.from())) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /// The `c` of (@p column, @p row), and the `row` around it, stated where the
+  /// file states neither.
+  [[nodiscard]] ElementIdentifier insert_cell(const ElementIdentifier sheet_id,
+                                              const std::uint32_t column,
+                                              const std::uint32_t row) const {
+    const pugi::xml_node sheet_node = get_node(sheet_id);
+    const TablePosition position(column, row);
+    if (is_covered_by_merge(sheet_node, position)) {
+      throw UnsupportedOperation();
+    }
+
+    ElementRegistry::Sheet &sheet = m_registry->sheet_element_at(sheet_id);
+
+    pugi::xml_node row_node;
+    if (const ElementRegistry::Sheet::Row *row_entry = sheet.row(row);
+        row_entry != nullptr) {
+      row_node = row_entry->node;
+    } else {
+      pugi::xml_node sheet_data = sheet_node.child("sheetData");
+      if (!sheet_data) {
+        throw UnsupportedOperation(); // 18.3.1.99 states one for every sheet
+      }
+      row_node = insert_row_node(sheet_data, row);
+      row_node.append_attribute("r").set_value(row + 1);
+      sheet.register_row(row, row_node);
+    }
+
+    pugi::xml_node cell_node = insert_cell_node(row_node, column);
+    cell_node.append_attribute("r").set_value(position.to_string().c_str());
+
+    const auto &[cell_id, unused1, unused2] =
+        m_registry->create_sheet_cell_element(cell_node, position);
+    m_registry->append_sheet_cell(sheet_id, cell_id);
+    sheet.register_cell(column, row, cell_node, cell_id);
+
+    grow_dimension(sheet_node, sheet, position);
+
+    return cell_id;
+  }
+
   [[nodiscard]] std::pair<const Relations *, AbsPath>
   get_relations_and_origin(const ElementIdentifier element_id) const {
     if (element_id == null_element_id) {

test/src/internal/common/table_range_test.cpp

@@ -24,3 +24,15 @@ TEST(TableRange, string1) {
   EXPECT_EQ(54, tr.to().row);
   EXPECT_EQ(input, tr.to_string());
 }
+
+/// `to` is the last position of the range, not one past it.
+TEST(TableRange, contains) {
+  const TableRange tr("B2:D4");
+  EXPECT_TRUE(tr.contains({1, 1}));
+  EXPECT_TRUE(tr.contains({3, 3}));
+  EXPECT_TRUE(tr.contains({2, 3}));
+  EXPECT_FALSE(tr.contains({0, 1}));
+  EXPECT_FALSE(tr.contains({1, 0}));
+  EXPECT_FALSE(tr.contains({4, 3}));
+  EXPECT_FALSE(tr.contains({3, 4}));
+}

test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp

@@ -25,12 +25,14 @@ inline void insert(internal::zip::ZipArchive &zip, const std::string &path,
 
 /// The smallest workbook that opens: one sheet, whose `<sheetData>` is
 /// @p sheet_data and which carries @p sheet_extra - `<mergeCells>`, say -
-/// after it. @p shared_strings writes a `sharedStrings.xml` where it is given,
-/// and @p workbook_extra follows `<sheets>` in `workbook.xml`.
+/// after it and @p sheet_prefix - `<dimension>` - before it.
+/// @p shared_strings writes a `sharedStrings.xml` where it is given, and
+/// @p workbook_extra follows `<sheets>` in `workbook.xml`.
 inline std::shared_ptr<internal::abstract::File>
 workbook(const std::string &sheet_data, const std::string &sheet_extra = "",
          const std::string &shared_strings = "",
-         const std::string &workbook_extra = "") {
+         const std::string &workbook_extra = "",
+         const std::string &sheet_prefix = "") {
   internal::zip::ZipArchive zip;
   insert(
       zip, "[Content_Types].xml",
@@ -59,9 +61,9 @@ workbook(const std::string &sheet_data, const std::string &sheet_extra = "",
       R"(<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"/>)");
   insert(
       zip, "xl/worksheets/sheet1.xml",
-      R"(<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">)"
-      R"(<sheetData>)" +
-          sheet_data + R"(</sheetData>)" + sheet_extra + R"(</worksheet>)");
+      R"(<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">)" +
+          sheet_prefix + R"(<sheetData>)" + sheet_data + R"(</sheetData>)" +
+          sheet_extra + R"(</worksheet>)");
 
   if (!shared_strings.empty()) {
     insert(