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); +}