diff --git a/CHANGELOG.md b/CHANGELOG.md index 1324af4da..af8bda91a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- 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. + - 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 envelope `Document::edit` takes. diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index ed3515e49..a6c05c461 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 next.** This +Status: **steps 0 and 1 landed, and 2.1 with them; step 2 is under way.** 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. @@ -40,7 +40,7 @@ results go stale the moment an input changes. | ODS string-cell edit | `odf_document.cpp::text_set_content` | Works: `Document.edit_ods_diff` edits five cells in memory. Only the run's text changes; `office:value` on a number cell is not touched | | 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 cells | `odf_document.cpp::split_repeat` | A write cuts the run and `reindex_sheet` rebuilds the index (step 2.1, landed) | +| 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) | | 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 | @@ -389,10 +389,15 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. rebuilds it off the dom, a cell node keeping the element it carries — which avoids a second copy of the parser's row loop. The `repeated` lock is gone. - **Open:** the same primitive for a position the file states no element for. - A run with a node but no element (``) only needs the split plus a `text:p`; a position past the row's - last cell or the sheet's last row needs appending and growing the extent. + **Landed for empty cells.** A run with a node but no element + (``) is cut the same + way, and the write states the `text:p`, because the reindex gives an element + to a node that is not empty. A cell a merge spans and that holds no + 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. 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 @@ -473,8 +478,9 @@ Ordered by value over cost; all in step 0 or 1. 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 with no element — carries no lock, so the page takes the edit - and `Document::edit` throws it back at the host. Until step 2, it says so. + 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. - **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 ee9182a73..3633e7cea 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -188,14 +188,21 @@ 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 one the file states no element for. + paragraph refuses, as does a position past the last cell the file states. - A **repeated** cell is written by cutting the run: `split_repeat` copies the + 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 the original node as the one written, so its element and children survive. `reindex_sheet` then rebuilds the sheet's position index off the dom, a cell node keeping the element it already carries. Both refusals are decided - before any of that, so a refused write leaves the run uncut. Two costs: + before any of that, so a refused write leaves the run uncut. + + An **empty** cell carries no element, because `index_sheet_rows` builds one + only for a node with content or a span. `claim_cell` therefore appends the + `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: 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 615ff66d9..d785dece3 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -370,21 +370,22 @@ 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 || cell->element_id == null_element_id) { - throw UnsupportedOperation(); // an empty cell is written as no element + if (cell == nullptr) { + throw UnsupportedOperation(); // the sheet states no node here } ElementIdentifier cell_id = cell->element_id; - // both refusals are decided on the run, before the split writes anything - if (get_node(cell_id).attribute("table:formula")) { + // both refusals are decided before the split writes anything + if (cell->node.attribute("table:formula")) { throw UnsupportedOperation(); // its dependants would go stale } - if (!holds_one_run(cell_id)) { + if (cell_id != null_element_id && !holds_one_run(cell_id)) { throw UnsupportedOperation(); } - if (m_registry->sheet_cell_element_at(cell_id).is_repeated) { - cell_id = split_repeat(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); @@ -911,12 +912,12 @@ class ElementAdapter final : public AdapterBase { set_repeat(node, attribute, 1); } - /// Gives (@p column, @p row) a cell of its own, splitting the row and the - /// cell run it is one position of. Reindexes: every pointer read before is - /// stale. - [[nodiscard]] ElementIdentifier split_repeat(const ElementIdentifier sheet_id, - const std::uint32_t column, - const std::uint32_t row) const { + /// 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. + [[nodiscard]] ElementIdentifier claim_cell(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { const ElementRegistry::Sheet &sheet = m_registry->sheet_element_at(sheet_id); @@ -931,13 +932,18 @@ class ElementAdapter final : public AdapterBase { const std::size_t cell_index = cell_entry - cells.data(); const std::uint32_t cell_begin = cell_index == 0 ? 0 : cells[cell_index - 1].end; + 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_run(cell_entry->node, "table:number-columns-repeated", cell_begin, + split_run(cell_node, "table:number-columns-repeated", cell_begin, cell_entry->end, column); + if (!cell_node.first_child()) { + cell_node.append_child("text:p"); // a node with no content gets none + } + reindex_sheet(*m_registry, sheet_id); return m_registry->sheet_element_at(sheet_id).cell(column, row)->element_id; @@ -947,8 +953,10 @@ class ElementAdapter final : public AdapterBase { /// at most. Richer markup is kept rather than overwritten. [[nodiscard]] bool holds_one_run(const ElementIdentifier cell_id) const { const ElementIdentifier paragraph_id = element_first_child(cell_id); - if (paragraph_id == null_element_id || - element_next_sibling(paragraph_id) != null_element_id || + if (paragraph_id == null_element_id) { + return true; // a spanned cell states no paragraph; the write states one + } + if (element_next_sibling(paragraph_id) != null_element_id || element_type(paragraph_id) != ElementType::paragraph) { return false; } @@ -958,11 +966,18 @@ class ElementAdapter final : public AdapterBase { element_type(text_id) == ElementType::text); } - /// That run, created where the paragraph is empty - what a cleared cell is. - /// @ref holds_one_run has to pass. + /// That run, and the paragraph around it, created where the cell states + /// neither. @ref holds_one_run has to pass. [[nodiscard]] ElementIdentifier text_run_of(const ElementIdentifier cell_id) const { - const ElementIdentifier paragraph_id = element_first_child(cell_id); + ElementIdentifier paragraph_id = element_first_child(cell_id); + if (paragraph_id == null_element_id) { + pugi::xml_node cell_node = get_node(cell_id); + const auto &[new_id, unused] = m_registry->create_element( + ElementType::paragraph, cell_node.append_child("text:p")); + m_registry->append_child(cell_id, new_id); + paragraph_id = new_id; + } if (const ElementIdentifier text_id = element_first_child(paragraph_id); text_id != null_element_id) { return text_id; diff --git a/test/src/internal/odf/odf_sheet_write_test.cpp b/test/src/internal/odf/odf_sheet_write_test.cpp index 906cca005..e1ede1446 100644 --- a/test/src/internal/odf/odf_sheet_write_test.cpp +++ b/test/src/internal/odf/odf_sheet_write_test.cpp @@ -208,14 +208,125 @@ TEST(OdfSheetWrite, a_formula_cell_refuses_to_be_written) { EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); } -/// An empty cell is written as no element at all. -TEST(OdfSheetWrite, an_absent_cell_refuses_to_be_written) { +/// 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) { 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); } +/// An empty cell is parsed as no element, so a write has to make one. +TEST(OdfSheetWrite, an_empty_cell_is_written) { + const Document document = + document_of(flat_sheet(string_cell("a") + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(1, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(1, 0).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); +} + +TEST(OdfSheetWrite, an_empty_cell_takes_a_number) { + const Document document = document_of(flat_sheet(R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue(12.5, "12.5")); + + const CellValue value = sheet.cell(0, 0).value(); + EXPECT_EQ(value.type(), ValueType::float_number); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 12.5); + EXPECT_EQ(value.text(), "12.5"); +} + +/// The style is the cell's, not the paragraph's, so writing keeps it. +TEST(OdfSheetWrite, an_empty_cell_keeps_its_style) { + const Document document = + document_of(flat_sheet(R"()")); + first_sheet(document).set_cell(0, 0, CellValue("y")); + + std::ostringstream saved; + document.save(saved); + EXPECT_NE(saved.str().find(R"(table:style-name="ce1")"), std::string::npos); +} + +TEST(OdfSheetWrite, an_empty_repeated_cell_is_split_by_a_write) { + const Document document = document_of( + flat_sheet(R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(2, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(2, 0).value().text(), "y"); + // the cells around it are still no element, so they state nothing at all + for (const std::uint32_t column : {0u, 1u, 3u}) { + EXPECT_FALSE(sheet.cell(column, 0).value().has_text()) << column; + } + EXPECT_EQ(sheet.dimensions().rows, 1); +} + +TEST(OdfSheetWrite, an_empty_cell_of_a_repeated_row_is_written) { + const Document document = + document_of(R"()" + R"()" + R"()" + 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_FALSE(sheet.cell(0, 1).value().has_text()); + EXPECT_FALSE(sheet.cell(1, 0).value().has_text()); + EXPECT_FALSE(sheet.cell(1, 2).value().has_text()); + EXPECT_EQ(sheet.dimensions().rows, 3); +} + +/// A formula cell can hold no cached value, and the formula refuses either +/// way. +TEST(OdfSheetWrite, an_empty_formula_cell_refuses_to_be_written) { + const Document document = document_of(flat_sheet( + R"xml()xml")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); +} + +/// A merged cell holds no paragraph until something is written into it. +TEST(OdfSheetWrite, an_empty_spanned_cell_is_written) { + const Document document = document_of( + flat_sheet(R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "y"); + EXPECT_EQ(sheet.cell(0, 0).span().columns, 2); +} + +TEST(OdfSheetWrite, an_empty_cell_written_into_saves_and_reopens) { + const Document document = document_of( + flat_sheet(R"()")); + first_sheet(document).set_cell(2, 0, CellValue(41.5, "41.5")); + + std::ostringstream saved; + document.save(saved); + const Document reopened = document_of(saved.str()); + const Sheet sheet = first_sheet(reopened); + + ASSERT_TRUE(sheet.cell(2, 0).value().has_number()); + EXPECT_DOUBLE_EQ(sheet.cell(2, 0).value().number(), 41.5); + EXPECT_FALSE(sheet.cell(1, 0).value().has_text()); + EXPECT_FALSE(sheet.cell(3, 0).value().has_text()); +} + TEST(OdfSheetWrite, a_cell_of_several_paragraphs_refuses_to_be_written) { const Document document = document_of( flat_sheet(R"()"