diff --git a/CHANGELOG.md b/CHANGELOG.md index 21832dedf..ce1a0260a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- **Breaking**: `DocumentPath`, `Element::document_path()` and + `Element::navigate_path()` are gone, with their python and Java mirrors. + An element is addressed by `Element::identifier()` and + `Document::element_by_id()`, which is what the edit envelope uses. + - **Breaking**: `ValueType` gains `boolean`, `date`, `time` and `error`, and the odf and xlsx readers report them; a boolean states 1 or 0 as its number. `Sheet::set_cell` writes a boolean and refuses the other three. The enum diff --git a/CMakeLists.txt b/CMakeLists.txt index 6e8b48815..28b526d9e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -140,7 +140,6 @@ set(ODR_SOURCE_FILES "src/odr/archive.cpp" "src/odr/document.cpp" "src/odr/document_element.cpp" - "src/odr/document_path.cpp" "src/odr/error_code.cpp" "src/odr/exceptions.cpp" "src/odr/file.cpp" @@ -343,7 +342,6 @@ set(ODR_SOURCE_FILES "src/odr/internal/util/byte_util.cpp" "src/odr/internal/util/byte_stream_util.cpp" "src/odr/internal/util/byte_string.cpp" - "src/odr/internal/util/document_util.cpp" "src/odr/internal/util/file_util.cpp" "src/odr/internal/util/hash_util.cpp" "src/odr/internal/util/number_util.cpp" diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index f96f8b4e9..34f0a10ba 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -53,7 +53,6 @@ results go stale the moment an input changes. | Browser: text editor | `html/frontend/document.js` | The skeleton, attached to the mode: the whole view editable, runs keyed by `data-odr-id`, one `setText` op per changed run. No undo | | Browser: sheet editor | `html/frontend/sheet-editing.js` | The cell overlay, the locks and the position map (steps 1.1 to 1.4, landed), attached to the mode as one editor | | Wire format | `document.cpp::Document::edit` | The op envelope, `setCell` and `setText` (step 0.4, landed) | -| Addressing | `DocumentPath` | Already spells a cell by position: `/child:0/cell:A1/...` | | Capabilities | `file_type_table.cpp` | `ods` and `xlsx` declare `edit` and `save` (step 0.2, landed); `csv` declares neither. `odr_test` checks the declaration against `Document::is_editable` | ## Decisions diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index 5006aecc3..434e97edf 100644 --- a/jni/CMakeLists.txt +++ b/jni/CMakeLists.txt @@ -78,7 +78,6 @@ add_jar(odr_java "java/app/opendocument/core/DirectionalString.java" "java/app/opendocument/core/Document.java" "java/app/opendocument/core/DocumentFile.java" - "java/app/opendocument/core/DocumentPath.java" "java/app/opendocument/core/DocumentType.java" "java/app/opendocument/core/DrawingLine.java" "java/app/opendocument/core/DrawingPath.java" diff --git a/jni/java/app/opendocument/core/DocumentPath.java b/jni/java/app/opendocument/core/DocumentPath.java deleted file mode 100644 index 9d873dbb5..000000000 --- a/jni/java/app/opendocument/core/DocumentPath.java +++ /dev/null @@ -1,59 +0,0 @@ -package app.opendocument.core; - -/** A path to a specific element in a document. Mirrors {@code odr::DocumentPath}. */ -public final class DocumentPath extends NativeResource { - static { - NativeLibrary.load(); - } - - DocumentPath(long handle) { - super(handle, null, DocumentPath::destroy); - } - - public DocumentPath(String path) { - this(create(path)); - } - - public boolean empty() { - return emptyNative(handle()); - } - - public DocumentPath parent() { - return new DocumentPath(parentNative(handle())); - } - - public DocumentPath join(DocumentPath other) { - try { - return new DocumentPath(joinNative(handle(), other.handle())); - } finally { - other.keepAlive(); - } - } - - @Override - public boolean equals(Object other) { - return other instanceof DocumentPath path && toString().equals(path.toString()); - } - - @Override - public int hashCode() { - return toString().hashCode(); - } - - @Override - public String toString() { - return toStringNative(handle()); - } - - private static native long create(String path); - - private static native void destroy(long handle); - - private native boolean emptyNative(long handle); - - private native long parentNative(long handle); - - private native long joinNative(long handle, long otherHandle); - - private native String toStringNative(long handle); -} diff --git a/jni/java/app/opendocument/core/Element.java b/jni/java/app/opendocument/core/Element.java index ec540de32..590624a53 100644 --- a/jni/java/app/opendocument/core/Element.java +++ b/jni/java/app/opendocument/core/Element.java @@ -61,18 +61,6 @@ public long identifier() { return identifierNative(handle()); } - public DocumentPath documentPath() { - return new DocumentPath(documentPathNative(handle())); - } - - public Element navigatePath(DocumentPath path) { - try { - return wrap(navigatePathNative(handle(), path.handle())); - } finally { - path.keepAlive(); - } - } - public List children() { List result = new ArrayList<>(); for (Element child = firstChild(); child != null; child = child.nextSibling()) { @@ -215,10 +203,6 @@ final List wrapAll(long[] handles) { private native long identifierNative(long handle); - private native long documentPathNative(long handle); - - private native long navigatePathNative(long handle, long pathHandle); - private native long asTextRootNative(long handle); private native long asSlideNative(long handle); diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 30bf42845..6dc0b4032 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -3,7 +3,6 @@ #include #include -#include #include #include @@ -245,57 +244,6 @@ Java_app_opendocument_core_Document_asFilesystemNative(JNIEnv *env, jobject, }); } -// app.opendocument.core.DocumentPath - -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_DocumentPath_create(JNIEnv *env, jclass, - jstring path) { - return guarded(env, [&] { - return make_handle(odr::DocumentPath(to_string(env, path))); - }); -} - -extern "C" JNIEXPORT void JNICALL -Java_app_opendocument_core_DocumentPath_destroy(JNIEnv *env, jclass, - jlong handle) { - destroy_handle(env, handle); -} - -extern "C" JNIEXPORT jboolean JNICALL -Java_app_opendocument_core_DocumentPath_emptyNative(JNIEnv *env, jobject, - jlong handle) { - return guarded(env, [&] { - return static_cast( - from_handle(handle)->empty()); - }); -} - -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_DocumentPath_parentNative(JNIEnv *env, jobject, - jlong handle) { - return guarded(env, [&] { - return make_handle(from_handle(handle)->parent()); - }); -} - -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_DocumentPath_joinNative(JNIEnv *env, jobject, - jlong handle, - jlong other_handle) { - return guarded(env, [&] { - return make_handle(from_handle(handle)->join( - *from_handle(other_handle))); - }); -} - -extern "C" JNIEXPORT jstring JNICALL -Java_app_opendocument_core_DocumentPath_toStringNative(JNIEnv *env, jobject, - jlong handle) { - return guarded(env, [&] { - return to_jstring(env, from_handle(handle)->to_string()); - }); -} - // app.opendocument.core.Element extern "C" JNIEXPORT void JNICALL @@ -375,23 +323,6 @@ Java_app_opendocument_core_Element_isSameNative(JNIEnv *env, jobject, }); } -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_Element_documentPathNative(JNIEnv *env, jobject, - jlong handle) { - return guarded(env, - [&] { return make_handle(element(handle).document_path()); }); -} - -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_Element_navigatePathNative(JNIEnv *env, jobject, - jlong handle, - jlong path_handle) { - return guarded(env, [&] { - return wrap_element(element(handle).navigate_path( - *from_handle(path_handle))); - }); -} - // The typed accessors return a fresh `odr::Element` copy when the element // supports the typed view, 0 otherwise. #define ODR_JNI_ELEMENT_AS(java_name, method) \ diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index 620a876db..5d06beb4a 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -79,21 +79,6 @@ void documentFilesystem() throws IOException { assertTrue(filesystem.isFile("/content.xml")); } - @Test - void documentPath() throws IOException { - Document document = openDocument(); - Element first = document.rootElement().firstChild(); - DocumentPath path = first.documentPath(); - assertNotNull(path.toString()); - assertEquals(path, first.documentPath()); - - // join() and navigatePath() take another wrapper's handle as an argument - DocumentPath rejoined = path.parent().join(path); - assertTrue(path.parent().empty()); - assertEquals(path, rejoined); - assertTrue(document.rootElement().navigatePath(rejoined).isSame(first)); - } - @Test void editAppliesADiff() throws IOException { Document document = openDocument(); diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index d92d3a32b..c8a0327ac 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -123,23 +122,6 @@ void odr_python::bind_document(py::module_ &m) { .def_static("to_row_string", &odr::TablePosition::to_row_string, py::arg("row")); - py::class_(m, "DocumentPath") - .def(py::init<>()) - .def(py::init(), py::arg("string")) - .def("empty", &odr::DocumentPath::empty) - .def("parent", &odr::DocumentPath::parent) - .def("join", &odr::DocumentPath::join, py::arg("other")) - .def( - "__eq__", - [](const odr::DocumentPath &lhs, const odr::DocumentPath &rhs) { - return lhs == rhs; - }, - py::is_operator()) - .def("__str__", &odr::DocumentPath::to_string) - .def("__repr__", [](const odr::DocumentPath &path) { - return "DocumentPath('" + path.to_string() + "')"; - }); - py::class_(m, "Element") .def(py::init<>()) .def("__bool__", &odr::Element::operator bool) @@ -158,9 +140,6 @@ void odr_python::bind_document(py::module_ &m) { .def("is_unique", &odr::Element::is_unique) .def("is_self_locatable", &odr::Element::is_self_locatable) .def("is_editable", &odr::Element::is_editable) - .def("document_path", &odr::Element::document_path) - .def("navigate_path", &odr::Element::navigate_path, py::arg("path"), - keep_self_alive) .def("children", &make_children_iterator, keep_self_alive) .def("__iter__", &make_children_iterator, keep_self_alive) .def("as_text_root", &odr::Element::as_text_root, keep_self_alive) diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 72ac097e4..32c3c68bb 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -1,6 +1,5 @@ #include -#include #include #include #include @@ -143,18 +142,6 @@ bool Element::is_editable() const { return exists_() ? m_adapter->element_is_editable(m_identifier) : false; } -DocumentPath Element::document_path() const { - return exists_() ? m_adapter->element_document_path(m_identifier) - : DocumentPath(); -} - -Element Element::navigate_path(const DocumentPath &path) const { - return exists_() - ? Element(m_adapter, - m_adapter->element_navigate_path(m_identifier, path)) - : Element(); -} - TextRoot Element::as_text_root() const { if (!exists_()) { return {}; diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index bdbc78450..e38dbd356 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -13,7 +13,6 @@ namespace odr { struct TablePosition; struct TableDimensions; -class DocumentPath; class File; struct TextStyle; struct ParagraphStyle; @@ -224,8 +223,6 @@ class Element { [[nodiscard]] bool is_unique() const; [[nodiscard]] bool is_self_locatable() const; [[nodiscard]] bool is_editable() const; - [[nodiscard]] DocumentPath document_path() const; - [[nodiscard]] Element navigate_path(const DocumentPath &path) const; [[nodiscard]] ElementRange children() const; diff --git a/src/odr/document_path.cpp b/src/odr/document_path.cpp deleted file mode 100644 index 4b847bcb4..000000000 --- a/src/odr/document_path.cpp +++ /dev/null @@ -1,142 +0,0 @@ -#include - -#include - -namespace odr { - -const std::string &DocumentPath::Child::prefix_string() { - static std::string result(prefix); - return result; -} - -DocumentPath::Child::Child(const std::uint32_t number) : m_number{number} {} - -std::uint32_t DocumentPath::Child::number() const { return m_number; } - -bool DocumentPath::Child::operator==(const Child &other) const noexcept { - return m_number == other.m_number; -} - -[[nodiscard]] std::string DocumentPath::Child::to_string() const noexcept { - return prefix_string() + ":" + std::to_string(m_number); -} - -const std::string &DocumentPath::Cell::prefix_string() { - static std::string result(prefix); - return result; -} - -DocumentPath::Cell::Cell(const TablePosition &position) - : m_position{position} {} - -TablePosition DocumentPath::Cell::position() const { return m_position; } - -bool DocumentPath::Cell::operator==(const Cell &other) const noexcept { - return m_position == other.m_position; -} - -[[nodiscard]] std::string DocumentPath::Cell::to_string() const noexcept { - return prefix_string() + ":" + m_position.to_string(); -} - -DocumentPath::Component -DocumentPath::component_from_string(const std::string &string) { - const auto colon = string.find(':'); - if (colon == std::string::npos) { - throw std::invalid_argument("string"); - } - - const std::string prefix = string.substr(0, colon); - - if (prefix == Child::prefix_string()) { - const std::uint32_t number = std::stoul(string.substr(colon + 1)); - return Child(number); - } - if (prefix == Cell::prefix_string()) { - const TablePosition position(string.substr(colon + 1)); - return Cell(position); - } - - throw std::invalid_argument("string"); -} - -DocumentPath::DocumentPath() noexcept = default; - -DocumentPath::DocumentPath(const Container &components) - : m_components{components} {} - -DocumentPath::DocumentPath(Container &&components) - : m_components{std::move(components)} {} - -DocumentPath::DocumentPath(const std::string_view string) { - if (string.empty()) { - return; - } - - std::size_t pos = 0; - while (pos < string.size()) { - if (string[pos] != '/') { - throw std::invalid_argument("missing /"); - } - std::size_t next = string.find('/', pos + 1); - if (next == std::string_view::npos) { - next = string.size(); - } - m_components.push_back(component_from_string( - std::string(string.substr(pos + 1, next - pos - 1)))); - pos = next; - } -} - -bool DocumentPath::operator==(const DocumentPath &other) const noexcept { - return m_components == other.m_components; -} - -bool DocumentPath::operator!=(const DocumentPath &other) const noexcept { - return m_components != other.m_components; -} - -std::string DocumentPath::to_string() const { - std::string result; - - for (auto &&component : m_components) { - result.append("/"); - std::visit([&result](auto &&c) { result.append(c.to_string()); }, - component); - } - - return result; -} - -bool DocumentPath::empty() const noexcept { return m_components.empty(); } - -const DocumentPath::Component &DocumentPath::back() const { - return m_components.back(); -} - -DocumentPath DocumentPath::parent() const { - if (empty()) { - throw std::invalid_argument("there is no parent"); - } - DocumentPath result = *this; - result.m_components.pop_back(); - return result; -} - -DocumentPath DocumentPath::join(const DocumentPath &other) const { - DocumentPath result = *this; - result.m_components.insert(std::end(result.m_components), - std::begin(other.m_components), - std::end(other.m_components)); - return result; -} - -DocumentPath::const_iterator DocumentPath::begin() const { - return std::begin(m_components); -} - -DocumentPath::const_iterator DocumentPath::end() const { - return std::end(m_components); -} - -} // namespace odr diff --git a/src/odr/document_path.hpp b/src/odr/document_path.hpp deleted file mode 100644 index 14e3d90a6..000000000 --- a/src/odr/document_path.hpp +++ /dev/null @@ -1,79 +0,0 @@ -#pragma once - -#include - -#include -#include -#include -#include -#include - -namespace odr { - -/// A path to a specific element in a document. -class DocumentPath final { -public: - class Child final { - public: - static constexpr std::string_view prefix = "child"; - - static const std::string &prefix_string(); - - explicit Child(std::uint32_t number); - - [[nodiscard]] std::uint32_t number() const; - - bool operator==(const Child &other) const noexcept; - [[nodiscard]] std::string to_string() const noexcept; - - private: - std::uint32_t m_number{0}; - }; - - class Cell final { - public: - static constexpr std::string_view prefix = "cell"; - - static const std::string &prefix_string(); - - explicit Cell(const TablePosition &position); - - [[nodiscard]] TablePosition position() const; - - bool operator==(const Cell &other) const noexcept; - [[nodiscard]] std::string to_string() const noexcept; - - private: - TablePosition m_position; - }; - - using Component = std::variant; - using Container = std::vector; - using const_iterator = Container::const_iterator; - - static Component component_from_string(const std::string &string); - - DocumentPath() noexcept; - explicit DocumentPath(const Container &components); - explicit DocumentPath(Container &&components); - explicit DocumentPath(std::string_view string); - - bool operator==(const DocumentPath &other) const noexcept; - bool operator!=(const DocumentPath &other) const noexcept; - - [[nodiscard]] std::string to_string() const; - - [[nodiscard]] bool empty() const noexcept; - - [[nodiscard]] const Component &back() const; - [[nodiscard]] DocumentPath parent() const; - [[nodiscard]] DocumentPath join(const DocumentPath &other) const; - - [[nodiscard]] const_iterator begin() const; - [[nodiscard]] const_iterator end() const; - -private: - Container m_components; -}; - -} // namespace odr diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index 31757dfad..a6a750ab0 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -18,7 +18,6 @@ namespace odr { class File; enum class FileType; enum class DocumentType; -class DocumentPath; struct PageLayout; struct TableDimensions; struct TablePosition; @@ -111,11 +110,6 @@ class ElementAdapter { element_is_self_locatable(ElementIdentifier element_id) const = 0; [[nodiscard]] virtual bool element_is_editable(ElementIdentifier element_id) const = 0; - [[nodiscard]] virtual DocumentPath - element_document_path(ElementIdentifier element_id) const = 0; - [[nodiscard]] virtual ElementIdentifier - 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. diff --git a/src/odr/internal/common/element_adapter.hpp b/src/odr/internal/common/element_adapter.hpp index 26fd2de04..799da6960 100644 --- a/src/odr/internal/common/element_adapter.hpp +++ b/src/odr/internal/common/element_adapter.hpp @@ -2,10 +2,8 @@ #include #include -#include #include -#include #include @@ -29,15 +27,6 @@ class ElementAdapter : public abstract::ElementAdapter, public Adapters... { [[maybe_unused]] const ElementIdentifier element_id) const override { return false; } - [[nodiscard]] DocumentPath - element_document_path(const ElementIdentifier element_id) const override { - return util::document::extract_path(*this, element_id, null_element_id); - } - [[nodiscard]] ElementIdentifier - element_navigate_path(const ElementIdentifier element_id, - const DocumentPath &path) const override { - return util::document::navigate_path(*this, element_id, path); - } [[nodiscard]] const abstract::TextRootAdapter * text_root_adapter(const ElementIdentifier element_id) const override { diff --git a/src/odr/internal/csv/csv_document.cpp b/src/odr/internal/csv/csv_document.cpp index f5679c15f..41905b679 100644 --- a/src/odr/internal/csv/csv_document.cpp +++ b/src/odr/internal/csv/csv_document.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index 0519de1ab..dc6f9328c 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -1,7 +1,6 @@ #include #include -#include #include #include #include diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index ebef6bf11..ae9c7b35d 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -74,10 +74,10 @@ decodes it against the sheet's cell index — the shared `RegistryElementAdapter` navigates through `element_at`, so nothing else needs to know. -Three consequences: `SheetCell::position()` and `DocumentPath` name the cell +Three consequences: `SheetCell::position()` and the cell's id name the cell asked for; a handle follows the index rather than the element it found, so a run can be split under one; and children are **shared**, so the position stops -at the cell and a path to a run inside one names the anchor. +at the cell and a run inside one is the anchor's. The three containers are **sorted vectors, not maps**: parsing appends in document order, so the keys only grow, and a rb-tree node costs more than the 12 diff --git a/src/odr/internal/util/document_util.cpp b/src/odr/internal/util/document_util.cpp deleted file mode 100644 index 58ed0fb06..000000000 --- a/src/odr/internal/util/document_util.cpp +++ /dev/null @@ -1,121 +0,0 @@ -#include - -#include -#include - -#include - -#include -#include -#include - -namespace odr::internal::util { - -namespace { - -DocumentPath::Component -extract_path_component(const abstract::ElementAdapter &element_adapter, - const ElementIdentifier element_id) { - if (element_adapter.element_type(element_id) == ElementType::sheet_cell) { - const abstract::SheetCellAdapter *sheet_cell_adapter = - element_adapter.sheet_cell_adapter(element_id); - if (sheet_cell_adapter == nullptr) { - throw std::invalid_argument("Sheet cell adapter not found."); - } - return DocumentPath::Cell( - sheet_cell_adapter->sheet_cell_position(element_id)); - } - - std::uint32_t distance = 0; - for (ElementIdentifier current_id = - element_adapter.element_previous_sibling(element_id); - current_id != null_element_id; - current_id = element_adapter.element_previous_sibling(current_id)) { - ++distance; - } - - return DocumentPath::Child(distance); -} - -ElementIdentifier -navigate_path_component(const abstract::ElementAdapter &element_adapter, - const ElementIdentifier element_id, - const DocumentPath::Component &component) { - if (const auto *child = std::get_if(&component); - child != nullptr) { - ElementIdentifier result_id = - element_adapter.element_first_child(element_id); - if (result_id == null_element_id) { - throw std::invalid_argument("child not found"); - } - for (std::uint32_t i = 0; i < child->number(); ++i) { - result_id = element_adapter.element_next_sibling(result_id); - if (result_id == null_element_id) { - throw std::invalid_argument("child not found"); - } - } - return result_id; - } - - if (const auto *cell = std::get_if(&component); - cell != nullptr) { - const abstract::SheetAdapter *sheet_adapter = - element_adapter.sheet_adapter(element_id); - if (sheet_adapter == nullptr) { - throw std::invalid_argument("sheet adapter not found"); - } - return sheet_adapter->sheet_cell(element_id, cell->position().column, - cell->position().row); - } - - throw std::invalid_argument("unknown document path component"); -} - -} // namespace - -DocumentPath -document::extract_path(const abstract::ElementAdapter &element_adapter, - const ElementIdentifier to_element_id, - const ElementIdentifier from_element_id) { - if (to_element_id == null_element_id) { - throw std::invalid_argument("Element identifier cannot be null."); - } - - std::vector reverse; - - for (ElementIdentifier current_id = to_element_id; - current_id != from_element_id;) { - const ElementIdentifier parent_id = - element_adapter.element_parent(current_id); - if (parent_id == null_element_id) { - // the walk reached the root; a named origin we never met is not an - // ancestor, and the path collected so far would be rooted elsewhere - if (from_element_id != null_element_id) { - throw std::invalid_argument( - "Element is not a descendant of the specified root."); - } - break; - } - - reverse.push_back(extract_path_component(element_adapter, current_id)); - - current_id = parent_id; - } - - std::ranges::reverse(reverse); - return DocumentPath(std::move(reverse)); -} - -ElementIdentifier -document::navigate_path(const abstract::ElementAdapter &element_adapter, - const ElementIdentifier from_element_id, - const DocumentPath &path) { - ElementIdentifier current_id = from_element_id; - for (const DocumentPath::Component &component : path) { - current_id = - navigate_path_component(element_adapter, current_id, component); - } - return current_id; -} - -} // namespace odr::internal::util diff --git a/src/odr/internal/util/document_util.hpp b/src/odr/internal/util/document_util.hpp deleted file mode 100644 index c49196504..000000000 --- a/src/odr/internal/util/document_util.hpp +++ /dev/null @@ -1,25 +0,0 @@ -#pragma once - -#include - -namespace odr { -class DocumentPath; -} - -namespace odr::internal::abstract { -class ElementAdapter; -} - -namespace odr::internal::util::document { - -/// The path from @p from_element_id (`null_element_id` for the root) down to -/// @p to_element_id; throws if the latter is not a descendant of the former. -DocumentPath extract_path(const abstract::ElementAdapter &element_adapter, - ElementIdentifier to_element_id, - ElementIdentifier from_element_id); - -ElementIdentifier navigate_path(const abstract::ElementAdapter &element_adapter, - ElementIdentifier from_element_id, - const DocumentPath &path); - -} // namespace odr::internal::util::document diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b1a4e39b0..7c8f43b73 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -29,7 +29,6 @@ add_executable(odr_test "src/cell_value_test.cpp" "src/document_edit_test.cpp" "src/document_list_test.cpp" - "src/document_path_test.cpp" "src/enum_ordinals_test.cpp" "src/error_code_test.cpp" "src/document_test.cpp" diff --git a/test/public_headers_cpp20.cpp b/test/public_headers_cpp20.cpp index 21b1b3b01..954c0d9a9 100644 --- a/test/public_headers_cpp20.cpp +++ b/test/public_headers_cpp20.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include diff --git a/test/src/document_path_test.cpp b/test/src/document_path_test.cpp deleted file mode 100644 index 372a01cbf..000000000 --- a/test/src/document_path_test.cpp +++ /dev/null @@ -1,13 +0,0 @@ -#include - -#include - -using namespace odr; - -TEST(DocumentPath, empty) { EXPECT_EQ("", DocumentPath().to_string()); } - -TEST(DocumentPath, example1) { - EXPECT_EQ("/child:3/child:2/child:17/child:0", - DocumentPath("/child:3/child:2/child:17/child:0").to_string()); - EXPECT_EQ("/child:3/cell:A17", DocumentPath("/child:3/cell:A17").to_string()); -} diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index ad159f431..56c615fe7 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -1,11 +1,11 @@ #include #include -#include #include #include #include #include #include +#include #include @@ -13,6 +13,8 @@ #include +#include +#include #include #include #include @@ -63,12 +65,58 @@ void expect_every_text(const Element element, const std::string &content) { } } +/// The element `/child:N/cell:A1/...` names under @p root: a child by its +/// index, a sheet cell by its reference. What the tests address by, since an +/// id is meaningless outside the decode that assigned it. +Element navigate(const Element &root, const std::string &path) { + Element at = root; + std::size_t pos = 0; + while (at && pos < path.size()) { + if (path[pos] == '/') { + ++pos; + continue; + } + const std::size_t end = path.find('/', pos); + const std::string segment = path.substr(pos, end - pos); + pos = end == std::string::npos ? path.size() : end; + const std::size_t colon = segment.find(':'); + const std::string kind = segment.substr(0, colon); + const std::string argument = segment.substr(colon + 1); + if (kind == "child") { + at = at.first_child(); + for (auto n = std::stoul(argument); n > 0 && at; --n) { + at = at.next_sibling(); + } + } else if (kind == "cell") { + const TablePosition position(argument); + at = at.as_sheet().cell(position.column, position.row); + } else { + return {}; + } + } + return at; +} + +/// The `/child:N/...` path from the root down to @p element, which +/// `navigate` reads back. +std::string path_of(Element element) { + std::string path; + while (const Element parent = element.parent()) { + std::uint32_t index = 0; + for (Element sibling = element.previous_sibling(); sibling; + sibling = sibling.previous_sibling()) { + ++index; + } + path = "/child:" + std::to_string(index) + path; + element = parent; + } + return path; +} + void expect_text_at(const Document &document, const std::string &path, const std::string &expected) { - EXPECT_EQ(expected, document.root_element() - .navigate_path(DocumentPath(path)) - .as_text() - .content()); + EXPECT_EQ(expected, + navigate(document.root_element(), path).as_text().content()); } using TextEdits = std::vector>; @@ -78,8 +126,7 @@ using TextEdits = std::vector>; std::string set_text_ops(const Document &document, const TextEdits &edits) { nlohmann::json ops = nlohmann::json::array(); for (const auto &[path, text] : edits) { - const Element element = - document.root_element().navigate_path(DocumentPath(path)); + const Element element = navigate(document.root_element(), path); EXPECT_TRUE(element) << "no element at " << path; ops.push_back( {{"op", "setText"}, {"id", element.identifier()}, {"text", text}}); @@ -229,7 +276,7 @@ TEST(Document, xlsx_sheet_names) { names); } -TEST(Document, odt_element_path) { +TEST(Document, odt_element_round_trips_through_its_id_and_its_path) { const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file = @@ -248,12 +295,11 @@ TEST(Document, odt_element_path) { const Element t1 = p1.first_child(); EXPECT_EQ(t1.type(), ElementType::text); - const DocumentPath t1_path = t1.document_path(); - const Element t1_via_path = root.navigate_path(t1_path); - EXPECT_EQ(t1_via_path, t1); + EXPECT_EQ(document.element_by_id(t1.identifier()), t1); + EXPECT_EQ(navigate(root, path_of(t1)), t1); } -TEST(Document, odt_element_path2) { +TEST(Document, odt_element_at_a_path) { const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file = @@ -269,8 +315,8 @@ TEST(Document, odt_element_path2) { const Element root = document.root_element(); - const Element cell_via_path = root.navigate_path( - DocumentPath("/child:41/child:0/child:1/child:0/child:0")); + const Element cell_via_path = + navigate(root, "/child:41/child:0/child:1/child:0/child:0"); EXPECT_EQ(cell_via_path.type(), ElementType::text); EXPECT_EQ(cell_via_path.as_text().content(), "B1"); } @@ -499,14 +545,13 @@ std::string edit_across_runs(const std::string &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(); + paragraph_path = path_of(paragraph); 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))); + return text_of(navigate(document.root_element(), paragraph_path)); } } // namespace @@ -536,7 +581,7 @@ split_a_paragraph(const std::string &path, const std::string &output_name) { [&](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(); + paragraph_path = path_of(paragraph); const std::vector runs = runs_of(paragraph); return nlohmann::json{{"version", 2}, {"ops", @@ -555,9 +600,8 @@ split_a_paragraph(const std::string &path, const std::string &output_name) { }, output_name); - const DocumentPath head(paragraph_path); - return {text_of(document.root_element().navigate_path(head)), - text_of(document.root_element().navigate_path(head).next_sibling())}; + const Element head = navigate(document.root_element(), paragraph_path); + return {text_of(head), text_of(head.next_sibling())}; } } // namespace @@ -613,13 +657,12 @@ TEST(Document, edit_pptx_across_runs) { EXPECT_TRUE(opened.is_savable()); const Element paragraph = slide_paragraph_of_several_runs(opened); EXPECT_TRUE(paragraph) << path << " holds no paragraph of three runs"; - paragraph_path = paragraph.document_path().to_string(); + paragraph_path = path_of(paragraph); return rewrite_paragraph_ops(runs_of(paragraph)); }, "pptx_edit_runs.pptx"); - EXPECT_EQ(text_of(document.root_element().navigate_path( - DocumentPath(paragraph_path))), + EXPECT_EQ(text_of(navigate(document.root_element(), paragraph_path)), "head tail and more"); } @@ -632,7 +675,7 @@ TEST(Document, edit_pptx_splits_a_paragraph) { [&](const Document &opened) { const Element paragraph = slide_paragraph_of_several_runs(opened); EXPECT_TRUE(paragraph) << path << " holds no paragraph of three runs"; - paragraph_path = paragraph.document_path().to_string(); + paragraph_path = path_of(paragraph); const std::vector runs = runs_of(paragraph); return nlohmann::json{{"version", 2}, {"ops", @@ -647,8 +690,7 @@ TEST(Document, edit_pptx_splits_a_paragraph) { }, "pptx_edit_split.pptx"); - const Element head = - document.root_element().navigate_path(DocumentPath(paragraph_path)); + const Element head = navigate(document.root_element(), paragraph_path); EXPECT_EQ(text_of(head), "head"); EXPECT_EQ(head.next_sibling().type(), ElementType::paragraph); EXPECT_FALSE(text_of(head.next_sibling()).empty()); diff --git a/test/src/internal/csv/csv_file_test.cpp b/test/src/internal/csv/csv_file_test.cpp index 1a13a193f..5a0f41667 100644 --- a/test/src/internal/csv/csv_file_test.cpp +++ b/test/src/internal/csv/csv_file_test.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include #include @@ -480,9 +479,8 @@ TEST(CsvDocument, a_cell_path_round_trips) { const Sheet sheet = (*document.root_element().children().begin()).as_sheet(); const SheetCell cell = sheet.cell(1, 2); - const DocumentPath path = cell.document_path(); - const Element found = document.root_element().navigate_path(path); + const Element found = document.element_by_id(cell.identifier()); EXPECT_EQ(found.type(), ElementType::sheet_cell); EXPECT_EQ((*found.as_sheet_cell().children().begin()).as_text().content(), "4"); diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp index 8c5f1dce2..23a17bba0 100644 --- a/test/src/internal/odf/odf_sheet_repeat_test.cpp +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -3,7 +3,6 @@ #include #include -#include #include #include #include @@ -116,8 +115,8 @@ TEST(OdfSheetRepeat, two_positions_of_one_run_are_not_the_same_handle) { EXPECT_EQ(sheet.cell(2, 0), sheet.cell(2, 0)); } -/// `DocumentPath` spells a cell by position, so it names the one asked for. -TEST(OdfSheetRepeat, a_repeated_cell_round_trips_through_its_path) { +/// A repeated cell's id carries its position, so it names the one asked for. +TEST(OdfSheetRepeat, a_repeated_cell_round_trips_through_its_id) { const std::shared_ptr held = document_of(flat_sheet(repeated_rows(4, 3))); const odr::Document document(held); @@ -125,11 +124,11 @@ TEST(OdfSheetRepeat, a_repeated_cell_round_trips_through_its_path) { const SheetCell cell = sheet.cell(2, 1); - EXPECT_EQ(document.root_element().navigate_path(cell.document_path()), cell); + EXPECT_EQ(document.element_by_id(cell.identifier()), cell); } -/// The position stops at the cell: one run stands for every position, so a -/// path into a repeated cell names the anchor. +/// The position stops at the cell: one run stands for every position, so its +/// id names the anchor's run. TEST(OdfSheetRepeat, the_children_of_a_repeated_cell_are_shared) { const std::shared_ptr held = document_of(flat_sheet(repeated_rows(4, 3))); @@ -139,7 +138,7 @@ TEST(OdfSheetRepeat, the_children_of_a_repeated_cell_are_shared) { const Element text = *(*sheet.cell(2, 1).children().begin()).children().begin(); EXPECT_EQ(*(*sheet.cell(0, 0).children().begin()).children().begin(), text); - EXPECT_EQ(document.root_element().navigate_path(text.document_path()), text); + EXPECT_EQ(document.element_by_id(text.identifier()), text); } /// A write cuts the run into three rather than expanding it, so what it costs