Pre-major cleanup: composition for csv/markdown, bind the edit API everywhere, clear the cheap TODOs - #881
Merged
Merged
Conversation
`annotation` and `viewport` kept their own `check()` and printed no `#summary`, so a headless run could not read them at a glance. Both now load `checks.js`, which gains an optional failure `detail` and a `note()` heading. `checks.js` also guards against a throw in an event listener: it does not stop the script that dispatched it, so a page could pass over a live exception. The README said editing is limited to odt, odp, odg, ods and docx. ods, pptx, xlsx and txt all declare `edit` + `save` now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
`abstract::CsvFile` and `abstract::MarkdownFile` derived from `abstract::TextFile`, so `is_text_file()` answered true for a `.csv` and a `.md`. Both types declare `save = false`, but `TextFile::is_savable()` asks only whether the encoding decodes - so both reported savable, and `html::translate` had to test them ahead of its text branch. Both now derive from `abstract::DecodedFile` and *hold* the `text::TextFile` they already composed. `CsvFile::text_file()` and `MarkdownFile::text_file()` hand out the plain-text view, so reading the bytes as text still needs no reopening. The capability test now asserts a text file's `is_savable()` against the declared `save`, which is what caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
…e notes `HtmlResource`, `HtmlView`, `HtmlService`, `LogFormat` and `TablePosition` carried no doc comment; every other public class does. `document-editing.md` still named the registry links `insert_child_after` / `insert_child_before`, renamed to `insert_sibling_*` before they landed, and its format table left `.xlsx` under "everything else — read-only". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
`Document::remove`, `insert_text_before` / `_after`, `append_text`, `split_paragraph`, `merge_paragraph_with_next`, `insert_paragraph_after` and `TextFile::write_edited` were C++ only. A host could drive them through the op envelope, but not in process. python, jni and apple take elements, as their `Document` already does. `element_by_id` joins them, so the id `Element::identifier` hands out resolves in all three rather than in python alone. wasm addresses the same operations **by element id**: nothing escapes that binding as a handle, and the render already writes ids into the page as `data-odr-id`. An apple method that returns `nil` for a legitimate answer must not also take an `error:` - Swift imports the pair as `throws` and turns the `nil` into `nilError`. `elementWithIdentifier:` is a plain nullable getter, like the rest of the navigation. Tests: python 80, jni junit, apple 40 XCTest, wasm 46 node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
Three were defects rather than notes: - `text_set_content` on an `.xlsx` run was a silent no-op. Every other engine that cannot write a run throws `UnsupportedOperation`; this one now does too, because the format declares `edit` and a caller had no other way to find out. - The pdf object parser skipped 4 or 5 bytes for `null`/`true`/`false` without reading them, so `nXYZ` parsed as null. `expect_keyword` checks them, without case, as the `peek_` around them already allow. - The three `oldms` element adapters each held a `const Document *` that nothing read, kept alive by a `[[maybe_unused]]`. Gone, with the parameter that fed it. The 21 bare `// TODO` are down to none: each now states what is missing, so a reader learns it without going to the format spec. `contents_reference` was marked `// TODO remove` while four call sites use it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
…ywhere Making them hold a text file rather than be one took `.text()` on a `.csv` and a `.md` away from jni and apple, which had no `CsvFile` wrapper to reach it through. Neither could get at the spreadsheet or prose view either, and python had `CsvFile` but no `MarkdownFile`. All three now expose both types with `document()` and `text_file()`, so the composition costs a caller nothing. Four design questions that had sat in headers as bare `TODO` are answered where they were asked: `File::impl()` is the escape hatch every public wrapper offers, `Frame::anchor_type` has no neutral value to report, the text file is the last resort of `open_strategy`, and a walker builds its path rather than holding one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A survey of what is open before a new major version, with the small things done
directly.
is_savable()lied for a.csvand a.mdabstract::CsvFileandabstract::MarkdownFilederived fromabstract::TextFile, sois_text_file()answered true for both. Theircapability rows declare
save = false, butTextFile::is_savable()asks onlywhether the encoding decodes — so both reported savable, and
html::translatehad to test them ahead of its text branch or a csv rendered as a line list.
Both now derive from
abstract::DecodedFileand hold thetext::TextFilethey already composed. Breaking:
is_text_file()is false for a.csvanda
.md; the plain-text view isas_csv_file().text_file().The capability test now asserts a text file's
is_savable()against thedeclared
save. That is what caught this.Every binding reaches the structural edit API
Document::remove,insert_text_before/_after,append_text,split_paragraph,merge_paragraph_with_next,insert_paragraph_afterandTextFile::write_editedwere C++ only. python, jni and apple take elements, astheir
Documentalready does;element_by_idjoins them, so the idElement::identifierhands out resolves in all three rather than in pythonalone.
wasm addresses the same operations by element id: nothing escapes that
binding as a handle, and the render already writes ids into the page as
data-odr-id.CsvFileandMarkdownFileare bound in all three element bindings too, withdocument()andtext_file()— so the composition above costs a callernothing.
An apple method that returns
nilfor a legitimate answer must not also takean
error:: Swift imports the pair asthrowsand turns thenilintonilError.elementWithIdentifier:is a plain nullable getter.TODOs
Three were defects rather than notes:
text_set_contenton an.xlsxrun was a silent no-op. Every otherengine that cannot write a run throws
UnsupportedOperation; this one nowdoes too, because the format declares
editand a caller had no other way tofind out the write was dropped.
null/true/falsewithout reading them, so
nXYZparsed as null.expect_keywordchecksthem, without case, as the
peek_around them already allow.oldmselement adapters each held aconst Document *thatnothing read, kept alive by a
[[maybe_unused]].The 21 bare
// TODOare down to none: each now states what is missing, so areader learns it without going to the format spec. Four design questions sitting
in headers are answered where they were asked.
Check pages and documentation
annotation/tests.htmlandviewport/tests.htmlkept their owncheck()andprinted no
#summary. Both now loadchecks.js, which gains an optionalfailure
detail, anote()heading, and a guard against a throw in an eventlistener. All nine pages pass: 14, 51, 10, 22, 8, 93, 45, 41, 17 checks.
Five public classes carried no doc comment. The README's editing paragraph named
the wrong formats, and
document-editing.mdnamed registry links that wererenamed before they landed.
Checks run
C++ 1684 passed; python 81; jni junit; apple 40 XCTest; wasm 46 node. The 14
touched TUs are clean under
g++-15 -Wall -Wextra -Werror -fsyntax-only.Left open, deliberately
fo:marginin odf is dropped. CSS takes%here with thesame meaning, so passing it through would work — but it moves the reference
output for every document that uses one.
AnchorTypehas no neutral value, soFrame::anchor_type()on a framethat does not exist answers
as_char, which a real frame can also answer.Adding
AnchorType::noneis safe for the bindings only if appended, whichreads wrong; it needs a decision rather than a quiet edit.
each now stating what it is missing. The largest clusters are streaming
decrypt/inflate, big-endian support, and pptx master pages and links.