diff --git a/CHANGELOG.md b/CHANGELOG.md index 159ad3249..af3f0bab4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,26 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- **Fix**: `Text::set_content` on an `.xlsx` run did nothing and said nothing. + An xlsx declares `edit`, so a caller had no way to learn the write was + dropped; it now throws `UnsupportedOperation`, as every other engine that + cannot write a run does. + +- **Fix**: the pdf object parser skipped over `null`, `true` and `false` + without reading them, so `nXYZ` parsed as null. The keyword is checked now, + without case, as the surrounding parser already allows. + +- Every binding reaches the structural edit API. python, jni and apple take + elements (`remove`, `insertTextBefore`/`After`, `appendText`, + `splitParagraph`, `mergeParagraphWithNext`, `insertParagraphAfter`); wasm + addresses the same operations by element id, because nothing escapes it as a + handle. `element_by_id` and `TextFile.write_edited` are bound too. + +- **Breaking**: a csv and a markdown file hold a text file instead of being + one. `is_text_file()` answers false for them; the plain-text view is + `as_csv_file().text_file()` / `as_markdown_file().text_file()`. `CsvFile` and + `MarkdownFile` are bound in python, jni and apple to carry both views. + - A selection reaching over a picture is taken, and the picture goes with the text: a frame carries `data-odr-id`, so an operation can name it. A frame holding text of its own is still refused. diff --git a/README.md b/README.md index 084baf52a..372715119 100644 --- a/README.md +++ b/README.md @@ -53,9 +53,9 @@ to a system file picker. It is a declared upper bound; `DecodedFile::capabilitie and `Document::is_editable` / `is_savable` give the precise answer for a concrete file. -Editing and saving are currently limited to odt, odp, odg (`edit` + `save`), -ods (`save` only) and docx (`edit` + `save`); saving with a password is not -supported for any format. +Editing and saving are currently limited to odt, odp, ods, odg, docx, pptx, +xlsx and txt (`edit` + `save` each); saving with a password is not supported +for any format. ## Unsupported files diff --git a/apple/include/OdrCoreObjC/ODRDocument.h b/apple/include/OdrCoreObjC/ODRDocument.h index 7caf32cfd..6aed0b1a0 100644 --- a/apple/include/OdrCoreObjC/ODRDocument.h +++ b/apple/include/OdrCoreObjC/ODRDocument.h @@ -50,6 +50,56 @@ NS_SWIFT_NAME(Document) - (nullable ODRElement *)rootElementWithError:(NSError **)error NS_SWIFT_NAME(rootElement()); +/// The element `ODRElement.identifier` handed out, or `nil` where this +/// document holds no such id. Not an error, so it does not throw — an id that +/// is gone is the ordinary answer. +- (nullable ODRElement *)elementWithIdentifier:(uint64_t)identifier + NS_SWIFT_NAME(element(identifier:)); + +#pragma mark - Structural edits + +/// Each fails where the engine cannot write, and for an element of another +/// document. + +/// Removes an element and its subtree; its identifier stays taken. +- (BOOL)removeElement:(ODRElement *)element + error:(NSError **)error NS_SWIFT_NAME(remove(_:)); + +/// A run before `anchor`, in the same parent, so it takes the same style. +- (nullable ODRText *)insertTextBefore:(ODRText *)anchor + text:(NSString *)text + error:(NSError **)error + NS_SWIFT_NAME(insertText(before:text:)); + +/// A run after `anchor`, in the same parent. +- (nullable ODRText *)insertTextAfter:(ODRText *)anchor + text:(NSString *)text + error:(NSError **)error + NS_SWIFT_NAME(insertText(after:text:)); + +/// A run as the last child of `parent`. +- (nullable ODRText *)appendTextTo:(ODRElement *)parent + text:(NSString *)text + error:(NSError **)error + NS_SWIFT_NAME(appendText(to:text:)); + +/// Splits `paragraph` after `after` — one of its descendants — into a new +/// paragraph of the same style. A `nil` `after` moves every child. +- (nullable ODRParagraph *)splitParagraph:(ODRParagraph *)paragraph + after:(nullable ODRElement *)after + error:(NSError **)error + NS_SWIFT_NAME(splitParagraph(_:after:)); + +/// `paragraph` takes the children of the paragraph after it, which then goes. +- (BOOL)mergeParagraphWithNext:(ODRParagraph *)paragraph + error:(NSError **)error + NS_SWIFT_NAME(mergeParagraphWithNext(_:)); + +/// An empty paragraph after `paragraph`, of the same style. +- (nullable ODRParagraph *)insertParagraphAfter:(ODRParagraph *)paragraph + error:(NSError **)error + NS_SWIFT_NAME(insertParagraph(after:)); + @end NS_ASSUME_NONNULL_END diff --git a/apple/include/OdrCoreObjC/ODRFile.h b/apple/include/OdrCoreObjC/ODRFile.h index 8639f5b06..6d3ae4bde 100644 --- a/apple/include/OdrCoreObjC/ODRFile.h +++ b/apple/include/OdrCoreObjC/ODRFile.h @@ -8,6 +8,8 @@ NS_ASSUME_NONNULL_BEGIN @class ODRLogger; @class ODRTextFile; +@class ODRCsvFile; +@class ODRMarkdownFile; @class ODRImageFile; @class ODRArchiveFile; @class ODRDocumentFile; @@ -336,6 +338,10 @@ NS_SWIFT_NAME(DecodedFile) @property(nonatomic, readonly) ODRFileTypeCapabilities *capabilities; @property(nonatomic, readonly) BOOL isTextFile; +/// A csv holds a text file rather than being one, so `isTextFile` is `NO`. +@property(nonatomic, readonly) BOOL isCsvFile; +/// Markdown holds a text file the same way a csv does. +@property(nonatomic, readonly) BOOL isMarkdownFile; @property(nonatomic, readonly) BOOL isImageFile; @property(nonatomic, readonly) BOOL isArchiveFile; @property(nonatomic, readonly) BOOL isDocumentFile; @@ -345,6 +351,10 @@ NS_SWIFT_NAME(DecodedFile) /// The typed views. Each fails unless the matching `is…` is true. - (nullable ODRTextFile *)asTextFileWithError:(NSError **)error NS_SWIFT_NAME(asTextFile()); +- (nullable ODRCsvFile *)asCsvFileWithError:(NSError **)error + NS_SWIFT_NAME(asCsvFile()); +- (nullable ODRMarkdownFile *)asMarkdownFileWithError:(NSError **)error + NS_SWIFT_NAME(asMarkdownFile()); - (nullable ODRImageFile *)asImageFileWithError:(NSError **)error NS_SWIFT_NAME(asImageFile()); - (nullable ODRArchiveFile *)asArchiveFileWithError:(NSError **)error @@ -369,6 +379,38 @@ NS_SWIFT_NAME(TextFile) @property(nonatomic, readonly, nullable, copy) NSString *charset; /// The decoded text. - (nullable NSString *)textWithError:(NSError **)error NS_SWIFT_NAME(text()); +/// `NO` where the file type is one this library does not write, or the +/// encoding cannot be decoded. +@property(nonatomic, readonly) BOOL isSavable; +/// Applies the operations and returns the result, as UTF-8 whatever the source +/// encoding was. +- (nullable NSData *)writeEdited:(NSString *)operations + error:(NSError **)error + NS_SWIFT_NAME(writeEdited(operations:)); +@end + +/// A decoded csv — `odr::CsvFile`. It *holds* a text file rather than being +/// one; `document` and `textFile` are the two views of the same bytes. +NS_SWIFT_NAME(CsvFile) +@interface ODRCsvFile : ODRDecodedFile +/// The csv as a one-sheet spreadsheet. +- (nullable ODRDocument *)documentWithError:(NSError **)error + NS_SWIFT_NAME(document()); +/// The same bytes as plain text, so reading them needs no reopening. +- (nullable ODRTextFile *)textFileWithError:(NSError **)error + NS_SWIFT_NAME(textFile()); +@end + +/// A decoded markdown file — `odr::MarkdownFile`. Holds a text file the way +/// `ODRCsvFile` does. +NS_SWIFT_NAME(MarkdownFile) +@interface ODRMarkdownFile : ODRDecodedFile +/// The markdown as a text document. +- (nullable ODRDocument *)documentWithError:(NSError **)error + NS_SWIFT_NAME(document()); +/// The same bytes as plain text, so reading them needs no reopening. +- (nullable ODRTextFile *)textFileWithError:(NSError **)error + NS_SWIFT_NAME(textFile()); @end /// A decoded image file — `odr::ImageFile`. diff --git a/apple/src/ODRDocument.mm b/apple/src/ODRDocument.mm index f5700d311..560f9d681 100644 --- a/apple/src/ODRDocument.mm +++ b/apple/src/ODRDocument.mm @@ -4,6 +4,7 @@ #import "ODRPrivate.h" #include +#include #include #include @@ -13,6 +14,11 @@ using odr::apple::guarded_value; using odr::apple::to_string; +@interface ODRDocument () +- (nullable ODRText *)wrapText:(odr::Text)handle; +- (nullable ODRParagraph *)wrapParagraph:(odr::Paragraph)handle; +@end + @implementation ODRDocument { std::optional _handle; } @@ -97,6 +103,89 @@ - (nullable ODRElement *)rootElementWithError:(NSError **)error { }); } +- (nullable ODRElement *)elementWithIdentifier:(uint64_t)identifier { + return guarded_value( + [&]() -> ODRElement * { + return [ODRElement elementWithHandle:_handle->element_by_id(identifier) + owner:self]; + }, + nil); +} + +#pragma mark - Structural edits + +- (BOOL)removeElement:(ODRElement *)element error:(NSError **)error { + return guarded(error, [&] { + _handle->remove(element.handle); + return YES; + }); +} + +/// The run the structural edits hand back, wrapped in the class the picker +/// gives it — `ODRText` for every one of them. +- (nullable ODRText *)wrapText:(odr::Text)handle { + return static_cast([ODRElement elementWithHandle:std::move(handle) + owner:self]); +} + +- (nullable ODRParagraph *)wrapParagraph:(odr::Paragraph)handle { + return static_cast( + [ODRElement elementWithHandle:std::move(handle) owner:self]); +} + +- (nullable ODRText *)insertTextBefore:(ODRText *)anchor + text:(NSString *)text + error:(NSError **)error { + return guarded(error, [&]() -> ODRText * { + return [self wrapText:_handle->insert_text_before(anchor.handle.as_text(), + to_string(text))]; + }); +} + +- (nullable ODRText *)insertTextAfter:(ODRText *)anchor + text:(NSString *)text + error:(NSError **)error { + return guarded(error, [&]() -> ODRText * { + return [self wrapText:_handle->insert_text_after(anchor.handle.as_text(), + to_string(text))]; + }); +} + +- (nullable ODRText *)appendTextTo:(ODRElement *)parent + text:(NSString *)text + error:(NSError **)error { + return guarded(error, [&]() -> ODRText * { + return [self wrapText:_handle->append_text(parent.handle, to_string(text))]; + }); +} + +- (nullable ODRParagraph *)splitParagraph:(ODRParagraph *)paragraph + after:(nullable ODRElement *)after + error:(NSError **)error { + return guarded(error, [&]() -> ODRParagraph * { + return + [self wrapParagraph:_handle->split_paragraph( + paragraph.handle.as_paragraph(), + after == nil ? odr::Element() : after.handle)]; + }); +} + +- (BOOL)mergeParagraphWithNext:(ODRParagraph *)paragraph + error:(NSError **)error { + return guarded(error, [&] { + _handle->merge_paragraph_with_next(paragraph.handle.as_paragraph()); + return YES; + }); +} + +- (nullable ODRParagraph *)insertParagraphAfter:(ODRParagraph *)paragraph + error:(NSError **)error { + return guarded(error, [&]() -> ODRParagraph * { + return [self wrapParagraph:_handle->insert_paragraph_after( + paragraph.handle.as_paragraph())]; + }); +} + - (nullable ODRFilesystem *)filesystemWithError:(NSError **)error { return guarded(error, [&]() -> ODRFilesystem * { return [ODRFilesystem filesystemWithHandle:_handle->as_filesystem()]; diff --git a/apple/src/ODRFile.mm b/apple/src/ODRFile.mm index 85b33b045..db8eab719 100644 --- a/apple/src/ODRFile.mm +++ b/apple/src/ODRFile.mm @@ -300,6 +300,10 @@ + (instancetype)decodedFileWithHandle:(odr::DecodedFile)handle { klass = [ODRPdfFile class]; } else if (handle.is_document_file()) { klass = [ODRDocumentFile class]; + } else if (handle.is_csv_file()) { + klass = [ODRCsvFile class]; + } else if (handle.is_markdown_file()) { + klass = [ODRMarkdownFile class]; } else if (handle.is_text_file()) { klass = [ODRTextFile class]; } else if (handle.is_image_file()) { @@ -475,6 +479,15 @@ - (ODRFileTypeCapabilities *)capabilities { - (BOOL)isTextFile { return guarded_value([&] { return _handle->is_text_file() ? YES : NO; }, NO); } + +- (BOOL)isCsvFile { + return guarded_value([&] { return _handle->is_csv_file() ? YES : NO; }, NO); +} + +- (BOOL)isMarkdownFile { + return guarded_value([&] { return _handle->is_markdown_file() ? YES : NO; }, + NO); +} - (BOOL)isImageFile { return guarded_value([&] { return _handle->is_image_file() ? YES : NO; }, NO); } @@ -502,6 +515,20 @@ - (nullable ODRTextFile *)asTextFileWithError:(NSError **)error { }); } +- (nullable ODRCsvFile *)asCsvFileWithError:(NSError **)error { + return guarded(error, [&]() -> ODRCsvFile * { + return static_cast( + [ODRDecodedFile decodedFileWithHandle:_handle->as_csv_file()]); + }); +} + +- (nullable ODRMarkdownFile *)asMarkdownFileWithError:(NSError **)error { + return guarded(error, [&]() -> ODRMarkdownFile * { + return static_cast( + [ODRDecodedFile decodedFileWithHandle:_handle->as_markdown_file()]); + }); +} + - (nullable ODRImageFile *)asImageFileWithError:(NSError **)error { return guarded(error, [&]() -> ODRImageFile * { return static_cast( @@ -571,6 +598,57 @@ - (nullable NSString *)textWithError:(NSError **)error { }); } +- (BOOL)isSavable { + return guarded_value( + [&] { return self.handle.as_text_file().is_savable() ? YES : NO; }, NO); +} + +- (nullable NSData *)writeEdited:(NSString *)operations + error:(NSError **)error { + return guarded(error, [&]() -> NSData * { + std::ostringstream out; + self.handle.as_text_file().write_edited(to_string(operations), out); + const std::string bytes = out.str(); + return [NSData dataWithBytes:bytes.data() length:bytes.size()]; + }); +} + +@end + +@implementation ODRCsvFile + +- (nullable ODRDocument *)documentWithError:(NSError **)error { + return guarded(error, [&]() -> ODRDocument * { + return + [ODRDocument documentWithHandle:self.handle.as_csv_file().document()]; + }); +} + +- (nullable ODRTextFile *)textFileWithError:(NSError **)error { + return guarded(error, [&]() -> ODRTextFile * { + return static_cast([ODRDecodedFile + decodedFileWithHandle:self.handle.as_csv_file().text_file()]); + }); +} + +@end + +@implementation ODRMarkdownFile + +- (nullable ODRDocument *)documentWithError:(NSError **)error { + return guarded(error, [&]() -> ODRDocument * { + return [ODRDocument + documentWithHandle:self.handle.as_markdown_file().document()]; + }); +} + +- (nullable ODRTextFile *)textFileWithError:(NSError **)error { + return guarded(error, [&]() -> ODRTextFile * { + return static_cast([ODRDecodedFile + decodedFileWithHandle:self.handle.as_markdown_file().text_file()]); + }); +} + @end @implementation ODRImageFile diff --git a/apple/tests/OdrCoreTests.swift b/apple/tests/OdrCoreTests.swift index f0f5203bb..5cf15e5b3 100644 --- a/apple/tests/OdrCoreTests.swift +++ b/apple/tests/OdrCoreTests.swift @@ -104,15 +104,21 @@ final class DecodeTests: XCTestCase { } } - /// A csv is a *text* file to odrcore, not a document file. It does have an - /// element tree — `CsvFile.document()` is a second view of the same bytes — - /// but that does not move it out of `FileCategory.text`. - func testCsvIsTextRatherThanADocument() throws { + /// A csv is neither a document file nor a text file: it *holds* a text file, + /// and `CsvFile.document()` is the other view of the same bytes. Its bytes + /// are still text, so it stays in `FileCategory.text`. + func testCsvIsNeitherADocumentNorATextFile() throws { let path = try write("a,b\n1,2\n", as: "table.csv") let decoded = try DecodedFile.decode(path: path) XCTAssertEqual(decoded.fileType, .commaSeparatedValues) XCTAssertEqual(decoded.fileCategory, .text) XCTAssertFalse(decoded.isDocumentFile) + XCTAssertFalse(decoded.isTextFile) + XCTAssertTrue(decoded.isCsvFile) + + let csv = try decoded.asCsvFile() + XCTAssertEqual(try csv.textFile().text(), "a,b\n1,2\n") + XCTAssertNotNil(try csv.document().rootElement()) } /// `odr::Filesystem::exists("")` throws `std::invalid_argument`. Unguarded, @@ -382,6 +388,91 @@ final class DocumentSaveTests: XCTestCase { } } +final class DocumentStructuralEditTests: XCTestCase { + private func document() throws -> Document { + try DecodedFile.decode(path: try Fixture.odt()) + .asDocumentFile().document() + } + + func testElementByIdentifierResolvesWhatIdentifierHandedOut() throws { + let document = try self.document() + let root = try XCTUnwrap(try document.rootElement()) + let run = try XCTUnwrap(root.firstDescendant(ofType: Text.self)) + + let found = try XCTUnwrap(document.element(identifier: run.identifier)) + + XCTAssertEqual((found as? Text)?.content, run.content) + XCTAssertNil(document.element(identifier: 999_999)) + } + + func testInsertedRunsSurroundTheAnchor() throws { + let document = try self.document() + let root = try XCTUnwrap(try document.rootElement()) + let run = try XCTUnwrap(root.firstDescendant(ofType: Text.self)) + + let before = try XCTUnwrap(try document.insertText(before: run, text: "before ")) + let after = try XCTUnwrap(try document.insertText(after: run, text: " after")) + + XCTAssertEqual(before.content, "before ") + XCTAssertEqual(after.content, " after") + XCTAssertEqual( + root.descendants(ofType: Text.self).prefix(3).map(\.content), + ["before ", Fixture.odtText[0], " after"]) + } + + func testAnAddedParagraphTakesAnAddedRun() throws { + let document = try self.document() + let root = try XCTUnwrap(try document.rootElement()) + let first = try XCTUnwrap(root.firstDescendant(ofType: Paragraph.self)) + + let added = try XCTUnwrap(try document.insertParagraph(after: first)) + let run = try XCTUnwrap(try document.appendText(to: added, text: "a new paragraph")) + + XCTAssertEqual(run.content, "a new paragraph") + XCTAssertTrue( + root.descendants(ofType: Text.self).contains { $0.content == "a new paragraph" }) + } + + func testRemoveTakesTheElementOut() throws { + let document = try self.document() + let root = try XCTUnwrap(try document.rootElement()) + let run = try XCTUnwrap(root.firstDescendant(ofType: Text.self)) + + try document.remove(run) + + // The fixture repeats its runs, so what proves the removal is the sequence. + XCTAssertEqual( + root.descendants(ofType: Text.self).map(\.content), + Array(Fixture.odtText.dropFirst())) + } + + func testSplitAndMergeAreInverse() throws { + let document = try self.document() + let root = try XCTUnwrap(try document.rootElement()) + let first = try XCTUnwrap(root.firstDescendant(ofType: Paragraph.self)) + let run = try XCTUnwrap(first.firstDescendant(ofType: Text.self)) + + _ = try document.splitParagraph(first, after: run) + try document.mergeParagraphWithNext(first) + + XCTAssertEqual(root.descendants(ofType: Text.self).map(\.content), Fixture.odtText) + } +} + +final class TextFileEditTests: XCTestCase { + func testWritesAnEditBack() throws { + let path = try write("hello text file\n", as: "note.txt") + let file = try DecodedFile.decode(path: path).asTextFile() + XCTAssertTrue(file.isSavable) + + let edited = try XCTUnwrap( + try file.writeEdited( + operations: #"{"version":2,"ops":[{"op":"setContent","text":"rewritten"}]}"#)) + + XCTAssertEqual(String(data: edited, encoding: .utf8), "rewritten") + } +} + final class PdfAnnotationTests: XCTestCase { private static let highlight = """ {"version": 1, "annotations": [{"page": 0, "type": "highlight", diff --git a/docs/design/document-editing.md b/docs/design/document-editing.md index 914b65ff3..f48e5c949 100644 --- a/docs/design/document-editing.md +++ b/docs/design/document-editing.md @@ -263,8 +263,8 @@ registry links.** Only the tag names differ — `text:p` / `text:span` against `w:p` / `w:r` / `a:p` / `a:r`. The shared `internal::ElementRegistry` grows the links the structural ops need: -`unlink_child`, `insert_child_after` and `insert_child_before`. It has only -`append_child` today, because until now nothing built a tree except a parser +`unlink_child`, `insert_sibling_after` and `insert_sibling_before`. It had only +`append_child` before, because until then nothing built a tree except a parser reading forward. ## Which formats @@ -274,6 +274,7 @@ reading forward. | `.odt`, `.odp`, `.ods`, `.odg` | `odf` | edits and saves today; the new ops land here | | `.docx` | `ooxml/text` | edits and saves today; the new ops land here | | `.pptx` | `ooxml/presentation` | edits and saves; the same operations over `a:p` / `a:r` | +| `.xlsx` | `ooxml/spreadsheet` | cells only; see [`spreadsheet-editing.md`](spreadsheet-editing.md) | | `.txt` | `text` | not a document at all; see [`txt-editing.md`](txt-editing.md) | | everything else | — | read-only, and says so by decision 7 | diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index d71783d98..707b0f3b4 100644 --- a/jni/CMakeLists.txt +++ b/jni/CMakeLists.txt @@ -70,6 +70,7 @@ add_jar(odr_java "java/app/opendocument/core/Bookmark.java" "java/app/opendocument/core/BreakType.java" "java/app/opendocument/core/Color.java" + "java/app/opendocument/core/CsvFile.java" "java/app/opendocument/core/CsvOptions.java" "java/app/opendocument/core/DecodeOptions.java" "java/app/opendocument/core/DecodedFile.java" @@ -101,6 +102,7 @@ add_jar(odr_java "java/app/opendocument/core/LogLevel.java" "java/app/opendocument/core/Logger.java" "java/app/opendocument/core/LoggerBridge.java" + "java/app/opendocument/core/MarkdownFile.java" "java/app/opendocument/core/SourceLocation.java" "java/app/opendocument/core/Frame.java" "java/app/opendocument/core/GraphicStyle.java" diff --git a/jni/java/app/opendocument/core/CsvFile.java b/jni/java/app/opendocument/core/CsvFile.java new file mode 100644 index 000000000..54f07261e --- /dev/null +++ b/jni/java/app/opendocument/core/CsvFile.java @@ -0,0 +1,22 @@ +package app.opendocument.core; + +/** A decoded csv. Mirrors {@code odr::CsvFile}. */ +public final class CsvFile extends DecodedFile { + CsvFile(long handle) { + super(handle); + } + + /** The csv as a one-sheet spreadsheet. */ + public Document document() { + return new Document(documentNative(handle())); + } + + /** The same bytes as plain text, so reading them needs no reopening. */ + public TextFile textFile() { + return new TextFile(textFileNative(handle())); + } + + private native long documentNative(long handle); + + private native long textFileNative(long handle); +} diff --git a/jni/java/app/opendocument/core/DecodedFile.java b/jni/java/app/opendocument/core/DecodedFile.java index b1ba8f2eb..0d6dcdc04 100644 --- a/jni/java/app/opendocument/core/DecodedFile.java +++ b/jni/java/app/opendocument/core/DecodedFile.java @@ -60,6 +60,16 @@ public boolean isTextFile() { return isTextFileNative(handle()); } + /** A csv holds a text file rather than being one; {@link #isTextFile} is false. */ + public boolean isCsvFile() { + return isCsvFileNative(handle()); + } + + /** Markdown holds a text file the same way a csv does. */ + public boolean isMarkdownFile() { + return isMarkdownFileNative(handle()); + } + public boolean isImageFile() { return isImageFileNative(handle()); } @@ -84,6 +94,14 @@ public TextFile asTextFile() { return new TextFile(asTextFileNative(handle())); } + public CsvFile asCsvFile() { + return new CsvFile(asCsvFileNative(handle())); + } + + public MarkdownFile asMarkdownFile() { + return new MarkdownFile(asMarkdownFileNative(handle())); + } + public ImageFile asImageFile() { return new ImageFile(asImageFileNative(handle())); } @@ -126,6 +144,10 @@ public FontFile asFontFile() { private native boolean isTextFileNative(long handle); + private native boolean isCsvFileNative(long handle); + + private native boolean isMarkdownFileNative(long handle); + private native boolean isImageFileNative(long handle); private native boolean isArchiveFileNative(long handle); @@ -138,6 +160,10 @@ public FontFile asFontFile() { private native long asTextFileNative(long handle); + private native long asCsvFileNative(long handle); + + private native long asMarkdownFileNative(long handle); + private native long asImageFileNative(long handle); private native long asArchiveFileNative(long handle); diff --git a/jni/java/app/opendocument/core/Document.java b/jni/java/app/opendocument/core/Document.java index fa0f4f083..d00d22ec7 100644 --- a/jni/java/app/opendocument/core/Document.java +++ b/jni/java/app/opendocument/core/Document.java @@ -57,6 +57,63 @@ public void edit(String diff) { editNative(handle(), diff); } + /** The element {@link Element#identifier()} handed out, or {@code null}. */ + public Element elementById(long identifier) { + long h = elementByIdNative(handle(), identifier); + return h == 0 ? null : new Element(h, this); + } + + /** + * Removes an element and its subtree; its identifier stays taken. + * + *

The structural edits below all throw where the engine cannot write, and + * for an element of another document. + */ + public void remove(Element element) { + removeNative(handle(), element.handle()); + } + + /** A run before another, in the same parent, so it takes the same style. */ + public Text insertTextBefore(Text anchor, String text) { + return new Text(insertTextBeforeNative(handle(), anchor.handle(), text), this); + } + + /** A run after another, in the same parent. */ + public Text insertTextAfter(Text anchor, String text) { + return new Text(insertTextAfterNative(handle(), anchor.handle(), text), this); + } + + /** A run as the last child of an element. */ + public Text appendText(Element parent, String text) { + return new Text(appendTextNative(handle(), parent.handle(), text), this); + } + + /** Splits before every child of the paragraph. */ + public Paragraph splitParagraph(Paragraph paragraph) { + return splitParagraph(paragraph, null); + } + + /** + * Splits a paragraph after {@code after}, one of its descendants, into a new + * paragraph of the same style. A {@code null} {@code after} moves every child. + */ + public Paragraph splitParagraph(Paragraph paragraph, Element after) { + long handle = + splitParagraphNative( + handle(), paragraph.handle(), after == null ? 0 : after.handle()); + return new Paragraph(handle, this); + } + + /** Takes the children of the paragraph after this one, which then goes. */ + public void mergeParagraphWithNext(Paragraph paragraph) { + mergeParagraphWithNextNative(handle(), paragraph.handle()); + } + + /** An empty paragraph after this one, of the same style. */ + public Paragraph insertParagraphAfter(Paragraph paragraph) { + return new Paragraph(insertParagraphAfterNative(handle(), paragraph.handle()), this); + } + private static native void destroy(long handle); private native void editNative(long handle, String diff); @@ -80,4 +137,20 @@ public void edit(String diff) { private native long rootElementNative(long handle); private native long asFilesystemNative(long handle); + + private native long elementByIdNative(long handle, long identifier); + + private native void removeNative(long handle, long elementHandle); + + private native long insertTextBeforeNative(long handle, long anchorHandle, String text); + + private native long insertTextAfterNative(long handle, long anchorHandle, String text); + + private native long appendTextNative(long handle, long parentHandle, String text); + + private native long splitParagraphNative(long handle, long paragraphHandle, long afterHandle); + + private native void mergeParagraphWithNextNative(long handle, long paragraphHandle); + + private native long insertParagraphAfterNative(long handle, long paragraphHandle); } diff --git a/jni/java/app/opendocument/core/MarkdownFile.java b/jni/java/app/opendocument/core/MarkdownFile.java new file mode 100644 index 000000000..a471ae5b4 --- /dev/null +++ b/jni/java/app/opendocument/core/MarkdownFile.java @@ -0,0 +1,22 @@ +package app.opendocument.core; + +/** A decoded markdown file. Mirrors {@code odr::MarkdownFile}. */ +public final class MarkdownFile extends DecodedFile { + MarkdownFile(long handle) { + super(handle); + } + + /** The markdown as a text document. */ + public Document document() { + return new Document(documentNative(handle())); + } + + /** The same bytes as plain text, so reading them needs no reopening. */ + public TextFile textFile() { + return new TextFile(textFileNative(handle())); + } + + private native long documentNative(long handle); + + private native long textFileNative(long handle); +} diff --git a/jni/java/app/opendocument/core/TextFile.java b/jni/java/app/opendocument/core/TextFile.java index 3dc25b2b4..fb04cab90 100644 --- a/jni/java/app/opendocument/core/TextFile.java +++ b/jni/java/app/opendocument/core/TextFile.java @@ -26,7 +26,27 @@ public String text() { return textNative(handle()); } + /** + * False where the file type is one this library does not write, or the + * encoding cannot be decoded. + */ + public boolean isSavable() { + return isSavableNative(handle()); + } + + /** + * Applies the operations and returns the result, as UTF-8 whatever the source + * encoding was. + */ + public byte[] writeEdited(String operations) { + return writeEditedNative(handle(), operations); + } + private native int encodingNative(long handle); + private native boolean isSavableNative(long handle); + + private native byte[] writeEditedNative(long handle, String operations); + private native String textNative(long handle); } diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 2a7a99b9b..f54cff102 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -150,6 +150,93 @@ Java_app_opendocument_core_Document_rootElementNative(JNIEnv *env, jobject, }); } +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_elementByIdNative(JNIEnv *env, jobject, + jlong handle, + jlong identifier) { + return guarded(env, [&] { + return wrap_element(from_handle(handle)->element_by_id( + static_cast(identifier))); + }); +} + +// Structural edits. A zero element handle is the element that does not exist, +// which `splitParagraph` takes to mean "move every child". + +extern "C" JNIEXPORT void JNICALL +Java_app_opendocument_core_Document_removeNative(JNIEnv *env, jobject, + jlong handle, + jlong element_handle) { + guarded(env, [&] { + from_handle(handle)->remove(element(element_handle)); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_insertTextBeforeNative(JNIEnv *env, jobject, + jlong handle, + jlong anchor_handle, + jstring text) { + return guarded(env, [&] { + return wrap_element(from_handle(handle)->insert_text_before( + element(anchor_handle).as_text(), to_string(env, text))); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_insertTextAfterNative(JNIEnv *env, jobject, + jlong handle, + jlong anchor_handle, + jstring text) { + return guarded(env, [&] { + return wrap_element(from_handle(handle)->insert_text_after( + element(anchor_handle).as_text(), to_string(env, text))); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_appendTextNative(JNIEnv *env, jobject, + jlong handle, + jlong parent_handle, + jstring text) { + return guarded(env, [&] { + return wrap_element(from_handle(handle)->append_text( + element(parent_handle), to_string(env, text))); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_splitParagraphNative(JNIEnv *env, jobject, + jlong handle, + jlong paragraph_handle, + jlong after_handle) { + return guarded(env, [&] { + const odr::Element after = + after_handle == 0 ? odr::Element() : element(after_handle); + return wrap_element(from_handle(handle)->split_paragraph( + element(paragraph_handle).as_paragraph(), after)); + }); +} + +extern "C" JNIEXPORT void JNICALL +Java_app_opendocument_core_Document_mergeParagraphWithNextNative( + JNIEnv *env, jobject, jlong handle, jlong paragraph_handle) { + guarded(env, [&] { + from_handle(handle)->merge_paragraph_with_next( + element(paragraph_handle).as_paragraph()); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Document_insertParagraphAfterNative( + JNIEnv *env, jobject, jlong handle, jlong paragraph_handle) { + return guarded(env, [&] { + return wrap_element( + from_handle(handle)->insert_paragraph_after( + element(paragraph_handle).as_paragraph())); + }); +} + extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Document_asFilesystemNative(JNIEnv *env, jobject, jlong handle) { diff --git a/jni/src/jni_file.cpp b/jni/src/jni_file.cpp index 8a7f36403..5c8dd89d1 100644 --- a/jni/src/jni_file.cpp +++ b/jni/src/jni_file.cpp @@ -185,6 +185,23 @@ Java_app_opendocument_core_DecodedFile_isTextFileNative(JNIEnv *env, jobject, }); } +extern "C" JNIEXPORT jboolean JNICALL +Java_app_opendocument_core_DecodedFile_isCsvFileNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return static_cast(decoded(handle).is_csv_file()); + }); +} + +extern "C" JNIEXPORT jboolean JNICALL +Java_app_opendocument_core_DecodedFile_isMarkdownFileNative(JNIEnv *env, + jobject, + jlong handle) { + return guarded(env, [&] { + return static_cast(decoded(handle).is_markdown_file()); + }); +} + extern "C" JNIEXPORT jboolean JNICALL Java_app_opendocument_core_DecodedFile_isImageFileNative(JNIEnv *env, jobject, jlong handle) { @@ -234,6 +251,61 @@ Java_app_opendocument_core_DecodedFile_asTextFileNative(JNIEnv *env, jobject, }); } +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_DecodedFile_asCsvFileNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle(odr::DecodedFile(decoded(handle).as_csv_file())); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_DecodedFile_asMarkdownFileNative(JNIEnv *env, + jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle(odr::DecodedFile(decoded(handle).as_markdown_file())); + }); +} + +// app.opendocument.core.CsvFile + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_CsvFile_documentNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle(decoded(handle).as_csv_file().document()); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_CsvFile_textFileNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle( + odr::DecodedFile(decoded(handle).as_csv_file().text_file())); + }); +} + +// app.opendocument.core.MarkdownFile + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_MarkdownFile_documentNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle(decoded(handle).as_markdown_file().document()); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_MarkdownFile_textFileNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return make_handle( + odr::DecodedFile(decoded(handle).as_markdown_file().text_file())); + }); +} + extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_DecodedFile_asImageFileNative(JNIEnv *env, jobject, jlong handle) { @@ -293,6 +365,26 @@ Java_app_opendocument_core_TextFile_textNative(JNIEnv *env, jobject, }); } +extern "C" JNIEXPORT jboolean JNICALL +Java_app_opendocument_core_TextFile_isSavableNative(JNIEnv *env, jobject, + jlong handle) { + return guarded(env, [&] { + return static_cast(decoded(handle).as_text_file().is_savable()); + }); +} + +extern "C" JNIEXPORT jbyteArray JNICALL +Java_app_opendocument_core_TextFile_writeEditedNative(JNIEnv *env, jobject, + jlong handle, + jstring operations) { + return guarded(env, [&] { + std::ostringstream out; + decoded(handle).as_text_file().write_edited(to_string(env, operations), + out); + return to_jbytes(env, out.str()); + }); +} + // app.opendocument.core.ImageFile extern "C" JNIEXPORT jbyteArray JNICALL diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index f17f0715e..cb510a82c 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -1,7 +1,9 @@ package app.opendocument.core; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; @@ -124,4 +126,64 @@ void saveToMemoryRoundTripsAnEdit() throws IOException { assertTrue(walkText(reloaded.rootElement()).contains("saved to memory")); } + + @Test + void elementByIdResolvesWhatIdentifierHandedOut() throws IOException { + Document document = openDocument(); + Element run = document.rootElement().firstChild().firstChild(); + + Element found = document.elementById(run.identifier()); + + assertNotNull(found); + assertEquals(run.asText().content(), found.asText().content()); + assertNull(document.elementById(999999)); + } + + @Test + void structuralEditsBuildADocumentInProcess() throws IOException { + Document document = openDocument(); + Paragraph first = document.rootElement().firstChild().asParagraph(); + Text run = first.firstChild().asText(); + + document.insertTextBefore(run, "before "); + document.insertTextAfter(run, " after"); + + Paragraph added = document.insertParagraphAfter(first); + document.appendText(added, "a new paragraph"); + + List text = walkText(document.rootElement()); + assertTrue(text.contains("before ")); + assertTrue(text.contains(" after")); + assertTrue(text.contains("a new paragraph")); + } + + @Test + void removeTakesTheElementOut() throws IOException { + Document document = openDocument(); + Element run = document.rootElement().firstChild().firstChild(); + + document.remove(run); + + // The fixture repeats its runs, so what proves the removal is the count. + List text = walkText(document.rootElement()); + assertEquals(TestFiles.ODT_TEXT.size() - 1, text.size()); + assertEquals(TestFiles.ODT_TEXT.subList(1, TestFiles.ODT_TEXT.size()), text); + } + + @Test + void splitAndMergeAreInverse() throws IOException { + Document document = openDocument(); + Paragraph first = document.rootElement().firstChild().asParagraph(); + Text run = first.firstChild().asText(); + + document.splitParagraph(first, run); + document.mergeParagraphWithNext(first); + + byte[] saved = document.saveToMemory(); + Path path = tempDir.resolve("split.odt"); + Files.write(path, saved); + Document reloaded = Odr.open(path.toString()).asDocumentFile().document(); + + assertTrue(walkText(reloaded.rootElement()).contains(TestFiles.ODT_TEXT.get(0))); + } } diff --git a/jni/tests/app/opendocument/core/FileTest.java b/jni/tests/app/opendocument/core/FileTest.java index f30f0c4a4..4f506770f 100644 --- a/jni/tests/app/opendocument/core/FileTest.java +++ b/jni/tests/app/opendocument/core/FileTest.java @@ -58,6 +58,28 @@ void openCsv() throws IOException { Path csv = TestFiles.csvFile(tempDir); try (DecodedFile file = Odr.open(csv.toString())) { assertEquals(FileType.COMMA_SEPARATED_VALUES, file.fileType()); + // A csv holds a text file rather than being one; both views stay open. + assertFalse(file.isTextFile()); + assertTrue(file.isCsvFile()); + + CsvFile decodedCsv = file.asCsvFile(); + assertTrue(decodedCsv.textFile().text().startsWith("name,")); + assertNotNull(decodedCsv.document().rootElement()); + } + } + + @Test + void textFileWritesAnEditBack() throws IOException { + Path txt = TestFiles.txtFile(tempDir); + try (DecodedFile file = Odr.open(txt.toString())) { + TextFile text = file.asTextFile(); + assertTrue(text.isSavable()); + + byte[] edited = + text.writeEdited( + "{\"version\":2,\"ops\":[{\"op\":\"setContent\",\"text\":\"rewritten\"}]}"); + + assertEquals("rewritten", new String(edited, java.nio.charset.StandardCharsets.UTF_8)); } } diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index dd7839e0f..7526de280 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -325,6 +325,33 @@ void odr_python::bind_document(py::module_ &m) { "Apply the operations our browser-side editor produces.") .def("element_by_id", &odr::Document::element_by_id, py::arg("identifier"), keep_self_alive) + // Structural edits. Each raises for an engine that cannot write, and for + // an element of another document. + .def("remove", &odr::Document::remove, py::arg("element"), + "Remove an element and its subtree.") + .def("insert_text_before", &odr::Document::insert_text_before, + py::arg("anchor"), py::arg("text"), keep_self_alive, + "A run before another, in the same parent, so it takes the same " + "style.") + .def("insert_text_after", &odr::Document::insert_text_after, + py::arg("anchor"), py::arg("text"), keep_self_alive, + "A run after another, in the same parent.") + .def("append_text", &odr::Document::append_text, py::arg("parent"), + py::arg("text"), keep_self_alive, + "A run as the last child of an element.") + .def("split_paragraph", &odr::Document::split_paragraph, + py::arg("paragraph"), py::arg("after") = odr::Element(), + keep_self_alive, + "Split a paragraph after one of its descendants into a new " + "paragraph of the same style. The default splits before every " + "child.") + .def("merge_paragraph_with_next", + &odr::Document::merge_paragraph_with_next, py::arg("paragraph"), + "Take the children of the paragraph after this one, which then " + "goes.") + .def("insert_paragraph_after", &odr::Document::insert_paragraph_after, + py::arg("paragraph"), keep_self_alive, + "An empty paragraph after this one, of the same style.") .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_file.cpp b/python/src/bind_file.cpp index 3d1826927..1bb02647a 100644 --- a/python/src/bind_file.cpp +++ b/python/src/bind_file.cpp @@ -273,6 +273,7 @@ void odr_python::bind_file(py::module_ &m) { .def("capabilities", &odr::DecodedFile::capabilities) .def("is_text_file", &odr::DecodedFile::is_text_file) .def("is_csv_file", &odr::DecodedFile::is_csv_file) + .def("is_markdown_file", &odr::DecodedFile::is_markdown_file) .def("is_image_file", &odr::DecodedFile::is_image_file) .def("is_archive_file", &odr::DecodedFile::is_archive_file) .def("is_document_file", &odr::DecodedFile::is_document_file) @@ -280,19 +281,26 @@ void odr_python::bind_file(py::module_ &m) { .def("is_font_file", &odr::DecodedFile::is_font_file) .def("as_text_file", &odr::DecodedFile::as_text_file) .def("as_csv_file", &odr::DecodedFile::as_csv_file) + .def("as_markdown_file", &odr::DecodedFile::as_markdown_file) .def("as_image_file", &odr::DecodedFile::as_image_file) .def("as_archive_file", &odr::DecodedFile::as_archive_file) .def("as_document_file", &odr::DecodedFile::as_document_file) .def("as_pdf_file", &odr::DecodedFile::as_pdf_file) .def("as_font_file", &odr::DecodedFile::as_font_file); - // A csv is a text file too, so `CsvFile` derives from `TextFile` the way the - // C++ handle does - `text()` still reads the raw bytes. py::class_(m, "CsvFile") .def("options", &odr::CsvFile::options, "The options in use, every field resolved.") .def("document", &odr::CsvFile::document, - "The csv as a one-sheet spreadsheet."); + "The csv as a one-sheet spreadsheet.") + .def("text_file", &odr::CsvFile::text_file, + "The same bytes as plain text."); + + py::class_(m, "MarkdownFile") + .def("document", &odr::MarkdownFile::document, + "The markdown as a text document.") + .def("text_file", &odr::MarkdownFile::text_file, + "The same bytes as plain text."); py::class_(m, "TextFile") .def("encoding", &odr::TextFile::encoding, @@ -305,7 +313,23 @@ void odr_python::bind_file(py::module_ &m) { } return std::string(odr::text_encoding_to_string(encoding)); }) - .def("text", &odr::TextFile::text); + .def("text", &odr::TextFile::text) + .def("is_savable", &odr::TextFile::is_savable, + "False where the file type is one this library does not write, or " + "the encoding cannot be decoded.") + .def( + "write_edited", + [](const odr::TextFile &file, const std::string &operations) { + std::ostringstream out; + { + py::gil_scoped_release release; + file.write_edited(operations, out); + } + return py::bytes(out.str()); + }, + py::arg("operations"), + "Apply the operations and return the result, as UTF-8 whatever the " + "source encoding was."); py::class_(m, "ImageFile") .def("read", [](const odr::ImageFile &file) { diff --git a/python/tests/test_document.py b/python/tests/test_document.py index 6b2359d02..460aba2c6 100644 --- a/python/tests/test_document.py +++ b/python/tests/test_document.py @@ -148,3 +148,52 @@ def test_save_to_memory_carries_an_edit(odt_path, tmp_path): reloaded = pyodr.open(str(path)).as_document_file().document() assert "edited in python" in walk_text(reloaded.root_element()) + + +def test_structural_edits_build_a_document_in_process(odt_path, tmp_path): + document = pyodr.open(str(odt_path)).as_document_file().document() + body = document.root_element().first_child() + first = body.as_paragraph() + run = first.first_child().as_text() + + document.insert_text_before(run, "before ") + document.insert_text_after(run, " after") + + added = document.insert_paragraph_after(first) + document.append_text(added, "a new paragraph") + + path = tmp_path / "structural.odt" + path.write_bytes(document.save_to_memory()) + text = walk_text(pyodr.open(str(path)).as_document_file().document().root_element()) + + assert "before Hello from pyodr! after" in text + assert "a new paragraph" in text + + +def test_remove_takes_the_element_out(odt_path, tmp_path): + document = pyodr.open(str(odt_path)).as_document_file().document() + run = document.root_element().first_child().first_child().as_text() + + document.remove(run) + + path = tmp_path / "removed.odt" + path.write_bytes(document.save_to_memory()) + text = walk_text(pyodr.open(str(path)).as_document_file().document().root_element()) + + assert "Hello from pyodr!" not in text + assert "Second paragraph" in text + + +def test_split_and_merge_are_inverse(odt_path, tmp_path): + document = pyodr.open(str(odt_path)).as_document_file().document() + first = document.root_element().first_child().as_paragraph() + run = first.first_child().as_text() + + document.split_paragraph(first, run) + document.merge_paragraph_with_next(first) + + path = tmp_path / "split.odt" + path.write_bytes(document.save_to_memory()) + text = walk_text(pyodr.open(str(path)).as_document_file().document().root_element()) + + assert "Hello from pyodr!" in text diff --git a/python/tests/test_file.py b/python/tests/test_file.py index 44de5255e..e614c3aa8 100644 --- a/python/tests/test_file.py +++ b/python/tests/test_file.py @@ -239,3 +239,31 @@ def test_file_and_path_entry_points_agree(odt_path): pyodr.open(file).as_document_file().file_type() == pyodr.open(path).as_document_file().file_type() ) + + +def test_text_file_writes_an_edit_back(txt_path): + text_file = pyodr.open(str(txt_path)).as_text_file() + assert text_file.is_savable() + + edited = text_file.write_edited( + '{"version":2,"ops":[{"op":"setContent","text":"rewritten\\n"}]}' + ) + assert edited == b"rewritten\n" + + +def test_a_csv_holds_a_text_file_rather_than_being_one(csv_path): + file = pyodr.open(str(csv_path)) + assert not file.is_text_file() + assert file.is_csv_file() + assert file.as_csv_file().text_file().text().startswith("name,value") + + +def test_a_markdown_file_holds_a_text_file_too(tmp_path): + path = tmp_path / "note.md" + path.write_text("# hello\n") + + file = pyodr.open(str(path)) + assert not file.is_text_file() + assert file.is_markdown_file() + assert file.as_markdown_file().text_file().text() == "# hello\n" + assert file.as_markdown_file().document().document_type() == pyodr.DocumentType.text diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 6ac8129ef..16bb32d8b 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -640,8 +640,10 @@ ShapeType Frame::shape_type() const { } AnchorType Frame::anchor_type() const { + // `AnchorType` has no neutral value, so a frame that does not exist answers + // the commonest one. Ask @ref Element::operator bool to tell the two apart. return exists_() ? m_adapter2->frame_anchor_type(m_identifier) - : AnchorType::as_char; // TODO default? + : AnchorType::as_char; } std::optional Frame::x() const { diff --git a/src/odr/file.cpp b/src/odr/file.cpp index eedb3cfb7..195ef93e6 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -331,6 +331,8 @@ CsvFile::CsvFile(std::shared_ptr impl) Document CsvFile::document() const { return Document(m_impl->document()); } +TextFile CsvFile::text_file() const { return TextFile(m_impl->text_file()); } + CsvOptions CsvFile::options() const { return m_impl->options(); } std::shared_ptr CsvFile::impl() const { @@ -343,6 +345,10 @@ MarkdownFile::MarkdownFile( Document MarkdownFile::document() const { return Document(m_impl->document()); } +TextFile MarkdownFile::text_file() const { + return TextFile(m_impl->text_file()); +} + std::shared_ptr MarkdownFile::impl() const { return m_impl; } diff --git a/src/odr/file.hpp b/src/odr/file.hpp index d425d69c6..f0eeb75ac 100644 --- a/src/odr/file.hpp +++ b/src/odr/file.hpp @@ -370,7 +370,8 @@ class File final { void pipe(std::ostream &out) const; void copy(const std::string &path) const; - // TODO `impl()` might be a bit dirty + /// The internal file this wraps. The bindings need it to hand one wrapper's + /// value to another; every public wrapper here offers the same. [[nodiscard]] std::shared_ptr impl() const; protected: @@ -460,11 +461,14 @@ class CsvFile final : public DecodedFile { public: explicit CsvFile(std::shared_ptr); - /// The csv as a one-sheet spreadsheet. The other view of the same bytes — a - /// csv stays a text file, so @ref TextFile::text still works. + /// The csv as a one-sheet spreadsheet. /// @throws UnsupportedTextEncoding if the encoding cannot be decoded. [[nodiscard]] Document document() const; + /// The same bytes as plain text. A csv holds a text file rather than being + /// one, so @ref DecodedFile::is_text_file is false for it. + [[nodiscard]] TextFile text_file() const; + /// The options in use, every field resolved. [[nodiscard]] CsvOptions options() const; @@ -479,11 +483,13 @@ class MarkdownFile final : public DecodedFile { public: explicit MarkdownFile(std::shared_ptr); - /// The markdown as a text document. The other view of the same bytes — - /// markdown stays a text file, so @ref TextFile::text still works. + /// The markdown as a text document. /// @throws UnsupportedTextEncoding if the encoding cannot be decoded. [[nodiscard]] Document document() const; + /// The same bytes as plain text, as @ref CsvFile::text_file is. + [[nodiscard]] TextFile text_file() const; + [[nodiscard]] std::shared_ptr impl() const; private: diff --git a/src/odr/html.cpp b/src/odr/html.cpp index 3365d569d..0c67e574f 100644 --- a/src/odr/html.cpp +++ b/src/odr/html.cpp @@ -255,16 +255,14 @@ HtmlService translate_font_file(const FontFile &font_file, HtmlService html::translate(const DecodedFile &file, const HtmlConfig &config, const Logger &logger) { - // before the text branch: a csv is a text file, and rendering one as a line - // list rather than a table is never what a viewer wants if (file.is_csv_file()) { return translate(file.as_csv_file().document(), config, logger); } - // markdown too — its point is the prose it parses to if (file.is_markdown_file()) { return translate(file.as_markdown_file().document(), config, logger); } - // and before it for the same reason; open as `text_file` for the line list + // an xml file *is* a text file, so this has to come first; open as + // `text_file` for the plain line list if (file.file_type() == FileType::xml) { return internal::html::create_xml_service(file.as_text_file(), config, logger); diff --git a/src/odr/html.hpp b/src/odr/html.hpp index ff85c9070..ecd05b135 100644 --- a/src/odr/html.hpp +++ b/src/odr/html.hpp @@ -39,6 +39,8 @@ enum class HtmlResourceType { file, }; +/// One file a rendered view needs: a stylesheet, a script, an image, a +/// font. @ref HtmlConfig decides which are embedded and which are linked. class HtmlResource final { public: HtmlResource(); @@ -242,6 +244,9 @@ struct HtmlSheetCut final { TableDimensions rendered; }; +/// One page of a rendering: a slide, a sheet, a pdf page, or the whole of +/// a text document. @ref write_html emits it and names the resources it +/// needs. class HtmlView final { public: HtmlView(); @@ -269,6 +274,9 @@ class HtmlView final { using HtmlViews = std::vector; +/// A whole document, rendered. Its @ref list_views are the pages, and it +/// serves them and their resources by path — what @ref odr::HttpServer +/// puts on a socket. class HtmlService final { public: HtmlService(); diff --git a/src/odr/internal/abstract/file.hpp b/src/odr/internal/abstract/file.hpp index c731d075b..0d786bb29 100644 --- a/src/odr/internal/abstract/file.hpp +++ b/src/odr/internal/abstract/file.hpp @@ -107,11 +107,18 @@ class DocumentFile : public DecodedFile { [[nodiscard]] virtual std::shared_ptr document() const = 0; }; -/// A csv is a text file that can also be loaded as a document — a one-sheet -/// spreadsheet. It stays in @ref FileCategory::text, so reading it as text -/// needs no reopening; @ref document is the other view of the same bytes. -class CsvFile : public TextFile { +/// A csv holds a text file rather than being one: @ref document is what a +/// caller wants from it, and the plain-text view is @ref text_file. It stays +/// in @ref FileCategory::text, so the bytes are still bytes of text. +class CsvFile : public DecodedFile { public: + [[nodiscard]] FileCategory file_category() const noexcept final { + return FileCategory::text; + } + + /// The same bytes as plain text, so reading them needs no reopening. + [[nodiscard]] virtual std::shared_ptr text_file() const = 0; + /// The options in use, every field resolved. [[nodiscard]] virtual CsvOptions options() const = 0; @@ -123,11 +130,17 @@ class CsvFile : public TextFile { [[nodiscard]] virtual std::shared_ptr document() const = 0; }; -/// A markdown file is a text file that can also be loaded as a document — its -/// prose, parsed. Like @ref CsvFile it stays in @ref FileCategory::text; @ref -/// document is the other view of the same bytes. -class MarkdownFile : public TextFile { +/// Markdown holds a text file the same way @ref CsvFile does; @ref document is +/// its prose, parsed. +class MarkdownFile : public DecodedFile { public: + [[nodiscard]] FileCategory file_category() const noexcept final { + return FileCategory::text; + } + + /// The same bytes as plain text, so reading them needs no reopening. + [[nodiscard]] virtual std::shared_ptr text_file() const = 0; + /// The markdown as a text document. [[nodiscard]] virtual std::shared_ptr document() const = 0; }; diff --git a/src/odr/internal/abstract/filesystem.hpp b/src/odr/internal/abstract/filesystem.hpp index 0d5bb899a..d7e3ba3df 100644 --- a/src/odr/internal/abstract/filesystem.hpp +++ b/src/odr/internal/abstract/filesystem.hpp @@ -19,7 +19,8 @@ class FileWalker { [[nodiscard]] virtual bool end() const = 0; [[nodiscard]] virtual std::uint32_t depth() const = 0; - // TODO by reference? + /// By value: a walker builds the path from where it stands rather than + /// holding one. [[nodiscard]] virtual AbsPath path() const = 0; [[nodiscard]] virtual bool is_file() const = 0; [[nodiscard]] virtual bool is_directory() const = 0; diff --git a/src/odr/internal/common/filesystem.cpp b/src/odr/internal/common/filesystem.cpp index f28ef3b02..350d0839f 100644 --- a/src/odr/internal/common/filesystem.cpp +++ b/src/odr/internal/common/filesystem.cpp @@ -28,8 +28,10 @@ class SystemFileWalker final : public abstract::FileWalker { return std::make_unique(*this); } + /// TODO always false: two walkers at the same place do not compare equal. + /// Nothing calls this yet, so nothing depends on the answer. [[nodiscard]] bool equals(const FileWalker & /*rhs*/) const override { - return false; // TODO + return false; } [[nodiscard]] bool end() const override { diff --git a/src/odr/internal/csv/AGENTS.md b/src/odr/internal/csv/AGENTS.md index 131e68d73..338b0a2d8 100644 --- a/src/odr/internal/csv/AGENTS.md +++ b/src/odr/internal/csv/AGENTS.md @@ -52,18 +52,18 @@ reads it. `NoCsvFile` is a detection failure only. An incoherent dialect — a separator equal to the quote, a line break as a separator — is `std::invalid_argument`, a caller mistake rather than bad input. -## A csv is a text file that also loads as a document - -`FileCategory::text`, `DocumentType::spreadsheet` — so `is_text_file()` is true -for a csv and `is_document_file()` is false. `abstract::CsvFile` derives from -`abstract::TextFile`, and `CsvFile::document()` is the *other* view of the same -bytes rather than the only one: `TextFile::text()` keeps working, so reading a -csv as text needs no reopening. Opening it as `FileType::text_file` stays the -escape hatch when detection was wrong about it being a csv at all. - -The one thing that costs: `html::translate` has to test `is_csv_file()` ahead of -its text branch (`html.cpp:215`), because a csv answers `is_text_file()` and a -line list is never what a viewer wants from a table. +## A csv holds a text file and also loads as a document + +`FileCategory::text`, `DocumentType::spreadsheet` — so `is_text_file()` and +`is_document_file()` are both false for a csv. `abstract::CsvFile` derives from +`abstract::DecodedFile` and *holds* a `text::TextFile`, which +`CsvFile::text_file()` hands out, so reading a csv as text needs no reopening. +`CsvFile::document()` is the other view of the same bytes. + +Composition rather than inheritance, because a `TextFile` is the thing this +library edits and writes back as plain text, and a csv is not that. Opening it +as `FileType::text_file` stays the escape hatch when detection was wrong about +it being a csv at all. Text has to be UTF-8 by the time it reaches a cell: `Text::content()` returns `std::string` and every binding treats it as UTF-8. That is why an encoding diff --git a/src/odr/internal/csv/PLAN.md b/src/odr/internal/csv/PLAN.md index b84aeb1d4..2ceaf4a2c 100644 --- a/src/odr/internal/csv/PLAN.md +++ b/src/odr/internal/csv/PLAN.md @@ -237,10 +237,9 @@ output at any file size worth caring about today. files. - csv **stays** `FileCategory::text` (`file_type_table.cpp:416`), carrying `DocumentType::spreadsheet`. Moving it to `document` was the plan and is not - what landed: a csv is still text, `TextFile::text()` still reads it, and - `document()` is the second view rather than a replacement. So `is_text_file()` - keeps answering true and `html::translate` orders its csv branch first. The - capabilities test enforces whatever the row claims. + what landed: the bytes are still text. But a csv *holds* a `TextFile` instead + of being one, so `is_text_file()` answers false and `CsvFile::text_file()` is + the second view. The capabilities test enforces whatever the row claims. - `html_output_test` skips csv (`// TODO enable zip, csv, json`). Csv now produces real output, so enabling it needs reference output committed to the output repo and the pointer advanced — a separate repo, a separate change. diff --git a/src/odr/internal/csv/csv_file.cpp b/src/odr/internal/csv/csv_file.cpp index 5ee04b87f..5a9f8a897 100644 --- a/src/odr/internal/csv/csv_file.cpp +++ b/src/odr/internal/csv/csv_file.cpp @@ -78,21 +78,23 @@ FileMeta CsvFile::file_meta() const noexcept { } bool CsvFile::is_decodable() const noexcept { - return text_encoding_is_decodable(encoding()); + return text_encoding_is_decodable(m_file->encoding()); } std::shared_ptr CsvFile::document() const { if (!is_decodable()) { - throw UnsupportedTextEncoding(encoding()); + throw UnsupportedTextEncoding(m_file->encoding()); } - return std::make_shared(*m_file->file(), encoding(), m_dialect, - m_separator_directive); + return std::make_shared(*m_file->file(), m_file->encoding(), + m_dialect, m_separator_directive); } -TextEncoding CsvFile::encoding() const noexcept { return m_file->encoding(); } +std::shared_ptr CsvFile::text_file() const { + return m_file; +} CsvOptions CsvFile::options() const { - return {.encoding = encoding(), + return {.encoding = m_file->encoding(), .separator = m_dialect.separator, .quote = m_dialect.quote}; } diff --git a/src/odr/internal/csv/csv_file.hpp b/src/odr/internal/csv/csv_file.hpp index ef9354613..48ae268d0 100644 --- a/src/odr/internal/csv/csv_file.hpp +++ b/src/odr/internal/csv/csv_file.hpp @@ -28,9 +28,9 @@ class CsvFile final : public abstract::CsvFile { [[nodiscard]] bool is_decodable() const noexcept override; - [[nodiscard]] std::shared_ptr document() const override; + [[nodiscard]] std::shared_ptr text_file() const override; - [[nodiscard]] TextEncoding encoding() const noexcept override; + [[nodiscard]] std::shared_ptr document() const override; [[nodiscard]] CsvOptions options() const override; [[nodiscard]] std::shared_ptr diff --git a/src/odr/internal/markdown/AGENTS.md b/src/odr/internal/markdown/AGENTS.md index 75069efa5..ee217dc5f 100644 --- a/src/odr/internal/markdown/AGENTS.md +++ b/src/odr/internal/markdown/AGENTS.md @@ -4,13 +4,17 @@ Read the root [`AGENTS.md`](../../../../AGENTS.md) first, and [`PLAN.md`](PLAN.md) for where this is going. This file covers what markdown does differently, and why. -## A text file that also loads as a document +## A file that holds a text file and also loads as a document `FileCategory::text` / `DocumentType::text` — the shape `abstract::CsvFile` -already has. Markdown is plain text by construction, so the file stays text and -the document is the other view of the same bytes: `as_text_file().text()` is -the source, `as_markdown_file().document()` the prose. `html::translate` takes -the document view, as it does for a csv. +already has. `abstract::MarkdownFile` derives from `abstract::DecodedFile` and +*holds* a `text::TextFile`, so `is_text_file()` is false for a `.md`: +`as_markdown_file().text_file().text()` is the source and +`as_markdown_file().document()` the prose. `html::translate` takes the document +view, as it does for a csv. + +Composition rather than inheritance, because a `TextFile` is the thing this +library edits and writes back as plain text, and markdown is not that. Decoding to a `TextRoot` is the whole argument for a decoder rather than a markdown→HTML renderer next to `html/text_file.cpp`: the latter would produce diff --git a/src/odr/internal/markdown/PLAN.md b/src/odr/internal/markdown/PLAN.md index 8f460cf65..824f10fe8 100644 --- a/src/odr/internal/markdown/PLAN.md +++ b/src/odr/internal/markdown/PLAN.md @@ -63,12 +63,10 @@ work in stage 2: - md4c parses bytes and assumes UTF-8 (`MD4C_USE_UTF8`), so decoding happens before it, not inside it. -**Markdown is a `DocumentFile`, not a `TextFile`.** `abstract::TextFile` fixes -`file_category()` to `text` (`abstract/file.hpp`); a document has to be -`FileCategory::document` with `DocumentType::text`. The table row changes -category with it. This is api-visible for `FileType::markdown` — and free, -because the row declares no capabilities today, so nothing can be relying on -it. +**Markdown is neither a `DocumentFile` nor a `TextFile`.** It keeps +`FileCategory::text` with `DocumentType::text`, derives from +`abstract::DecodedFile`, and holds a `text::TextFile` that +`MarkdownFile::text_file()` hands out. **Input is UTF-8, produced by `internal/encoding`.** `MarkdownFile` takes a `std::shared_ptr` exactly as `CsvFile` and `JsonFile` do diff --git a/src/odr/internal/markdown/markdown_file.cpp b/src/odr/internal/markdown/markdown_file.cpp index 33ad1de76..681a20d6e 100644 --- a/src/odr/internal/markdown/markdown_file.cpp +++ b/src/odr/internal/markdown/markdown_file.cpp @@ -32,21 +32,21 @@ FileMeta MarkdownFile::file_meta() const noexcept { } bool MarkdownFile::is_decodable() const noexcept { - return text_encoding_is_decodable(encoding()); + return text_encoding_is_decodable(m_file->encoding()); } std::shared_ptr MarkdownFile::document() const { // `Text::content()` is UTF-8 to every binding, so bytes we cannot decode // have no document at all — the text rendering path stays open to them. if (!is_decodable()) { - throw UnsupportedTextEncoding(encoding()); + throw UnsupportedTextEncoding(m_file->encoding()); } const std::string text = m_file->text(); return std::make_shared(text); } -TextEncoding MarkdownFile::encoding() const noexcept { - return m_file->encoding(); +std::shared_ptr MarkdownFile::text_file() const { + return m_file; } } // namespace odr::internal::markdown diff --git a/src/odr/internal/markdown/markdown_file.hpp b/src/odr/internal/markdown/markdown_file.hpp index 24ea40066..5120a4cd5 100644 --- a/src/odr/internal/markdown/markdown_file.hpp +++ b/src/odr/internal/markdown/markdown_file.hpp @@ -21,11 +21,11 @@ class MarkdownFile final : public abstract::MarkdownFile { [[nodiscard]] bool is_decodable() const noexcept override; + [[nodiscard]] std::shared_ptr text_file() const override; + /// @throws UnsupportedTextEncoding if the encoding cannot be decoded. [[nodiscard]] std::shared_ptr document() const override; - [[nodiscard]] TextEncoding encoding() const noexcept override; - private: std::shared_ptr m_file; }; diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index a2b65f4cc..2ca8c1934 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -336,7 +336,8 @@ void Style::resolve_text_style_(const StyleRegistry *registry, } if (const std::optional font_size = read_measure(text_properties.attribute("fo:font-size"))) { - // TODO + // A percentage is of the parent's size, resolved here rather than passed + // to css - the parent style is not on the element in the render. if (font_size->unit().name() != "%") { result.font_size = font_size; } else { @@ -413,35 +414,45 @@ void Style::resolve_paragraph_style_(const pugi::xml_node node, } if (const std::optional margin = read_measure(paragraph_properties.attribute("fo:margin"))) { - // TODO + // TODO a percentage margin is dropped. css takes `%` here with the same + // meaning, so passing it through would work - but it moves the reference + // output for every document that uses one. if (margin->unit().name() != "%") { result.margin = DirectionalStyle(margin); } } if (const std::optional margin_right = read_measure(paragraph_properties.attribute("fo:margin-right"))) { - // TODO + // TODO a percentage margin is dropped. css takes `%` here with the same + // meaning, so passing it through would work - but it moves the reference + // output for every document that uses one. if (margin_right->unit().name() != "%") { result.margin.right = margin_right; } } if (const std::optional margin_top = read_measure(paragraph_properties.attribute("fo:margin-top"))) { - // TODO + // TODO a percentage margin is dropped. css takes `%` here with the same + // meaning, so passing it through would work - but it moves the reference + // output for every document that uses one. if (margin_top->unit().name() != "%") { result.margin.top = margin_top; } } if (const std::optional margin_left = read_measure(paragraph_properties.attribute("fo:margin-left"))) { - // TODO + // TODO a percentage margin is dropped. css takes `%` here with the same + // meaning, so passing it through would work - but it moves the reference + // output for every document that uses one. if (margin_left->unit().name() != "%") { result.margin.left = margin_left; } } if (const std::optional margin_bottom = read_measure(paragraph_properties.attribute("fo:margin-bottom"))) { - // TODO + // TODO a percentage margin is dropped. css takes `%` here with the same + // meaning, so passing it through would work - but it moves the reference + // output for every document that uses one. if (margin_bottom->unit().name() != "%") { result.margin.bottom = margin_bottom; } diff --git a/src/odr/internal/oldms/presentation/ppt_document.cpp b/src/odr/internal/oldms/presentation/ppt_document.cpp index 07d6c5a80..5dc29f8f6 100644 --- a/src/odr/internal/oldms/presentation/ppt_document.cpp +++ b/src/odr/internal/oldms/presentation/ppt_document.cpp @@ -17,7 +17,7 @@ namespace odr::internal::oldms::presentation { namespace { std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry); } @@ -27,7 +27,7 @@ Document::Document(std::shared_ptr files) m_root_element = parse_tree(m_element_registry, m_style_registry, *m_files); m_element_adapter = - create_element_adapter(*this, m_element_registry, m_style_registry); + create_element_adapter(m_element_registry, m_style_registry); } ElementRegistry &Document::element_registry() { return m_element_registry; } @@ -49,10 +49,8 @@ using AdapterBase = internal::RegistryElementAdapter< class ElementAdapter final : public AdapterBase { public: - ElementAdapter(const Document &document, ElementRegistry ®istry, - const StyleRegistry &style_registry) - : AdapterBase(registry), m_document(&document), - m_style_registry(&style_registry) {} + ElementAdapter(ElementRegistry ®istry, const StyleRegistry &style_registry) + : AdapterBase(registry), m_style_registry(&style_registry) {} [[nodiscard]] PageLayout slide_page_layout( [[maybe_unused]] const ElementIdentifier element_id) const override { @@ -131,14 +129,17 @@ class ElementAdapter final : public AdapterBase { return {}; } + /// TODO the character run of a line break is not read. [[nodiscard]] TextStyle line_break_style( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } + /// TODO paragraph properties ([MS-PPT] `TextPFRun`) are not read: alignment, + /// indent and spacing all render as the default. [[nodiscard]] ParagraphStyle paragraph_style( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } [[nodiscard]] TextStyle paragraph_text_style(const ElementIdentifier element_id) const override { @@ -201,15 +202,13 @@ class ElementAdapter final : public AdapterBase { return Measure(select(*anchor) / master_units_per_inch, DynamicUnit("in")); } - [[maybe_unused]] - const Document *m_document{nullptr}; const StyleRegistry *m_style_registry{nullptr}; }; std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry) { - return std::make_unique(document, registry, style_registry); + return std::make_unique(registry, style_registry); } } // namespace diff --git a/src/odr/internal/oldms/spreadsheet/xls_document.cpp b/src/odr/internal/oldms/spreadsheet/xls_document.cpp index da70f814f..31f4a94b1 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_document.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_document.cpp @@ -13,7 +13,7 @@ namespace odr::internal::oldms::spreadsheet { namespace { std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry); } @@ -23,7 +23,7 @@ Document::Document(std::shared_ptr files) m_root_element = parse_tree(m_element_registry, m_style_registry, *m_files); m_element_adapter = - create_element_adapter(*this, m_element_registry, m_style_registry); + create_element_adapter(m_element_registry, m_style_registry); } ElementRegistry &Document::element_registry() { return m_element_registry; } @@ -44,10 +44,8 @@ using AdapterBase = internal::RegistryElementAdapter< class ElementAdapter final : public AdapterBase { public: - ElementAdapter(const Document &document, ElementRegistry ®istry, - const StyleRegistry &style_registry) - : AdapterBase(registry), m_document(&document), - m_style_registry(&style_registry) {} + ElementAdapter(ElementRegistry ®istry, const StyleRegistry &style_registry) + : AdapterBase(registry), m_style_registry(&style_registry) {} [[nodiscard]] std::string sheet_name(const ElementIdentifier element_id) const override { @@ -174,9 +172,6 @@ class ElementAdapter final : public AdapterBase { } private: - // TODO remove maybe_unused - [[maybe_unused]] - const Document *m_document{nullptr}; const StyleRegistry *m_style_registry{nullptr}; /// The font style of the sheet_cell ancestor (paragraph and text elements @@ -198,9 +193,9 @@ class ElementAdapter final : public AdapterBase { }; std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry) { - return std::make_unique(document, registry, style_registry); + return std::make_unique(registry, style_registry); } } // namespace diff --git a/src/odr/internal/oldms/text/doc_document.cpp b/src/odr/internal/oldms/text/doc_document.cpp index 25ee70758..a69758a84 100644 --- a/src/odr/internal/oldms/text/doc_document.cpp +++ b/src/odr/internal/oldms/text/doc_document.cpp @@ -12,7 +12,7 @@ namespace odr::internal::oldms::text { namespace { std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry); } @@ -22,7 +22,7 @@ Document::Document(std::shared_ptr files) m_root_element = parse_tree(m_element_registry, m_style_registry, *m_files); m_element_adapter = - create_element_adapter(*this, m_element_registry, m_style_registry); + create_element_adapter(m_element_registry, m_style_registry); } ElementRegistry &Document::element_registry() { return m_element_registry; } @@ -43,10 +43,8 @@ using AdapterBase = internal::RegistryElementAdapter< class ElementAdapter final : public AdapterBase { public: - ElementAdapter(const Document &document, ElementRegistry ®istry, - const StyleRegistry &style_registry) - : AdapterBase(registry), m_document(&document), - m_style_registry(&style_registry) {} + ElementAdapter(ElementRegistry ®istry, const StyleRegistry &style_registry) + : AdapterBase(registry), m_style_registry(&style_registry) {} [[nodiscard]] PageLayout text_root_page_layout( [[maybe_unused]] const ElementIdentifier element_id) const override { @@ -59,16 +57,19 @@ class ElementAdapter final : public AdapterBase { return {}; } + /// TODO the `PAP`/`CHP` of a line break is not read. [[nodiscard]] TextStyle line_break_style(const ElementIdentifier element_id) const override { (void)element_id; - return {}; // TODO + return {}; } + /// TODO paragraph properties ([MS-DOC] `PAPX`) are not read: alignment, + /// indent and spacing all render as the default. [[nodiscard]] ParagraphStyle paragraph_style(const ElementIdentifier element_id) const override { (void)element_id; - return {}; // TODO + return {}; } [[nodiscard]] TextStyle paragraph_text_style(const ElementIdentifier element_id) const override { @@ -98,9 +99,6 @@ class ElementAdapter final : public AdapterBase { } private: - // TODO remove maybe_unused - [[maybe_unused]] - const Document *m_document{nullptr}; const StyleRegistry *m_style_registry{nullptr}; /// The character style stored for a paragraph or span element. @@ -112,9 +110,9 @@ class ElementAdapter final : public AdapterBase { }; std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry, +create_element_adapter(ElementRegistry ®istry, const StyleRegistry &style_registry) { - return std::make_unique(document, registry, style_registry); + return std::make_unique(registry, style_registry); } } // namespace diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp index f15e7eb5f..fc40afbd6 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp @@ -212,9 +212,11 @@ class ElementAdapter final : public AdapterBase { slide_page_layout(const ElementIdentifier element_id) const override { return m_document->slide_page_layout(element_id); } + /// TODO the slide master is not parsed into the tree, so a slide reports + /// none. What the master paints is missing from the render with it. [[nodiscard]] ElementIdentifier slide_master_page( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } [[nodiscard]] std::string slide_name(const ElementIdentifier element_id) const override { @@ -334,9 +336,11 @@ class ElementAdapter final : public AdapterBase { return get_intermediate_style(element_id).text_style; } + /// TODO an `a:hlinkClick` relationship is not resolved, so a link in a + /// slide has no href. [[nodiscard]] std::string link_href( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } [[nodiscard]] std::string diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index a5e2a6229..befc12e6c 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -94,5 +94,6 @@ Coverage is in [`README.md`](README.md). Foundational gaps, roughly by value: borders rendered as `0.75pt solid` regardless of actual style (`// TODO thin only`); cell protection unhandled. 4. **Writing is one cell value.** `sheet_set_cell` writes a number or a string, - into a cell the file spells or one it states; `text_set_content` is still a - no-op stub. Links and comments/annotations not modelled. + into a cell the file spells or one it states; `text_set_content` throws + `UnsupportedOperation` — a run inside a cell is not writable. Links and + comments/annotations not modelled. diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index 69a51af3b..7bfc930d3 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -192,7 +192,9 @@ class ElementAdapter final : public AdapterBase { sheet_content(const ElementIdentifier element_id, [[maybe_unused]] const std::optional range) const override { - return sheet_dimensions(element_id); // TODO + // TODO the range is ignored: this answers the whole `` rather + // than trimming to the populated cells inside it. + return sheet_dimensions(element_id); } [[nodiscard]] ElementIdentifier sheet_cell(const ElementIdentifier element_id, const std::uint32_t column, @@ -271,9 +273,11 @@ class ElementAdapter final : public AdapterBase { } } + /// TODO a sheet carries no style of its own here; `sheetFormatPr` (default + /// row height and column width) is not read. [[nodiscard]] TableStyle sheet_style( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } [[nodiscard]] TableColumnStyle sheet_column_style(const ElementIdentifier element_id, @@ -398,19 +402,23 @@ class ElementAdapter final : public AdapterBase { } return result; } + /// A cell's value goes in through `sheet_set_cell`; a run inside one is not + /// writable. Refusing beats the silent no-op this was: the document declares + /// `edit`, so a caller has no other way to learn nothing happened. void text_set_content([[maybe_unused]] const ElementIdentifier element_id, [[maybe_unused]] const std::string &text) const override { - // TODO + throw UnsupportedOperation(); } [[nodiscard]] TextStyle text_style(const ElementIdentifier element_id) const override { return get_intermediate_style(element_id).text_style; } + /// TODO a `` is not modelled, so a link in a sheet has no href. [[nodiscard]] std::string link_href( [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + return {}; } [[nodiscard]] AnchorType frame_anchor_type( @@ -492,7 +500,8 @@ class ElementAdapter final : public AdapterBase { } } } - return ""; // TODO + // an unresolvable relationship leaves no href rather than a broken one + return ""; } private: diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index 129cd10dc..6f4cd2113 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -125,9 +125,10 @@ ResolvedStyle StyleRegistry::cell_style(const std::uint32_t i) const { } } + // TODO `protection` (locked/hidden) is read out and dropped; nothing in + // `TableCellStyle` carries it, and the render has no lock to show. if (const pugi::xml_node protection = cell_format.child("protection"); cell_format.attribute("applyProtection").as_bool() && protection) { - // TODO } return result; diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index e6a1587e5..902fe3b4c 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -503,7 +503,8 @@ class ElementAdapter final : public AdapterBase { return AbsPath("/word").join(RelPath(rel->second)).string(); } } - return ""; // TODO + // an unresolvable relationship leaves no href rather than a broken one + return ""; } private: diff --git a/src/odr/internal/ooxml/text/ooxml_text_style.cpp b/src/odr/internal/ooxml/text/ooxml_text_style.cpp index 575d3c6f3..bcfe50db9 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_style.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_style.cpp @@ -159,9 +159,10 @@ void resolve_table_cell_style_(const pugi::xml_node node, TableCellStyle &result) { const pugi::xml_node table_cell_properties = node.child("w:tcPr"); + // TODO `w:tcW` is read and dropped. A cell width here fights the column + // width the table already states, and the two disagree in real documents. if (const std::optional width = read_width_attribute(table_cell_properties.child("w:tcW"))) { - // result.width = width; // TODO } if (const std::optional vertical_align = read_vertical_align_attribute( diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index afb71ccb6..814dc84c6 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -609,8 +609,9 @@ open_by_cascade(const std::shared_ptr &file, ODR_VERBOSE(logger, "failed to open as xml"); } + // the last resort: bytes that read as text are a text file, whatever + // else the probes above made of them ODR_VERBOSE(logger, "open as text file"); - // TODO looks dirty return std::make_unique(file); } catch (...) { ODR_VERBOSE(logger, "failed to open as text"); diff --git a/src/odr/internal/pdf/pdf_document_element.hpp b/src/odr/internal/pdf/pdf_document_element.hpp index b8512e88c..b5e1cd69a 100644 --- a/src/odr/internal/pdf/pdf_document_element.hpp +++ b/src/odr/internal/pdf/pdf_document_element.hpp @@ -79,7 +79,7 @@ struct Page final : Element { Object crop_box; // rectangle array (defaults to media_box) Integer rotate{0}; // normalized to {0, 90, 180, 270} - // TODO remove + /// The `/Contents` streams to paint, in order. std::vector contents_reference; }; diff --git a/src/odr/internal/pdf/pdf_object_parser.cpp b/src/odr/internal/pdf/pdf_object_parser.cpp index 0e56a4f7b..bcbed1e7a 100644 --- a/src/odr/internal/pdf/pdf_object_parser.cpp +++ b/src/odr/internal/pdf/pdf_object_parser.cpp @@ -2,6 +2,8 @@ #include +#include +#include #include #include #include @@ -202,6 +204,20 @@ bool ObjectParser::skip_past(const std::string_view marker) { } } +/// The keyword is lowercase in ISO 32000-1 7.3.2, but the `peek_` above take +/// either case, so what follows has to as well - and it has to be read, or +/// `nXYZ` parses as null. +void ObjectParser::expect_keyword(const std::string &keyword) { + const std::string observed = bumpnc(keyword.size()); + if (!std::ranges::equal(observed, keyword, [](char a, char b) { + return std::tolower(static_cast(a)) == + std::tolower(static_cast(b)); + })) { + throw std::runtime_error("unexpected keyword (expected: " + keyword + + ", observed: " + observed + ")"); + } +} + void ObjectParser::expect_characters(const std::string &string) { const std::string observed = bumpnc(string.size()); if (observed != string) { @@ -346,10 +362,7 @@ bool ObjectParser::peek_null() { return c != eof && (c == 'n' || c == 'N'); } -void ObjectParser::read_null() { - std::ignore = bumpnc<4>(); - // TODO check ignore case -} +void ObjectParser::read_null() { expect_keyword("null"); } bool ObjectParser::peek_boolean() { const int_type c = geti(); @@ -360,16 +373,12 @@ Boolean ObjectParser::read_boolean() { const int_type c = geti(); if (c == 't' || c == 'T') { - std::ignore = bumpnc<4>(); - // TODO check ignore case - + expect_keyword("true"); return true; } if (c == 'f' || c == 'F') { - std::ignore = bumpnc<5>(); - // TODO check ignore case - + expect_keyword("false"); return false; } diff --git a/src/odr/internal/pdf/pdf_object_parser.hpp b/src/odr/internal/pdf/pdf_object_parser.hpp index 9ca154285..8fe27035b 100644 --- a/src/odr/internal/pdf/pdf_object_parser.hpp +++ b/src/odr/internal/pdf/pdf_object_parser.hpp @@ -56,6 +56,9 @@ class ObjectParser { /// if it was found; on false the stream has been consumed to eof. Operates on /// raw bytes, so the marker may straddle line breaks. bool skip_past(std::string_view marker); + /// @p keyword, compared without case. @throws std::runtime_error on a + /// mismatch. + void expect_keyword(const std::string &keyword); void expect_characters(const std::string &string); [[nodiscard]] bool peek_number(); diff --git a/src/odr/logger.hpp b/src/odr/logger.hpp index d6c3c1d2d..63282a801 100644 --- a/src/odr/logger.hpp +++ b/src/odr/logger.hpp @@ -18,6 +18,7 @@ enum class LogLevel { fatal, }; +/// The column widths and the clock a stdio logger writes its lines with. struct LogFormat { std::string time_format{"%H:%M:%S"}; std::size_t level_width{7}; diff --git a/src/odr/table_position.hpp b/src/odr/table_position.hpp index 2b810c812..f6743e7a7 100644 --- a/src/odr/table_position.hpp +++ b/src/odr/table_position.hpp @@ -6,6 +6,7 @@ namespace odr { +/// A cell by column and row, and the spreadsheet spelling of one: `B3`. struct TablePosition final { static std::uint32_t to_column_num(const std::string &string); static std::uint32_t to_row_num(const std::string &string); diff --git a/test/browser/annotation/tests.html b/test/browser/annotation/tests.html index 6d24c1dca..09493478b 100644 --- a/test/browser/annotation/tests.html +++ b/test/browser/annotation/tests.html @@ -19,13 +19,6 @@ border-left: 1px solid #ccc; z-index: 10; } - .ok { - color: #0a7d28; - } - .fail { - color: #c00; - font-weight: bold; - } /* stands in for a rendered page: laid out in inches, as the pdf view does */ .p { position: relative; @@ -67,24 +60,9 @@ +