Skip to content

fix: honor requested PDF page dimensions - #3

Merged
kolkov merged 1 commit into
gogpu:mainfrom
besmpl:agent/honor-pdf-page-dimensions
Aug 12, 2026
Merged

fix: honor requested PDF page dimensions#3
kolkov merged 1 commit into
gogpu:mainfrom
besmpl:agent/honor-pdf-page-dimensions

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • create PDF pages with the exact dimensions passed to Backend.Begin
  • preserve per-page dimensions in Document.NewPage
  • upgrade gxpdf from v0.2.0 to the minimal v0.4.0 release that exposes NewPageWithDimensions
  • stage page creation before mutating backend state so a failed Begin leaves the prior page coherent
  • add serialized MediaBox regressions for single-page and mixed-size documents

Why

The backend accepted width and height but always created an A4 page. For example, Begin(320, 240) serialized /MediaBox [0 0 595 842] instead of 320 x 240. Recording playback passes its canvas dimensions directly to Backend.Begin, so the output page size must preserve them.

gxpdf v0.2.0 has no public arbitrary-page-size API. v0.4.0 is the first release with Creator.NewPageWithDimensions, making it the smallest dependency update that can implement the contract cleanly.

Verification

  • baseline Begin(320, 240) produces A4 MediaBox
  • single-page output now contains the exact requested MediaBox
  • mixed-size Document pages retain their independent dimensions
  • non-positive dimensions return an error
  • a failed second Begin preserves the previously initialized page
  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go test -count=1 -coverprofile=coverage.out ./...
  • local added executable-line patch coverage: 100% (21/21)
  • go build ./...
  • go vet ./...
  • go mod tidy -diff
  • gofmt and git diff --check

Upstream GitHub Actions and codecov/patch are configured but await maintainer approval of the fork workflow run.

Coordination

This is independent of #2, although both patches touch page initialization and may need a trivial rebase depending on merge order.

Review follow-up (2026-08-12)

Rebased onto the merged lifecycle/y-flip work. The combined implementation preserves Scale(1, -1).Then(Translate(0, height)), retains the injectable page-creation failure seam, and creates every page with its exact requested dimensions.

@besmpl
besmpl marked this pull request as ready for review August 10, 2026 17:46
@besmpl
besmpl requested a review from kolkov as a code owner August 10, 2026 17:46

@kolkov kolkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real bug — the backend accepted width/height but always created A4 pages. NewPageWithDimensions is the right fix.

Good defensive detail: staging new state before mutation so a failed Begin leaves the prior page coherent. Non-positive dimension rejection is delegated to gxpdf's own validation rather than duplicated — reasonable since NewPageWithDimensions already guards this. The MediaBox serialization tests verify the actual PDF output, not just internal state.

gxpdf v0.4.0 bump is required for NewPageWithDimensions — confirmed available.

LGTM, merging.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kolkov

kolkov commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

This PR has a merge conflict after #1, #2, and #4 were merged. The conflict is in Backend.Begin() (the Y-flip transform formula changed in #2) and Document.NewPage().

Could you rebase on the updated main? The conflicts should be straightforward — the flip transform is now Scale(1, -1).Then(Translate(0, h)) per #2.

@besmpl
besmpl force-pushed the agent/honor-pdf-page-dimensions branch from 1f52dc6 to 7acf334 Compare August 12, 2026 06:52
@kolkov
kolkov merged commit de51b9b into gogpu:main Aug 12, 2026
10 checks passed
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.

2 participants