From ba6e5d39d48ee51510efccd7cb9ab8aeaca9560f Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 12 Sep 2026 00:16:09 +0200 Subject: [PATCH] feat(odf): a write takes the cached result of what reads it away An edited input leaves every formula reading it computing an old number, and the saved file went on showing the result its producer cached. ODF states no switch asking a reader to recompute - ooxml's `fullCalcOnLoad`, which every xlsx save already sets - so the result goes instead: a cell stating a formula and no result is one a reader has to compute, and none can show a wrong number for. `drop_stale_results` asks `SheetDependencies` which cells read the written position, directly or through another formula, and takes the attributes stating a value off each. The `text:p` showing it goes as an element rather than as a node, so the registry keeps no dangling one. The formula, the cell's style and a drawing anchored in the cell all stay. A formula whose references could not be read keeps its result: nothing says it is wrong, and blanking every one of them on an unrelated edit would cost more than it is worth. Until the evaluator lands, a cleared cell renders empty here too. The spike, as far as a CLI can answer it: LibreOffice's `--convert-to` recomputes whatever a file cached - a hand-edited wrong result came back corrected - so it cannot show what a reader that does not recompute would display, and its `ODFRecalcMode` setting changed nothing. An end-to-end check confirms the saved package is valid and that LibreOffice computes the cell we cleared. Step 3.4 of `docs/design/spreadsheet-editing.md`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VVmjmddv2Ui17Nptc1ggue --- CHANGELOG.md | 8 + docs/design/spreadsheet-editing.md | 29 +++- src/odr/document_element.hpp | 6 + src/odr/internal/odf/AGENTS.md | 11 ++ src/odr/internal/odf/odf_document.cpp | 79 +++++++++ test/CMakeLists.txt | 1 + .../src/internal/odf/odf_sheet_stale_test.cpp | 164 ++++++++++++++++++ 7 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 test/src/internal/odf/odf_sheet_stale_test.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 22753b648..e4e050111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,14 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Writing a cell of an `.ods` takes the cached result of every formula reading + it - and of every formula reading those - away, keeping the formula, the + cell's style and any drawing anchored in it. ODF states no switch asking a + reader to recompute, so a cell stating a formula and no result is the only + way the saved file can avoid showing a number that is now wrong. Such a cell + renders empty until a reader computes it. An `.xlsx` keeps its cached + results, because every save already sets `calcPr/@fullCalcOnLoad`. + - An editable sheet view states what each formula cell computes (`data-odr-formula`, which `odr.sheet.formulaAt` hands out) and the cells it reads. Committing an edit marks the formula cells reading it - and the ones diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 96993139c..dd4afa151 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -1,6 +1,6 @@ # Spreadsheet editing design -Status: **steps 0, 1 and 2 landed; step 3 is under way.** This +Status: **steps 0 to 3 landed; step 4 — the evaluator — is next.** This records why spreadsheet editing is staged the way it is, what the code already gives us, and the order the steps go in. It is a plan, not a record — update it as steps land. @@ -516,8 +516,25 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. The page carries the graph rather than asking the host for it, because a mark has to keep up with typing. The engine's own graph (item 2) is what the file side uses. -4. File: the `.ods` answer from the spike — drop the cached value of dirty - dependents, or whatever LibreOffice needs to recompute. +4. **Landed.** File: an odf write takes the cached result of every formula + reading it away — the attributes stating a value, and the `text:p` showing + it, removed as an element so the registry keeps no dangling node. The + formula, the cell's style and a drawing anchored in it stay. An ooxml + write keeps them, because every save already sets `fullCalcOnLoad`. + + **The spike, answered as far as it can be.** ODF states no switch asking a + reader to recompute, so there is nothing to set. LibreOffice's + `--convert-to` is no oracle for what a reader shows: a file whose cached + result was hand-edited to a wrong number converted to the *right* one, so + the filter recomputes whatever the file cached, and its `ODFRecalcMode` + setting (default: ask, and only for a file its own generator did not + write) changed nothing. What is left is the rule the file itself can + carry: a cell stating a formula and no result is one a reader has to + compute, and none can show a wrong number for. + + The cost until step 4: such a cell renders empty here too. A formula whose + references could not be read (`unresolved_formulas`) keeps its result + rather than blanking on every unrelated edit — nothing says it is wrong. ### Step 4 — Formulas, evaluate @@ -572,8 +589,10 @@ Ordered by value over cost; all in step 0 or 1. 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. +- **Stale formula results in the file** for `.ods`: **answered** in step 3.4 — + the result is dropped rather than left wrong, because ODF states no switch + asking a reader to recompute. Until step 4 there is still no way to write a + correct value, so such a cell renders empty here. - **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 diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index acdc5dca5..05502ab9c 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -373,6 +373,12 @@ class Sheet final : public ElementBase { /// 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. + /// + /// Every formula reading the cell is now computing an old input, so an odf + /// document takes their cached results away - a cell stating a formula and + /// no result is one a reader has to compute. An ooxml one asks its reader + /// to recompute everything on load instead (`calcPr/@fullCalcOnLoad`), so + /// it keeps them. Nothing here computes a formula. /// @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. diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index c59aef2df..e8d86d717 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -213,6 +213,17 @@ The structural/foundational gaps, roughly by value: reachable. Their `pugi::xml_node` is dangling from then on, which is the cost `ooxml/spreadsheet` already pays for the same tombstoning. + A write also **takes the cached result of every formula reading it away** + (`drop_stale_results`), through `SheetDependencies`: the attributes that + state a value go, and the `text:p` showing it is removed as an element so + the registry does not keep a dangling node. ODF states no switch asking a + reader to recompute — ooxml's `fullCalcOnLoad` — so a cell stating a + formula and no result is the only way the file can avoid showing a wrong + number. The formula, the cell's style and a drawing anchored in it stay. A + formula whose references could not be read + (`SheetDependencies::unresolved`) keeps its result: nothing says it is + wrong. Until the evaluator lands, such a cell renders empty here too. + A position the sheet stops before is reached by `grow_to_cell`, which appends the rows and the runs of empty cells it takes and declares the columns, so `dimensions` covers the new cell. A repeated row is cut before diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 1c85a320d..31ff8cb16 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -2,10 +2,12 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -490,6 +492,83 @@ class ElementAdapter final : public AdapterBase { .set_value(fmt::format("{}", value.number()).c_str()); break; } + + drop_stale_results(element_id, column, row); + } + + /// The sheets of the document in the order an operation names them by. + [[nodiscard]] std::vector sheets_() const { + std::vector result; + for (ElementIdentifier id = element_first_child(m_document->root_element()); + id != null_element_id; id = element_next_sibling(id)) { + if (element_type(id) == ElementType::sheet) { + result.push_back(id); + } + } + return result; + } + + /// A write leaves every formula reading it computing an old input. ODF + /// states no switch asking a reader to recompute - ooxml's + /// `fullCalcOnLoad` - so the cached result goes instead: a cell stating a + /// formula and no result is one a reader has to compute, and none can show + /// a wrong number for. A formula whose references could not be read + /// (`SheetDependencies::unresolved`) keeps its result, because nothing says + /// it is wrong. + void drop_stale_results(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { + const std::vector sheets = sheets_(); + const auto written = std::ranges::find(sheets, sheet_id); + if (written == sheets.end()) { + return; + } + + const SheetPosition position( + static_cast(written - sheets.begin()), + TablePosition(column, row)); + for (const SheetPosition &stale : + m_document->sheet_dependencies().dependents({position})) { + if (stale.sheet >= sheets.size()) { + continue; + } + if (const ElementRegistry::Sheet::Cell *cell = + m_registry->sheet_element_at(sheets[stale.sheet]) + .cell(stale.cell.column, stale.cell.row); + cell != nullptr) { + drop_cell_result(*cell); + } + } + } + + /// What a cell states about a result, and nothing else: the formula, the + /// cell's own style and a drawing anchored in it all stay. + void drop_cell_result(const ElementRegistry::Sheet::Cell &cell) const { + pugi::xml_node node = cell.node; + 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); + } + if (cell.element_id == null_element_id) { + return; + } + + // Collected first: removing one relinks the chain the walk is on. The + // elements over the paragraph keep their ids and stop being reachable, + // which is the tombstoning every write here does. + std::vector paragraphs; + for (ElementIdentifier child = element_first_child(cell.element_id); + child != null_element_id; child = element_next_sibling(child)) { + if (element_type(child) == ElementType::paragraph) { + paragraphs.push_back(child); + } + } + for (const ElementIdentifier paragraph : paragraphs) { + element_remove(paragraph); + } } [[nodiscard]] TableStyle diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e9b53d91b..b1a4e39b0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -87,6 +87,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_stale_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" diff --git a/test/src/internal/odf/odf_sheet_stale_test.cpp b/test/src/internal/odf/odf_sheet_stale_test.cpp new file mode 100644 index 000000000..9039299d3 --- /dev/null +++ b/test/src/internal/odf/odf_sheet_stale_test.cpp @@ -0,0 +1,164 @@ +#include +#include +#include +#include + +#include +#include +#include + +#include + +#include +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +/// A flat spreadsheet of @p sheets, each written as a `table:table`. +std::string flat_document(const std::string &sheets) { + return R"()" + R"()" + R"()" + + sheets + R"()"; +} + +std::string sheet_of(const std::string &name, const std::string &cells) { + return R"()" + + cells + R"()"; +} + +std::string number_cell(const std::string &text) { + return R"()" + text + R"()"; +} + +/// A cell computing @p formula, with the result its producer cached. +std::string computed_cell(const std::string &formula, + const std::string &cached) { + return R"()" + cached + + 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 sheet_at(const Document &document, const std::uint32_t index) { + Element element = *document.root_element().children().begin(); + for (std::uint32_t i = 0; i < index; ++i) { + element = element.next_sibling(); + } + return element.as_sheet(); +} + +/// One row: A1 a number, B1 the sum of A1 and nothing else, C1 twice B1. +std::string chain_sheet() { + return sheet_of("s", number_cell("1") + + computed_cell("of:=SUM([.A1:.A1])", "1") + + computed_cell("of:=[.B1]*2", "2")); +} + +} // namespace + +/// ODF states no switch asking a reader to recompute, so the result goes: a +/// cell stating a formula and no result is one a reader has to compute. +TEST(OdfSheetStale, a_write_takes_the_result_of_what_reads_it_away) { + const Document document = document_of(flat_document(chain_sheet())); + const Sheet sheet = sheet_at(document, 0); + + sheet.set_cell(0, 0, CellValue(10.0, "10")); + + const CellValue stale = sheet.cell(1, 0).value(); + EXPECT_FALSE(stale.has_number()); + EXPECT_EQ(stale.text(), ""); + ASSERT_TRUE(stale.has_formula()); + EXPECT_EQ(stale.formula(), "of:=SUM([.A1:.A1])"); +} + +/// A formula reading a formula is wrong for the same reason. +TEST(OdfSheetStale, the_whole_chain_loses_its_results) { + const Document document = document_of(flat_document(chain_sheet())); + const Sheet sheet = sheet_at(document, 0); + + sheet.set_cell(0, 0, CellValue(10.0, "10")); + + EXPECT_FALSE(sheet.cell(2, 0).value().has_number()); + EXPECT_EQ(sheet.cell(2, 0).value().text(), ""); +} + +TEST(OdfSheetStale, a_formula_reading_something_else_keeps_its_result) { + const Document document = document_of( + flat_document(sheet_of("s", number_cell("1") + number_cell("2") + + computed_cell("of:=[.B1]", "2")))); + const Sheet sheet = sheet_at(document, 0); + + sheet.set_cell(0, 0, CellValue(10.0, "10")); + + EXPECT_DOUBLE_EQ(sheet.cell(2, 0).value().number(), 2); + EXPECT_EQ(sheet.cell(2, 0).value().text(), "2"); +} + +TEST(OdfSheetStale, a_formula_on_another_sheet_loses_its_result_too) { + const Document document = document_of( + flat_document(sheet_of("Data", number_cell("1")) + + sheet_of("Report", computed_cell("of:=[Data.A1]*2", "2")))); + + sheet_at(document, 0).set_cell(0, 0, CellValue(10.0, "10")); + + EXPECT_FALSE(sheet_at(document, 1).cell(0, 0).value().has_number()); +} + +/// The cell keeps everything but the result, so a reader that computes one +/// shows it the way the file states. +TEST(OdfSheetStale, the_cell_keeps_its_style_and_its_formula) { + const Document document = document_of(flat_document(sheet_of( + "s", number_cell("1") + + R"(1)"))); + const Sheet sheet = sheet_at(document, 0); + + sheet.set_cell(0, 0, CellValue(10.0, "10")); + + std::ostringstream saved; + document.save(saved); + + EXPECT_NE(saved.str().find(R"(table:style-name="ce1")"), std::string::npos); + EXPECT_NE(saved.str().find(R"(table:formula="of:=[.A1]")"), + std::string::npos); + EXPECT_EQ(saved.str().find(R"(office:value="1")"), std::string::npos); +} + +TEST(OdfSheetStale, the_saved_file_states_no_result_either) { + const Document document = document_of(flat_document(chain_sheet())); + sheet_at(document, 0).set_cell(0, 0, CellValue(10.0, "10")); + + std::ostringstream saved; + document.save(saved); + + const Document reopened = document_of(saved.str()); + EXPECT_FALSE(sheet_at(reopened, 0).cell(1, 0).value().has_number()); + EXPECT_DOUBLE_EQ(sheet_at(reopened, 0).cell(0, 0).value().number(), 10); +} + +/// Nothing reads the written cell, so no result is taken away. +TEST(OdfSheetStale, a_write_nothing_reads_leaves_every_result_alone) { + const Document document = document_of(flat_document( + sheet_of("s", number_cell("1") + computed_cell("of:=[.A1]", "1")))); + const Sheet sheet = sheet_at(document, 0); + + sheet.set_cell(2, 0, CellValue(10.0, "10")); + + EXPECT_DOUBLE_EQ(sheet.cell(1, 0).value().number(), 1); +}