Skip to content

fix(pptx): narrow a paragraph's wrap width by its right margin - #348

Open
dsaad68 wants to merge 1 commit into
openooxml:mainfrom
dsaad68:fix/pptx-paragraph-marr
Open

fix(pptx): narrow a paragraph's wrap width by its right margin#348
dsaad68 wants to merge 1 commit into
openooxml:mainfrom
dsaad68:fix/pptx-paragraph-marr

Conversation

@dsaad68

@dsaad68 dsaad68 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

TL;DR:

Before/After:

LibreOffice (reference), BetterOffice before, BetterOffice after, on the committed repro deck.
LibreOffice breaks all five paragraphs at exactly the words this change breaks them at:

paragraph right margin

Five paragraphs, each asking for a different right margin; before, all five wrap at the
full 600 px box width. No corpus deck that reproduces this is publishable — see
"Where the metric disagrees" below.

Repro file:

crates/pptx-render/tests/fixtures/paragraph-right-margin.pptx, with
crates/pptx-render/tests/fixtures/paragraph-right-margin.md describing what each shape exercises
and .before.png / .after.png beside it.

Summary:

  • marR is parsed into ParagraphProperties::margin_right. parse_paragraph_properties serves
    the master's p:txStyles, every a:lstStyle level, a:defPPr and every a:pPr, so one read
    covers the whole cascade; the field is skip_serializing_if = "Option::is_none", so a package
    without a right margin serializes exactly as it does today.
  • merge_paragraph_properties overrides it with the same is_some() rule as margin_left, so a
    shape's a:lstStyle beats the layout's, the layout's beats the master's, and an explicit
    marR="0" on a paragraph resets an inherited margin instead of being ignored.
  • layout_content subtracts the resolved right margin from the paragraph's wrap width. The line's
    start does not move, so marL and the bullet gutter are unaffected.
  • Collaboration schema 21, rebased onto main at 20. merge_source_paragraph_properties recovers
    the margin from the source package for snapshots written before the field existed.
  • Sequencing: this merges before fix(pptx): open the space a paragraph asks for before and after itself #354, which is stacked on it and takes schema 22. fix(pptx): open the space a paragraph asks for before and after itself #354's
    earlier revision listed a migratable version whose migration re-serialised packageJson from a
    model without margin_right, dropping the field; stacking it on this branch removes that path.

Where the metric disagrees:

  • The change moves pixels on exactly one corpus slide, arena-bibliography/18, and fine_pct
    there rises 12.08 → 12.67 against a same-commit baseline. The other four slides in that deck
    with a non-zero marR, and every slide of the two decks whose shape list styles carry marR,
    render byte-identically.
  • The rise is not the fix being wrong. That slide's shape carries
    <a:pPr marL="218800" marR="5012302" indent="210908" algn="r"> on its row labels; the reference
    renders them as a narrow right-aligned column of seven lines, before ran them onto one line
    across the slide, and after reproduces the reference's column and pushes the caption block
    below it into the reference's position. Split by region, the diff inside the table's own rows
    falls (19,330 → 13,622 differing pixels) and rises outside it, because the caption block is now
    drawn where the reference draws it and the two do not agree line for line — that shape also
    carries per-run spc (character spacing, unimplemented) and spAutoFit, so its wrap is still
    wrong once it is in the right place. Text drawn in roughly the right place scores worse than text
    not drawn at all.
  • No publishable deck reproduces the defect. arena-bibliography is a third-party presentation, so
    its renders are not attached; the before/after above is the committed synthetic fixture, whose
    paragraphs LibreOffice breaks at exactly the words the fix breaks them at.

Test plan:

  • cargo test -p betteroffice-pptx-parse -p betteroffice-pptx-render -p betteroffice-pptx-edit
    (347 tests, all passing)
  • crates/pptx-render/tests/paragraph_margins.rs fails without the layout_content change:
    two of its three tests break, the third is the control that must keep passing
  • a_v15_snapshot_recovers_paragraph_right_margins_from_source fails without the
    merge_source_paragraph_properties change
  • cargo fmt --all -- --check and cargo clippy --all-targets on the three crates
  • cargo check inside bindings/python-pptx, which is a separate workspace
  • Corpus re-render: arena-bibliography/18 is the only slide in 48 decks whose pixels change

Closes #347

@openooxml-bot

openooxml-bot Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA — thank you! ✍️ ✅

Posted by the CLA bot.

@greptile-apps

greptile-apps Bot commented Sep 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds end-to-end support for DrawingML paragraph right margins:

  • Parses and serializes marR across paragraph and list-style properties.
  • Resolves inherited right margins and subtracts them from paragraph wrap width.
  • Advances collaboration snapshots to schema 18 and recovers margins from attached source packages.
  • Adds a synthetic presentation fixture and focused parser, renderer, and migration coverage.
  • The implementation appears behaviorally sound, but the changeset names the wrong schema version.

Confidence Score: 4/5

