feat(odf): a write takes the cached result of what reads it away

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

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

src/odr/document_element.hpp

@@ -373,6 +373,12 @@ class Sheet final : public ElementBase<internal::abstract::SheetAdapter> {
 
   /// 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.

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

src/odr/internal/odf/odf_document.cpp

@@ -2,10 +2,12 @@
 
 #include <odr/document_element.hpp>
 #include <odr/exceptions.hpp>
+#include <odr/sheet_position.hpp>
 
 #include <odr/internal/abstract/filesystem.hpp>
 #include <odr/internal/common/element_adapter.hpp>
 #include <odr/internal/common/file.hpp>
+#include <odr/internal/common/sheet_dependencies.hpp>
 #include <odr/internal/common/table_cursor.hpp>
 #include <odr/internal/crypto/crypto_util.hpp>
 #include <odr/internal/odf/odf_chart.hpp>
@@ -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<ElementIdentifier> sheets_() const {
+    std::vector<ElementIdentifier> 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<ElementIdentifier> sheets = sheets_();
+    const auto written = std::ranges::find(sheets, sheet_id);
+    if (written == sheets.end()) {
+      return;
+    }
+
+    const SheetPosition position(
+        static_cast<std::uint32_t>(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<ElementIdentifier> 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

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"

test/src/internal/odf/odf_sheet_stale_test.cpp

@@ -0,0 +1,164 @@
+#include <odr/document.hpp>
+#include <odr/document_element.hpp>
+#include <odr/file.hpp>
+#include <odr/logger.hpp>
+
+#include <odr/internal/abstract/file.hpp>
+#include <odr/internal/common/file.hpp>
+#include <odr/internal/open_strategy.hpp>
+
+#include <gtest/gtest.h>
+
+#include <memory>
+#include <sstream>
+#include <string>
+
+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"(<?xml version="1.0" encoding="UTF-8"?>)"
+         R"(<office:document office:mimetype=")"
+         R"(application/vnd.oasis.opendocument.spreadsheet">)"
+         R"(<office:body><office:spreadsheet>)" +
+         sheets + R"(</office:spreadsheet></office:body></office:document>)";
+}
+
+std::string sheet_of(const std::string &name, const std::string &cells) {
+  return R"(<table:table table:name=")" + name + R"("><table:table-row>)" +
+         cells + R"(</table:table-row></table:table>)";
+}
+
+std::string number_cell(const std::string &text) {
+  return R"(<table:table-cell office:value-type="float" office:value=")" +
+         text + R"("><text:p>)" + text + R"(</text:p></table:table-cell>)";
+}
+
+/// 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"(<table:table-cell table:formula=")" + formula +
+         R"(" office:value-type="float" office:value=")" + cached +
+         R"(" calcext:value-type="float"><text:p>)" + cached +
+         R"(</text:p></table:table-cell>)";
+}
+
+Document document_of(const std::string &source) {
+  return DecodedFile(
+             open_strategy::open_file(std::make_shared<MemoryFile>(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"(<table:table-cell table:style-name="ce1")"
+               R"( table:formula="of:=[.A1]" office:value-type="float")"
+               R"( office:value="1"><text:p>1</text:p></table:table-cell>)")));
+  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);
+}