diff --git a/CHANGELOG.md b/CHANGELOG.md index af8bda91a..1b05890b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,10 +16,14 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- 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, + so the extent covers the new cell. + - An empty `.ods` cell can be written: the write cuts the run of empty cells the position belongs to and states the `text:p` the file spells none of. A - merged cell that holds no paragraph takes one the same way. A position past - the last cell the file states still refuses. + merged cell that holds no paragraph takes one the same way. - A sheet cell can be typed into: an overlay opens on a double click or a key, Enter and Tab commit, and `odr.editing.getOperations()` hands the host the diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index a6c05c461..3ed140a44 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -1,6 +1,6 @@ # Spreadsheet editing design -Status: **steps 0 and 1 landed, and 2.1 with them; step 2 is under way.** This +Status: **steps 0, 1 and 2.1 landed; 2.2 and 2.3 are 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. @@ -41,6 +41,7 @@ results go stale the moment an input changes. | ODS save | `odf_document.cpp::save` | Re-serialises `content.xml`, byte-copies the rest — the same shape a sheet needs | | ODS cell index | `odf_element_registry.cpp::Sheet::register_cell` | Per row a run of `(end, element_id, node)` entries; repeats collapse onto one entry. Written once at parse; nothing inserts | | ODS repeated and empty cells | `odf_document.cpp::claim_cell` | A write cuts the run and states the `text:p` an empty cell has none of; `reindex_sheet` rebuilds the index (step 2.1, landed) | +| 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 | @@ -396,8 +397,18 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. paragraph takes one too: the page reads it as editable, so the engine has to agree. - **Open:** a position past the row's last cell or the sheet's last row needs - appending and growing the extent. + **Landed for a position past the sheet.** `grow_to_cell` + appends the rows and the empty cells it takes to reach the position — a + repeated row is cut first, because its cells stand for every row it repeats + over — and `grow_columns` declares the columns the sheet stops before, so + `sheet_dimensions` covers the new cell. A `table:table-row` goes before + `table:named-expressions` and a `table:table-column` before the rows, which + is where [ODF 1.2] 9.1.2 orders them. + + Nothing caps the position: ODF states no grid limit, and the page can only + 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 ``. 3. Rich cells: replace with one plain paragraph, keeping the cell style. The @@ -477,10 +488,9 @@ 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 ``, - an `.ods` one past the last the file states — carries no lock, so the page - takes the edit and `Document::edit` throws it back at the host. Until step 2, - it says so. +- **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/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 3633e7cea..406e63bc2 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -188,7 +188,8 @@ The structural/foundational gaps, roughly by value: 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 holding a formula or richer markup than one plain - paragraph refuses, as does a position past the last cell the file states. + paragraph refuses; a position never does, because the write reaches any of + them. A **repeated** cell is written by cutting the run: `claim_cell` copies the `table:table-row` and the `table:table-cell` around the position and leaves @@ -202,7 +203,14 @@ The structural/foundational gaps, roughly by value: `text:p` *before* the reindex, which then sees a node that is not empty and builds the element for it. A spanned cell that holds no paragraph takes one from `text_run_of` instead, because the page already reads it as editable. - Two costs: + + A position the sheet stops before is reached by `grow_to_cell`, which + appends the rows and the runs of empty cells it takes and declares the + columns, so `dimensions` covers the new cell. A repeated row is cut before + a cell is appended to it, because its cells stand for every row it repeats + over. Nothing caps the position — ODF states no grid limit — so a write far + past what LibreOffice holds saves a valid package, and LibreOffice drops + that cell. 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. diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index d785dece3..634440ef1 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -19,12 +19,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -370,22 +372,26 @@ class ElementAdapter final : public AdapterBase { const CellValue &value) const override { const ElementRegistry::Sheet::Cell *cell = m_registry->sheet_element_at(element_id).cell(column, row); - if (cell == nullptr) { - throw UnsupportedOperation(); // the sheet states no node here - } - ElementIdentifier cell_id = cell->element_id; - // both refusals are decided before the split writes anything - if (cell->node.attribute("table:formula")) { - throw UnsupportedOperation(); // its dependants would go stale - } - if (cell_id != null_element_id && !holds_one_run(cell_id)) { - throw UnsupportedOperation(); - } + ElementIdentifier cell_id = null_element_id; + if (cell == nullptr) { + // the sheet stops before the position, so there is nothing to refuse + cell_id = grow_to_cell(element_id, column, row); + } else { + cell_id = cell->element_id; + + // both refusals are decided before the split writes anything + if (cell->node.attribute("table:formula")) { + throw UnsupportedOperation(); // its dependants would go stale + } + if (cell_id != null_element_id && !holds_one_run(cell_id)) { + throw UnsupportedOperation(); + } - if (cell_id == null_element_id || - m_registry->sheet_cell_element_at(cell_id).is_repeated) { - cell_id = claim_cell(element_id, column, row); // `cell` is stale after + if (cell_id == null_element_id || + m_registry->sheet_cell_element_at(cell_id).is_repeated) { + cell_id = claim_cell(element_id, column, row); // `cell` is stale after + } } pugi::xml_node node = get_node(cell_id); @@ -912,6 +918,21 @@ class ElementAdapter final : public AdapterBase { set_repeat(node, attribute, 1); } + /// Cuts the row run @p row is one position of, so its node stands for that + /// row alone, and hands that node back. The caller reindexes. + static pugi::xml_node split_row_at(const ElementRegistry::Sheet &sheet, + const std::uint32_t row) { + 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; + + split_run(row_entry->node, "table:number-rows-repeated", row_begin, + row_entry->end, row); + + return row_entry->node; + } + /// Gives (@p column, @p row) an element of its own: cuts the row and the /// cell run it is one position of, and states the `text:p` an empty cell /// has none of. Reindexes: every pointer read before is stale. @@ -921,13 +942,8 @@ class ElementAdapter final : public AdapterBase { 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); + sheet.row_cells(*sheet.row(row)); 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 = @@ -935,8 +951,7 @@ class ElementAdapter final : public AdapterBase { pugi::xml_node cell_node = cell_entry->node; // 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_row_at(sheet, row); split_run(cell_node, "table:number-columns-repeated", cell_begin, cell_entry->end, column); @@ -949,6 +964,106 @@ class ElementAdapter final : public AdapterBase { return m_registry->sheet_element_at(sheet_id).cell(column, row)->element_id; } + /// What has to follow a `table:table-row` under a `table:table`, and what + /// has to follow a `table:table-column` ([ODF 1.2] 9.1.2 orders them). + static constexpr std::array after_rows{ + std::string_view("table:named-expressions")}; + static constexpr std::array after_columns{ + std::string_view("table:table-header-rows"), + std::string_view("table:table-rows"), + std::string_view("table:table-row-group"), + std::string_view("table:table-row"), + std::string_view("table:named-expressions")}; + + /// A new @p name child of @p table, before the first one that has to follow + /// it. + static pugi::xml_node + insert_ordered(pugi::xml_node table, const char *name, + const std::span after) { + for (const pugi::xml_node child : table.children()) { + if (std::ranges::find(after, std::string_view(child.name())) != + std::end(after)) { + return table.insert_child_before(name, child); + } + } + return table.append_child(name); + } + + static void append_empty_cells(pugi::xml_node row, + const std::uint32_t repeated) { + set_repeat(row.append_child("table:table-cell"), + "table:number-columns-repeated", repeated); + } + + /// Declares the columns the sheet stops before, so its extent reaches + /// @p column ([ODF 1.2] 9.1.6). + static void grow_columns(pugi::xml_node sheet_node, + ElementRegistry::Sheet &sheet, + const std::uint32_t column) { + if (column < sheet.dimensions.columns) { + return; + } + const std::uint32_t repeated = column + 1 - sheet.dimensions.columns; + + // every declaration comes before the rows, so this lands after all of them + pugi::xml_node node = + insert_ordered(sheet_node, "table:table-column", after_columns); + set_repeat(node, "table:number-columns-repeated", repeated); + + sheet.register_column(sheet.dimensions.columns, repeated, node); + sheet.dimensions.columns = column + 1; + } + + /// States the rows and the cells the sheet stops before, so (@p column, + /// @p row) is a cell of its own holding the `text:p` a value needs. + /// Reindexes: every pointer read before is stale. + [[nodiscard]] ElementIdentifier grow_to_cell(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { + ElementRegistry::Sheet &sheet = m_registry->sheet_element_at(sheet_id); + pugi::xml_node sheet_node = get_node(sheet_id); + + pugi::xml_node row_node; + std::uint32_t cells_end = 0; + + if (const ElementRegistry::Sheet::Row *row_entry = sheet.row(row); + row_entry == nullptr) { + const std::uint32_t rows_end = + sheet.rows.empty() ? 0 : sheet.rows.back().end; + if (row > rows_end) { + // a row states at least one cell, so the filler holds an empty one + pugi::xml_node filler = + insert_ordered(sheet_node, "table:table-row", after_rows); + set_repeat(filler, "table:number-rows-repeated", row - rows_end); + append_empty_cells(filler, sheet.dimensions.columns); + } + row_node = insert_ordered(sheet_node, "table:table-row", after_rows); + } else { + const std::span cells = + sheet.row_cells(*row_entry); + cells_end = cells.empty() ? 0 : cells.back().end; + // its cells stand for every position the row repeats over + row_node = split_row_at(sheet, row); + } + + if (column > cells_end) { + append_empty_cells(row_node, column - cells_end); + } + row_node.append_child("table:table-cell").append_child("text:p"); + + grow_columns(sheet_node, sheet, column); + reindex_sheet(*m_registry, sheet_id); + + const ElementRegistry::Sheet::Cell *cell = + m_registry->sheet_element_at(sheet_id).cell(column, row); + if (cell == nullptr || cell->element_id == null_element_id) { + // a rowspan out of an earlier row can push the cell off the position; + // what was appended is empty, so no reader sees a difference + throw UnsupportedOperation(); + } + return cell->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 { diff --git a/test/src/document_edit_test.cpp b/test/src/document_edit_test.cpp index 470cc990c..398047a35 100644 --- a/test/src/document_edit_test.cpp +++ b/test/src/document_edit_test.cpp @@ -133,7 +133,7 @@ TEST(DocumentEdit, the_ops_before_a_refusal_are_applied) { document.edit(R"({"version":1,"ops":[)" R"({"op":"setCell","sheet":0,"column":0,"row":0,)" R"("value":{"type":"string","text":"written"}},)" - R"({"op":"setCell","sheet":0,"column":9,"row":9,)" + R"({"op":"setCell","sheet":9,"column":0,"row":0,)" R"("value":{"type":"string","text":"absent"}}]})")); EXPECT_EQ(first_sheet(document).cell(0, 0).value().text(), "written"); diff --git a/test/src/internal/odf/odf_sheet_write_test.cpp b/test/src/internal/odf/odf_sheet_write_test.cpp index e1ede1446..d14fd7330 100644 --- a/test/src/internal/odf/odf_sheet_write_test.cpp +++ b/test/src/internal/odf/odf_sheet_write_test.cpp @@ -208,12 +208,156 @@ TEST(OdfSheetWrite, a_formula_cell_refuses_to_be_written) { EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); } -/// Past the last cell the file states there is no node to write into. -TEST(OdfSheetWrite, a_cell_past_the_sheet_refuses_to_be_written) { +/// Past the last cell the file states, the write states the cells it takes to +/// reach the position. +TEST(OdfSheetWrite, a_cell_past_the_row_grows_the_row) { const Document document = document_of(flat_sheet(string_cell("a"))); const Sheet sheet = first_sheet(document); - EXPECT_THROW(sheet.set_cell(4, 4, CellValue("y")), UnsupportedOperation); + sheet.set_cell(3, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(3, 0).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_FALSE(sheet.cell(1, 0).value().has_text()); + EXPECT_EQ(sheet.dimensions().columns, 4); + EXPECT_EQ(sheet.dimensions().rows, 1); +} + +TEST(OdfSheetWrite, a_cell_past_the_last_row_grows_the_sheet) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 2, CellValue("y")); + + EXPECT_EQ(sheet.cell(0, 2).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_FALSE(sheet.cell(0, 1).value().has_text()); + EXPECT_EQ(sheet.dimensions().rows, 3); +} + +TEST(OdfSheetWrite, a_cell_past_both_grows_both) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(4, 4, CellValue(7, "7")); + + const CellValue value = sheet.cell(4, 4).value(); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 7); + EXPECT_EQ(sheet.dimensions().columns, 5); + EXPECT_EQ(sheet.dimensions().rows, 5); +} + +/// A row's cells stand for every position it repeats over, so growing it cuts +/// the run first. +TEST(OdfSheetWrite, growing_a_repeated_row_leaves_the_others_short) { + const Document document = + document_of(R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"(x)" + R"()" + R"()"); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(2, 1, CellValue("y")); + + EXPECT_EQ(sheet.cell(2, 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_FALSE(sheet.cell(2, row).value().has_text()) << row; + } + EXPECT_EQ(sheet.dimensions().rows, 3); +} + +/// A sheet the producer wrote no row for is grown from nothing. +TEST(OdfSheetWrite, an_empty_sheet_grows_a_row) { + const Document document = + document_of(R"()" + R"()" + R"()" + 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.dimensions().rows, 2); + EXPECT_EQ(sheet.dimensions().columns, 2); +} + +/// The columns are declared as well, so the extent survives a reopen. +TEST(OdfSheetWrite, a_grown_sheet_saves_and_reopens) { + const Document document = document_of(flat_sheet(string_cell("a"))); + first_sheet(document).set_cell(3, 2, CellValue("y")); + + std::ostringstream saved; + document.save(saved); + const Document reopened = document_of(saved.str()); + const Sheet sheet = first_sheet(reopened); + + EXPECT_EQ(sheet.cell(3, 2).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_EQ(sheet.dimensions().columns, 4); + EXPECT_EQ(sheet.dimensions().rows, 3); +} + +/// A new declaration goes after the ones the file states, so the columns before +/// it keep their width. +TEST(OdfSheetWrite, a_grown_column_follows_the_declared_ones) { + const Document document = + document_of(R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + + string_cell("a") + + R"()" + R"()"); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(2, 0, CellValue("y")); + + EXPECT_EQ(sheet.dimensions().columns, 3); + + std::ostringstream saved; + document.save(saved); + const std::string xml = saved.str(); + EXPECT_LT( + xml.find(R"(table:style-name="co1")"), + xml.find(R"()" + R"()" + R"()" + R"()" + + string_cell("a") + + R"()" + R"()"); + first_sheet(document).set_cell(0, 1, CellValue("y")); + + std::ostringstream saved; + document.save(saved); + const std::string xml = saved.str(); + EXPECT_LT(xml.rfind("