feat(spreadsheet): write a value into a cell, and save an xlsx

CHANGELOG.md

@@ -16,9 +16,20 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
-- `SheetCell::value` reads what a cell holds past the text it shows: the number
-  the file states, and the formula behind a cached result. Filled by odf, ooxml
-  and csv; `value_type` is unchanged and stays the question the renderer asks.
+- `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.
+
+- `.xlsx` gains `Document::save`, with `edit` and `save` capabilities to match.
+  A saved workbook sets `fullCalcOnLoad`, since nothing here computes a formula.
+
+- **Fix**: a `.xlsx` cell holding an inline string (`t="inlineStr"`) read as
+  empty.
+
+- `CellValue` is what a cell holds — type, number, text, formula — read by
+  `SheetCell::value` and written by `Sheet::set_cell`. Immutable, built from a
+  text or a number, its getters throwing `ValueNotStated` where a cell states
+  none. `value_type` is unchanged and stays what the renderer asks.
 
 - `PdfFile::annotate` writes highlight, underline, strike-out, squiggly and ink
   annotations into a pdf as an incremental update — source bytes untouched,

docs/design/spreadsheet-editing.md

@@ -40,8 +40,8 @@ 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 cells | `ElementRegistry::SheetCell::is_repeated` | Already refused by `element_is_editable` |
-| XLSX edit | `ooxml_spreadsheet_document.cpp::text_set_content` | `// TODO`, a no-op |
-| XLSX save | — | Throws. `ooxml_text_document.cpp::save` is the template: re-serialise the mutated part, copy everything else |
+| 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 |
 | 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) |
@@ -50,7 +50,7 @@ results go stale the moment an input changes.
 | Browser: editing script | `frontend.cpp::document_js` | The `modifiedText` collector: a `MutationObserver` over `contenteditable` runs keyed by `data-odr-path`; `odr.generateDiff()` |
 | Wire format | `document.cpp::Document::edit` | Parses `modifiedText` only, path-addressed, calls `Text::set_content` |
 | Addressing | `DocumentPath` | Already spells a cell by position: `/child:0/cell:A1/...` |
-| Capabilities | `file_type_table.cpp` | `ods` declares `save`, not `edit`; `xlsx` and `csv` declare neither. `odr_test` checks the declaration against `Document::is_editable` |
+| Capabilities | `file_type_table.cpp` | `ods` and `xlsx` declare `edit` and `save` (step 0.2, landed); `csv` declares neither. `odr_test` checks the declaration against `Document::is_editable` |
 
 One inconsistency worth fixing on day one: `translate_sheet` stamps
 `contenteditable` on every run inside an `.ods` cell when `config.editable` is
