Fix default FXL spread positions#714
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts fixed-layout EPUB spread construction and JSON page positioning so the first FXL resource is displayed alone by default (Apple Books-like), and unpaired single pages get a corrected default left/right position.
Changes:
- Updates FXL two-page spread generation to default the first resource to a centered single spread (unless overridden by
offsetFirstPageor explicitpageproperty). - Reworks default page position for
EPUBSingleSpreadJSON output to distinguish first vs. non-first single pages. - Adds comprehensive navigator spread unit tests and documents the fixes in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/NavigatorTests/EPUB/EPUBSpreadTests.swift | Adds new unit tests covering spread creation, pairing rules, and spread helper APIs. |
| Sources/Navigator/Preferences/Types.swift | Removes ReadingProgression.startingPage helper used for default positioning. |
| Sources/Navigator/EPUB/EPUBSpread.swift | Implements new first-page centering behavior for FXL and fixes default page position for single spreads. |
| CHANGELOG.md | Records the Navigator fixes under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5b95710 to
da190d6
Compare
| /// Builds a list of two-page spreads for the given Publication. | ||
| /// | ||
| /// - Parameter offsetFirstPage: User preference to offset the first | ||
| /// resource. |
There was a problem hiding this comment.
Can all be parameters be documented here? swift build -Xswiftc otherwise throws warnings for parameters that aren't documented if at least one is.
There was a problem hiding this comment.
I updated to remove the - Parameter and inline the explanation for offsetFirstPage instead. I'm not a fan of having a list of - Parameter when they are self-explanatory, but I would compromise for public APIs.
Are you able to only document the public APIs? I think documenting (or checking) all the internal APIs would add too much noise.
| ] | ||
| } | ||
|
|
||
| /// Returns the default spread position (left or right) for the single |
There was a problem hiding this comment.
Since this is new, it would be helpful to use the returns section. https://developer.apple.com/library/archive/documentation/Xcode/Reference/xcode_markup_formatting_ref/Returns.html
There was a problem hiding this comment.
I don't think this improves the Quick Help, as this is the short description of what this function does. I prefer to use - Returns to clarify the return value in the context of the function. For example:
swift-toolkit/Sources/Navigator/Input/InputObservable.swift
Lines 12 to 17 in bc32270
There was a problem hiding this comment.
In this case, I think I should have used "Resolve the default spread position..." instead of "Returns...". That was lazy wording 😅


Fixed
Navigator