diff --git a/CHANGELOG.md b/CHANGELOG.md index d689b01a9..f2a30acf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ 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. + - `PdfFile::annotate` writes highlight, underline, strike-out, squiggly and ink annotations into a pdf as an incremental update — source bytes untouched, any viewer reading them — in every binding, with an `annotate` capability. diff --git a/docs/design/README.md b/docs/design/README.md index 6201a8304..a3c9728b0 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -5,6 +5,10 @@ - [Editing design](editing.md) — architecture for in-browser editing of ODF/OOXML: fat-browser op log replayed on save, stable element ids, and a preliminary implementation plan. +- [Spreadsheet editing design](spreadsheet-editing.md) — cells edited by + position through the same op log, a browser-side editing mode with refusal + feedback, and formulas recomputed once, in C++; staged from number/string + cells to a formula engine. ## Diagrams diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md new file mode 100644 index 000000000..1253346c5 --- /dev/null +++ b/docs/design/spreadsheet-editing.md @@ -0,0 +1,448 @@ +# Spreadsheet editing design + +Status: **proposed; nothing scheduled.** 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. + +Related: [`editing.md`](editing.md) is the accepted direction for text +documents (op log, ids, browser-side undo). This builds on its decisions and +takes the pieces a sheet makes cheap first. [`odf/AGENTS.md`](../../src/odr/internal/odf/AGENTS.md) +and [`ooxml/spreadsheet/AGENTS.md`](../../src/odr/internal/ooxml/spreadsheet/AGENTS.md) +describe the read side. + +## Problem + +Spreadsheets render but do not edit. `odf::Document::is_editable` hardcodes +`false` for `.ods`, `.xlsx` is read-only end to end, and the browser side has +nothing a sheet needs: no way to type into an empty cell, no notion of a +number behind the string, no answer when a cell cannot be edited. + +The two complications the formats add over text are well understood: + +- **ODS collapses repeats.** One `` stands for a thousand cells, a row + can repeat the same way, and an empty cell has no element at all + (`odf_parser.cpp::is_cell_empty`). A cell to write into may not exist as a + node yet. +- **XLSX shares strings.** A `t="s"` cell's text is parsed out of + `sharedStrings.xml`, so the registry's text nodes for that cell live in a + part every other cell with the same string points at. Writing into them + edits every one of those cells. + +And a spreadsheet adds the thing text never had: **formulas**, whose cached +results go stale the moment an input changes. + +## What the code gives us + +| Piece | Where | State | +|---|---|---| +| 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 | `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 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) | +| Formulas | `sheet_cell_value` | The expression is read and handed out as a string (step 0.1, landed); nothing parses or evaluates it. XLSX shows the cached ``, ODS the cached `text:p`. `xls` and `numbers` drop the expression at parse time | +| Browser: sheet script | `frontend.cpp::spreadsheet_js` | Hover/pin, raise a clipped cell over its neighbours, sort rows in the DOM. Sorting reorders ``s, so a row's identity is its `` label, not its index | +| 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` | + +One inconsistency worth fixing on day one: `translate_sheet` stamps +`contenteditable` on every run inside an `.ods` cell when `config.editable` is +set — the writer asks the *element* (`element_is_editable`, true for a +non-repeated cell) and never the document. The reference output for +`style-color+fixed-1.ods` carries 594 of them. Two consequences: an app that +turns `editable` on gets a half-working sheet editor (strings save, numbers +desync), and the editable output lays out differently, because +`plain_text` refuses to fold an editable run into its `td`. + +## Decisions + +### 1. A cell is the unit of editing, addressed by position + +The op is `setCell {sheet, column, row, value}`. Not the run's element id, not +a path. + +**Why:** the cell a user types into may have no element — every empty cell in +both formats, every repeated cell in ODS — so an id cannot name it. A position +can, and it is what the file itself uses (`r="B3"`, the repeat cursor). It also +side-steps [`editing.md`](editing.md)'s id-stability question entirely for +sheets: nothing is renumbered when the only op replaces a cell's whole content. +Ids stay the answer for text documents, where an insertion point inside a +paragraph has no position of its own. + +Coalescing is a map keyed by position, last write wins. Undo keeps the previous +value beside the op. The whole log is idempotent, which decision 5 leans on. + +### 2. One op-log envelope replaces `modifiedText` + +```json +{ + "version": 1, + "ops": [ + {"op": "setCell", "sheet": 0, "column": 1, "row": 2, + "value": {"type": "number", "number": 12.5, "text": "12.5"}}, + {"op": "setCell", "sheet": 0, "column": 1, "row": 3, + "value": {"type": "string", "text": "total"}}, + {"op": "setCell", "sheet": 0, "column": 1, "row": 4, + "value": {"type": "empty"}} + ] +} +``` + +`Document::edit` becomes a dispatcher over `ops`, throws on the first op it +cannot apply, and applies nothing on failure (a document is decoded fresh by +`DocumentFile::document()`, so the host replays onto a copy by construction; +the wasm session, which holds one document, has to replay onto a fresh decode +too). The text-document op `setText {id, text}` joins the same envelope when +[`editing.md`](editing.md) phase 1 lands; `modifiedText` goes. The bindings +pass a string through and do not change. + +`version` is the wire version. A document stamp (decision 7 in `editing.md`) +is deferred: a sheet op names a position, and a position is meaningful against +any decode of the same file. + +### 3. Editing is a browser mode, not markup + +`odr.editing.enable()` / `disable()` turns the mode on; `HtmlConfig::editable` +stops changing what a sheet writes. The page carries only what the browser +cannot work out for itself: + +- a **lock** on a cell that cannot be edited, as a class plus its reason — + `formula`, `repeated` (ODS, until step 2), `rich` (several runs, several + paragraphs, a link, a line break), `shapes` only where the cell is nothing + but its anchored drawings; +- whether the **document** can be edited at all, one attribute on the table, + so `enable()` can refuse with a reason before the user clicks anything. + +Everything else — including every empty cell — is editable. The cost is a +class on the locked cells only, nothing on the half million others. + +**Why:** the user should not have to translate twice to switch modes, and a +static `contenteditable` is the wrong tool for a cell anyway: a `td` holds +`x-p` wrappers, the raise wrapper, shapes. The editor is an **overlay** the +script places over the cell (as every spreadsheet does), reusing the raise +geometry; the sheet's DOM is untouched until the commit patches the cell. + +**Refusal is a first-class event.** Clicking a locked cell, or any cell of a +read-only document, outlines it briefly and calls `odr.onEditRefused` so the +host can say why — a snackbar on mobile. A silent no-op is the frustrating +outcome the mode exists to avoid. Decision 7 is the channel. + +### 4. The type follows the content + +The typed string is parsed by a strict grammar: optional sign, digits, one `.`, +optional exponent — a number. Anything else is a string. A leading `'` forces +a string, as every spreadsheet does. `=` is refused in step 1 (formula input +comes with step 4). + +**Why not keep a number cell numeric:** telling the user "this cell holds a +number" is a rule no spreadsheet has, and the file has no such rule either — +`office:value-type` and `c/@t` are per cell and change freely. Letting the +type follow keeps both the value and its string right by construction: a number +cell writes `office:value` *and* the `text:p`, or `` alone; a string cell +drops `office:value`, or becomes `t="inlineStr"` with ``. + +The one thing the file has that we lack is the **number format**: a cell +formatted `€ 1.234,50` and edited to `2000` shows `2000` until the file is +reopened, where the producer formats it. Step 1 accepts that and the overlay +says it (the raw string is what the user typed). Parsing number formats is +step 5, and is a read-side gain on its own — `.xlsx` shows raw serials today. + +Decimal comma: step 1 parses `.` only. The document's locale is not read +anywhere; see open questions. + +### 5. Formulas are recomputed in C++, once, and reached through the host + +Step 1 locks formula cells and lets their cached results go stale. Step 3 +parses formulas for their *references* only, which is enough to mark the +dependents stale in the view and to keep the file honest (below). Step 4 adds +the evaluator. + +When it exists, the evaluator runs in C++ and nowhere else. The browser asks +the host — the WebView bridge on droid/ios, the worker on wasm — with the op +log, and gets back the cells whose display changed. Not per keystroke: per +commit, and only when the edited cell has dependents. + +**Why not JavaScript generated from the expression tree:** two evaluators, +one per language, with the function library — `SUM`, `VLOOKUP`, date +arithmetic, error propagation — written twice and drifting. The drift +`editing.md` accepts for op *replay* is a few tree edits; a formula engine is +hundreds of functions. + +**Why not ship the engine as wasm inside the HTML:** it is the right answer +for a host with no bridge at all, and it is the *same* C++, so the door stays +open. But it costs an emscripten build inside every platform build (the bytes +have to be compiled into the library to be written beside the document), and +no current host needs it — droid, ios and the npm package all have the engine +in process. Revisit when a static host appears. The `embed_shipped_resources` +mechanism is where such a blob would go. + +**The file has to stay honest without our engine.** An edited input leaves +cached formula results wrong in the saved file. `.xlsx` has a switch for +exactly this: `workbook.xml` `calcPr/@fullCalcOnLoad="1"` (ECMA-376 +18.2.2), set on any edited workbook. `.ods` has no such switch, and +LibreOffice trusts a file its own generator wrote — whether it recomputes a +formula cell whose cached value we *remove* is the first spike below. + +### 6. The page stays up until the user saves + +The host holds the log the page hands out (`odr.editing.getOperations()`), +the page is never re-translated for a save, and after a successful save the +host tells the page (`odr.editing.committed()`) so the log resets and undo +starts over with the file the page now matches. The `sheet{index}.html` views +each run their own script, so a host showing sheets as separate pages collects +a log per view; every op carries its sheet, so concatenation is the merge. + +### 7. Host events are flat `odr.on*` callbacks, and the host owns the wording + +The page calls out; the host listens. Commands live on `odr.editing` the way +`odr.annotation` holds the annotator's, and events stay flat on `odr`, +following `odr.onError` and `odr.onZoomChange`: + +```js +// {sheet, column, row, reason, code, message} +odr.onEditRefused = function (event) {}; +// {dirty, operations, canUndo, canRedo} +odr.onEditChange = function (event) {}; +// {editing, editable, reason, code, message} +odr.onEditModeChange = function (event) {}; +``` + +**The message is for the console; the code is for the host.** A mobile +snackbar is written in the app's own string catalogue, and nothing in this +library is localised — so the host maps `code` to its wording, and `reason` +(`"formula"`, `"repeated"`, `"rich"`, `"readOnly"`, `"encrypted"`, `"cut"`) +is the same thing spelled for a reader of the log. We still ship an English +`message`, so a developer who wires nothing sees it in the console (the +`odr.onError` default does exactly this) and a desktop host with no catalogue +can show it as it stands. + +**Codes are appended, never renumbered**, and share one space with +`odr.onError`'s — `errorIllegalEditNewLine` holds 1. The rule the wasm enum +ordinals already live under: appending stays silent, reordering goes loud. +Pin them in `test/browser/sheet` the way `tests/enums.test.mjs` pins the +enums. + +**One object argument, never positional.** `onError(code, message)` cannot +grow a field without breaking every host that implements it; an object can. +Every callback added from here takes one. + +**The page suppresses its own repeats.** Tapping a locked cell four times is +one snackbar, not four: an identical refusal within a couple of seconds of the +last is dropped by the page, which knows what it just fired. Cheaper here than +in three hosts. + +**`onEditChange` is what decision 6 needs.** `dirty` is how the app lights its +save button and warns on back-press while the page holds unsaved edits; +`canUndo`/`canRedo` drive the toolbar. It fires on every commit, undo, redo +and on `committed()`. + +**Attaching**, per host: + +- **droid / ios**: the WebView loads the view at the top level, so the host + assigns the callbacks once the page has finished loading + (`evaluateJavascript` / `evaluateJavaScript`) and hops to the UI thread to + show the snackbar. The scripts are written at the end of ``, so the + load event is late enough. +- **Browser / npm**: the view is an iframe, and the wasm example already + renders it `allow-same-origin`, so the embedder assigns on + `contentWindow.odr` at the frame's `load`. A cross-origin sandboxed frame + cannot be reached this way and is out of scope — `postMessage` if one ever + appears. +- No C++ is involved: these are page-to-host, so the wasm rule about callbacks + being worker-local and synchronous does not apply to them. + +**Why not reuse `odr.onError`:** a refusal is expected UX, not a fault. It is +frequent, it carries a position, and a host wants it on a snackbar while a real +error goes to a dialog or a log. Sharing the code table keeps one lookup for +both. + +## Staging + +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. +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. +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. + +### Step 1 — The browser editor + +1. `odr.editing` mode: enable/disable, lock classes and the document attribute + from `translate_sheet`, and the three `odr.on*` callbacks with their code + table (decision 7). +2. Overlay editor: double-click / Enter / typing opens it over the cell; Enter, + Tab and blur commit; Escape cancels; arrow keys move the pin. Position + comes from the row `` and a per-row colspan/rowspan walk, cached — never + `cellIndex`, which merged cells and sorting both break. +3. Commit: parse per decision 4, record the op with its inverse, patch the + cell — text, `odr-value-type-float` for alignment, keep any shapes in A1 — + and **reflow the row**: the spill and clip `translate_sheet` measured for + the neighbours (`clip-path:inset`, `overflow:hidden`) are stale once a blank + cell fills or a full one empties. The script already has the measuring + half (`visibleRight`, `cutOff`). +4. Undo/redo over the in-memory log; `getOperations()`; `committed()`; both + raise `onEditChange`, which is what a host's save button and back-press + warning read. +5. `test/browser/sheet` grows the editing cases; the wasm example gets an + edit-and-save button, which is also the host-wiring reference for droid/ios. + +### Step 2 — Materialise the cells that are not there + +1. ODS repeat splitting: a write into a run of `n` repeated cells becomes + left (`k`), the cell, right (`n-k-1`); a repeated row is cloned the same + way first. The `Sheet::cells` run index gets an insert (entries after the + split shift; `Row::first_cell` re-indexed). New elements are appended, ids + never move. This one primitive unlocks empty cells *and* the repeated cells + step 0 refused, so the `repeated` lock goes. +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 + `rich` lock stays on a cell with a link or a line break; it goes for + several runs of the same paragraph. + +### Step 3 — Formulas, read side + +1. Parse both syntaxes into one AST: OpenFormula (`of:=SUM([.A1:.B2])`, + `table:formula`) and OOXML (`SUM(A1:B2)`, ``, shared and array + formulas). References, ranges, sheet-qualified references, named ranges + left as opaque. +2. Reference extraction → dependency graph per document; `Document` answers + "which cells depend on this position". +3. View: a commit marks dependents stale (a class, the host is told); the + locked formula cell exposes its text (`data-odr-formula`, formula cells + only) so a formula bar or a tooltip can show it. +4. File: the `.ods` answer from the spike — drop the cached value of dirty + dependents, or whatever LibreOffice needs to recompute. + +### Step 4 — Formulas, evaluate + +1. Evaluator over the AST with a typed value (number, string, boolean, error, + empty), error propagation, and a function library opened with the + frequent thirty or so (arithmetic, `SUM`/`AVERAGE`/`MIN`/`MAX`/`COUNT`, + `IF`/`AND`/`OR`, `ROUND`, `CONCATENATE`, `VLOOKUP`/`INDEX`/`MATCH`, + `TODAY`/`DATE` with the 1900/1904 epochs). An unknown function leaves the + cached value and flags the cell rather than guessing. +2. Incremental: recompute the dirty set in topological order, cycles detected + and reported as `#REF!`-style errors. +3. `Document::recalculate(operations) → changed cells` (position, display + string, kind) — the query the host relays per decision 5; writing cached + results on save. +4. Formula input in the editor (`=`), with the parse error surfaced. + +### Step 5 — Later + +- Number formats (`number:number-style`, `numFmt`) for display, dates and + booleans as their own kinds; fixes `.xlsx` serials on the read side too. +- Multi-line cells (Alt+Enter), cell style edits, insert/delete rows and + columns (the moment ids for rows appear, `editing.md`'s append-only rules + apply). +- `.csv`: `save` is a serialiser and the cell op fits its packed ids; cheap + once the envelope exists, low value. +- The wasm-in-HTML engine, if a bridge-less host appears. + +## Low-hanging fruit + +Ordered by value over cost; all in step 0 or 1. + +- **XLSX save** — the docx save with three path names changed. +- **ODS number sync** — `office:value` beside the text, a few lines in + `text_set_content`'s successor. +- **`fullCalcOnLoad`** — one attribute, and the file stops lying after an + edit. +- **Lock classes + refusal event** — the feedback the mode needs, cheap to + emit, and the read-side view gains a marker for formula cells. The callback + is three assignments on each host, and the same channel then carries the + dirty flag the save button needs. +- **`SheetCell::value()`** — a missing read accessor; the sort script and any + binding user wants it regardless of editing. +- **The `contenteditable` gate** — one condition, removes 594 attributes from + a reference `.ods` and a layout difference between the two modes. + +## Complications to budget for + +- **ODS repeat splitting is the write primitive**, and the run index was built + to be written once. Design the insert before step 2, and keep ids + append-only (`editing.md` decision 4). +- **XLSX shared strings**: converting to `inlineStr` is self-contained but the + cell's registry subtree points into `sharedStrings.xml` — it must be + rebuilt, not patched. Verify with the oracle that a workbook mixing + `inlineStr` and shared cells round-trips. +- **Stale formula results in the file** for `.ods` (the spike). Until step 4 + there is no way to write a correct value. +- **Row reflow after a commit**: the spill/clip geometry is computed at + translate time from the neighbours; the browser has to redo it for the + edited row. Without it an edit into a blank cell shows the left neighbour's + overflow painting across the new text. +- **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`. +- **Several views, one log**: the host merges; a save with a partial log is + a partial save. The wasm package can hide this in the session. +- **Encrypted packages** are not savable (`is_savable` false after decrypt); + `enable()` refuses on the document attribute rather than after typing. +- **Decimal separator and locale** are read nowhere; a german user typing + `1,5` gets a string in step 1. +- **A1 anchors every shape** (`anchors_shapes`): the commit patch must keep + the shape nodes and replace only the text. +- **Sort and edit together**: sorting reorders ``s in the DOM and keeps an + `original` snapshot; an edit patches the `` in place, so both survive, + but the position must come from the row label. + +## Spikes before step 0 + +1. **LibreOffice and a formula cell without a cached value** in an `.ods` it + generated itself: does it recompute on load, or show empty? Also what it + does when `meta:generator` is ours. Build the probe with `soffice + --convert-to`, then hand-edit `content.xml`; render and round-trip before + trusting the spec. +2. **Excel/LibreOffice on a mixed `inlineStr` workbook** — expected fine, + worth ten minutes. +3. **The ODS run-index insert**: sketch `Sheet::insert_cell` against + `register_cell` and the `SortedSideTable` (ids appended out of position + order are fine for a hashed table, not a sorted one — `m_sheet_cells` is + sorted by id, and new ids are larger, so it holds). + +## Open questions + +- Should a string cell that receives a numeric-looking string stay a string? + Real spreadsheets say no; a phone-number column says yes. The `'` escape is + the compromise for step 1. +- Where does the document locale come from for the decimal separator — + `settings.xml`, the number format, the host? +- Does the read-only document attribute belong on the table or in a + page-level `data-odr-*` block the text editor will want too? +- Should the refusal codes be generated from one C++ table so the bindings can + hand a host the same list, rather than living only in the emitted script? diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index a3c657e79..5f65a8bb3 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -374,6 +374,10 @@ ValueType SheetCell::value_type() const { : ValueType::unknown; } +CellValue SheetCell::value() const { + return exists_() ? m_adapter2->sheet_cell_value(m_identifier) : CellValue(); +} + std::string Page::name() const { return exists_() ? m_adapter2->page_name(m_identifier) : ""; } diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index 6e5b3f9a8..46bd3be1b 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -137,6 +137,19 @@ 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; +}; + /// Collection of list types. enum class ListType { unordered, @@ -332,6 +345,8 @@ class SheetCell final [[nodiscard]] bool is_covered() const; [[nodiscard]] TableDimensions span() const; [[nodiscard]] ValueType value_type() const; + /// @ref value_type is the narrower and cheaper question the renderer asks. + [[nodiscard]] CellValue value() const; }; /// Represents a page element in a document. diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index e8740faf4..5820a18be 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -276,6 +276,8 @@ class SheetCellAdapter { sheet_cell_span(ElementIdentifier element_id) const = 0; [[nodiscard]] virtual ValueType sheet_cell_value_type(ElementIdentifier element_id) const = 0; + [[nodiscard]] virtual CellValue + sheet_cell_value(ElementIdentifier element_id) const = 0; }; class MasterPageAdapter { diff --git a/src/odr/internal/csv/csv_document.cpp b/src/odr/internal/csv/csv_document.cpp index 038eeca18..24f8bacbe 100644 --- a/src/odr/internal/csv/csv_document.cpp +++ b/src/odr/internal/csv/csv_document.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -193,6 +194,16 @@ class ElementAdapter final : public AdapterBase { sheet_cell_value_type(const ElementIdentifier element_id) const override { return m_document->value_type(column_of(element_id), row_of(element_id)); } + [[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))); + } + return result; + } // TextAdapter diff --git a/src/odr/internal/iwork/iwork_document.cpp b/src/odr/internal/iwork/iwork_document.cpp index c17bf45dd..bdea612e1 100644 --- a/src/odr/internal/iwork/iwork_document.cpp +++ b/src/odr/internal/iwork/iwork_document.cpp @@ -179,6 +179,14 @@ class ElementAdapter final : public AdapterBase { sheet_cell_value_type(const ElementIdentifier element_id) const override { return m_registry->cell_element_at(element_id).value_type; } + /// A number stays the decimal the file states, which is the cell's text. + /// 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; + } [[nodiscard]] TableDimensions table_dimensions(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/odf/README.md b/src/odr/internal/odf/README.md index 89b4c94e7..b41fd2423 100644 --- a/src/odr/internal/odf/README.md +++ b/src/odr/internal/odf/README.md @@ -113,9 +113,10 @@ Roughly ordered by importance. - [x] sheets - [x] dimensions, content range detection - [x] cell value types (float, string) + - [x] cell values (`office:value`, and `table:formula` as its own string) - [x] shapes anchored to a sheet - - [ ] computed values (stored values are used as-is; formulas are not - evaluated) + - [ ] computed values (stored values are used as-is; formulas are read but + not evaluated) - [ ] edit (currently disabled, see `Document::is_editable`) ### Presentation documents (`.odp`) diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 87dddbc1e..a3b8a4534 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -438,6 +439,22 @@ class ElementAdapter final : public AdapterBase { } return ValueType::string; } + /// [ODF 1.2] 19.386 `office:value`, 19.642 `table:formula`. A date, a time + /// and a boolean state their value elsewhere and are read as their text. + [[nodiscard]] CellValue + 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()); + } + if (const pugi::xml_attribute formula = node.attribute("table:formula")) { + result.formula = formula.value(); + } + return result; + } [[nodiscard]] PageLayout master_page_page_layout(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/oldms/spreadsheet/xls_document.cpp b/src/odr/internal/oldms/spreadsheet/xls_document.cpp index 8cee4648c..4f85aafbf 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_document.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_document.cpp @@ -134,6 +134,15 @@ class ElementAdapter final : public AdapterBase { (void)element_id; return ValueType::string; } + /// Every cell is read into its display string at parse time, so neither the + /// number behind one nor a formula expression survives. + [[nodiscard]] CellValue sheet_cell_value( + [[maybe_unused]] const ElementIdentifier element_id) const override { + (void)element_id; + CellValue result; + result.type = ValueType::string; + return result; + } [[nodiscard]] ParagraphStyle paragraph_style(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index 6c8d8cf47..9a1f71ef4 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -32,8 +32,11 @@ otherwise its own ``/`` children. `get_text` concatenates `t` and `v` nodes verbatim, so a **formula's cached `` result is shown and `` is never evaluated**. `sheet_cell_value_type` derives number-vs-string from `c/@t` (default "n" → `float_number` when a `` exists; dates/booleans/errors -report `string`). Merged ranges from `mergeCells` land in the `SheetCell` side -map as anchor `span` + `is_covered` flags at parse time. +report `string`). `sheet_cell_value` adds what that leaves out — `` parsed +as a number where the type is one, and `` as its own string. A shared +formula writes its expression on the group's master, so a member's formula is +**set and empty** rather than absent. Merged ranges from `mergeCells` land in +the `SheetCell` side map as anchor `span` + `is_covered` flags at parse time. **Style resolution: styles.xml index vectors.** `StyleRegistry` loads positional `fonts`/`fills`/`borders`/`cellStyleXfs`/`cellXfs`. A cell's `s` attribute diff --git a/src/odr/internal/ooxml/spreadsheet/README.md b/src/odr/internal/ooxml/spreadsheet/README.md index 3a0dde0c3..b09581b53 100644 --- a/src/odr/internal/ooxml/spreadsheet/README.md +++ b/src/odr/internal/ooxml/spreadsheet/README.md @@ -25,8 +25,9 @@ Roughly ordered by importance. - [x] shapes / images anchored to a sheet (`xdr:twoCellAnchor`) - [x] cell value types (number vs. string; dates/booleans/errors reported as string) - - [ ] computed values (formulas are not evaluated; the cached `` result is - shown) + - [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 diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index c8134e3fe..f915061ef 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -199,6 +200,22 @@ class ElementAdapter final : public AdapterBase { } return ValueType::string; } + /// ECMA-376 18.3.1.4 `c`: `v` is the value, `f` the formula, whose + /// expression a shared group spells on its master only. + [[nodiscard]] CellValue + 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()); + } + if (const pugi::xml_node formula = node.child("f")) { + result.formula = formula.text().get(); + } + return result; + } [[nodiscard]] TextStyle line_break_style(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/util/number_util.cpp b/src/odr/internal/util/number_util.cpp index f439d4411..d7d0f3935 100644 --- a/src/odr/internal/util/number_util.cpp +++ b/src/odr/internal/util/number_util.cpp @@ -2,11 +2,30 @@ #include #include +#include +#include +#include +#include #include namespace odr::internal::util { +std::optional number::parse(const std::string_view text) { + std::istringstream stream{std::string(text)}; + // every format we decode writes a `.`, whatever the host's locale is + stream.imbue(std::locale::classic()); + + double value = 0; + stream >> value; + if (stream.fail()) { + return {}; + } + // `>>` stops at the first character it cannot use rather than failing, which + // would take `1,5` for `1` + return (stream >> std::ws).eof() ? std::optional(value) : std::nullopt; +} + std::string number::to_string_significant(const double value, const int significant_digits) { if (!std::isfinite(value)) { diff --git a/src/odr/internal/util/number_util.hpp b/src/odr/internal/util/number_util.hpp index 81f105632..dfd3b7c97 100644 --- a/src/odr/internal/util/number_util.hpp +++ b/src/odr/internal/util/number_util.hpp @@ -1,9 +1,16 @@ #pragma once +#include #include +#include namespace odr::internal::util::number { +/// Reads @p text as a decimal number with a `.` separator, whatever the host's +/// locale — a german one would read `1234.5` as `1234`. Only blanks may +/// surround it: a unit or a group separator is refused, not truncated. +[[nodiscard]] std::optional parse(std::string_view text); + /// Renders @p value with @p significant_digits significant digits, without /// trailing zeros, never in scientific notation, which CSS and SVG lengths do /// not accept, and never in the host's locale, where a german one would write diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f57807b87..a67de9596 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -81,6 +81,7 @@ add_executable(odr_test "src/internal/odf/odf_frame_test.cpp" "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_table_test.cpp" "src/internal/oldms/doc_test.cpp" @@ -91,6 +92,7 @@ add_executable(odr_test "src/internal/ooxml/ooxml_crypto_test.cpp" "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_util_test.cpp" "src/internal/ooxml/ooxml_presentation_style_test.cpp" diff --git a/test/src/internal/odf/odf_sheet_value_test.cpp b/test/src/internal/odf/odf_sheet_value_test.cpp new file mode 100644 index 000000000..f9f2fc6fb --- /dev/null +++ b/test/src/internal/odf/odf_sheet_value_test.cpp @@ -0,0 +1,100 @@ +#include +#include +#include +#include + +#include +#include +#include + +#include + +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +/// A flat sheet holding one cell, written as @p cell. +std::string flat_sheet(const std::string &cell) { + return R"()" + R"()" + R"()" + R"()" + + cell + + R"()" + R"()"; +} + +CellValue value_of(const std::string &cell) { + const Document document = + DecodedFile(open_strategy::open_file( + std::make_shared(flat_sheet(cell)), {}, + Logger::null())) + .as_document_file() + .document(); + const Sheet sheet = (*document.root_element().children().begin()).as_sheet(); + return sheet.cell(0, 0).value(); +} + +} // namespace + +/// [ODF 1.2] 19.386: the number is `office:value`; the `text:p` beside it is +/// the producer's formatting of it. +TEST(OdfSheetValue, a_float_cell_states_its_number) { + const CellValue value = value_of( + 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()); +} + +TEST(OdfSheetValue, a_string_cell_states_no_number) { + const CellValue value = + value_of(R"()" + R"(1234.5)"); + + EXPECT_EQ(value.type, ValueType::string); + EXPECT_FALSE(value.number.has_value()); +} + +/// [ODF 1.2] 19.642 `table:formula`, whose namespace prefix is the syntax it +/// is written in and stays part of the string. +TEST(OdfSheetValue, a_formula_cell_states_both_formula_and_result) { + // a `)"` inside the attribute would close a default-delimited raw string + const CellValue value = + value_of(R"xml()" + 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); +} + +/// The type is read from `office:value-type` alone; reading the number format +/// is what would settle it. +TEST(OdfSheetValue, a_percentage_states_a_number_the_type_does_not_admit) { + const CellValue value = value_of( + R"()" + R"(25%)"); + + EXPECT_EQ(value.type, ValueType::string); + ASSERT_TRUE(value.number.has_value()); + EXPECT_DOUBLE_EQ(*value.number, 0.25); +} + +TEST(OdfSheetValue, a_number_is_read_in_one_spelling_only) { + const CellValue value = value_of( + R"()" + R"(1234,5)"); + + EXPECT_FALSE(value.number.has_value()); +} diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp index 00ec92115..69eff8549 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp @@ -1,84 +1,18 @@ #include #include -#include -#include #include -#include -#include -#include -#include -#include -#include +#include #include -#include -#include #include using namespace odr; -using namespace odr::internal; +using namespace odr::test::ooxml; namespace { -void insert(zip::ZipArchive &zip, const std::string &path, - const std::string &content) { - zip.insert_file(std::end(zip), RelPath(path), - std::make_shared(content)); -} - -/// The smallest workbook that opens: one sheet, whose `` and -/// `` are @p sheet_data and @p merge_cells. -std::shared_ptr workbook(const std::string &sheet_data, - const std::string &merge_cells) { - zip::ZipArchive zip; - insert( - zip, "[Content_Types].xml", - R"()" - R"()" - R"()" - R"()"); - insert( - zip, "_rels/.rels", - R"()" - R"()" - R"()"); - insert( - zip, "xl/workbook.xml", - R"()" - R"()"); - insert( - zip, "xl/_rels/workbook.xml.rels", - R"()" - R"()" - R"()"); - insert( - zip, "xl/styles.xml", - R"()"); - insert( - zip, "xl/worksheets/sheet1.xml", - R"()" - R"()" + - sheet_data + R"()" + merge_cells + R"()"); - - std::stringstream out; - zip.save(out); - return std::make_shared(out.str()); -} - -Sheet first_sheet(const Document &document) { - return (*document.root_element().children().begin()).as_sheet(); -} - -Document decode(const std::shared_ptr &file) { - return Document( - DecodedFile(open_strategy::open_file(file, {}, Logger::null())) - .as_document_file() - .document()); -} - constexpr const char *two_cells = R"(a)" R"(b)"; diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp new file mode 100644 index 000000000..f54a49e48 --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp @@ -0,0 +1,78 @@ +#pragma once + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace odr::test::ooxml { + +inline void insert(internal::zip::ZipArchive &zip, const std::string &path, + const std::string &content) { + zip.insert_file(std::end(zip), internal::RelPath(path), + std::make_shared(content)); +} + +/// The smallest workbook that opens: one sheet, whose `` is +/// @p sheet_data and which carries @p sheet_extra - ``, say - +/// after it. +inline std::shared_ptr +workbook(const std::string &sheet_data, const std::string &sheet_extra = "") { + internal::zip::ZipArchive zip; + insert( + zip, "[Content_Types].xml", + R"()" + R"()" + R"()" + R"()"); + insert( + zip, "_rels/.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/workbook.xml", + R"()" + R"()"); + insert( + zip, "xl/_rels/workbook.xml.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/styles.xml", + R"()"); + insert( + zip, "xl/worksheets/sheet1.xml", + R"()" + R"()" + + sheet_data + R"()" + sheet_extra + R"()"); + + std::stringstream out; + zip.save(out); + return std::make_shared(out.str()); +} + +inline Document decode(const std::shared_ptr &file) { + return Document( + DecodedFile(internal::open_strategy::open_file(file, {}, Logger::null())) + .as_document_file() + .document()); +} + +inline Sheet first_sheet(const Document &document) { + return (*document.root_element().children().begin()).as_sheet(); +} + +} // namespace odr::test::ooxml diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp new file mode 100644 index 000000000..c4051076b --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_value_test.cpp @@ -0,0 +1,69 @@ +#include +#include + +#include + +#include + +#include + +using namespace odr; +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(); +} + +} // namespace + +/// 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()); +} + +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()); +} + +/// The `` is the result the producer cached; `` is what computed it. +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)"); +} + +/// Set-and-empty says the member computes; unset would claim it does not. +TEST(OoxmlSpreadsheetValue, + a_shared_formula_member_holds_a_formula_it_cannot_spell) { + const CellValue value = value_of( + R"(8)"); + + ASSERT_TRUE(value.formula.has_value()); + EXPECT_TRUE(value.formula->empty()); +} + +/// `` holds `1`, not a quantity, and `c/@t="b"` types the cell a string. +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()); +} diff --git a/test/src/internal/util/number_util_test.cpp b/test/src/internal/util/number_util_test.cpp index 8d3636421..820439d78 100644 --- a/test/src/internal/util/number_util_test.cpp +++ b/test/src/internal/util/number_util_test.cpp @@ -1,11 +1,30 @@ #include #include +#include #include using namespace odr::internal::util::number; +TEST(Parse, reads_a_decimal_in_the_classic_spelling) { + EXPECT_EQ(parse("1234.5"), std::optional(1234.5)); + EXPECT_EQ(parse("-0.25"), std::optional(-0.25)); + EXPECT_EQ(parse("2.5e-3"), std::optional(2.5e-3)); +} + +TEST(Parse, allows_blanks_around_the_number) { + EXPECT_EQ(parse(" \t1234.5\n"), std::optional(1234.5)); +} + +/// Anything it cannot read whole is refused, so a number spelled in another +/// locale is not truncated to the part before the separator. +TEST(Parse, refuses_what_it_cannot_read_whole) { + EXPECT_FALSE(parse("1,5").has_value()); + EXPECT_FALSE(parse("12pt").has_value()); + EXPECT_FALSE(parse("").has_value()); +} + TEST(ToStringSignificant, trims_trailing_zeros) { EXPECT_EQ(to_string_significant(1.5, 7), "1.5"); EXPECT_EQ(to_string_significant(2.0, 7), "2");