From 51aec1f3ef0ae87969efe007bdd30b4e94b4f29c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Wed, 9 Sep 2026 20:22:39 +0200 Subject: [PATCH] feat(spreadsheet): state the cell an xlsx file spells none of An `.xlsx` file spells a `c` only for a cell that holds something, so a write into an empty position refused. The page carries no lock on such a cell, so the host got the refusal back after the user had typed. `insert_cell` states the `c` in its row in column order, and the `row` in `sheetData` in row order where the file states none, 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. The check reads `mergeCells` again, since the parsed flags only exist for a cell the file states. `TableRange::contains` left the last row and column of the range out, which made the parser's two covered-cell branches disagree: a merge too big to walk cell by cell marked one row and one column too few. The range is closed everywhere else it is read, so `contains` is closed now too. LibreOffice opens a grown workbook and reads the new cells, checked with `soffice --convert-to`. The reference output is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW --- CHANGELOG.md | 10 ++ docs/design/spreadsheet-editing.md | 15 +- src/odr/internal/common/table_range.cpp | 4 +- src/odr/internal/common/table_range.hpp | 1 + src/odr/internal/ooxml/spreadsheet/AGENTS.md | 19 ++- .../ooxml_spreadsheet_document.cpp | 135 ++++++++++++++++-- test/src/internal/common/table_range_test.cpp | 12 ++ .../ooxml/ooxml_spreadsheet_test_util.hpp | 14 +- .../ooxml/ooxml_spreadsheet_write_test.cpp | 110 +++++++++++++- 9 files changed, 288 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b05890b5..659c0d3ec 100644 --- a/CHANGELOG.md +++ b/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, diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 3ed140a44..8e90c54f6 100644 --- a/docs/design/spreadsheet-editing.md +++ b/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 `` node | +| XLSX cells | `Sheet.cells` `(col,row) → {node, id}` map | Off-tree; a position the file states no `` 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 `` (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 ``, 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 `` in column order into its ``, create the - `` in row order, grow ``. +2. **Landed.** XLSX: `insert_cell` states the `` in its `` in + column order, the `` in `sheetData` in row order where the file states + none, and widens `` 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 `` — - 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`. diff --git a/src/odr/internal/common/table_range.cpp b/src/odr/internal/common/table_range.cpp index 5c7839927..0e3acfb04 100644 --- a/src/odr/internal/common/table_range.cpp +++ b/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 diff --git a/src/odr/internal/common/table_range.hpp b/src/odr/internal/common/table_range.hpp index 06eb0dcce..13d95faca 100644 --- a/src/odr/internal/common/table_range.hpp +++ b/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: diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index ea414a7dc..a5e2a6229 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/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. diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index a3373684b..69a51af3b 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -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 get_relations_and_origin(const ElementIdentifier element_id) const { if (element_id == null_element_id) { diff --git a/test/src/internal/common/table_range_test.cpp b/test/src/internal/common/table_range_test.cpp index 823388be1..7c0a069be 100644 --- a/test/src/internal/common/table_range_test.cpp +++ b/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})); +} diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp index ac1fdf572..9c7f309e3 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp +++ b/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 `` is /// @p sheet_data and which carries @p sheet_extra - ``, say - -/// after it. @p shared_strings writes a `sharedStrings.xml` where it is given, -/// and @p workbook_extra follows `` in `workbook.xml`. +/// after it and @p sheet_prefix - `` - before it. +/// @p shared_strings writes a `sharedStrings.xml` where it is given, and +/// @p workbook_extra follows `` in `workbook.xml`. inline std::shared_ptr 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"()"); insert( zip, "xl/worksheets/sheet1.xml", - R"()" - R"()" + - sheet_data + R"()" + sheet_extra + R"()"); + R"()" + + sheet_prefix + R"()" + sheet_data + R"()" + + sheet_extra + R"()"); if (!shared_strings.empty()) { insert( diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp index 92d88c68a..cd19369cf 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -17,6 +18,21 @@ using namespace odr::test::ooxml; namespace { +/// What the sheet holds after a save, which is where the order of the rows and +/// the cells shows. +std::string worksheet_of(const Document &document) { + std::ostringstream saved; + document.save(saved); + const Document reopened = + open(File::from_memory(saved.str())).as_document_file().document(); + std::ostringstream xml; + xml << reopened.as_filesystem() + .open("/xl/worksheets/sheet1.xml") + .stream() + ->rdbuf(); + return xml.str(); +} + constexpr const char *two_shared = R"(0)" R"(0)"; constexpr const char *one_string = R"(same)"; @@ -92,12 +108,102 @@ TEST(OoxmlSpreadsheetWrite, a_covered_cell_refuses_to_be_written) { EXPECT_THROW(sheet.set_cell(1, 0, CellValue("x")), UnsupportedOperation); } -TEST(OoxmlSpreadsheetWrite, an_absent_cell_refuses_to_be_written) { +/// A cell the file states no `c` for is written by stating one. +TEST(OoxmlSpreadsheetWrite, an_absent_cell_is_written) { const Document document = decode(workbook(R"(1)")); const Sheet sheet = first_sheet(document); - EXPECT_THROW(sheet.set_cell(4, 4, CellValue("x")), UnsupportedOperation); + sheet.set_cell(4, 4, CellValue("x")); + + EXPECT_EQ(sheet.cell(4, 4).value().text(), "x"); + EXPECT_EQ(sheet.dimensions().columns, 5); + EXPECT_EQ(sheet.dimensions().rows, 5); +} + +/// 18.3.1.73 states the cells of a row in column order. +TEST(OoxmlSpreadsheetWrite, an_inserted_cell_lands_in_column_order) { + const Document document = + decode(workbook(R"(3)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("a")); + sheet.set_cell(1, 0, CellValue("b")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_EQ(sheet.cell(1, 0).value().text(), "b"); + EXPECT_DOUBLE_EQ(sheet.cell(2, 0).value().number(), 3); + + const std::string xml = worksheet_of(document); + EXPECT_LT(xml.find(R"(r="A1")"), xml.find(R"(r="B1")")); + EXPECT_LT(xml.find(R"(r="B1")"), xml.find(R"(r="C1")")); +} + +/// 18.3.1.80 states the rows of a sheet in row order. +TEST(OoxmlSpreadsheetWrite, an_inserted_row_lands_in_row_order) { + const Document document = + decode(workbook(R"(1)" + R"(3)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 1, CellValue("two")); + + EXPECT_EQ(sheet.cell(0, 1).value().text(), "two"); + + const std::string xml = worksheet_of(document); + EXPECT_LT(xml.find(R"(r="A1")"), xml.find(R"(r="A2")")); + EXPECT_LT(xml.find(R"(r="A2")"), xml.find(R"(r="A3")")); +} + +/// 18.3.1.35 states the range the cells span, so a new cell widens it. +TEST(OoxmlSpreadsheetWrite, an_inserted_cell_widens_the_dimension) { + const Document document = + decode(workbook(R"(1)", "", "", "", + R"()")); + + first_sheet(document).set_cell(3, 3, CellValue("x")); + + EXPECT_NE(worksheet_of(document).find(R"(ref="B2:D4")"), std::string::npos); +} + +/// The anchor answers for the whole range, whether or not the file states a +/// `c` for the position covered. +TEST(OoxmlSpreadsheetWrite, an_absent_covered_cell_refuses_to_be_written) { + const Document document = decode( + workbook(R"(a)" + R"()", + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(1, 0, CellValue("x")), UnsupportedOperation); +} + +/// A `ref` bigger than the cells the sheet states is resolved by walking the +/// cells, and its last row and column are covered like any other. +TEST(OoxmlSpreadsheetWrite, the_last_cell_of_a_wide_merge_refuses_too) { + const Document document = decode( + workbook(R"(a)" + R"(c)", + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(2, 0, CellValue("x")), UnsupportedOperation); +} + +TEST(OoxmlSpreadsheetWrite, an_inserted_cell_saves_and_reopens) { + const Document document = + decode(workbook(R"(1)")); + first_sheet(document).set_cell(2, 3, CellValue(41.5, "41.5")); + + std::ostringstream saved; + document.save(saved); + const Document reopened = + open(File::from_memory(saved.str())).as_document_file().document(); + const Sheet sheet = first_sheet(reopened); + + ASSERT_TRUE(sheet.cell(2, 3).value().has_number()); + EXPECT_DOUBLE_EQ(sheet.cell(2, 3).value().number(), 41.5); + EXPECT_DOUBLE_EQ(sheet.cell(0, 0).value().number(), 1); } TEST(OoxmlSpreadsheetWrite, a_written_workbook_saves_and_reopens) {