perf(docx): typed paragraph measurement path - #232
Open
eliahilse wants to merge 2 commits into
Open
Conversation
Contributor
|
All contributors have signed the CLA — thank you! ✍️ ✅ Posted by the CLA bot. |
Greptile SummaryThe PR replaces DOCX layout’s per-paragraph JSON serialization boundary with a borrowed typed measurement request while retaining the public JSON and
Confidence Score: 4/5The PR appears safe to merge, with only non-blocking cleanup needed for newly added comments and docstrings that exceed the repository’s concision standard. The typed conversion preserves the existing measurement and synthetic-fallback boundaries, and the investigated mappings did not establish a runtime regression; the remaining accepted issue is documentation maintainability. Files Needing Attention: crates/docx-layout/src/typed_measure.rs, crates/ooxml-text/src/measure/input.rs
|
| Filename | Overview |
|---|---|
| crates/docx-layout/src/typed_measure.rs | Adds the manual typed conversion and output bridge with broad JSON-path parity tests; its explanatory comments exceed the repository’s concision rule. |
| crates/docx-layout/src/measure_blocks.rs | Replaces the resident JSON envelope round trip with typed measurement while retaining synthetic fallback on failure. |
| crates/ooxml-text/src/measure/input.rs | Introduces the borrowed MeasureRequest and dual-map FontChains abstraction without changing the owned public input. |
| crates/ooxml-text/src/measure/mod.rs | Moves the measurement implementation onto the typed request and delegates both owned and JSON entry points to it. |
| crates/docx-layout/src/lib.rs | Adds the crate-local resident typed measurement entry point against the existing font store. |
Sequence Diagram
sequenceDiagram
participant Layout as DOCX layout
participant Bridge as Typed conversion
participant Text as OOXML text measurement
participant Fonts as Resident font store
Layout->>Bridge: ParagraphBlock + MeasurementConfig
Bridge->>Text: MeasureRequest (borrowed)
Text->>Fonts: Resolve font chains and shape runs
Fonts-->>Text: Font metrics and glyph data
Text-->>Bridge: ParagraphExtentOut
alt Measurement succeeds
Bridge-->>Layout: ParagraphExtent
else Conversion or measurement fails
Bridge-->>Layout: None
Layout->>Layout: Build synthetic extent
end
Reviews (1): Last reviewed commit: "perf(docx): typed paragraph measurement ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+7
| //! Typed bridge between layout blocks and `ooxml-text` measurement. | ||
| //! | ||
| //! Field-for-field equivalent of the JSON envelope this used to serialize: | ||
| //! every mapping here mirrors what serde did at that boundary, including its | ||
| //! strictness — a value the JSON parse would have rejected makes | ||
| //! [`measure_paragraph`] return `None` so the caller falls back to the | ||
| //! synthetic extent, exactly as an `"invalid: "` error used to. |
There was a problem hiding this comment.
The new typed measurement path adds several multi-line explanations where the names, types, and implementation already convey the behavior, increasing maintenance burden and the opportunity for documentation to drift. Please keep these docstrings concise and reserve inline comments for behavior that is not otherwise understandable.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
eliahilse
force-pushed
the
perf/docx-typed-measure
branch
3 times, most recently
from
August 26, 2026 14:32
f9dab51 to
e95c3e2
Compare
The typed path diverged from the envelope it replaced, found by a randomised differential against the old JSON path: - non-finite floats were cast straight to f32, where `serde_json` wrote them as `null` and the envelope read them back as absent. Affected run fontSize/fontSizeCs/letterSpacing/horizontalScale/kerningMinPt, image width/height/distTop/distBottom, every spacing and indent number, defaultFontSize/listMarkerFontSize/defaultTabStopTwips, and paragraphYOffset: the paragraph fell back to the synthetic extent where it used to measure at the default. - `defaults` and `compat` accepted only objects, but serde's derived `Deserialize` also takes a struct from a positional sequence, so `[12.0, "Arial"]` and `[true, false]` measured before and synthesized after. Both now defer to the same derive the envelope parse used, as `rotationBounds` already does. - required engine inputs (tab positions, float-zone coordinates) reached the engine carrying a non-finite value and were refused there, where the envelope had failed to parse before delivering anything. The outcome always agreed; constructing them fallibly makes the request delivered to the engine identical at every stage, not just the result. Adds a randomised whole-surface differential (3,000 paragraphs, every mapped field, both compat flags, defaults, `fullWidthBlock` and `authoritativeShaping` varied) that compares the `BlockIn` reaching the engine, the fallback decision, and the f32-narrowed extent, plus a sweep of all 24 numeric sites against NaN and both infinities. The existing fixtures did not discriminate: deleting any of eighteen field mappings left them green, including the list-marker and bidi mappings they are named for.
eliahilse
force-pushed
the
perf/docx-typed-measure
branch
from
August 26, 2026 14:56
e95c3e2 to
411898e
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.
TL;DR:
Summary:
MeasureRequest<'a>) instead of serializing block+font-chains to JSON and parsing them back per callMeasureInput/measure_paragraphunchanged; wasm JSON path delegates to the same typed core; malformed-input fallback parity pinned by tests incl. serde struct-from-seq quirksTest plan:
cargo test -p betteroffice-ooxml-text -p betteroffice-docx-layoutgreen