Skip to content

fix: preserve document pages during playback - #4

Merged
kolkov merged 2 commits into
gogpu:mainfrom
besmpl:agent/fix-document-playback
Aug 11, 2026
Merged

fix: preserve document pages during playback#4
kolkov merged 2 commits into
gogpu:mainfrom
besmpl:agent/fix-document-playback

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep document pages attached to the document-owned PDF creator during recording playback
  • override page lifecycle methods so recording Begin validates instead of replacing the page
  • finalize each page exactly once across playback and Document.Finish
  • reject dimension mismatches, failed initialization, finalized pages, and pages added after document finish
  • add direct, convenience, multi-page, failure, and ownership regressions

Why

Document.NewPage already creates a document-owned page and pushes its coordinate transform. Recording.Playback then called the promoted Backend.Begin, which replaced that page's creator/page/surface with a standalone creator. Playback popped the replacement surface, and Document.Finish popped it again, deterministically panicking with:

Pop() called without matching Push*()

A document page now validates the page prepared by NewPage, retains creator ownership, and makes finalization idempotent.

Verification

  • clean baseline reproduces creator replacement and the double-Pop panic
  • direct Recording.Playback(doc.NewPage(...))
  • Document.Playback with multiple pages
  • creator/page/surface identity and balanced transform stack
  • dimension mismatch, page-factory failure propagation, failed initialization, finalized-page, and post-Finish errors
  • repeated End/Finish safety
  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go test -count=1 -coverprofile=coverage.out ./...
  • local added executable-line patch coverage: 100% (50/50)
  • go build ./...
  • go vet ./...
  • go mod tidy -diff
  • Linux, Windows, Darwin, and ARM64 cross-builds
  • golangci-lint v2: 0 issues
  • gofmt and git diff --check

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

This is independent of #2 (Y-axis transform order) and #3 (custom page dimensions), but touches the same page-initialization lines. Rebase simulations with each PR and their combined state pass all tests, race, vet, and build checks.

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

@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.

This is the most architecturally significant PR in the batch. The core problem: Document.NewPage creates a page on the document's creator, but Recording.Playback calls Begin() which creates a second page on a new creator — losing the document-owned one.

The fix is correct: pageBackend.Begin() validates instead of recreating, and pageBackend.End() is idempotent (safe for both playback and Document.Finish to call). The newPage callback for error injection in tests is a nice touch.

Lifecycle guards (finished document, dimension mismatch, double-end) are thorough. 231 lines of tests covering ownership, multi-page, and failure paths.

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 merged commit 5149c67 into gogpu:main Aug 11, 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