From 07199f0205ce2eca4ad50fe198c1eda97b5bd4d4 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 10 Sep 2026 14:15:08 +0200 Subject: [PATCH] feat(document): a text edit that spans several runs `insertText` puts a run beside one that is there, in the same parent so it carries the same style, and `removeElement` takes an element and its subtree away. With `setText` on each end, a selection that starts in one run and ends in another replays as the two ends rewritten and what lay between removed. `Document::remove`, `Document::insert_text_before` and `insert_text_after` are the same three in the C++ API. They sit on the document rather than on a handle because a handle says what an element holds and the document says what the tree holds - an `Element` is immutable, and restructuring the tree through one would leave a handle naming something unreachable. Each refuses an element of another document. An operation that creates an element states a negative id for it, and a later operation names it by the same number, so replay stays a pure function of the log. The shared registry grows the links a structural edit needs - `insert_sibling_after`, `insert_sibling_before` and `unlink_child`. Until now nothing built a tree except a parser reading forward, so `append_child` was the whole surface. An unlinked element keeps its id and stops being reachable, so an id an edit already handed out never names something else. `xml::TreeEditor` is where the dom half lives, over any registry whose elements carry a `pugi::xml_node`. odf, ooxml text and ooxml presentation all resolve an id to a node, splice the subtree and fix up the links; only the tag names differ, and those come from the nodes. Each engine writes its own text nodes and nothing else. Fixes a `.docx` edit dropping the space at either end of a run: the `w:t` now states `xml:space="preserve"` where the text needs it. A lone space is part of a `string` token, so the token type never said one was there. Verified with headless LibreOffice on the saved package. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW --- CHANGELOG.md | 7 + docs/design/document-editing.md | 12 + src/odr/definitions.hpp | 3 + src/odr/document.cpp | 110 ++++++++- src/odr/document.hpp | 23 ++ src/odr/internal/abstract/document.hpp | 17 ++ src/odr/internal/common/element_registry.hpp | 77 ++++++ src/odr/internal/odf/odf_document.cpp | 135 ++++++----- src/odr/internal/ooxml/ooxml_util.cpp | 51 ++++ src/odr/internal/ooxml/ooxml_util.hpp | 7 + .../ooxml/text/ooxml_text_document.cpp | 86 +++---- src/odr/internal/xml/xml_tree_edit.hpp | 56 +++++ test/src/document_edit_test.cpp | 222 ++++++++++++++++++ test/src/document_test.cpp | 114 ++++++++- test/src/internal/ooxml/ooxml_util_test.cpp | 46 ++++ 15 files changed, 846 insertions(+), 120 deletions(-) create mode 100644 src/odr/internal/xml/xml_tree_edit.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index d81ba4afa..2129d9aa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,13 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- An edit can span several runs: the new `insertText` and `removeElement` + operations, and `Document::remove` / `insert_text_before` / `insert_text_after` + in C++. ODF and `.docx`; every other format refuses. + +- **Fix**: a `.docx` edit keeps the space at either end of a run, by stating + `xml:space="preserve"` on the `w:t` that needs it. + - **Breaking** (wire only): an edit operation names its element by the id the render writes into the page, `data-odr-id`, not by a document path. The envelope is `{"version": 2, ...}`; version 1 is refused. An editable render diff --git a/docs/design/document-editing.md b/docs/design/document-editing.md index b33c2a35a..ff6ad7da2 100644 --- a/docs/design/document-editing.md +++ b/docs/design/document-editing.md @@ -206,6 +206,16 @@ Every other id is one the page wrote. {"op": "mergeParagraph", "paragraph": 9}] ``` +## Where the C++ API puts an edit + +A **handle** says what an element holds — `Text::set_content`, +`Sheet::set_cell`. The **document** says what the tree holds — +`Document::remove`, `Document::insert_text_before` / `insert_text_after`, and +the paragraph operations below. An `Element` is an immutable handle, so +restructuring the tree through one would leave a handle naming something +unreachable; and the document is what owns the tree either way. Each structural +call refuses an element of another document. + ## The adapter surface Alongside `TextAdapter::text_set_content`, all defaulting to @@ -255,8 +265,10 @@ Each step is a pull request that builds and tests on its own. 1. **Address by id.** `data-odr-id` on runs and paragraphs, `Document::element_by_id`, `setText` by id, envelope version 2. + **Landed.** 2. **Runs come and go.** `insertText` and `removeElement`, the registry links they need, odf and ooxml text. A selection spanning runs is replayable. + **Landed.** 3. **Paragraphs split and merge.** `splitParagraph`, `mergeParagraph`, `insertParagraph`. 4. **The browser editor.** Model-first, owns the DOM mutation, records the ops, diff --git a/src/odr/definitions.hpp b/src/odr/definitions.hpp index 3627eac34..9b3f40ebd 100644 --- a/src/odr/definitions.hpp +++ b/src/odr/definitions.hpp @@ -8,4 +8,7 @@ using ElementIdentifier = std::uint64_t; static constexpr ElementIdentifier null_element_id{0}; +/// Which side of an anchor a new element goes on. +enum class Placement : std::uint8_t { before, after }; + } // namespace odr diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 302214867..3d47aa29a 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -127,18 +128,59 @@ void Document::edit(const std::string_view operations, throw std::invalid_argument("unsupported edit version"); } + // an operation that creates an element states a negative id for it, and a + // later one names it by that number (`docs/design/document-editing.md`) + std::unordered_map minted; + // the element @p field names, checked to be one this document holds const auto element_of = [&](const nlohmann::json &operation, const char *field) { - const auto identifier = operation.at(field).get(); + const auto address = operation.at(field).get(); + ElementIdentifier identifier{}; + if (address < 0) { + const auto entry = minted.find(address); + if (entry == std::end(minted)) { + throw std::invalid_argument("element " + std::to_string(address) + + " has not been created"); + } + identifier = entry->second; + } else { + identifier = static_cast(address); + } const Element element = element_by_id(identifier); if (!element) { - throw std::invalid_argument("element " + std::to_string(identifier) + + throw std::invalid_argument("element " + std::to_string(address) + " not found"); } return element; }; + // the run @p field names, refusing an element that is not one + const auto text_of = [&](const nlohmann::json &operation, const char *field) { + const Element element = element_of(operation, field); + const Text text = element.as_text(); + if (!text) { + throw std::invalid_argument("element " + + std::to_string(element.identifier()) + + " is not a text element"); + } + return text; + }; + + // the negative id an operation reserves, checked before anything is created + // so that a refusal changes nothing + const auto reserve = [&](const nlohmann::json &operation) { + const auto address = operation.at("id").get(); + if (address >= 0) { + throw std::invalid_argument("a created element needs a negative id"); + } + if (minted.contains(address)) { + throw std::invalid_argument("element " + std::to_string(address) + + " has been created twice"); + } + return address; + }; + for (const nlohmann::json &operation : json.at("ops")) { const auto name = operation.at("op").get(); @@ -151,14 +193,28 @@ void Document::edit(const std::string_view operations, } if (name == "setText") { - const Element element = element_of(operation, "id"); - const Text text = element.as_text(); - if (!text) { - throw std::invalid_argument("element " + - std::to_string(element.identifier()) + - " is not a text element"); + text_of(operation, "id") + .set_content(operation.at("text").get()); + continue; + } + + if (name == "insertText") { + const bool after = operation.contains("after"); + if (after == operation.contains("before")) { + throw std::invalid_argument( + "insertText names one of `after` and `before`"); } - text.set_content(operation.at("text").get()); + const std::int64_t address = reserve(operation); + const Text anchor = text_of(operation, after ? "after" : "before"); + const auto text = operation.at("text").get(); + const Text created = after ? insert_text_after(anchor, text) + : insert_text_before(anchor, text); + minted.emplace(address, created.identifier()); + continue; + } + + if (name == "removeElement") { + remove(element_of(operation, "id")); continue; } @@ -181,6 +237,42 @@ Element Document::element_by_id(const ElementIdentifier identifier) const { return {adapter, identifier}; } +ElementIdentifier Document::check_(const Element &element) const { + if (element_by_id(element.identifier()) != element) { + throw std::invalid_argument("element is not this document's"); + } + return element.identifier(); +} + +void Document::remove(const Element &element) const { + m_impl->element_adapter()->element_remove(check_(element)); +} + +Text Document::insert_text_before(const Text &anchor, + const std::string &text) const { + return insert_text_(anchor, Placement::before, text); +} + +Text Document::insert_text_after(const Text &anchor, + const std::string &text) const { + return insert_text_(anchor, Placement::after, text); +} + +Text Document::insert_text_(const Text &anchor, const Placement where, + const std::string &text) const { + const internal::abstract::ElementAdapter *adapter = m_impl->element_adapter(); + const ElementIdentifier anchor_id = check_(anchor); + const internal::abstract::TextAdapter *runs = + adapter->text_adapter(anchor_id); + if (runs == nullptr) { + throw std::invalid_argument("element " + std::to_string(anchor_id) + + " is not a text element"); + } + const ElementIdentifier identifier = + runs->text_insert(anchor_id, where, text); + return {adapter, identifier, adapter->text_adapter(identifier)}; +} + Filesystem Document::as_filesystem() const { if (std::shared_ptr files = m_impl->as_filesystem()) { diff --git a/src/odr/document.hpp b/src/odr/document.hpp index c286896e6..2e90e227e 100644 --- a/src/odr/document.hpp +++ b/src/odr/document.hpp @@ -19,6 +19,7 @@ class DocumentFile; class Element; class File; class Filesystem; +class Text; /// Represents a document. class Document final { @@ -65,6 +66,22 @@ class Document final { /// exist where this document holds no such id. [[nodiscard]] Element element_by_id(ElementIdentifier identifier) const; + /// @name Structural edits + /// Each throws `UnsupportedOperation` where the engine cannot write, and + /// `std::invalid_argument` for an element of another document. + /// @{ + + /// Removes @p element and its subtree; its identifier stays taken. + void remove(const Element &element) const; + + /// A run beside @p anchor, in the same parent, so it takes the same style. + [[nodiscard]] Text insert_text_before(const Text &anchor, + const std::string &text) const; + [[nodiscard]] Text insert_text_after(const Text &anchor, + const std::string &text) const; + + /// @} + /// The files the document is packaged from; empty for a document that is /// one file. [[nodiscard]] Filesystem as_filesystem() const; @@ -72,6 +89,12 @@ class Document final { private: std::shared_ptr m_impl; + /// @p element 's identifier, checked to be one this document holds. + [[nodiscard]] ElementIdentifier check_(const Element &element) const; + + [[nodiscard]] Text insert_text_(const Text &anchor, Placement where, + const std::string &text) const; + friend DocumentFile; }; diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index ebfbedfcf..f7312004d 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -4,6 +4,7 @@ // for the element model itself: `FrameAdapter` defaults its shape readers, and // a default needs the complete type. #include +#include #include #include @@ -107,6 +108,13 @@ class ElementAdapter { element_navigate_path(ElementIdentifier element_id, const DocumentPath &path) const = 0; + /// Removes @p element_id and its subtree; it keeps its id and stops being + /// reachable, so an id already handed out never names something else. + virtual void + element_remove([[maybe_unused]] const ElementIdentifier element_id) const { + throw UnsupportedOperation(); + } + [[nodiscard]] virtual const TextRootAdapter * text_root_adapter([[maybe_unused]] const ElementIdentifier element_id) const { return nullptr; @@ -332,6 +340,15 @@ class TextAdapter { virtual void text_set_content(ElementIdentifier element_id, const std::string &text) const = 0; + /// A run beside @p element_id, in the same parent, so it takes the same + /// style. + virtual ElementIdentifier + text_insert([[maybe_unused]] const ElementIdentifier element_id, + [[maybe_unused]] const Placement where, + [[maybe_unused]] const std::string &text) const { + throw UnsupportedOperation(); + } + [[nodiscard]] virtual TextStyle text_style(ElementIdentifier element_id) const = 0; }; diff --git a/src/odr/internal/common/element_registry.hpp b/src/odr/internal/common/element_registry.hpp index 4e91a1d0a..609153f41 100644 --- a/src/odr/internal/common/element_registry.hpp +++ b/src/odr/internal/common/element_registry.hpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -123,6 +124,45 @@ class ElementRegistry { parent.last_child_id); } + /// Links @p child_id beside @p anchor_id. Indices, as @ref link_child wants + /// them, and the element's own chain rather than one a payload holds. + void insert_sibling_after(const ElementIdentifier anchor_id, + const ElementIdentifier child_id) { + insert_sibling_(anchor_id, child_id, Placement::after); + } + void insert_sibling_before(const ElementIdentifier anchor_id, + const ElementIdentifier child_id) { + insert_sibling_(anchor_id, child_id, Placement::before); + } + + /// Unlinks @p child_id from that same chain; it keeps its id and its + /// payload and stops being reachable. + void unlink_child(const ElementIdentifier child_id) { + Element &child = element_at(child_id); + const Id parent_id = child.parent_id; + if (parent_id == null_element_id) { + throw std::invalid_argument( + "ElementRegistry::unlink_child: child has no parent"); + } + const Id previous_id = child.previous_sibling_id; + const Id next_id = child.next_sibling_id; + + if (previous_id != null_element_id) { + element_at(previous_id).next_sibling_id = next_id; + } else { + element_at(parent_id).first_child_id = next_id; + } + if (next_id != null_element_id) { + element_at(next_id).previous_sibling_id = previous_id; + } else { + element_at(parent_id).last_child_id = previous_id; + } + + child.parent_id = null_element_id; + child.previous_sibling_id = null_element_id; + child.next_sibling_id = null_element_id; + } + protected: ~ElementRegistry() = default; @@ -160,6 +200,43 @@ class ElementRegistry { last_id = static_cast(child_id); } + void insert_sibling_(const ElementIdentifier anchor_id, + const ElementIdentifier child_id, + const Placement where) { + Element &anchor = element_at(anchor_id); + const Id parent_id = anchor.parent_id; + if (parent_id == null_element_id) { + throw std::invalid_argument( + "ElementRegistry::insert_sibling: anchor has no parent"); + } + Element &child = element_at(child_id); + if (child.parent_id != null_element_id) { + throw std::invalid_argument( + "ElementRegistry::insert_sibling: child already has a parent"); + } + + const Id previous_id = where == Placement::after + ? static_cast(anchor_id) + : anchor.previous_sibling_id; + const Id next_id = where == Placement::after ? anchor.next_sibling_id + : static_cast(anchor_id); + + child.parent_id = parent_id; + child.previous_sibling_id = previous_id; + child.next_sibling_id = next_id; + + if (previous_id != null_element_id) { + element_at(previous_id).next_sibling_id = static_cast(child_id); + } else { + element_at(parent_id).first_child_id = static_cast(child_id); + } + if (next_id != null_element_id) { + element_at(next_id).previous_sibling_id = static_cast(child_id); + } else { + element_at(parent_id).last_child_id = static_cast(child_id); + } + } + void check_element_id(const ElementIdentifier id) const { if (id == null_element_id) { throw std::out_of_range( diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 169df9933..f5c2b9b9b 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -172,6 +173,57 @@ std::optional connector_box(const pugi::xml_node node) { return read_path(node); } +using TreeEditor = xml::TreeEditor; +using xml::NodeSpan; + +/// Writes @p text as odf text nodes before @p before, or at the end of +/// @p parent where that is null. Empty text still gets a node to anchor to. +NodeSpan write_text_nodes(pugi::xml_node parent, const pugi::xml_node before, + const std::string &text) { + NodeSpan span; + + const auto track = [&](const pugi::xml_node new_node) { + if (!span.first) { + span.first = new_node; + } + span.last = new_node; + return new_node; + }; + const auto insert = [&](const pugi::xml_node_type type) { + return track(before ? parent.insert_child_before(type, before) + : parent.append_child(type)); + }; + const auto insert_named = [&](const char *name) { + return track(before ? parent.insert_child_before(name, before) + : parent.append_child(name)); + }; + + for (const xml::StringToken &token : xml::tokenize_text(text)) { + switch (token.type) { + case xml::StringToken::Type::none: + break; + case xml::StringToken::Type::string: { + auto text_node = insert(pugi::xml_node_type::node_pcdata); + text_node.text().set(token.string.c_str()); + } break; + case xml::StringToken::Type::spaces: { + auto space_node = insert_named("text:s"); + space_node.prepend_attribute("text:c").set_value(token.string.size()); + } break; + case xml::StringToken::Type::tabs: { + for (std::size_t i = 0; i < token.string.size(); ++i) { + insert_named("text:tab"); + } + } break; + } + } + + if (!span.first) { + insert(pugi::xml_node_type::node_pcdata); + } + return span; +} + using AdapterBase = internal::RegistryElementAdapter< ElementRegistry, abstract::TextRootAdapter, abstract::SlideAdapter, abstract::PageAdapter, abstract::SheetAdapter, abstract::SheetCellAdapter, @@ -559,63 +611,40 @@ class ElementAdapter final : public AdapterBase { ElementRegistry::Text &text_element = m_registry->text_element_at(element_id); - pugi::xml_node parent = get_node(element_id).parent(); - const pugi::xml_node old_first = get_node(element_id); - const pugi::xml_node old_last = text_element.last; - // the removal loop below invalidates `old_last` - const pugi::xml_node old_end = old_last.next_sibling(); - pugi::xml_node new_first = old_first; - pugi::xml_node new_last = old_last; - - const auto track = [&](const pugi::xml_node new_node) { - if (new_first == old_first) { - new_first = new_node; - } - new_last = new_node; - return new_node; - }; - const auto insert_pcdata = [&] { - return track(parent.insert_child_before(pugi::xml_node_type::node_pcdata, - old_first)); - }; - const auto insert_node = [&](const char *node) { - return track(parent.insert_child_before(node, old_first)); - }; + const NodeSpan old_span{element.node, text_element.last}; + pugi::xml_node parent = old_span.first.parent(); + const NodeSpan new_span = write_text_nodes(parent, old_span.first, text); - for (const xml::StringToken &token : xml::tokenize_text(text)) { - switch (token.type) { - case xml::StringToken::Type::none: - break; - case xml::StringToken::Type::string: { - auto text_node = insert_pcdata(); - text_node.text().set(token.string.c_str()); - } break; - case xml::StringToken::Type::spaces: { - auto space_node = insert_node("text:s"); - space_node.prepend_attribute("text:c").set_value(token.string.size()); - } break; - case xml::StringToken::Type::tabs: { - for (std::size_t i = 0; i < token.string.size(); ++i) { - insert_node("text:tab"); - } - } break; - } - } + element.node = new_span.first; + text_element.last = new_span.last; - if (new_first == old_first) { - // empty text still needs a live node to anchor the element to, or the - // removal below would leave the registry pointing at freed nodes - insert_pcdata(); - } - - element.node = new_first; - text_element.last = new_last; + xml::remove_nodes(old_span); + } - for (pugi::xml_node node = old_first; node != old_end;) { - const pugi::xml_node next = node.next_sibling(); - parent.remove_child(node); - node = next; + [[nodiscard]] ElementIdentifier + text_insert(const ElementIdentifier element_id, const Placement where, + const std::string &text) const override { + const ElementRegistry::Text &anchor = + m_registry->text_element_at(element_id); + pugi::xml_node parent = get_node(element_id).parent(); + // a run beside this one in the same parent carries the same style + const pugi::xml_node before = where == Placement::after + ? anchor.last.next_sibling() + : get_node(element_id); + + const NodeSpan span = write_text_nodes(parent, before, text); + const auto &[new_id, unused_element, unused_text] = + m_registry->create_text_element(span.first, span.last); + if (where == Placement::after) { + m_registry->insert_sibling_after(element_id, new_id); + } else { + m_registry->insert_sibling_before(element_id, new_id); } + return new_id; + } + + void element_remove(const ElementIdentifier element_id) const override { + TreeEditor(*m_registry).remove(element_id); } [[nodiscard]] TextStyle text_style(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/ooxml/ooxml_util.cpp b/src/odr/internal/ooxml/ooxml_util.cpp index 3712961db..21bee22b0 100644 --- a/src/odr/internal/ooxml/ooxml_util.cpp +++ b/src/odr/internal/ooxml/ooxml_util.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -36,6 +37,56 @@ std::optional font_style_from_value(const char *value) { } // namespace +xml::NodeSpan ooxml::write_text_nodes(pugi::xml_node parent, + const pugi::xml_node before, + const std::string &text) { + xml::NodeSpan span; + + const auto insert = [&](const char *name) { + const pugi::xml_node node = before + ? parent.insert_child_before(name, before) + : parent.append_child(name); + if (!span.first) { + span.first = node; + } + span.last = node; + return node; + }; + // [ECMA-376] Part 1 17.3.3.31: without `xml:space` a reader collapses the + // space at either end of a `w:t`, and a lone space is part of a `string` + // token - so the text says whether one is there, not the token type. + const auto insert_text = [&](const std::string &token) { + pugi::xml_node node = insert("w:t"); + if (token.starts_with(' ') || token.ends_with(' ')) { + node.append_attribute("xml:space").set_value("preserve"); + } + node.append_child(pugi::xml_node_type::node_pcdata) + .text() + .set(token.c_str()); + }; + + for (const xml::StringToken &token : xml::tokenize_text(text)) { + switch (token.type) { + case xml::StringToken::Type::none: + break; + case xml::StringToken::Type::string: + case xml::StringToken::Type::spaces: + insert_text(token.string); + break; + case xml::StringToken::Type::tabs: + for (std::size_t i = 0; i < token.string.size(); ++i) { + insert("w:tab"); + } + break; + } + } + + if (!span.first) { + insert("w:t"); + } + return span; +} + std::optional ooxml::read_string_attribute(const pugi::xml_attribute attribute) { if (!attribute) { diff --git a/src/odr/internal/ooxml/ooxml_util.hpp b/src/odr/internal/ooxml/ooxml_util.hpp index b1748f0e9..aaba5561d 100644 --- a/src/odr/internal/ooxml/ooxml_util.hpp +++ b/src/odr/internal/ooxml/ooxml_util.hpp @@ -2,6 +2,8 @@ #include +#include + #include #include #include @@ -26,6 +28,11 @@ class AbsPath; namespace odr::internal::ooxml { +/// Writes @p text as `w:t` / `w:tab` nodes before @p before, or at the end of +/// @p parent where that is null. Empty text still gets a node to anchor to. +xml::NodeSpan write_text_nodes(pugi::xml_node parent, pugi::xml_node before, + const std::string &text); + std::optional read_string_attribute(pugi::xml_attribute); std::optional read_color_attribute(pugi::xml_attribute); std::optional read_half_point_attribute(pugi::xml_attribute); diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index 32f902dc6..9100eec10 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -162,6 +162,9 @@ using AdapterBase = internal::RegistryElementAdapter< abstract::TableColumnAdapter, abstract::TableRowAdapter, abstract::TableCellAdapter, abstract::FrameAdapter, abstract::ImageAdapter>; +using TreeEditor = xml::TreeEditor; +using xml::NodeSpan; + class ElementAdapter final : public AdapterBase { public: ElementAdapter(const Document &document, ElementRegistry ®istry) @@ -221,65 +224,42 @@ class ElementAdapter final : public AdapterBase { ElementRegistry::Text &text_element = m_registry->text_element_at(element_id); - const pugi::xml_node first = get_node(element_id); - const pugi::xml_node last = text_element.last; + const NodeSpan old_span{element.node, text_element.last}; + pugi::xml_node parent = old_span.first.parent(); + const NodeSpan new_span = write_text_nodes(parent, old_span.first, text); - pugi::xml_node parent = first.parent(); - const pugi::xml_node old_first = first; - const pugi::xml_node old_last = last; - pugi::xml_node new_first = old_first; - pugi::xml_node new_last = last; - - const auto insert_node = [&](const char *node) { - const pugi::xml_node new_node = - parent.insert_child_before(node, old_first); - if (new_first == old_first) { - new_first = new_node; - } - new_last = new_node; - return new_node; - }; + element.node = new_span.first; + text_element.last = new_span.last; - for (const xml::StringToken &token : xml::tokenize_text(text)) { - switch (token.type) { - case xml::StringToken::Type::none: - break; - case xml::StringToken::Type::string: { - auto text_node = insert_node("w:t"); - text_node.append_child(pugi::xml_node_type::node_pcdata) - .text() - .set(token.string.c_str()); - } break; - case xml::StringToken::Type::spaces: { - auto text_node = insert_node("w:t"); - text_node.append_attribute("xml:space").set_value("preserve"); - text_node.append_child(pugi::xml_node_type::node_pcdata) - .text() - .set(token.string.c_str()); - } break; - case xml::StringToken::Type::tabs: { - for (std::size_t i = 0; i < token.string.size(); ++i) { - insert_node("w:tab"); - } - } break; - } - } + xml::remove_nodes(old_span); + } - if (new_first == old_first) { - // empty text still needs a live node to anchor the element to, or the - // removal below would leave the registry pointing at freed nodes - insert_node("w:t"); + [[nodiscard]] ElementIdentifier + text_insert(const ElementIdentifier element_id, const Placement where, + const std::string &text) const override { + const ElementRegistry::Text &anchor = + m_registry->text_element_at(element_id); + const pugi::xml_node first = get_node(element_id); + pugi::xml_node parent = first.parent(); + // a run beside this one in the same `w:r` carries the same `w:rPr` + const pugi::xml_node before = + where == Placement::after ? anchor.last.next_sibling() : first; + + const NodeSpan span = write_text_nodes(parent, before, text); + const auto &[new_id, unused_element, unused_text] = + m_registry->create_text_element(span.first, span.last); + if (where == Placement::after) { + m_registry->insert_sibling_after(element_id, new_id); + } else { + m_registry->insert_sibling_before(element_id, new_id); } + return new_id; + } - element.node = new_first; - text_element.last = new_last; - - for (pugi::xml_node node = old_first; node != old_last.next_sibling();) { - const pugi::xml_node next = node.next_sibling(); - parent.remove_child(node); - node = next; - } + void element_remove(const ElementIdentifier element_id) const override { + TreeEditor(*m_registry).remove(element_id); } + [[nodiscard]] TextStyle text_style(const ElementIdentifier element_id) const override { return get_intermediate_style(element_id).text_style; diff --git a/src/odr/internal/xml/xml_tree_edit.hpp b/src/odr/internal/xml/xml_tree_edit.hpp new file mode 100644 index 000000000..db6f4ba40 --- /dev/null +++ b/src/odr/internal/xml/xml_tree_edit.hpp @@ -0,0 +1,56 @@ +#pragma once + +#include +#include + +#include + +namespace odr::internal::xml { + +/// The nodes one element owns: a run spans several, everything else is one. +struct NodeSpan final { + pugi::xml_node first; + pugi::xml_node last; +}; + +/// Removes @p span and everything between its ends from the tree. +inline void remove_nodes(const NodeSpan span) { + pugi::xml_node parent = span.first.parent(); + // the loop invalidates `last`, so where it ends is read first + const pugi::xml_node end = span.last.next_sibling(); + for (pugi::xml_node node = span.first; node != end;) { + const pugi::xml_node next = node.next_sibling(); + parent.remove_child(node); + node = next; + } +} + +/// Splices the element tree of a registry whose elements carry a +/// `pugi::xml_node`, keeping the dom and the registry links in step. Every +/// engine holding its dom does this the same way, so nothing here names a tag. +template class TreeEditor final { +public: + explicit TreeEditor(Registry ®istry) : m_registry{®istry} {} + + /// The nodes @p element_id owns: a run ends where its `Text` payload says, + /// everything else is its node and the subtree under it. + [[nodiscard]] NodeSpan node_span(const ElementIdentifier element_id) const { + const auto &element = m_registry->element_at(element_id); + if (element.type == ElementType::text) { + return {element.node, m_registry->text_element_at(element_id).last}; + } + return {element.node, element.node}; + } + + /// Removes @p element_id and its subtree; it keeps its id and stops being + /// reachable. + void remove(const ElementIdentifier element_id) const { + remove_nodes(node_span(element_id)); + m_registry->unlink_child(element_id); + } + +private: + Registry *m_registry{nullptr}; +}; + +} // namespace odr::internal::xml diff --git a/test/src/document_edit_test.cpp b/test/src/document_edit_test.cpp index 59b0644d5..0ee9b3fb1 100644 --- a/test/src/document_edit_test.cpp +++ b/test/src/document_edit_test.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -166,3 +167,224 @@ TEST(DocumentEdit, the_ops_before_a_refusal_are_applied) { EXPECT_EQ(first_sheet(document).cell(0, 0).value().text(), "written"); } + +namespace { + +/// Two paragraphs, the first of three runs with the middle one under a span - +/// so a test can see which parent a new run lands in. +Document two_paragraph_text() { + const std::string source = + R"()" + R"()" + R"()" + R"(one two three)" + R"(second)" + R"()"; + return DecodedFile( + open_strategy::open_file(std::make_shared(source), {}, + Logger::null())) + .as_document_file() + .document(); +} + +/// Every run under @p element, in document order, joined. +std::string text_of(const Element element) { + std::string result; + if (element.type() == ElementType::text) { + result += element.as_text().content(); + } + for (const Element child : element.children()) { + result += text_of(child); + } + return result; +} + +Element paragraph_at(const Document &document, const std::uint32_t ordinal) { + std::uint32_t seen = 0; + for (const Element child : document.root_element().children()) { + if (child.type() == ElementType::paragraph && seen++ == ordinal) { + return child; + } + } + return {}; +} + +/// The @p ordinal -th run of the @p paragraph -th paragraph, counting a run +/// under a span as the paragraph's own. +Element run_at(const Document &document, const std::uint32_t paragraph, + const std::uint32_t ordinal) { + std::uint32_t seen = 0; + const auto walk = [&](this auto &&self, const Element element) -> Element { + if (element.type() == ElementType::text && seen++ == ordinal) { + return element; + } + for (const Element child : element.children()) { + if (const Element found = self(child)) { + return found; + } + } + return {}; + }; + return walk(paragraph_at(document, paragraph)); +} + +std::string ops(const std::string &body) { + return R"({"version":2,"ops":[)" + body + "]}"; +} + +std::string id_of(const Element element) { + return std::to_string(element.identifier()); +} + +} // namespace + +TEST(DocumentEdit, a_run_is_inserted_after_the_one_it_names) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"insertText","after":)" + + id_of(run_at(document, 0, 0)) + + R"(,"text":"and ","id":-1})")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one and two three"); +} + +TEST(DocumentEdit, a_run_is_inserted_before_the_one_it_names) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"insertText","before":)" + + id_of(run_at(document, 0, 2)) + R"(,"text":"!","id":-1})")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one two! three"); +} + +TEST(DocumentEdit, a_run_inserted_beside_a_styled_one_shares_its_parent) { + const Document document = two_paragraph_text(); + const Element styled = run_at(document, 0, 1); + + document.edit(ops(R"({"op":"insertText","after":)" + id_of(styled) + + R"(,"text":"!","id":-1})")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one two! three"); + EXPECT_EQ(run_at(document, 0, 2).parent(), styled.parent()); +} + +TEST(DocumentEdit, a_removed_run_takes_its_text_with_it) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"removeElement","id":)" + + id_of(run_at(document, 0, 1)) + "}")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one three"); +} + +TEST(DocumentEdit, a_removed_span_takes_its_subtree_with_it) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"removeElement","id":)" + + id_of(run_at(document, 0, 1).parent()) + "}")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one three"); +} + +/// What typing over a selection spanning three runs looks like on the wire. +TEST(DocumentEdit, an_edit_across_three_runs_is_three_ops) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"setText","id":)" + id_of(run_at(document, 0, 0)) + + R"(,"text":"oX"},)" + R"({"op":"removeElement","id":)" + + id_of(run_at(document, 0, 1)) + "}," + + R"({"op":"setText","id":)" + id_of(run_at(document, 0, 2)) + + R"(,"text":"ree"})")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "oXree"); +} + +TEST(DocumentEdit, a_later_op_names_a_run_an_earlier_one_created) { + const Document document = two_paragraph_text(); + + document.edit(ops(R"({"op":"insertText","after":)" + + id_of(run_at(document, 0, 2)) + + R"(,"text":"four","id":-1},)" + + R"({"op":"setText","id":-1,"text":"FOUR"})")); + + EXPECT_EQ(text_of(paragraph_at(document, 0)), "one two threeFOUR"); +} + +TEST(DocumentEdit, an_op_naming_a_created_element_that_is_not_there_refuses) { + const Document document = two_paragraph_text(); + + EXPECT_THROW(document.edit(ops(R"({"op":"setText","id":-1,"text":"x"})")), + std::invalid_argument); +} + +TEST(DocumentEdit, creating_two_elements_under_one_id_refuses) { + const Document document = two_paragraph_text(); + const std::string anchor = id_of(run_at(document, 0, 0)); + + EXPECT_THROW(document.edit(ops(R"({"op":"insertText","after":)" + anchor + + R"(,"text":"a","id":-1},)" + + R"({"op":"insertText","after":)" + anchor + + R"(,"text":"b","id":-1})")), + std::invalid_argument); +} + +TEST(DocumentEdit, a_created_element_with_a_positive_id_refuses) { + const Document document = two_paragraph_text(); + + EXPECT_THROW(document.edit(ops(R"({"op":"insertText","after":)" + + id_of(run_at(document, 0, 0)) + + R"(,"text":"a","id":7})")), + std::invalid_argument); +} + +TEST(DocumentEdit, an_insert_naming_neither_side_or_both_refuses) { + const Document document = two_paragraph_text(); + const std::string anchor = id_of(run_at(document, 0, 0)); + + EXPECT_THROW(document.edit(ops(R"({"op":"insertText","text":"a","id":-1})")), + std::invalid_argument); + EXPECT_THROW( + document.edit(ops(R"({"op":"insertText","after":)" + anchor + + R"(,"before":)" + anchor + R"(,"text":"a","id":-1})")), + std::invalid_argument); +} + +/// Rtf throws its source away as it parses, so its model is read-only. +TEST(DocumentEdit, a_read_only_engine_refuses_a_structural_op) { + const Document document = + DecodedFile(open_strategy::open_file(std::make_shared( + std::string(R"({\rtf1 hello})")), + {}, Logger::null())) + .as_document_file() + .document(); + ASSERT_FALSE(document.is_editable()); + + const Element run = run_at(document, 0, 0); + ASSERT_TRUE(run); + EXPECT_THROW( + document.edit(ops(R"({"op":"removeElement","id":)" + id_of(run) + "}")), + UnsupportedOperation); + EXPECT_THROW(document.edit(ops(R"({"op":"insertText","after":)" + id_of(run) + + R"(,"text":"x","id":-1})")), + UnsupportedOperation); +} + +TEST(DocumentEdit, a_structural_edit_refuses_another_documents_element) { + const Document document = two_paragraph_text(); + const Document other = two_paragraph_text(); + const Element run = run_at(other, 0, 0); + ASSERT_TRUE(run); + + EXPECT_THROW(document.remove(run), std::invalid_argument); + EXPECT_THROW((void)document.insert_text_after(run.as_text(), "x"), + std::invalid_argument); +} + +TEST(DocumentEdit, inserting_a_run_beside_something_that_is_not_one_refuses) { + const Document document = two_paragraph_text(); + const Element paragraph = paragraph_at(document, 0); + + EXPECT_THROW((void)document.insert_text_after(paragraph.as_text(), "x"), + std::invalid_argument); +} diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index b3c14a085..ce9f27b34 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -86,17 +87,19 @@ std::string set_text_ops(const Document &document, const TextEdits &edits) { return nlohmann::json{{"version", 2}, {"ops", ops}}.dump(); } -/// Applies @p edits to `path`'s document, saves to `output_name` in the working -/// directory and reopens it, so the assertions see what was written. -Document edit_and_reload(const std::string &path, const TextEdits &edits, - const std::string &output_name) { +/// Applies what @p make_ops states against the opened document, saves to +/// `output_name` and reopens it, so the assertions see what was written. +Document +edit_and_reload(const std::string &path, + const std::function &make_ops, + const std::string &output_name) { const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file = open(TestData::test_file_path(path), {}, logger).as_document_file(); const Document document = document_file.document(); - document.edit(set_text_ops(document, edits)); + document.edit(make_ops(document)); const std::string output_path = (std::filesystem::current_path() / output_name).string(); @@ -105,6 +108,14 @@ Document edit_and_reload(const std::string &path, const TextEdits &edits, return open(output_path).as_document_file().document(); } +Document edit_and_reload(const std::string &path, const TextEdits &edits, + const std::string &output_name) { + return edit_and_reload( + path, + [&](const Document &document) { return set_text_ops(document, edits); }, + output_name); +} + /// `pages.ods` is password-protected; every test that wants its content opens /// it this way. Document decrypted_pages_ods() { @@ -420,6 +431,99 @@ TEST(Document, a_decrypted_package_is_not_savable) { EXPECT_THROW((void)document.save_to_memory(), UnsupportedOperation); } +namespace { + +/// Every run under @p element, in document order, joined. +std::string text_of(const Element element) { + std::string result; + if (element.type() == ElementType::text) { + result += element.as_text().content(); + } + for (const Element child : element.children()) { + result += text_of(child); + } + return result; +} + +/// Every run under @p element, in document order. +std::vector runs_of(const Element element) { + std::vector runs; + const auto walk = [&](this auto &&self, const Element at) -> void { + if (at.type() == ElementType::text) { + runs.push_back(at); + } + for (const Element child : at.children()) { + self(child); + } + }; + walk(element); + return runs; +} + +/// The first paragraph of at least three runs - two ends and something +/// between them. Found rather than spelled: a data pin moves the index. +Element paragraph_of_several_runs(const Document &document) { + for (const Element child : document.root_element().children()) { + if (child.type() == ElementType::paragraph && runs_of(child).size() >= 3) { + return child; + } + } + return {}; +} + +/// A paragraph rewritten end to end, as an edit across its runs would. +std::string rewrite_paragraph_ops(const std::vector &runs) { + nlohmann::json ops = nlohmann::json::array(); + ops.push_back({{"op", "setText"}, + {"id", runs.front().identifier()}, + {"text", "head "}}); + for (std::size_t at = 1; at + 1 < runs.size(); ++at) { + ops.push_back({{"op", "removeElement"}, {"id", runs[at].identifier()}}); + } + ops.push_back( + {{"op", "setText"}, {"id", runs.back().identifier()}, {"text", "tail"}}); + ops.push_back({{"op", "insertText"}, + {"after", runs.back().identifier()}, + {"text", " and more"}, + {"id", -1}}); + return nlohmann::json{{"version", 2}, {"ops", ops}}.dump(); +} + +/// Rewrites a paragraph of @p path's document across its runs, saves and +/// reopens, and answers what that paragraph then reads. +std::string edit_across_runs(const std::string &path, + const std::string &output_name) { + std::string paragraph_path; + const Document document = edit_and_reload( + path, + [&](const Document &opened) { + const Element paragraph = paragraph_of_several_runs(opened); + EXPECT_TRUE(paragraph) << path << " holds no paragraph of three runs"; + paragraph_path = paragraph.document_path().to_string(); + return rewrite_paragraph_ops(runs_of(paragraph)); + }, + output_name); + + // no paragraph came or went, so the path still names the one that was edited + return text_of( + document.root_element().navigate_path(DocumentPath(paragraph_path))); +} + +} // namespace + +// Reopening is what proves the package the engine wrote is sound. +TEST(Document, edit_odt_across_runs) { + EXPECT_EQ(edit_across_runs("odr-public/odt/style-various-1.odt", + "style-various-1_edit_runs.odt"), + "head tail and more"); +} + +TEST(Document, edit_docx_across_runs) { + EXPECT_EQ(edit_across_runs("odr-public/docx/style-various-1.docx", + "style-various-1_edit_runs.docx"), + "head tail and more"); +} + TEST(Document, edit_docx_diff) { const Document document = edit_and_reload( "odr-public/docx/style-various-1.docx", diff --git a/test/src/internal/ooxml/ooxml_util_test.cpp b/test/src/internal/ooxml/ooxml_util_test.cpp index a0beac4e3..6722b9ccc 100644 --- a/test/src/internal/ooxml/ooxml_util_test.cpp +++ b/test/src/internal/ooxml/ooxml_util_test.cpp @@ -8,7 +8,11 @@ #include #include +#include #include +#include + +#include using namespace odr::internal; using namespace odr::internal::ooxml; @@ -106,3 +110,45 @@ TEST(ooxml_util, a_relationship_target_that_names_nothing_resolves_to_nothing) { layout_of(relationships_of(relationship(layout_type, "../../../x.xml"))) .has_value()); } + +namespace { + +/// The `w:r` @p text is written into, serialised. +std::string written(const std::string &text) { + pugi::xml_document document; + pugi::xml_node run = document.append_child("w:r"); + write_text_nodes(run, {}, text); + + std::ostringstream out; + document.print(out, "", pugi::format_raw); + return std::move(out).str(); +} + +} // namespace + +// [ECMA-376] Part 1 17.3.3.31: a reader collapses the space at either end of a +// `w:t` unless the node says to keep it. +TEST(ooxml_util, a_run_ending_in_a_space_keeps_it) { + EXPECT_EQ(written("head "), + R"(head )"); +} + +TEST(ooxml_util, a_run_starting_with_a_space_keeps_it) { + EXPECT_EQ(written(" and more"), + R"( and more)"); +} + +TEST(ooxml_util, a_run_with_no_space_at_its_ends_says_nothing) { + EXPECT_EQ(written("head and more"), R"(head and more)"); +} + +TEST(ooxml_util, a_run_of_spaces_keeps_them) { + EXPECT_EQ(written("a b"), + R"(a )" + R"(b)"); +} + +TEST(ooxml_util, a_tab_is_a_node_and_empty_text_is_still_one) { + EXPECT_EQ(written("a\tb"), R"(ab)"); + EXPECT_EQ(written(""), R"()"); +}