feat(odf.js): write .odg packages, vector primitives included - #904
Merged
Conversation
Mearman
marked this pull request as ready for review
September 4, 2026 05:26
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…side what they describe canonicalMetadata was declared identically in the odt and odp writers, and would have been a third copy in any further writer: what meta.xml can carry is a property of the part, not of which body element sits beside it, so it moves to typed/shared/canonicalise.ts alongside the paragraph/table/image forms the same two writers already share. canonicalShape moves the other way, from typed/odp/write.ts down to typed/draw/write-shapes.ts as canonicalDrawShape -- it states what reading a written draw:frame back produces, so it belongs beside writeDrawFrame, which produces it. Its documentIndex now comes from the caller rather than being assumed to be the shape's index in its own array, since a page that emits anything other than one frame per shape has different encounter indices. frameGeometryAttrs becomes exported for the same reason: a vector primitive's geometry resolves through the identical resolveOdfShapeGeometry a frame's does, so its inverse is this function unchanged rather than a second copy of the same trigonometry.
The write-side inverse of parseOdfPathData: one absolute M per subpath, an explicit absolute L or C per segment, and a trailing Z only for a closed subpath. The longest, most explicit form is written rather than the relative and H/V shorthands real producers favour -- the parser reads all of them, but the explicit form is unambiguous for any other consumer's parser too. formatOdfViewBox is its pair rather than a separate helper: stating the viewBox as "0 0 <frame width> <frame height>" makes the reader's own viewBox-to-frame scale factor exactly 1 and its minX/minY subtraction exactly zero, so a subpath's local-space points are the numbers written into svg:d with no scaling step to round-trip through. Both go through formatOdfNumber, which is the fixed-point spelling formatOdfLength already applied to lengths, now separated from the unit suffix so a bare number can reach it. svg:viewBox has no exponent form at all, so a small-magnitude extent spelled "1e-7" is silently unreadable there for the same reason it is in a length.
Every round-trip case writes subpaths, parses the result back through parseOdfPathData, and scales it through buildOdfSubpaths against the very viewBox formatOdfViewBox produced. Asserting only the literal svg:d text would pass just as happily for a serializer whose grammar the parser cannot read. Covers a closed and an open subpath mixing line and cubic segments, negative and fractional coordinates whose signs are their own separators, a segmentless subpath as a bare moveto, the empty-subpath-list case a caller has to refuse because it parses back to nothing, and the small-magnitude coordinate that would otherwise reach exponent notation.
…ments The write-side mirror of typed/draw/shapes.ts's vector-primitive readers, and the one piece of a drawing page a ContentShape has no vocabulary for. Every attribute is the read side's own -- draw:fill, draw:fill-color, svg:fill-rule, draw:stroke, svg:stroke-color, svg:stroke-width -- so the pair is a genuine inverse rather than two independently plausible spellings that agree on the common cases. draw:ellipse is written for every ellipse including a circular one, since the reader maps LibreOffice's own draw:circle spelling for that case onto the same variant. Every vector carries a draw:style-name even with neither fill nor stroke: an absent declaration means inherit in ODF, and a consumer's own default graphic style supplies a fill, so an unfilled rectangle that says nothing renders filled. draw:fill="none" and draw:stroke="none" are the format's own way of saying otherwise. Refused by name rather than approximated: a dotted or double stroke style, since draw:stroke is enumerated to exactly none/solid/dash and ODF's vector-stroke model has no double-line concept at all; a non-positive stroke width, which reads back as no stroke and takes a whole line element with it; and a path with no subpaths or a non-positive frame extent, both of which the reader discards outright rather than reading back smaller.
writeOdg and writeOdgContent, the inverse of readOdg/readOdgContent, at the two levels every writer here has. A drawing page is a presentation slide's structural twin -- draw:page's content model is one format-agnostic schema fragment shared by office:drawing and office:presentation -- so this writes the same one style:master-page/style:page-layout pair per page and delegates a page's text-in-a-frame shapes to the same writeDrawShapes, adding only what a drawing genuinely has: the vector primitives of ContentDrawPage's second array. normaliseOdgContent states two page-level facts the presentation form does not have to. A page's shapes and vectors share one document-encounter counter, because the reader walks a draw:page's children once and stamps both from it; this writer emits shapes first and vectors after, so a shape's encounter index is its array index and a vector's is offset past every shape. That emit order is a stated choice rather than an implied one -- the two arrays carry no interleaving information beyond paintOrder itself, so a page whose items state no paint order has no cross-array order to preserve and one has to be picked. And both arrays come back sorted by paint order, since readDrawPageContent sorts them, unlike the presentation walker, which stamps without reordering because a slide has no sibling vectors array for the value to compare against. A page's residue is dropped, the same exception every writer here makes.
Two suites, mirroring the split every other writer here has: write.test.ts asserts what the writer actually emits, construct by construct, since a writer and reader that agree with each other and with nobody else would round-trip perfectly and open nowhere; write-round-trip.test.ts states the law -- normaliseOdgContent(readOdgContent(writeOdgContent(d))) equals normaliseOdgContent(d) -- and every deviation from it by name. Between them they cover a rect and an ellipse with fill and stroke, a line, a path whose subpaths mix line and cubic segments in one closed and one open subpath, rotation, a shape and vectors coexisting on one page, multiple pages with different geometry, style interning across identically painted vectors, and every refusal. Paint order gets its own group, since it is the one thing a drawing has to get right that a presentation does not: orders that interleave shapes and vectors, the encounter-index fallback counting shapes before vectors, arrays that disagree with their own paint order and come back sorted, and a paintOrder ODF cannot spell falling back rather than rounding onto a neighbour. The rotation sweep runs 15 angles across 7 frames including ones centred on the origin, where the rotation inverse's terms cancel to rounding dust rather than a clean zero. That failure is silent and total rather than approximate -- an unparseable translate() drops the element to its own pivot, an unparseable svg:x drops it from the read entirely -- so the suite asserts the element count first.
The prefix audit exists to catch any writer here reaching for a prefix its own part's root does not declare, which produces bytes that look right, round-trip through this package's own prefix-string-matching reader, and that no XML parser will accept. It covered five of the seven content writers; writeSxi had never been added, and writeOdg is new. The drawing exercises what only a .odg page carries: every vector-primitive kind, each with the fill, stroke, and fill-rule vocabulary that mints a graphic style, alongside a shape so both halves of a draw:page are covered in one pass.
Status, Usage, and Architecture now state a real .odg writer rather than a planned one, including what a drawing page adds over a slide (the vector primitives, the shapes-before-vectors emit order, the paint-order sort on the way back) and what it refuses by name. The LibreOffice verification section records what a real, independent ODF implementation made of the output, and three things it does with a valid file that are worth stating precisely: svg:fill-rule does not survive its Draw round trip at all, a draw:path mixing closed and open subpaths renders unfilled because LibreOffice classifies a whole path as open or closed for its own shape model, and per-page geometry is preserved in the ODF but normalised to the first page's size on PDF export.
…der as required Both canonicalisers always set paintOrder -- ODF-spellable or the caller's own document-encounter index -- but ContentShape and ContentVector leave the field optional, since a hand-built document need not carry one. Saying so in the return type removes the `?? 0` the drawing page's sort comparator needed for an absence its inputs cannot actually have, which would have silently ordered a paintOrder-less item first rather than failing if one ever appeared. Also states the ordering constraint the sort sits under: canonicalisation runs in emit order and the sort happens after it, because canonicalDrawShape mints list identities off the shared ListPlanState and the reader reproduces those by walking the written document in document order. Sorting first would renumber every list on a page whose paint order disagrees with its array order.
…e and vector writeDrawFrame/writeDrawVector omitted draw:z-index entirely for an item with no ODF-spellable paintOrder, relying on the reader's own document-encounter fallback to reconstruct its position. Real LibreOffice does not treat an omitted attribute that way: it appends every item carrying none after every item that does carry one, regardless of where that item's own resolved order would otherwise place it -- so a page mixing an explicit-paintOrder item with an unspelled one reordered wrongly in a real consumer even though this package's own reader recovered the input correctly. Both writers now write the fully resolved paint order (the stated paintOrder when ODF can spell it, the item's own document-encounter index otherwise) as draw:z-index unconditionally, never omitting the attribute. writeDrawFrame/writeDrawVector take an explicit documentIndex parameter for this, threaded from the same array-index arithmetic canonicalDrawShape/canonicalDrawVector already use.
…notation frameGeometryAttrs's draw:transform ran its translate() lengths through formatOdfLength but interpolated the rotate() angle directly -- a bare radians value with no unit suffix, so it never passed through any fixed-point formatter. A very small non-zero rotationDeg (never exactly zero, which collapses to no transform at all) drives the angle itself into JavaScript's own exponent spelling, exactly as invalid to the ODF length/number grammar as the translate() case this formatter already closed, and undetectable by a soffice round trip alone since LibreOffice's own parser accepts the invalid spelling regardless. The angle now goes through formatOdfNumber, the same bare-number formatter formatOdfLength already uses internally for the length suffix cases.
…tionally shapeGraphicStyleName minted a style only when a shape's own insets were non-zero, so a plain text shape with no insets wrote no draw:style-name at all and inherited whatever default graphic style a real consumer applies. LibreOffice's own built-in "standard" graphic style fills solid and strokes coloured, so a document whose model carries only text rendered as a filled, bordered box -- the identical silent-inherit hazard write-vectors.ts's vectorGraphicStyleName already closes for a vector's own fill and stroke. ContentShape has no fill/stroke field to preserve either way, so stating draw:fill="none"/draw:stroke="none" unconditionally loses no round-trip fact -- readFrameInsets never reads either attribute off a frame's style. Insets remain written only when at least one is non-zero, since the reader already defaults an absent style to zero there.
Mearman
force-pushed
the
feat/odf-odg-writer
branch
from
September 4, 2026 06:56
8070c17 to
3c072be
Compare
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
1 participant
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.
odf.jshadreadOdg/readOdgContentand no write half. This addswriteOdg/writeOdgContent, the last of the four ODF content writers, and with it the one thing none of the others needed: a writer forContentDrawPage'svectorsarray.What's here
Most of a drawing page is a presentation slide's structural twin —
draw:page's content model is one format-agnostic schema fragment shared byoffice:drawingandoffice:presentation— so the shapes half reuseswriteDrawShapesverbatim and the page geometry reuses the same one-style:master-page/style:page-layout-pair-per-page structurewriteOdpestablished. Nothing there is a second copy.What's new is
typed/draw/write-vectors.ts: rect, ellipse, line and path as realdraw:rect/draw:ellipse/draw:line/draw:pathelements, with fill and stroke interned as graphic-family automatic styles, and a path's subpaths serialised into genuinesvg:dpath data. The six style attributes it writes (draw:fill,draw:fill-color,svg:fill-rule,draw:stroke,svg:stroke-color,svg:stroke-width) are exactly the sixreadOdfFillAndStrokereads, so the pair is a real inverse rather than two spellings that happen to agree on the common cases. Path serialisation lives intyped/shared/path.tsbeside the parser it inverts, and pins its own viewBox convention (0 0 <frame width> <frame height>) so the reader's scale factor is exactly 1 and coordinates round-trip exactly.Refused by name, never approximated: a
dottedordoublestroke style (draw:strokeis enumerated to exactly none/solid/dash, and ODF's vector-stroke model has no double-line concept), a non-positive stroke width, and a path with no subpaths or a zero-extent frame — the last two being things this package's own reader discards outright rather than reading back smaller.Two things came out of writing it that weren't strictly odg work but were the right fix rather than a third copy:
canonicalMetadatawas declared identically in the odt and odp writers and moved totyped/shared/canonicalise.ts, andcanonicalShapemoved fromtyped/odp/write.tsdown totyped/draw/write-shapes.tsascanonicalDrawShape, beside the writer whose output it describes.formatOdfLength's exponent-expanding number formatter is now also reachable without a unit suffix, sincesvg:viewBoxhas no exponent form either.Paint order, which is the part a drawing has to get right and a slide doesn't
readDrawPageContentwalks adraw:page's children once with a single counter, stamping every shape and every vector from it, then sorts both output arrays by the result. SonormaliseOdgContentstates two factsnormaliseOdpContentdoesn't have to: a page's shapes and vectors share one document-encounter counter (this writer emits shapes first and vectors after, so a shape's encounter index is its array index and a vector's is offset past every shape), and both arrays come back sorted by paint order. The shapes-before-vectors emit order is stated explicitly rather than left implied — the two arrays carry no interleaving information beyondpaintOrderitself, so a page whose items state no paint order has no cross-array order to preserve and one has to be picked; anything that does state an ODF-spellablepaintOrdercarries a realdraw:z-indexand is ordered by that instead.Verification
Round-tripping through this package's own reader proves internal consistency, not that anything else accepts the result. Both halves ran.
Self-consistency.
write.test.tspins the emitted XML construct by construct;write-round-trip.test.tsstates the law (normaliseOdgContent(readOdgContent(writeOdgContent(d)))equalsnormaliseOdgContent(d)) and every deviation by name — a rect and ellipse with fill and stroke, a line, a path mixing line and cubic segments across a closed and an open subpath, rotation, shapes and vectors on one page, multiple pages with different geometry, every refusal, and the cross-array paint-order cases. The rotation sweep runs 15 angles across 7 frames including ones centred on the origin, where the rotation inverse's terms cancel to trig rounding dust rather than a clean zero — the same silent-and-total failure classformatOdfLength's fixed-point spelling exists to close, which drops a rotated element to its own pivot or an unrotated one from the read entirely. The suite asserts the element count first for exactly that reason.LibreOffice 26.2.5.2,
soffice --headless. A sample.odgcovering all of the above — two pages with genuinely different geometry (720×540pt landscape, A4 portrait), a filled and stroked rectangle, a filled ellipse with a dashed stroke, a line, a path with one closed line-plus-cubic subpath and one open cubic subpath (fill,evenoddfill rule, stroke), a rectangle rotated 30°, text-in-a-frame shapes beside the vectors, and paint orders across both arrays deliberately unlike document order (shapes6, 1; vectors5, 0, 3, 2, 4):xmllintaccepts every part of the written package and the resulting flat XML. Bothsofficeruns exit0with no error, identifying the input asa Draw document.Every authored value appears in LibreOffice's re-serialised output, verbatim or unit-converted: all seven authored colours survive verbatim (
#0033ff,#cc0000,#00aa44,#000000,#ffcc33,#9933cc,#e6e6f2), as does every authored string; each element comes back as a real element of the matching kind (draw:rect,draw:ellipse— plusdraw:circlewhere width equals height, LibreOffice's own spelling for that case, which the reader already maps back onto'ellipse'—draw:line,draw:path,draw:frame), nothing dropped in either direction; lengths convert exactly (200pt width to7.056cm, 280ptsvg:xto9.878cm, stroke widths 3/2/4/1pt to0.106cm/0.071cm/0.141cm/0.035cm);draw:stroke="dash",draw:stroke="none"anddraw:fill="none"all survive as written; and the rotated rectangle's whole transform survives,rotate(-0.5235987755982988) translate(441.07695154586736pt 286.69872981077805pt)re-emitted asrotate (-0.523598775598299) translate (15.56cm 10.114cm). The path'ssvg:dis re-expressed in LibreOffice's own 1/100mm viewBox with relative and shorthand commands, and every coordinate maps back exactly — the authoredC 180,0 180,100 120,100becomesc2117 0 2117 3529 0 3529, the same +60pt/+100pt control offsets and +120pt/+100pt endpoint.This is not a byte-identity check and could not be:
--convert-to fodgre-serialises the whole document through LibreOffice's own writer, which renames styles, reorders and reformats attributes, converts units, and adds defaults of its own, so its bytes differ fromwriteOdg's by construction.The rendered PDF (2 pages) visually confirms each shape's geometry, its fill and stroke colours, the dashed ellipse border, the 30° clockwise rotation, and the path's curve, and carries the sample's
meta.xmltitle, author, subject, and keywords into its own PDF metadata.draw:z-indexis honoured across both arrays — the finding that matters most here. LibreOffice re-emitted page 1's seven elements in exactly the authored paint order0…6(an ellipse written second came out first, a text frame written first came out last), dropping the attribute and physically reordering the elements instead. Same behaviour thewriteOdpverification found, now confirmed for the case only a drawing has, where the ordering holds between the two arrays rather than within one.Three things LibreOffice does with a valid file, stated precisely
None is a defect in what this writer emits, and each is in the README:
svg:fill-ruledoes not survive a LibreOffice round trip. Real, spec-defined ODF vocabulary onstyle:graphic-properties(which is why the reader reads it and this writer emits it), but LibreOffice's Draw import/export drops it entirely — zero occurrences in the re-serialised output. It round-trips through this package's own reader; a real consumer just doesn't keep it. This also answers a questiontyped/draw/shapes.ts's own read-side comment had left open.draw:pathmixing closed and open subpaths renders unfilled in LibreOffice. A controlled test holding everything else identical — three paths, same colours, stroke and frame size, differing only in their subpaths — renders the closed-only path filled, the open-only path unfilled (correct: no area), and the mixed path unfilled even for its closed subpath. LibreOffice classifies a wholedraw:pathas open or closed for its own shape model, and an open shape is never filled. The fill is preserved in the file — the re-serialised style still readsdraw:fill="solid" draw:fill-color="#ffcc33"— only the render omits it.style:page-layouts and twostyle:master-pages with eachdraw:pagereferencing its own, LibreOffice's flat XML preserves both (25.4cm × 19.05cmlandscape,21cm × 29.7cmportrait), andreadOdgContentreads both back — but--convert-to pdfrenders every page at the first page's size. Separately, LibreOffice tightens adraw:path's frame to the path geometry's bounding box on re-serialisation: the sample's 240×140pt frame, whose curve only spans 240×120pt, came back as 240×120pt with its viewBox rescaled and every coordinate intact.Checks
pnpm exec turbo run _lint _typecheck _test _test:workers --filter=odf.js --force— green (1236 unit tests, 5 workerd)pnpm exec turbo run _test:smoke --filter=odf.js --force— greenpnpm exec turbo run _lint _typecheck --forceacross the whole workspace — green (57 tasks)What a reviewer should look at hardest
The two areas with no writer-side precedent in this package to copy:
nonevalues (typed/draw/write-vectors.ts). Each name is taken from the read side rather than from SVG intuition, but the judgement call is writingdraw:fill="none"/draw:stroke="none"explicitly for a vector that states neither, and minting a style for every vector rather than only for ones with something to say. The reasoning: an absent declaration means inherit in ODF, not none, and a consumer's default graphic style supplies a fill — LibreOffice's built-instandardfills solid — so an unfilled rect that says nothing renders filled. Confirmed in the render.svg:viewBoxconvention and path-data serialisation (typed/shared/path.ts). Writing the viewBox as the frame's own extent is what makes the round trip exact rather than approximate, but it means a file read from LibreOffice (whose viewBox is a large integer range unrelated to the frame's physical size) and written back is re-expressed in a different spelling of the identical curve. I think that's right, and the alternative — carrying the source viewBox through the schema — would need a schema field for a factContentVectordeliberately doesn't model. Worth a second opinion.Also worth a look: the shapes-before-vectors emit order and its
documentIndexarithmetic innormaliseOdgContent. It's a genuine choice, not forced, and the canonical form has to state it exactly or the round-trip law goes quietly wrong for any page whose items don't all carry apaintOrder.Scope
.sxdstill has no writer. This unblocks it — the wrapper iswriteOdg's output throughtransformToOoo1Package, exactly the waywriteSxiwrapswriteOdp— but building it belongs with #821 rather than here, so this PR doesn't close that issue.