diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a30acf3..563d9eccf 100644 --- a/CHANGELOG.md +++ b/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, diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 1253346c5..1ddcd5aa2 100644 --- a/docs/design/spreadsheet-editing.md +++ b/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 `` 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 `` (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 `` or ``, `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` — `` for a number, `t="inlineStr"` with + `` 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 diff --git a/jni/tests/app/opendocument/core/MetaTest.java b/jni/tests/app/opendocument/core/MetaTest.java index a54187c79..3d9bc655d 100644 --- a/jni/tests/app/opendocument/core/MetaTest.java +++ b/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); diff --git a/python/tests/test_meta.py b/python/tests/test_meta.py index d28d0f7ba..2fe997e8d 100644 --- a/python/tests/test_meta.py +++ b/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) diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 5f65a8bb3..362fd7d68 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -8,8 +9,87 @@ #include +#include + +#include + 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 { diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index 46bd3be1b..0ee81e353 100644 --- a/src/odr/document_element.hpp +++ b/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 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 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 m_number; + std::optional m_text; + std::optional m_formula; }; /// Collection of list types. @@ -328,6 +366,17 @@ class Sheet final : public ElementBase { [[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; diff --git a/src/odr/exceptions.cpp b/src/odr/exceptions.cpp index f58648ed2..8b27a7693 100644 --- a/src/odr/exceptions.cpp +++ b/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) diff --git a/src/odr/exceptions.hpp b/src/odr/exceptions.hpp index 87989aa5b..58064b1da 100644 --- a/src/odr/exceptions.hpp +++ b/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(); diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index 5820a18be..ebfbedfcf 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -251,6 +251,15 @@ class SheetAdapter { [[nodiscard]] virtual ElementIdentifier sheet_first_shape(ElementIdentifier element_id) const = 0; + /// Writes @p value into the cell at (@p column, @p row). A value stating + /// nothing clears it. + /// @throws UnsupportedOperation where the engine cannot write, the cell is + /// absent, repeated, covered, holding a formula or richer markup, or + /// @p value holds a formula. + virtual void sheet_set_cell(ElementIdentifier element_id, + std::uint32_t column, std::uint32_t row, + const CellValue &value) const = 0; + [[nodiscard]] virtual TableStyle sheet_style(ElementIdentifier element_id) const = 0; [[nodiscard]] virtual TableColumnStyle diff --git a/src/odr/internal/csv/csv_document.cpp b/src/odr/internal/csv/csv_document.cpp index 24f8bacbe..f5679c15f 100644 --- a/src/odr/internal/csv/csv_document.cpp +++ b/src/odr/internal/csv/csv_document.cpp @@ -154,6 +154,12 @@ class ElementAdapter final : public AdapterBase { [[maybe_unused]] const ElementIdentifier element_id) const override { return null_element_id; } + void sheet_set_cell([[maybe_unused]] const ElementIdentifier element_id, + [[maybe_unused]] const std::uint32_t column, + [[maybe_unused]] const std::uint32_t row, + [[maybe_unused]] const CellValue &value) const override { + throw UnsupportedOperation(); + } [[nodiscard]] TableStyle sheet_style( [[maybe_unused]] const ElementIdentifier element_id) const override { return {}; @@ -196,11 +202,12 @@ class ElementAdapter final : public AdapterBase { } [[nodiscard]] CellValue sheet_cell_value(const ElementIdentifier element_id) const override { - CellValue result; - result.type = sheet_cell_value_type(element_id); - if (result.type == ValueType::float_number) { - result.number = util::number::parse( - m_document->cell(column_of(element_id), row_of(element_id))); + CellValue result = CellValue(sheet_cell_value_type(element_id)); + if (result.type() == ValueType::float_number) { + if (const std::optional number = util::number::parse( + m_document->cell(column_of(element_id), row_of(element_id)))) { + result = result.with_number(*number); + } } return result; } diff --git a/src/odr/internal/file_type_table.cpp b/src/odr/internal/file_type_table.cpp index 437273e0d..7c8a9ce2b 100644 --- a/src/odr/internal/file_type_table.cpp +++ b/src/odr/internal/file_type_table.cpp @@ -306,7 +306,6 @@ constexpr std::array table{ .color_scheme = true, .edit = true, .save = true}}, - // ODF spreadsheet editing is force-disabled, see `odf::Document`. Row{FileType::opendocument_spreadsheet, "ods"sv, ods_extensions, @@ -318,6 +317,7 @@ constexpr std::array table{ .decrypt = true, .translate_html = true, .color_scheme = true, + .edit = true, .save = true}}, Row{FileType::opendocument_graphics, "odg"sv, @@ -367,7 +367,9 @@ constexpr std::array table{ .open = true, .decrypt = true, .translate_html = true, - .color_scheme = true}}, + .color_scheme = true, + .edit = true, + .save = true}}, Row{FileType::office_open_xml_encrypted, "ooxml_encrypted"sv, {}, diff --git a/src/odr/internal/iwork/iwork_document.cpp b/src/odr/internal/iwork/iwork_document.cpp index bdea612e1..6c560a3c8 100644 --- a/src/odr/internal/iwork/iwork_document.cpp +++ b/src/odr/internal/iwork/iwork_document.cpp @@ -139,6 +139,12 @@ class ElementAdapter final : public AdapterBase { // a chart or a text box on the sheet; none read yet return null_element_id; } + void sheet_set_cell([[maybe_unused]] const ElementIdentifier element_id, + [[maybe_unused]] const std::uint32_t column, + [[maybe_unused]] const std::uint32_t row, + [[maybe_unused]] const CellValue &value) const override { + throw UnsupportedOperation(); + } [[nodiscard]] TableStyle sheet_style( [[maybe_unused]] const ElementIdentifier element_id) const override { return {}; @@ -183,9 +189,7 @@ class ElementAdapter final : public AdapterBase { /// Formulas live in `CalculationEngine`, which is not read. [[nodiscard]] CellValue sheet_cell_value(const ElementIdentifier element_id) const override { - CellValue result; - result.type = m_registry->cell_element_at(element_id).value_type; - return result; + return CellValue(m_registry->cell_element_at(element_id).value_type); } [[nodiscard]] TableDimensions diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 691a69845..c55a926bf 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -169,8 +169,14 @@ The structural/foundational gaps, roughly by value: 1. **Editing is text-content only.** No structural edits (insert/delete/move elements), no attribute or style editing. `text_set_content` splices the DOM for one text run; that's the whole editor. -2. **Spreadsheet editing is force-disabled** (`is_editable` hardcodes `false` - for spreadsheets — `odf_document.cpp`, `// TODO fix spreadsheet editability`). +2. **Spreadsheet editing is one cell value.** `sheet_set_cell` writes + `office:value-type`/`office:value` *and* the `text:p` under the cell — the + 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 that is absent, repeated, holding a formula, or + holding richer markup than one plain paragraph refuses instead. Splitting a + repeat, which is what would let an absent or repeated cell be written, is + the next step in [`spreadsheet-editing.md`](../../../../docs/design/spreadsheet-editing.md). 3. **Save never re-encrypts**, and refuses rather than dropping the encryption: a document decrypted from a password-protected package reports `is_savable(false) == false` and every `save` overload throws diff --git a/src/odr/internal/odf/README.md b/src/odr/internal/odf/README.md index b41fd2423..2421b8b4c 100644 --- a/src/odr/internal/odf/README.md +++ b/src/odr/internal/odf/README.md @@ -117,7 +117,10 @@ Roughly ordered by importance. - [x] shapes anchored to a sheet - [ ] computed values (stored values are used as-is; formulas are read but not evaluated) -- [ ] edit (currently disabled, see `Document::is_editable`) +- [x] edit + - [x] cell values (number, string, cleared) + - [ ] a cell that is absent or repeated (the run has to be split first) + - [ ] a formula cell, and a cell of richer markup than one plain paragraph ### Presentation documents (`.odp`) diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index a3b8a4534..3554c90a5 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -19,12 +19,15 @@ #include #include +#include #include #include #include #include #include +#include + namespace odr::internal::odf { namespace { @@ -78,12 +81,7 @@ const StyleRegistry &Document::style_registry() const { return m_style_registry; } -bool Document::is_editable() const noexcept { - // TODO fix spreadsheet editability - return m_document_type == DocumentType::text || - m_document_type == DocumentType::presentation || - m_document_type == DocumentType::drawing; -} +bool Document::is_editable() const noexcept { return true; } bool Document::is_savable(const bool encrypted) const noexcept { return !encrypted && !is_decrypted(); @@ -371,6 +369,55 @@ class ElementAdapter final : public AdapterBase { sheet_first_shape(const ElementIdentifier element_id) const override { return m_registry->sheet_element_at(element_id).first_shape_id; } + /// [ODF 1.2] 19.385: the value is an attribute and the `text:p` under the + /// cell shows it, so both are written or the file contradicts itself. + void sheet_set_cell(const ElementIdentifier element_id, + const std::uint32_t column, const std::uint32_t row, + const CellValue &value) const override { + 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(); // an empty cell is written as no element + } + const ElementIdentifier cell_id = cell->element_id; + if (m_registry->sheet_cell_element_at(cell_id).is_repeated) { + throw UnsupportedOperation(); + } + + pugi::xml_node node = get_node(cell_id); + if (node.attribute("table:formula")) { + throw UnsupportedOperation(); // its dependants would go stale + } + + const ElementIdentifier text_id = only_text_run(cell_id); + if (text_id == null_element_id) { + throw UnsupportedOperation(); + } + text_set_content(text_id, value.has_text() ? value.text() : ""); + + static constexpr std::array stated = { + "office:value-type", "office:value", "office:boolean-value", + "office:date-value", "office:time-value", "office:string-value", + "office:currency", "calcext:value-type"}; + for (const char *attribute : stated) { + node.remove_attribute(attribute); + } + + switch (value.type()) { + case ValueType::unknown: + break; + case ValueType::string: + node.append_attribute("office:value-type").set_value("string"); + break; + case ValueType::float_number: + node.append_attribute("office:value-type").set_value("float"); + node.append_attribute("office:value") + .set_value(fmt::format("{}", value.number()).c_str()); + break; + } + } + [[nodiscard]] TableStyle sheet_style(const ElementIdentifier element_id) const override { return get_partial_style(element_id).table_style; @@ -445,13 +492,13 @@ class ElementAdapter final : public AdapterBase { sheet_cell_value(const ElementIdentifier element_id) const override { const pugi::xml_node node = get_node(element_id); - CellValue result; - result.type = sheet_cell_value_type(element_id); - if (const pugi::xml_attribute value = node.attribute("office:value")) { - result.number = util::number::parse(value.value()); + CellValue result = CellValue(sheet_cell_value_type(element_id)); + if (const std::optional number = + util::number::parse(node.attribute("office:value").value())) { + result = result.with_number(*number); } if (const pugi::xml_attribute formula = node.attribute("table:formula")) { - result.formula = formula.value(); + result = result.with_formula(formula.value()); } return result; } @@ -840,6 +887,33 @@ class ElementAdapter final : public AdapterBase { return m_registry->element_at(element_id).node; } + /// The single text run under a cell of one plain paragraph, created where + /// that paragraph is empty. Null where the markup is richer than that, which + /// a write then keeps rather than throws away. + [[nodiscard]] ElementIdentifier + only_text_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 || + element_type(paragraph_id) != ElementType::paragraph) { + return null_element_id; + } + const ElementIdentifier text_id = element_first_child(paragraph_id); + if (text_id == null_element_id) { + const pugi::xml_node text_node = + get_node(paragraph_id).append_child(pugi::xml_node_type::node_pcdata); + const auto &[new_id, unused1, unused2] = + m_registry->create_text_element(text_node, text_node); + m_registry->append_child(paragraph_id, new_id); + return new_id; + } + if (element_next_sibling(text_id) != null_element_id || + element_type(text_id) != ElementType::text) { + return null_element_id; + } + return text_id; + } + /// The image's base64 bytes where the markup carries them itself. [[nodiscard]] pugi::xml_node image_data(const ElementIdentifier element_id) const { diff --git a/src/odr/internal/oldms/spreadsheet/xls_document.cpp b/src/odr/internal/oldms/spreadsheet/xls_document.cpp index 4f85aafbf..da70f814f 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_document.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_document.cpp @@ -82,6 +82,12 @@ class ElementAdapter final : public AdapterBase { (void)element_id; return null_element_id; } + void sheet_set_cell([[maybe_unused]] const ElementIdentifier element_id, + [[maybe_unused]] const std::uint32_t column, + [[maybe_unused]] const std::uint32_t row, + [[maybe_unused]] const CellValue &value) const override { + throw UnsupportedOperation(); + } [[nodiscard]] TableStyle sheet_style( [[maybe_unused]] const ElementIdentifier element_id) const override { (void)element_id; @@ -139,9 +145,7 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] CellValue sheet_cell_value( [[maybe_unused]] const ElementIdentifier element_id) const override { (void)element_id; - CellValue result; - result.type = ValueType::string; - return result; + return CellValue(ValueType::string); } [[nodiscard]] ParagraphStyle diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index 9a1f71ef4..ea414a7dc 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -2,7 +2,7 @@ The **why**; the feature checklist is in [`README.md`](README.md), the shared OOXML mechanics (registry/adapter pattern, OPC relationships, encryption) in -[`../AGENTS.md`](../AGENTS.md). **Read-only.** +[`../AGENTS.md`](../AGENTS.md). Reads, and writes a cell value. **Scope.** Read `xl/workbook.xml`, its sheets, the shared-string table and drawings into the abstract model — a table per sheet. Cell styles resolved from @@ -47,6 +47,23 @@ A legacy indexed colour palette is hardcoded. Named-style masters (`cellStyleXfs`) are loaded but **never consulted** (no master-style inheritance). +**Writing a cell replaces its children, and a written string goes inline.** +`sheet_set_cell` (position-addressed, so the cell it names need not have an +element) rewrites the `c` and hands the registry a fresh text element for what +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`. + +**`save` writes back the parts it can have changed** — every worksheet and +`workbook.xml` — and byte-copies the rest, as `ooxml/text` does for +`document.xml`. pugixml is not asked to parse the declaration, so it cannot +write one back and `save` puts it there itself. Every save sets +`calcPr/@fullCalcOnLoad` (18.2.2): nothing here computes a formula, so the +reader is asked to. + ## Module layout | File (`spreadsheet/`) | Role | @@ -67,5 +84,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. **Read-only.** `text_set_content` is a no-op stub; `save` throws. 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 already spells; `text_set_content` is still a no-op + stub. Links and comments/annotations not modelled. diff --git a/src/odr/internal/ooxml/spreadsheet/README.md b/src/odr/internal/ooxml/spreadsheet/README.md index b09581b53..344f86fe7 100644 --- a/src/odr/internal/ooxml/spreadsheet/README.md +++ b/src/odr/internal/ooxml/spreadsheet/README.md @@ -21,6 +21,7 @@ Roughly ordered by importance. - [x] columns, rows, cells - [x] dimensions - [x] shared strings + - [x] inline strings (`t="inlineStr"`) - [x] merged cells (`mergeCells`) - [x] shapes / images anchored to a sheet (`xdr:twoCellAnchor`) - [x] cell value types (number vs. string; dates/booleans/errors reported as @@ -28,8 +29,10 @@ Roughly ordered by importance. - [x] cell values (`` as a number, `` as its own string) - [ ] computed values (formulas are read but not evaluated; the cached `` result is shown) -- [ ] edit -- [ ] save +- [x] edit + - [x] cell values (number, string, cleared), a written string going inline + - [ ] a cell the file writes no `c` for, a covered one, a formula one +- [x] save ### Styles diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index f915061ef..a3373684b 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -1,28 +1,58 @@ #include +#include #include #include #include #include +#include #include #include #include +#include +#include +#include +#include +#include +#include #include +#include + namespace odr::internal::ooxml::spreadsheet { namespace { std::unique_ptr create_element_adapter(const Document &document, ElementRegistry ®istry); + +/// The `workbook` `calcPr`, appended where it is missing. ECMA-376 18.2.27 +/// orders the children, so a new one goes before the first that must follow it. +pugi::xml_node calc_pr(pugi::xml_node workbook) { + if (const pugi::xml_node existing = workbook.child("calcPr")) { + return existing; + } + static constexpr std::array after = { + "oleSize", "customWorkbookViews", "pivotCaches", + "smartTagPr", "smartTagTypes", "webPublishing", + "fileRecoveryPr", "webPublishObjects", "extLst"}; + for (const pugi::xml_node child : workbook.children()) { + if (std::ranges::find(after, std::string_view(child.name())) != + std::end(after)) { + return workbook.insert_child_before("calcPr", child); + } + } + return workbook.append_child("calcPr"); } +} // namespace Document::Document(std::shared_ptr files) : internal::Document(FileType::office_open_xml_workbook, DocumentType::spreadsheet, std::move(files)) { const AbsPath workbook_path("/xl/workbook.xml"); const auto [workbook_xml, workbook_relations] = parse_xml_(workbook_path); + m_written_parts.push_back(workbook_path); const auto [styles_xml, _] = parse_xml_(AbsPath("/xl/styles.xml")); for (pugi::xml_node sheet_node : @@ -31,6 +61,7 @@ Document::Document(std::shared_ptr files) const AbsPath sheet_path = workbook_path.parent().join(RelPath(workbook_relations.at(id))); const auto [sheet_xml, sheet_relationships] = parse_xml_(sheet_path); + m_written_parts.push_back(sheet_path); if (const pugi::xml_node drawing = sheet_xml.document_element().child("drawing")) { @@ -68,6 +99,58 @@ const StyleRegistry &Document::style_registry() const { return m_style_registry; } +bool Document::is_editable() const noexcept { return true; } + +bool Document::is_savable(const bool encrypted) const noexcept { + return !encrypted && !is_decrypted(); +} + +void Document::save(std::ostream &out) const { + if (!is_savable(false)) { + throw UnsupportedOperation(); + } + + // ECMA-376 18.2.2: nothing here computes a formula, so every save asks the + // reader to recompute what this one may have invalidated + pugi::xml_node calc_node = + calc_pr(m_xml_documents_and_relations.at(AbsPath("/xl/workbook.xml")) + .first.document_element()); + calc_node.remove_attribute("fullCalcOnLoad"); + calc_node.append_attribute("fullCalcOnLoad").set_value("1"); + + // TODO this would decrypt/inflate and encrypt/deflate again + zip::ZipArchive archive; + + for (auto walker = m_files->file_walker(AbsPath("/")); !walker->end(); + walker->next()) { + const AbsPath &abs_path = walker->path(); + RelPath rel_path = abs_path.rebase(AbsPath("/")); + if (walker->is_directory()) { + archive.insert_directory(std::end(archive), rel_path); + continue; + } + if (std::ranges::find(m_written_parts, abs_path) != + std::end(m_written_parts)) { + // TODO stream + std::stringstream content; + // pugixml is never asked to parse the declaration, so it writes none back + content << R"()"; + m_xml_documents_and_relations.at(abs_path).first.print(content, "", + pugi::format_raw); + auto tmp = std::make_shared(content.str()); + archive.insert_file(std::end(archive), rel_path, tmp); + continue; + } + archive.insert_file(std::end(archive), rel_path, m_files->open(abs_path)); + } + + archive.save(out); +} + +void Document::save(std::ostream & /*out*/, const char * /*password*/) const { + throw UnsupportedOperation(); +} + std::pair Document::parse_xml_(const AbsPath &path) { pugi::xml_document document = xml::parse(*m_files, path); @@ -126,6 +209,63 @@ class ElementAdapter final : public AdapterBase { sheet_first_shape(const ElementIdentifier element_id) const override { return m_registry->sheet_element_at(element_id).first_shape_id; } + /// ECMA-376 18.3.1.4: a cell states its value as `v`, or as the text under + /// `is` with `t="inlineStr"`. A written string goes inline - rewriting the + /// shared entry would rewrite every other cell indexing it. + void sheet_set_cell(const ElementIdentifier element_id, + const std::uint32_t column, const std::uint32_t row, + const CellValue &value) const override { + 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 + } + + // the elements over the old children keep their ids and stop being + // reachable + while (const pugi::xml_node child = node.first_child()) { + node.remove_child(child); + } + node.remove_attribute("t"); + ElementRegistry::Element &cell_element = m_registry->element_at(cell_id); + cell_element.first_child_id = null_element_id; + cell_element.last_child_id = null_element_id; + + switch (value.type()) { + case ValueType::unknown: + break; + case ValueType::string: { + node.append_attribute("t").set_value("inlineStr"); + pugi::xml_node text_node = node.append_child("is").append_child("t"); + // the text is written verbatim, so a leading space in one has to survive + text_node.append_attribute("xml:space").set_value("preserve"); + text_node.text().set(value.has_text() ? value.text().c_str() : ""); + const auto &[text_id, unused1, unused2] = + m_registry->create_text_element(text_node, text_node); + m_registry->append_child(cell_id, text_id); + } break; + case ValueType::float_number: { + // `t` defaults to "n"; the number format, not a stored string, is + // what shows the number, so `value.text()` has nowhere to go + const pugi::xml_node value_node = node.append_child("v"); + value_node.text().set(fmt::format("{}", value.number()).c_str()); + const auto &[text_id, unused1, unused2] = + m_registry->create_text_element(value_node, value_node); + m_registry->append_child(cell_id, text_id); + } break; + } + } + [[nodiscard]] TableStyle sheet_style( [[maybe_unused]] const ElementIdentifier element_id) const override { return {}; // TODO @@ -206,13 +346,15 @@ class ElementAdapter final : public AdapterBase { sheet_cell_value(const ElementIdentifier element_id) const override { const pugi::xml_node node = get_node(element_id); - CellValue result; - result.type = sheet_cell_value_type(element_id); - if (result.type == ValueType::float_number) { - result.number = util::number::parse(node.child("v").text().get()); + CellValue result = CellValue(sheet_cell_value_type(element_id)); + if (result.type() == ValueType::float_number) { + if (const std::optional number = + util::number::parse(node.child("v").text().get())) { + result = result.with_number(*number); + } } if (const pugi::xml_node formula = node.child("f")) { - result.formula = formula.text().get(); + result = result.with_formula(formula.text().get()); } return result; } diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp index 21e3d8ad2..a9059847e 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -21,9 +22,17 @@ class Document final : public internal::Document { [[nodiscard]] const ElementRegistry &element_registry() const; [[nodiscard]] const StyleRegistry &style_registry() const; + [[nodiscard]] bool is_editable() const noexcept override; + [[nodiscard]] bool is_savable(bool encrypted) const noexcept override; + + void save(std::ostream &out) const override; + void save(std::ostream &out, const char *password) const override; + private: XmlDocumentsAndRelations m_xml_documents_and_relations; SharedStrings m_shared_strings; + /// The parts `save` writes back from their dom; the rest is byte-copied. + std::vector m_written_parts; ElementRegistry m_element_registry; StyleRegistry m_style_registry; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp index c30d7fb6e..9bc1bcac1 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp @@ -6,6 +6,8 @@ #include #include +#include +#include #include #include @@ -81,14 +83,22 @@ void parse_sheet_cell_children(ElementRegistry ®istry, const ParseContext &context, const ElementIdentifier parent_id, const pugi::xml_node node) { - if (const pugi::xml_attribute type_attr = node.attribute("t"); - type_attr.value() == std::string("s")) { + const std::string_view type = node.attribute("t").value(); + + // ECMA-376 18.3.1.4: a shared string indexes `sharedStrings.xml`, an inline + // one carries the same content model under `is`. Both hold the text one + // level below the cell, where the walker does not descend on its own. + if (type == "s") { const pugi::xml_node v_node = node.child("v"); const std::size_t ref = v_node.first_child().text().as_ullong(); const pugi::xml_node shared_node = context.shared_strings().at(ref); parse_any_element_children(registry, context, parent_id, shared_node); return; } + if (type == "inlineStr") { + parse_any_element_children(registry, context, parent_id, node.child("is")); + return; + } parse_any_element_children(registry, context, parent_id, node); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a67de9596..6e573462f 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -26,6 +26,7 @@ add_executable(odr_test "src/test_util.cpp" "${CMAKE_CURRENT_BINARY_DIR}/src/test_info.cpp" + "src/cell_value_test.cpp" "src/document_list_test.cpp" "src/document_path_test.cpp" "src/enum_ordinals_test.cpp" @@ -82,6 +83,7 @@ add_executable(odr_test "src/internal/odf/odf_geometry_test.cpp" "src/internal/odf/odf_sheet_repeat_test.cpp" "src/internal/odf/odf_sheet_value_test.cpp" + "src/internal/odf/odf_sheet_write_test.cpp" "src/internal/odf/odf_table_test.cpp" "src/internal/oldms/doc_test.cpp" @@ -93,6 +95,7 @@ add_executable(odr_test "src/internal/ooxml/ooxml_text_style_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_value_test.cpp" + "src/internal/ooxml/ooxml_spreadsheet_write_test.cpp" "src/internal/ooxml/ooxml_util_test.cpp" "src/internal/ooxml/ooxml_presentation_style_test.cpp" diff --git a/test/src/cell_value_test.cpp b/test/src/cell_value_test.cpp new file mode 100644 index 000000000..22d559a8a --- /dev/null +++ b/test/src/cell_value_test.cpp @@ -0,0 +1,64 @@ +#include +#include + +#include + +using namespace odr; + +TEST(CellValue, a_default_value_states_nothing) { + const CellValue value; + + EXPECT_EQ(value.type(), ValueType::unknown); + EXPECT_FALSE(value.has_number()); + EXPECT_FALSE(value.has_text()); + EXPECT_FALSE(value.has_formula()); +} + +TEST(CellValue, a_text_value_types_itself_a_string) { + const CellValue value("hello"); + + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_EQ(value.text(), "hello"); + EXPECT_FALSE(value.has_number()); +} + +TEST(CellValue, a_number_value_shows_the_text_it_is_given) { + const CellValue value(1234.5, "1 234,50"); + + EXPECT_EQ(value.type(), ValueType::float_number); + EXPECT_DOUBLE_EQ(value.number(), 1234.5); + EXPECT_EQ(value.text(), "1 234,50"); +} + +/// The shortest spelling that reads back as the number, whatever the host's +/// locale. +TEST(CellValue, a_number_value_spells_itself_where_no_text_is_given) { + EXPECT_EQ(CellValue(1234.5).text(), "1234.5"); + EXPECT_EQ(CellValue(2).text(), "2"); +} + +TEST(CellValue, asking_for_what_is_not_stated_throws) { + const CellValue value("hello"); + + EXPECT_THROW((void)value.number(), ValueNotStated); + EXPECT_THROW((void)value.formula(), ValueNotStated); +} + +/// A percentage states a number and is still typed a string, so the type is +/// not the number's to decide. +TEST(CellValue, a_type_outlives_what_is_put_beside_it) { + const CellValue value = + CellValue(ValueType::string).with_number(0.25).with_text("25%"); + + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_DOUBLE_EQ(value.number(), 0.25); +} + +TEST(CellValue, a_wither_leaves_the_value_it_was_asked_of_alone) { + const CellValue value("hello"); + const CellValue with = value.with_formula("of:=A1"); + + EXPECT_FALSE(value.has_formula()); + EXPECT_EQ(with.formula(), "of:=A1"); + EXPECT_EQ(with.text(), "hello"); +} diff --git a/test/src/internal/odf/odf_sheet_value_test.cpp b/test/src/internal/odf/odf_sheet_value_test.cpp index f9f2fc6fb..a5ca93eee 100644 --- a/test/src/internal/odf/odf_sheet_value_test.cpp +++ b/test/src/internal/odf/odf_sheet_value_test.cpp @@ -49,10 +49,10 @@ TEST(OdfSheetValue, a_float_cell_states_its_number) { R"()" R"(1 234,50)"); - EXPECT_EQ(value.type, ValueType::float_number); - ASSERT_TRUE(value.number.has_value()); - EXPECT_DOUBLE_EQ(*value.number, 1234.5); - EXPECT_FALSE(value.formula.has_value()); + EXPECT_EQ(value.type(), ValueType::float_number); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 1234.5); + EXPECT_FALSE(value.has_formula()); } TEST(OdfSheetValue, a_string_cell_states_no_number) { @@ -60,8 +60,8 @@ TEST(OdfSheetValue, a_string_cell_states_no_number) { value_of(R"()" R"(1234.5)"); - EXPECT_EQ(value.type, ValueType::string); - EXPECT_FALSE(value.number.has_value()); + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_FALSE(value.has_number()); } /// [ODF 1.2] 19.642 `table:formula`, whose namespace prefix is the syntax it @@ -73,10 +73,10 @@ TEST(OdfSheetValue, a_formula_cell_states_both_formula_and_result) { R"( office:value-type="float" office:value="7">)" R"(7)"); - ASSERT_TRUE(value.formula.has_value()); - EXPECT_EQ(*value.formula, "of:=SUM([.B1:.C1])"); - ASSERT_TRUE(value.number.has_value()); - EXPECT_DOUBLE_EQ(*value.number, 7); + ASSERT_TRUE(value.has_formula()); + EXPECT_EQ(value.formula(), "of:=SUM([.B1:.C1])"); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 7); } /// The type is read from `office:value-type` alone; reading the number format @@ -86,9 +86,9 @@ TEST(OdfSheetValue, a_percentage_states_a_number_the_type_does_not_admit) { R"()" R"(25%)"); - EXPECT_EQ(value.type, ValueType::string); - ASSERT_TRUE(value.number.has_value()); - EXPECT_DOUBLE_EQ(*value.number, 0.25); + EXPECT_EQ(value.type(), ValueType::string); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 0.25); } TEST(OdfSheetValue, a_number_is_read_in_one_spelling_only) { @@ -96,5 +96,5 @@ TEST(OdfSheetValue, a_number_is_read_in_one_spelling_only) { R"()" R"(1234,5)"); - EXPECT_FALSE(value.number.has_value()); + EXPECT_FALSE(value.has_number()); } diff --git a/test/src/internal/odf/odf_sheet_write_test.cpp b/test/src/internal/odf/odf_sheet_write_test.cpp new file mode 100644 index 000000000..c164c9e64 --- /dev/null +++ b/test/src/internal/odf/odf_sheet_write_test.cpp @@ -0,0 +1,213 @@ +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include + +#include +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +/// A flat sheet whose single row holds @p cells. +std::string flat_sheet(const std::string &cells) { + return R"()" + R"()" + R"()" + R"()" + + cells + + R"()" + R"()"; +} + +std::string string_cell(const std::string &text) { + return R"()" + text + + R"()"; +} + +Document document_of(const std::string &source) { + return DecodedFile( + open_strategy::open_file(std::make_shared(source), {}, + Logger::null())) + .as_document_file() + .document(); +} + +Sheet first_sheet(const Document &document) { + return (*document.root_element().children().begin()).as_sheet(); +} + +} // namespace + +/// [ODF 1.2] 19.385: the cell states the value and the `text:p` shows it. +TEST(OdfSheetWrite, a_number_is_written_as_both_value_and_text) { + const Document document = document_of(flat_sheet(string_cell("old"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue(1234.5, "1 234,50")); + + 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(), 1234.5); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "1 234,50"); +} + +TEST(OdfSheetWrite, a_string_written_over_a_number_takes_the_number_away) { + const Document document = document_of(flat_sheet( + R"()" + R"(7)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("seven")); + + const CellValue value = sheet.cell(0, 0).value(); + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_FALSE(value.has_number()); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "seven"); +} + +TEST(OdfSheetWrite, a_cleared_cell_states_nothing) { + const Document document = document_of(flat_sheet( + R"()" + R"(7)")); + const Sheet sheet = first_sheet(document); + + sheet.clear_cell(0, 0); + + const CellValue value = sheet.cell(0, 0).value(); + EXPECT_EQ(value.type(), ValueType::string); // no `office:value-type` left + EXPECT_FALSE(value.has_number()); + EXPECT_EQ(sheet.cell(0, 0).value().text(), ""); +} + +/// One element stands for every cell of the run, so a write hits all of them. +TEST(OdfSheetWrite, a_repeated_cell_refuses_to_be_written) { + const Document document = document_of(flat_sheet( + R"(x)")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); +} + +TEST(OdfSheetWrite, a_formula_cell_refuses_to_be_written) { + const Document document = document_of( + flat_sheet(R"xml()" + R"(7)")); + const Sheet sheet = first_sheet(document); + + 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) { + 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); +} + +TEST(OdfSheetWrite, a_cell_of_several_paragraphs_refuses_to_be_written) { + const Document document = document_of( + flat_sheet(R"()" + R"(ab)")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); +} + +/// A blank cell that carries a style is written as an empty `text:p`, which is +/// also what clearing one leaves behind, so it has to stay writable. +TEST(OdfSheetWrite, a_cell_of_an_empty_paragraph_is_written_through) { + const Document document = + document_of(flat_sheet(R"()" + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "y"); +} + +TEST(OdfSheetWrite, a_cleared_cell_is_written_again_after_a_reopen) { + const Document document = document_of(flat_sheet(string_cell("old"))); + first_sheet(document).clear_cell(0, 0); + + std::ostringstream saved; + document.save(saved); + const Document reopened = document_of(saved.str()); + first_sheet(reopened).set_cell(0, 0, CellValue("new")); + + EXPECT_EQ(first_sheet(reopened).cell(0, 0).value().text(), "new"); +} + +/// What a cell reads as is what writing it back takes, which is the point of +/// the one type. +TEST(OdfSheetWrite, a_value_read_out_of_a_cell_writes_into_another) { + const Document document = document_of( + flat_sheet(R"(1 234,50)" + R"()" + + string_cell("other"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(1, 0, sheet.cell(0, 0).value()); + + const CellValue value = sheet.cell(1, 0).value(); + EXPECT_EQ(value.type(), ValueType::float_number); + EXPECT_DOUBLE_EQ(value.number(), 1234.5); + EXPECT_EQ(value.text(), "1 234,50"); +} + +/// Writing one waits for an evaluator, so a formula cell's value cannot be +/// handed back either. +TEST(OdfSheetWrite, a_value_holding_a_formula_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( + 0, 0, CellValue(7, "7").with_formula("of:=SUM([.B1:.C1])")), + UnsupportedOperation); +} + +TEST(OdfSheetWrite, a_written_sheet_saves_and_reopens) { + const Document document = document_of(flat_sheet(string_cell("old"))); + ASSERT_TRUE(document.is_editable()); + ASSERT_TRUE(document.is_savable()); + + first_sheet(document).set_cell(0, 0, CellValue(41.5, "41.5")); + + std::ostringstream saved; + document.save(saved); + + const Document reopened = document_of(saved.str()); + const CellValue value = first_sheet(reopened).cell(0, 0).value(); + + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 41.5); + EXPECT_EQ(first_sheet(reopened).cell(0, 0).value().text(), "41.5"); +} + +/// Every refusal is decided before anything is written. +TEST(OdfSheetWrite, a_number_stating_none_leaves_the_cell_alone) { + const Document document = document_of(flat_sheet(string_cell("old"))); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue(ValueType::float_number)), + ValueNotStated); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "old"); +} diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp index f54a49e48..ac1fdf572 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp @@ -25,9 +25,12 @@ 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. +/// after 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 = "") { +workbook(const std::string &sheet_data, const std::string &sheet_extra = "", + const std::string &shared_strings = "", + const std::string &workbook_extra = "") { internal::zip::ZipArchive zip; insert( zip, "[Content_Types].xml", @@ -44,7 +47,8 @@ workbook(const std::string &sheet_data, const std::string &sheet_extra = "") { zip, "xl/workbook.xml", R"()" - R"()"); + R"()" + + workbook_extra + R"()"); insert( zip, "xl/_rels/workbook.xml.rels", R"()" @@ -59,6 +63,13 @@ workbook(const std::string &sheet_data, const std::string &sheet_extra = "") { R"()" + sheet_data + R"()" + sheet_extra + R"()"); + if (!shared_strings.empty()) { + insert( + zip, "xl/sharedStrings.xml", + R"()" + + shared_strings + R"()"); + } + std::stringstream out; zip.save(out); return std::make_shared(out.str()); diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp index c4051076b..51976588f 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp @@ -13,28 +13,47 @@ using namespace odr::test::ooxml; namespace { CellValue value_of(const std::string &sheet_data) { - return first_sheet(decode(workbook(sheet_data))).cell(0, 0).value(); + const Document document = decode(workbook(sheet_data)); + return first_sheet(document).cell(0, 0).value(); +} + +std::string text_of(const std::string &sheet_data) { + return value_of(sheet_data).text(); } } // namespace +/// ECMA-376 18.3.1.4: an inline string holds its text under `is`, one level +/// below the cell, which is where a shared one holds it too. The walker +/// descends into neither on its own, so the cell read as empty. +TEST(OoxmlSpreadsheetValue, an_inline_string_cell_reads_its_text) { + EXPECT_EQ( + text_of(R"(hello)" + R"()"), + "hello"); +} + +TEST(OoxmlSpreadsheetValue, a_number_cell_reads_its_cached_text) { + EXPECT_EQ(text_of(R"(42)"), "42"); +} + /// ECMA-376 18.3.1.4: `c/@t` defaults to "n", so a bare `` is a number. TEST(OoxmlSpreadsheetValue, a_number_cell_states_its_number) { const CellValue value = value_of(R"(12.5)"); - EXPECT_EQ(value.type, ValueType::float_number); - ASSERT_TRUE(value.number.has_value()); - EXPECT_DOUBLE_EQ(*value.number, 12.5); - EXPECT_FALSE(value.formula.has_value()); + EXPECT_EQ(value.type(), ValueType::float_number); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 12.5); + EXPECT_FALSE(value.has_formula()); } TEST(OoxmlSpreadsheetValue, an_inline_string_cell_states_no_number) { const CellValue value = value_of( R"(12.5)"); - EXPECT_EQ(value.type, ValueType::string); - EXPECT_FALSE(value.number.has_value()); + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_FALSE(value.has_number()); } /// The `` is the result the producer cached; `` is what computed it. @@ -42,11 +61,11 @@ TEST(OoxmlSpreadsheetValue, a_formula_cell_states_both_formula_and_result) { const CellValue value = value_of(R"(SUM(B1:C1)7)"); - EXPECT_EQ(value.type, ValueType::float_number); - ASSERT_TRUE(value.number.has_value()); - EXPECT_DOUBLE_EQ(*value.number, 7); - ASSERT_TRUE(value.formula.has_value()); - EXPECT_EQ(*value.formula, "SUM(B1:C1)"); + EXPECT_EQ(value.type(), ValueType::float_number); + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 7); + ASSERT_TRUE(value.has_formula()); + EXPECT_EQ(value.formula(), "SUM(B1:C1)"); } /// Set-and-empty says the member computes; unset would claim it does not. @@ -55,8 +74,8 @@ TEST(OoxmlSpreadsheetValue, const CellValue value = value_of( R"(8)"); - ASSERT_TRUE(value.formula.has_value()); - EXPECT_TRUE(value.formula->empty()); + ASSERT_TRUE(value.has_formula()); + EXPECT_TRUE(value.formula().empty()); } /// `` holds `1`, not a quantity, and `c/@t="b"` types the cell a string. @@ -64,6 +83,6 @@ TEST(OoxmlSpreadsheetValue, a_boolean_cell_states_no_number) { const CellValue value = value_of(R"(1)"); - EXPECT_EQ(value.type, ValueType::string); - EXPECT_FALSE(value.number.has_value()); + EXPECT_EQ(value.type(), ValueType::string); + EXPECT_FALSE(value.has_number()); } diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp new file mode 100644 index 000000000..92d88c68a --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp @@ -0,0 +1,157 @@ +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include + +using namespace odr; +using namespace odr::test::ooxml; + +namespace { + +constexpr const char *two_shared = R"(0)" + R"(0)"; +constexpr const char *one_string = R"(same)"; + +} // namespace + +TEST(OoxmlSpreadsheetWrite, a_string_lands_in_the_cell_it_was_written_to) { + const Document document = decode(workbook( + R"(old)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("new")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "new"); + EXPECT_EQ(sheet.cell(0, 0).value().type(), ValueType::string); +} + +TEST(OoxmlSpreadsheetWrite, a_number_lands_as_a_number) { + const Document document = + decode(workbook(R"(1)")); + 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(sheet.cell(0, 0).value().text(), "12.5"); +} + +/// The point of writing an inline string rather than a shared one: two cells +/// index the same `si`, and editing that entry would edit both. +TEST(OoxmlSpreadsheetWrite, + writing_a_shared_string_leaves_the_other_cell_alone) { + const Document document = decode(workbook(two_shared, "", one_string)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("mine")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "mine"); + EXPECT_EQ(sheet.cell(1, 0).value().text(), "same"); +} + +TEST(OoxmlSpreadsheetWrite, a_cleared_cell_states_nothing) { + const Document document = + decode(workbook(R"(7)")); + const Sheet sheet = first_sheet(document); + + sheet.clear_cell(0, 0); + + const CellValue value = sheet.cell(0, 0).value(); + EXPECT_FALSE(value.has_number()); + EXPECT_EQ(sheet.cell(0, 0).value().text(), ""); +} + +TEST(OoxmlSpreadsheetWrite, a_formula_cell_refuses_to_be_written) { + const Document document = decode( + workbook(R"(SUM(B1:C1)7)")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("x")), UnsupportedOperation); +} + +/// The anchor of the merge is the cell that holds the value. +TEST(OoxmlSpreadsheetWrite, a_covered_cell_refuses_to_be_written) { + const Document document = decode( + workbook(R"(a)" + R"(b)", + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(1, 0, CellValue("x")), UnsupportedOperation); +} + +TEST(OoxmlSpreadsheetWrite, an_absent_cell_refuses_to_be_written) { + const Document document = + decode(workbook(R"(1)")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(4, 4, CellValue("x")), UnsupportedOperation); +} + +TEST(OoxmlSpreadsheetWrite, a_written_workbook_saves_and_reopens) { + const Document document = + decode(workbook(R"(1)")); + ASSERT_TRUE(document.is_editable()); + ASSERT_TRUE(document.is_savable()); + + first_sheet(document).set_cell(0, 0, 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 CellValue value = first_sheet(reopened).cell(0, 0).value(); + + ASSERT_TRUE(value.has_number()); + EXPECT_DOUBLE_EQ(value.number(), 41.5); +} + +/// ECMA-376 18.2.27 orders the `workbook` children, and `extLst` comes after +/// `calcPr`: appending the new one would put it on the wrong side. +TEST(OoxmlSpreadsheetWrite, a_new_calc_pr_lands_where_the_schema_orders_it) { + const Document document = decode(workbook( + R"(1)", "", "", R"()")); + + std::ostringstream saved; + document.save(saved); + + const Document reopened = + open(File::from_memory(saved.str())).as_document_file().document(); + std::ostringstream workbook_xml; + workbook_xml + << reopened.as_filesystem().open("/xl/workbook.xml").stream()->rdbuf(); + + EXPECT_LT(workbook_xml.str().find("1)")); + + std::ostringstream saved; + document.save(saved); + + const Document reopened = + open(File::from_memory(saved.str())).as_document_file().document(); + std::ostringstream workbook_xml; + workbook_xml + << reopened.as_filesystem().open("/xl/workbook.xml").stream()->rdbuf(); + + EXPECT_NE(workbook_xml.str().find(R"(fullCalcOnLoad="1")"), + std::string::npos); +}