The PR appears safe to merge after correcting the non-blocking schema-version error in the release note.

Parsing, cascading, layout, and deferred source recovery are internally consistent and covered by focused tests; the only accepted issue is inaccurate release documentation.

Files Needing Attention: .changeset/pptx-paragraph-right-margin.md

Important Files Changed

Filename Overview
crates/pptx-parse/src/drawing.rs Parses marR into paragraph properties and tests list-style, paragraph, zero-value, and serialization behavior.
crates/pptx-parse/src/model.rs Adds the optional, backward-compatible margin_right model field.
crates/pptx-render/src/layout.rs Resolves inherited right margins and narrows paragraph wrap width without moving its left edge.
crates/pptx-edit/src/deck.rs Advances snapshots to schema 18 and restores right margins from attached source packages.
crates/pptx-render/tests/paragraph_margins.rs Covers cascade inheritance, direct zero override, expected wrapping, and the no-margin control.
.changeset/pptx-paragraph-right-margin.md Describes the rendering fix but incorrectly identifies the new collaboration schema as 16 rather than 18.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    XML["DrawingML marR"] --> Parse["ParagraphProperties.margin_right"]
    Master["Master text style"] --> Cascade["Paragraph-property cascade"]
    Layout["Layout list style"] --> Cascade
    Shape["Shape list style"] --> Cascade
    Direct["Direct paragraph properties"] --> Cascade
    Parse --> Cascade
    Cascade --> Resolve["Resolve EMU margin to pixels"]
    Resolve --> Width["Wrap width = box width - left margin - right margin"]
    Width --> Lines["Lay out and align text lines"]
    Legacy["Legacy collaboration snapshot"] --> V18["Schema 18 migration"]
    Source["Attached source package"] --> Recovery["Recover missing right margins"]
    V18 --> Recovery
Loading

Reviews (1): Last reviewed commit: "fix(pptx): narrow a paragraph's wrap wid..." | Re-trigger Greptile

Comment thread .changeset/pptx-paragraph-right-margin.md Outdated
@dsaad68

dsaad68 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Incorrect schema version — fixed (d50b74e2).

Right, and my slip. This migration was written as 16, renumbered when main took 17, and the
changeset was not renumbered with it. It now reads 18.

One thing that supersedes the number itself: main is now at schema 20, so 18 is stale too.
This branch needs rebasing to 21 before it merges, and I would rather do that once main stops
moving than renumber a third time — it has gone 15 → 17 → 18 → 20 while this has been open. The
sequencing is settled though, by the review on #354: this pull request merges first, because
#354 accepts a schema-18 snapshot while its own model has no marginRight and would drop the
field. See the reply there.

`a:pPr/@marR` was parsed nowhere, so a right margin never narrowed the wrap
width and text ran the full box. 8001 occurrences across the corpus.

Reads it into `ParagraphProperties::margin_right`, overrides it with the same
`is_some()` rule as its siblings so `marR="0"` resets an inherited margin, and
subtracts it from the wrap width without moving the paragraph's left edge.
Collaboration schema 18 persists it and recovers it from an attached source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dsaad68
dsaad68 force-pushed the fix/pptx-paragraph-marr branch from d50b74e to 74af2c3 Compare September 7, 2026 20:25
@dsaad68

dsaad68 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and renumbered: schema 18 → 21.

main moved 15 → 17 → 18 → 20 while this sat open, so 18 was stale twice over. I renumbered once
against main at 20 rather than chase each bump.

What the rebase touched, all of it bookkeeping — the rendering change is unchanged:

  • SCHEMA_VERSION, MIGRATABLE_SCHEMA_VERSIONS (now [f64; 21], listing 20), and
    migrate_doc_to_v18migrate_doc_to_v21.
  • The version vectors and test names in crates/pptx-edit/tests/schema_migration.rs, plus the
    current-version assertions in autonumber.rs, blip_effects.rs, gradient_outlines.rs,
    run_baseline.rs and run_spacing.rs.
  • The changeset now says 21. It said 16 when this was opened, which was the slip caught in review.

Two things the renumber exposed that were latent rather than mechanical:

  • deck.rs's migration-event tests assert (step == 20).then_some(true) for
    olePicturesPendingSource. That flag is set at v20 and persists, so with 20 as the last step
    == and >= were indistinguishable. Adding a step after it made the difference real; it is now
    >=, matching how the sibling test already treats spacingPendingSource.
  • run_spacing.rs landed on main after this branch was cut, so it never conflicted, but it
    asserts the current schema version and had to move too. Worth knowing for the next renumber:
    the conflicting files are not the complete list.

cargo test -p betteroffice-pptx-edit -p betteroffice-pptx-render -p betteroffice-pptx-parse,
clippy and cargo fmt --check are green, and cargo check --workspace --tests passes. Nothing
outside pptx-edit pins the number.

This should merge before #354, which is now stacked on it at schema 22.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pptx: a paragraph's marR right margin is never parsed, so text wraps at the full box width

1 participant