@@ -270,33 +270,47 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`.
 ### Step 0 — Foundation, C++ only
 
 1. **Landed.** `SheetCell::value()` → `CellValue`: the type, the number where
-   the file states one, and the formula where it states one. Abstract hook
-   `sheet_cell_value`, filled by odf, ooxml and csv; `xls` and `numbers` keep
-   only the display string, so they answer with the type alone. The text is
-   *not* repeated — it stays in the cell's children. This is also what a later
-   sort script needs instead of parsing the rendered text, though the number
-   still has to reach the page for that.
-2. Abstract write hook, position-addressed: `sheet_set_cell(sheet_id, column,
-   row, CellValue)`. ODS: an existing non-repeated cell gets its value type,
-   `office:value` and a fresh `text:p`; anything else throws until step 2.
-   XLSX: an existing cell gets `<v>` or `<is><t>`, `t` set accordingly; a
-   shared-string cell becomes `inlineStr` (`sharedStrings.xml` untouched); the
-   cell's registry subtree is re-parsed (old elements tombstoned, new ones
-   appended). Missing cell throws until step 2.
-3. XLSX `save`, mirroring docx: re-serialise every `sheetN.xml` that was
-   written to, set `fullCalcOnLoad`, copy the rest.
+   the file states one, the text showing it, and the formula where it states
+   one. Abstract hook `sheet_cell_value`, filled by odf, ooxml and csv; `xls`
+   and `numbers` state the type alone and let `SheetCell::value` collect the
+   text off the children, which is what every engine gets for free. This is
+   also what a later sort script needs instead of parsing the rendered text.
+
+   `CellValue` is **one type for reading and writing** — immutable, built by
+   explicit constructors from a text, a number, or a bare type, composed
+   further with the `with_*` withers a decoder needs, and read through getters
+   that throw `ValueNotStated` rather than hand back an empty optional. What a
+   cell reads as is what writing it back takes.
+2. **Landed.** `sheet_set_cell(sheet_id, column, row, CellValue)`, position-
+   addressed, behind `Sheet::set_cell` and `::clear_cell`. ODS writes
+   `office:value-type`, `office:value` and the `text:p`, through the cell's one
+   text run. XLSX rewrites the `c` — `<v>` for a number, `t="inlineStr"` with
+   `<is><t>` for a string — and hands the registry a fresh text element; the
+   old ones keep their ids and stop being reachable. A shared string is never
+   written back into `sharedStrings.xml`, which is what `inlineStr` is for.
+   Refused, rather than written badly: a cell the file spells no element for, a
+   repeated one (ODS), a covered one (XLSX), one holding a formula, and one
+   holding richer markup than a single plain paragraph. Every refusal is
+   decided before the engine writes anything. **Writing a formula cell waits
+   for step 4** — overwriting one leaves every value computed from it stale.
+3. **Landed.** XLSX `save`, mirroring docx: write back every worksheet and
+   `workbook.xml` from their dom, byte-copy the rest, and put back the xml
+   declaration pugixml never parsed. `fullCalcOnLoad` is set on every save
+   rather than only after an edit — we rewrote the file and compute no formula,
+   so the reader is asked to.
 4. The op envelope and dispatcher in `Document::edit`; `modifiedText` dropped
    (**Breaking**, wire only — changelog).
-5. `Document::is_editable` true for both; capability rows gain `edit` (`xlsx`
-   also `save`); `odr_test` keeps them honest.
+5. **Landed.** `Document::is_editable` true for both; capability rows gained
+   `edit` (`xlsx` also `save`); `odr_test` keeps them honest.
 6. Stop `translate_sheet` stamping `contenteditable` on a cell's runs at all.
    It cannot be gated on `Document::is_editable`, which item 5 makes *true* for
    a sheet: decision 3 puts a sheet's editing in an overlay, so the markup
    carries none. Changes the reference output — a reference `.ods` loses 594
    attributes — so it lands with a regen, on its own.
-7. Tests: set a number, a string, clear a cell, on both formats; save; reopen
-   with our reader *and* with the LibreOffice oracle (`soffice --convert-to`),
-   which is the only check that a package is really valid.
+7. **Landed.** Tests: set a number, a string, clear a cell, and each refusal,
+   on both formats, from inline fixtures; save and reopen. The LibreOffice
+   oracle (`soffice --convert-to`) stays a by-hand check — it is not in CI, and
+   it is the only one that says a written package is really valid.
 
 ### Step 1 — The browser editor
 

jni/tests/app/opendocument/core/MetaTest.java

@@ -81,8 +81,10 @@ void capabilitiesByFileType() {
     assertFalse(wpd.open);
     assertFalse(wpd.translateHtml);
 
-    // spreadsheet editing is force-disabled
-    assertFalse(Odr.capabilitiesByFileType(FileType.OPENDOCUMENT_SPREADSHEET).edit);
+    // a sheet cell can be written, and the package written back
+    FileTypeCapabilities ods = Odr.capabilitiesByFileType(FileType.OPENDOCUMENT_SPREADSHEET);
+    assertTrue(ods.edit);
+    assertTrue(ods.save);
 
     // a pdf renders, but paints its own page backgrounds
     FileTypeCapabilities pdf = Odr.capabilitiesByFileType(FileType.PORTABLE_DOCUMENT_FORMAT);

python/tests/test_meta.py

@@ -105,10 +105,10 @@ def test_capabilities_by_file_type():
     assert not wpd.open
     assert not wpd.translate_html
 
-    # spreadsheet editing is force-disabled
-    assert not pyodr.capabilities_by_file_type(
-        pyodr.FileType.opendocument_spreadsheet
-    ).edit
+    # a sheet cell can be written, and the package written back
+    ods = pyodr.capabilities_by_file_type(pyodr.FileType.opendocument_spreadsheet)
+    assert ods.edit
+    assert ods.save
 
     # a pdf renders, but paints its own page backgrounds
     pdf = pyodr.capabilities_by_file_type(pyodr.FileType.portable_document_format)

src/odr/document_element.cpp

@@ -1,15 +1,95 @@
 #include <odr/document_element.hpp>
 
 #include <odr/document_path.hpp>
+#include <odr/exceptions.hpp>
 #include <odr/file.hpp>
 #include <odr/style.hpp>
 #include <odr/table_dimension.hpp>
 #include <odr/table_position.hpp>
 
 #include <odr/internal/abstract/document.hpp>
 
+#include <utility>
+
+#include <fmt/format.h>
+
 namespace odr {
 
+namespace {
+
+/// The text under @p element, gathered out of its descendants.
+std::string element_text(const Element element) {
+  if (element.type() == ElementType::text) {
+    return element.as_text().content();
+  }
+  std::string result;
+  for (const Element child : element.children()) {
+    result += element_text(child);
+  }
+  return result;
+}
+
+} // namespace
+
+CellValue::CellValue(std::string text)
+    : m_type{ValueType::string}, m_text{std::move(text)} {}
+
+CellValue::CellValue(const double number, std::string text)
+    : m_type{ValueType::float_number}, m_number{number},
+      m_text{std::move(text)} {}
+
+CellValue::CellValue(const double number)
+    : CellValue(number, fmt::format("{}", number)) {}
+
+CellValue::CellValue(const ValueType type) : m_type{type} {}
+
+CellValue CellValue::with_number(const double number) const {
+  CellValue result = *this;
+  result.m_number = number;
+  return result;
+}
+
+CellValue CellValue::with_text(std::string text) const {
+  CellValue result = *this;
+  result.m_text = std::move(text);
+  return result;
+}
+
+CellValue CellValue::with_formula(std::string formula) const {
+  CellValue result = *this;
+  result.m_formula = std::move(formula);
+  return result;
+}
+
+ValueType CellValue::type() const noexcept { return m_type; }
+
+bool CellValue::has_number() const noexcept { return m_number.has_value(); }
+
+bool CellValue::has_text() const noexcept { return m_text.has_value(); }
+
+bool CellValue::has_formula() const noexcept { return m_formula.has_value(); }
+
+double CellValue::number() const {
+  if (!m_number.has_value()) {
+    throw ValueNotStated();
+  }
+  return *m_number;
+}
+
+const std::string &CellValue::text() const {
+  if (!m_text.has_value()) {
+    throw ValueNotStated();
+  }
+  return *m_text;
+}
+
+const std::string &CellValue::formula() const {
+  if (!m_formula.has_value()) {
+    throw ValueNotStated();
+  }
+  return *m_formula;
+}
+
 Element::Element() = default;
 
 Element::Element(const internal::abstract::ElementAdapter *adapter,
@@ -335,6 +415,27 @@ ElementRange Sheet::shapes() const {
   return ElementRange(ElementIterator(m_adapter, first_shape_id));
 }
 
+void Sheet::set_cell(const std::uint32_t column, const std::uint32_t row,
+                     const CellValue &value) const {
+  if (!exists_()) {
+    return;
+  }
+  if (value.has_formula()) {
+    throw UnsupportedOperation();
+  }
+  // checked before the engine writes anything, so a refusal leaves the cell
+  // as it was
+  if (value.type() == ValueType::float_number && !value.has_number()) {
+    throw ValueNotStated();
+  }
+  m_adapter2->sheet_set_cell(m_identifier, column, row, value);
+}
+
+void Sheet::clear_cell(const std::uint32_t column,
+                       const std::uint32_t row) const {
+  set_cell(column, row, CellValue());
+}
+
 TableStyle Sheet::style() const {
   return exists_() ? m_adapter2->sheet_style(m_identifier) : TableStyle();
 }
@@ -375,7 +476,12 @@ ValueType SheetCell::value_type() const {
 }
 
 CellValue SheetCell::value() const {
-  return exists_() ? m_adapter2->sheet_cell_value(m_identifier) : CellValue();
+  if (!exists_()) {
+    return {};
+  }
+  // no engine states the text: it is spread over the cell's children
+  const CellValue value = m_adapter2->sheet_cell_value(m_identifier);
+  return value.has_text() ? value : value.with_text(element_text(*this));
 }
 
 std::string Page::name() const {

src/odr/document_element.hpp

@@ -137,17 +137,55 @@ enum class ValueType {
   float_number,
 };
 
-/// What a sheet cell holds past the text it shows — the text stays in the
-/// cell's children. A formula cell describes the result its producer cached.
-struct CellValue final {
-  ValueType type{ValueType::unknown};
-  /// Wider than `type == ValueType::float_number`: a percentage or a currency
-  /// states a number and is typed a string until its format is read.
-  std::optional<double> number;
-  /// In the format's own syntax — `of:=SUM([.A1:.B2])` for odf, `SUM(A1:B2)`
-  /// for ooxml. Set and empty for an ooxml cell whose shared formula only the
-  /// group's master spells.
-  std::optional<std::string> formula;
+/// @brief What a sheet cell holds: what @ref SheetCell::value reads out of one,
+/// and what @ref Sheet::set_cell writes into one.
+///
+/// Immutable. A number cell states the number and the text showing it both,
+/// because only the two together say what the cell holds and how it reads.
+class CellValue final {
+public:
+  /// A cell stating no value.
+  CellValue() noexcept = default;
+  /// A string cell showing @p text.
+  explicit CellValue(std::string text);
+  /// A number cell showing @p text for @p number.
+  explicit CellValue(double number, std::string text);
+  /// A number cell showing @p number in the shortest spelling that reads back
+  /// as it.
+  explicit CellValue(double number);
+  /// A cell typed @p type and stating nothing else — what a decoder builds on,
+  /// since a file types a cell whatever it goes on to state.
+  explicit CellValue(ValueType type);
+
+  /// The same value stating @p number as well. Wider than
+  /// `type() == ValueType::float_number`: a percentage or a currency states a
+  /// number and is typed a string until its format is read.
+  [[nodiscard]] CellValue with_number(double number) const;
+  /// The same value shown as @p text.
+  [[nodiscard]] CellValue with_text(std::string text) const;
+  /// The same value behind @p formula, in the format's own syntax —
+  /// `of:=SUM([.A1:.B2])` for odf, `SUM(A1:B2)` for ooxml. Empty where an
+  /// ooxml cell shares a formula only the group's master spells.
+  [[nodiscard]] CellValue with_formula(std::string formula) const;
+
+  [[nodiscard]] ValueType type() const noexcept;
+
+  [[nodiscard]] bool has_number() const noexcept;
+  [[nodiscard]] bool has_text() const noexcept;
+  [[nodiscard]] bool has_formula() const noexcept;
+
+  /// @throws ValueNotStated where the cell states none.
+  [[nodiscard]] double number() const;
+  /// @throws ValueNotStated where the cell shows no text.
+  [[nodiscard]] const std::string &text() const;
+  /// @throws ValueNotStated where the cell holds no formula.
+  [[nodiscard]] const std::string &formula() const;
+
+private:
+  ValueType m_type{ValueType::unknown};
+  std::optional<double> m_number;
+  std::optional<std::string> m_text;
+  std::optional<std::string> m_formula;
 };
 
 /// Collection of list types.
@@ -328,6 +366,17 @@ class Sheet final : public ElementBase<internal::abstract::SheetAdapter> {
   [[nodiscard]] SheetCell cell(std::uint32_t column, std::uint32_t row) const;
   [[nodiscard]] ElementRange shapes() const;
 
+  /// Writes @p value into the cell. odf stores the number and its text both;
+  /// ooxml keeps no text for a number and shows it through its format.
+  /// @throws UnsupportedOperation where the cell cannot be written, or where
+  ///         @p value holds a formula - nothing here evaluates one.
+  /// @throws ValueNotStated where @p value is typed a number and states none.
+  void set_cell(std::uint32_t column, std::uint32_t row,
+                const CellValue &value) const;
+  /// Takes the cell's value away, keeping the style it carries. Not the same
+  /// as writing an empty string.
+  void clear_cell(std::uint32_t column, std::uint32_t row) const;
+
   [[nodiscard]] TableStyle style() const;
   [[nodiscard]] TableColumnStyle column_style(std::uint32_t column) const;
   [[nodiscard]] TableRowStyle row_style(std::uint32_t row) const;

src/odr/exceptions.cpp

@@ -91,6 +91,8 @@ MsUnsupportedCryptoAlgorithm::MsUnsupportedCryptoAlgorithm()
 UnknownDocumentType::UnknownDocumentType()
     : Exception("unknown document type") {}
 
+ValueNotStated::ValueNotStated() : Exception("value not stated") {}
+
 InvalidPrefix::InvalidPrefix() : Exception("invalid prefix string") {}
 
 InvalidPrefix::InvalidPrefix(const std::string &prefix)

src/odr/exceptions.hpp

@@ -190,6 +190,12 @@ struct UnknownDocumentType final : Exception {
   UnknownDocumentType();
 };
 
+/// A value asked of something that states none, e.g. `CellValue::number` on a
+/// cell holding a string
+struct ValueNotStated final : Exception {
+  ValueNotStated();
+};
+
 /// Invalid prefix string
 struct InvalidPrefix final : Exception {
   InvalidPrefix();