diff --git a/CHANGELOG.md b/CHANGELOG.md index 9319faa5d..3debe76f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- **Breaking**: `html::edit` becomes `Document::edit`, in every binding — + java's `Html.edit(document, diff)` becomes `document.edit(diff)`, and so on. + `Text::set_content` is unchanged. + - **Breaking**: `DecodePreference` becomes `DecodeOptions`, gains a `csv` field and is all `open` takes besides the file and logger. `CsvFile::from_file` and `::with_options` go — use `DecodeOptions::as(type)` / `::as_csv(options)`. diff --git a/apple/include/OdrCoreObjC/ODRDocument.h b/apple/include/OdrCoreObjC/ODRDocument.h index 6907f9e17..7caf32cfd 100644 --- a/apple/include/OdrCoreObjC/ODRDocument.h +++ b/apple/include/OdrCoreObjC/ODRDocument.h @@ -23,6 +23,13 @@ NS_SWIFT_NAME(Document) /// Whether `saveTo:password:` works. @property(nonatomic, readonly) BOOL isSavableEncrypted; +/// Applies the operations our browser-side editor produces, in order. +/// +/// Editing a single element in process is `ODRText.setContent:` and needs none +/// of this. +- (BOOL)edit:(NSString *)operations + error:(NSError **)error NS_SWIFT_NAME(edit(operations:)); + - (BOOL)saveTo:(NSString *)path error:(NSError **)error; - (BOOL)saveTo:(NSString *)path password:(NSString *)password diff --git a/apple/include/OdrCoreObjC/ODRHtml.h b/apple/include/OdrCoreObjC/ODRHtml.h index e12cd2efc..a904a1454 100644 --- a/apple/include/OdrCoreObjC/ODRHtml.h +++ b/apple/include/OdrCoreObjC/ODRHtml.h @@ -286,11 +286,6 @@ NS_SWIFT_NAME(HtmlTranslator) error:(NSError **)error NS_SWIFT_NAME(translate(archive:config:)); -/// Applies a diff produced by the browser-side JavaScript back to `document`. -+ (BOOL)editDocument:(ODRDocument *)document - diff:(NSString *)diff - error:(NSError **)error NS_SWIFT_NAME(edit(document:diff:)); - - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; diff --git a/apple/src/ODRDocument.mm b/apple/src/ODRDocument.mm index 7897f494a..f5700d311 100644 --- a/apple/src/ODRDocument.mm +++ b/apple/src/ODRDocument.mm @@ -39,6 +39,13 @@ - (BOOL)isEditable { return guarded_value([&] { return _handle->is_editable() ? YES : NO; }, NO); } +- (BOOL)edit:(NSString *)operations error:(NSError **)error { + return guarded(error, [&] { + _handle->edit(to_string(operations)); + return YES; + }); +} + - (BOOL)isSavable { return guarded_value([&] { return _handle->is_savable(false) ? YES : NO; }, NO); diff --git a/apple/src/ODRHtml.mm b/apple/src/ODRHtml.mm index 6d47171ca..0c182b0e0 100644 --- a/apple/src/ODRHtml.mm +++ b/apple/src/ODRHtml.mm @@ -553,13 +553,4 @@ + (nullable ODRHtmlService *)translateArchive:(ODRArchive *)archive }); } -+ (BOOL)editDocument:(ODRDocument *)document - diff:(NSString *)diff - error:(NSError **)error { - return guarded(error, [&] { - odr::html::edit(document.handle, to_string(diff)); - return YES; - }); -} - @end diff --git a/cli/src/back_translate.cpp b/cli/src/back_translate.cpp index d9af40e87..7adde03a4 100644 --- a/cli/src/back_translate.cpp +++ b/cli/src/back_translate.cpp @@ -34,7 +34,7 @@ int main(const int argc, char **argv) { const Document document = document_file.document(); const std::string diff = internal::util::file::read(diff_path); - html::edit(document, diff); + document.edit(diff); document.save(output); diff --git a/docs/design/api-v7.md b/docs/design/api-v7.md index 5eb49cbe1..a3d96d906 100644 --- a/docs/design/api-v7.md +++ b/docs/design/api-v7.md @@ -133,10 +133,21 @@ questions — but only if the header says which is which. input. [`editing.md`](editing.md) already commits to an operation log replacing the diff blob, which changes this signature anyway. -**Target:** `Document::apply(std::string_view operations)`, in -`document.hpp`; `html::edit` and the public `Text::set_content` go. The op-log -*semantics* stay exactly what they are today — this is the entry point moving -to where v7.x can fill it in without breaking again. +**Target:** `Document::edit(std::string_view operations)`, in `document.hpp`; +`html::edit` goes. The op-log *semantics* stay exactly what they are today — +this is the entry point moving to where v7.x can fill it in without breaking +again. Named `edit` rather than `apply` to sit beside `is_editable`, and +because JNI had already put it there: `jni_document.cpp` carried the comment +*"odr::html::edit, but it belongs to Document"*. + +**`Text::set_content` stays.** An earlier draft of this plan removed it as the +second road. That was wrong. The two are not one operation spelled twice: one +edits a named element in process, the other replays a log a browser produced. +`set_content` is mirrored in the java, python and objc bindings and exercised +by the Swift suite, so removing it would take capability away and force a +caller who wants to change one text run to assemble JSON. What was actually +wrong here was the *filing* — an editing entry point in `namespace html`, whose +only connection to html is that our JavaScript writes its input. ## Finding 4 — smaller things a major is the only chance to fix diff --git a/jni/java/app/opendocument/core/Document.java b/jni/java/app/opendocument/core/Document.java index e55d4a1b3..fa0f4f083 100644 --- a/jni/java/app/opendocument/core/Document.java +++ b/jni/java/app/opendocument/core/Document.java @@ -52,8 +52,8 @@ public Filesystem asFilesystem() { return new Filesystem(asFilesystemNative(handle()), this); } - /** Applies a diff; what {@link Html#edit} calls. */ - void edit(String diff) { + /** Applies the operations our browser-side editor produces. */ + public void edit(String diff) { editNative(handle(), diff); } diff --git a/jni/java/app/opendocument/core/Html.java b/jni/java/app/opendocument/core/Html.java index c6a7701f1..a4134ab62 100644 --- a/jni/java/app/opendocument/core/Html.java +++ b/jni/java/app/opendocument/core/Html.java @@ -81,11 +81,6 @@ public static HtmlService translate(Filesystem filesystem, HtmlConfig config) { } } - /** Applies a diff (produced by the browser-side editor) to a document. */ - public static void edit(Document document, String diff) { - document.edit(diff); - } - private static native long translateFile(long fileHandle, HtmlConfig config); private static native long translateDocument(long documentHandle, HtmlConfig config); diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 5ed9266c1..780061058 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -59,13 +59,11 @@ Java_app_opendocument_core_Document_destroy(JNIEnv *env, jclass, jlong handle) { destroy_handle(env, handle); } -// odr::html::edit, but it belongs to Document: a native taking a handle must be -// an instance method of its owner, or the wrapper can be collected mid-call. extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Document_editNative(JNIEnv *env, jobject, jlong handle, jstring diff) { guarded(env, [&] { - odr::html::edit(*from_handle(handle), to_string(env, diff)); + from_handle(handle)->edit(to_string(env, diff)); }); } diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index 5d63fb305..9cbf8a2c2 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -100,7 +100,7 @@ void editAppliesADiff() throws IOException { Element paragraph = document.rootElement().firstChild(); DocumentPath text = paragraph.firstChild().documentPath(); - Html.edit(document, "{\"modifiedText\":{\"" + text + "\":\"edited by the diff\"}}"); + document.edit("{\"modifiedText\":{\"" + text + "\":\"edited by the diff\"}}"); assertTrue(walkText(document.rootElement()).contains("edited by the diff")); } @@ -111,7 +111,7 @@ void saveToMemoryRoundTripsAnEdit() throws IOException { Element paragraph = document.rootElement().firstChild(); DocumentPath text = paragraph.firstChild().documentPath(); - Html.edit(document, "{\"modifiedText\":{\"" + text + "\":\"saved to memory\"}}"); + document.edit("{\"modifiedText\":{\"" + text + "\":\"saved to memory\"}}"); byte[] saved = document.saveToMemory(); assertTrue(saved.length > 0); diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index 761918149..38ab64741 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -315,6 +315,13 @@ void odr_python::bind_document(py::module_ &m) { py::class_(m, "Document") .def("is_editable", &odr::Document::is_editable) + .def( + "edit", + [](const odr::Document &document, const std::string &operations) { + document.edit(operations); + }, + py::arg("operations"), + "Apply the operations our browser-side editor produces.") .def("is_savable", &odr::Document::is_savable, py::arg("encrypted") = false) // saving serialises the whole document; holding the GIL for it blocks diff --git a/python/src/bind_html.cpp b/python/src/bind_html.cpp index cd1f2bdcb..9aea125cf 100644 --- a/python/src/bind_html.cpp +++ b/python/src/bind_html.cpp @@ -239,12 +239,4 @@ void odr_python::bind_html(py::module_ &m) { py::arg("logger") = odr::Logger::null(), py::call_guard(), "Translate a filesystem to HTML."); - - html.def( - "edit", - [](const odr::Document &document, const std::string &diff) { - odr::html::edit(document, diff); - }, - py::arg("document"), py::arg("diff"), - "Apply a diff (produced by the browser-side editor) to a document."); } diff --git a/python/tests/test_document.py b/python/tests/test_document.py index 7083f6745..2f843a035 100644 --- a/python/tests/test_document.py +++ b/python/tests/test_document.py @@ -137,7 +137,7 @@ def test_save_to_memory_carries_an_edit(odt_path, tmp_path): document = pyodr.open(str(odt_path)).as_document_file().document() diff = '{"modifiedText":{"/child:0/child:0":"edited in python"}}' - pyodr.html.edit(document, diff) + document.edit(diff) path = tmp_path / "edited.odt" path.write_bytes(document.save_to_memory()) diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 3eca9763c..d5e6639b8 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -12,8 +13,12 @@ #include #include #include +#include +#include #include +#include + namespace odr { Document::Document(std::shared_ptr impl) @@ -80,6 +85,22 @@ DocumentType Document::document_type() const noexcept { return m_impl->document_type(); } +void Document::edit(const std::string_view operations, + const Logger & /*logger*/) const { + const nlohmann::json json = nlohmann::json::parse(operations); + for (const auto &[key, value] : json["modifiedText"].items()) { + const Element element = root_element().navigate_path(DocumentPath(key)); + if (!element) { + throw std::invalid_argument("element with path " + key + " not found"); + } + if (!element.as_text()) { + throw std::invalid_argument("element with path " + key + + " is not a text element"); + } + element.as_text().set_content(value); + } +} + Element Document::root_element() const { return {m_impl->element_adapter(), m_impl->root_element()}; } diff --git a/src/odr/document.hpp b/src/odr/document.hpp index d8a5fa2b6..21fb40c55 100644 --- a/src/odr/document.hpp +++ b/src/odr/document.hpp @@ -1,8 +1,11 @@ #pragma once +#include + #include #include #include +#include namespace odr::internal::abstract { class Document; @@ -40,6 +43,15 @@ class Document final { [[nodiscard]] FileType file_type() const noexcept; [[nodiscard]] DocumentType document_type() const noexcept; + /// @brief Applies @p operations to the document, in order. + /// + /// The wire format our browser-side editor produces. Editing a single + /// element in process is @ref Text::set_content and needs none of this. + /// @throws std::invalid_argument if an operation names an element that is + /// not there, or not one it can be applied to. + void edit(std::string_view operations, + const Logger &logger = Logger::null()) const; + [[nodiscard]] Element root_element() const; /// The files the document is packaged from; empty for a document that is diff --git a/src/odr/html.cpp b/src/odr/html.cpp index c7349c7a6..3365d569d 100644 --- a/src/odr/html.cpp +++ b/src/odr/html.cpp @@ -1,8 +1,6 @@ #include #include -#include -#include #include #include #include @@ -25,8 +23,6 @@ #include #include -#include - using namespace odr::internal; namespace odr { @@ -345,21 +341,4 @@ HtmlService html::translate(const Document &document, const HtmlConfig &config, return internal::html::create_document_service(document, config, logger); } -void html::edit(const Document &document, const std::string_view diff, - const Logger & /*logger*/) { - const nlohmann::json json = nlohmann::json::parse(diff); - for (const auto &[key, value] : json["modifiedText"].items()) { - const Element element = - document.root_element().navigate_path(DocumentPath(key)); - if (!element) { - throw std::invalid_argument("element with path " + key + " not found"); - } - if (!element.as_text()) { - throw std::invalid_argument("element with path " + key + - " is not a text element"); - } - element.as_text().set_content(value); - } -} - } // namespace odr diff --git a/src/odr/html.hpp b/src/odr/html.hpp index 4408d4c83..7444e34bb 100644 --- a/src/odr/html.hpp +++ b/src/odr/html.hpp @@ -307,11 +307,6 @@ HtmlService translate(const Filesystem &filesystem, const HtmlConfig &config, HtmlService translate(const Archive &archive, const HtmlConfig &config, const Logger &logger = Logger::null()); -/// @brief Applies a diff to a document. The diff is what our JavaScript -/// produces in the browser. -void edit(const Document &document, std::string_view diff, - const Logger &logger = Logger::null()); - } // namespace html } // namespace odr diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index 2cccd35ef..037c08ebf 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -76,7 +76,7 @@ Document edit_and_reload(const std::string &path, const char *diff, open(TestData::test_file_path(path), {}, logger).as_document_file(); const Document document = document_file.document(); - html::edit(document, diff); + document.edit(diff); const std::string output_path = (std::filesystem::current_path() / output_name).string(); @@ -367,7 +367,7 @@ TEST(Document, edit_ods_diff) { R"({"modifiedText":{"/child:0/cell:A1/child:0/child:0":"Page 1 hi","/child:1/cell:A1/child:0/child:0":"Page 2 hihi","/child:2/cell:A1/child:0/child:0":"Page 3 hihihi","/child:3/cell:A1/child:0/child:0":"Page 4 hihihihi","/child:4/cell:A1/child:0/child:0":"Page 5 hihihihihi"}})"; const Document document = decrypted_pages_ods(); - html::edit(document, diff); + document.edit(diff); expect_text_at(document, "/child:0/cell:A1/child:0/child:0", "Page 1 hi"); expect_text_at(document, "/child:1/cell:A1/child:0/child:0", "Page 2 hihi"); diff --git a/wasm/src/wasm_html.cpp b/wasm/src/wasm_html.cpp index 203948087..ac20a8725 100644 --- a/wasm/src/wasm_html.cpp +++ b/wasm/src/wasm_html.cpp @@ -142,7 +142,7 @@ emscripten::val read_path(const Handle handle, const std::string &path) { emscripten::val edit(const Handle handle, const std::string &diff) { return guarded([&] { Session &s = session(handle); - html::edit(document_of(s), diff, s.logger); + document_of(s).edit(diff, s.logger); return ok(); }); }