refactor(document)!: one way to edit a document

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)`.

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

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;
 

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

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

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

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
 

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

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

jni/src/jni_document.cpp

@@ -59,13 +59,11 @@ Java_app_opendocument_core_Document_destroy(JNIEnv *env, jclass, jlong handle) {
   destroy_handle<odr::Document>(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<odr::Document>(handle), to_string(env, diff));
+    from_handle<odr::Document>(handle)->edit(to_string(env, diff));
   });
 }
 

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

python/src/bind_document.cpp

@@ -315,6 +315,13 @@ void odr_python::bind_document(py::module_ &m) {
 
   py::class_<odr::Document>(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

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<py::gil_scoped_release>(),
       "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.");
 }

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())

src/odr/document.cpp

@@ -1,6 +1,7 @@
 #include <odr/document.hpp>
 
 #include <odr/document_element.hpp>
+#include <odr/document_path.hpp>
 #include <odr/exceptions.hpp>
 #include <odr/file.hpp>
 #include <odr/filesystem.hpp>
@@ -12,8 +13,12 @@
 #include <fstream>
 #include <memory>
 #include <sstream>
+#include <stdexcept>
+#include <string>
 #include <utility>
 
+#include <nlohmann/json.hpp>
+
 namespace odr {
 
 Document::Document(std::shared_ptr<internal::abstract::Document> 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()};
 }

src/odr/document.hpp

@@ -1,8 +1,11 @@
 #pragma once
 
+#include <odr/logger.hpp>
+
 #include <iosfwd>
 #include <memory>
 #include <string>
+#include <string_view>
 
 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

src/odr/html.cpp

@@ -1,8 +1,6 @@
 #include <odr/html.hpp>
 
 #include <odr/archive.hpp>
-#include <odr/document_element.hpp>
-#include <odr/document_path.hpp>
 #include <odr/exceptions.hpp>
 #include <odr/filesystem.hpp>
 #include <odr/odr.hpp>
@@ -25,8 +23,6 @@
 #include <fstream>
 #include <unordered_set>
 
-#include <nlohmann/json.hpp>
-
 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

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

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");

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