Fix #37 and #35: Title styling, frame positioning, and unique text-style ids

[leogdion]

Closes #37. Closes #35.

Two issues, two commits, one PR. They are folded together because both rewrite the same ~14 lines of Title.build and would otherwise conflict in exactly the code being rewritten.

Commit 1 — #35: unique text-style-def ids

Title.build hardcoded "ts1" in both the TextStyle(ref:) and the TextStyleDef(id:) it emitted, for every title. The DTD declares text-style-def/@id as ID, which XML requires to be unique within a document, so any document containing two titles emitted invalid XML. Latent only because no shipped document had more than one title; the presentation deck has seven.

Adds a textStyleID() allocator to ResourceStore, modeled on the existing nextNumber counter but in its own ts1, ts2, … namespace. Ids are numbered globally across titles, matching what Final Cut itself writes — TestData/UntitledXML.fcpxml shows two titles producing ts1..ts4 rather than restarting per title.

The counter starts at 1, so single-title documents still emit exactly ts1 and no fixtures needed editing.

Commit 2 — #37: styling and positioning

Adds TitleStyle plus .font, .fontSize, .fontFace, .fontColor, .alignment, .bold, .name, and FramePosition with an alignment-first .position(_:inset:) and an absolute .position(x:y:) escape hatch.

The coordinate unit is verified, not assumed. Percent of frame height on both axes, from the centre, Y-up. The test reproduces real Final Cut output exactly — TestData/UntitledXML.fcpxml:448 has position="-17.8241 7.77778", and the test asserts both components land on those values rather than on a hand-derived number.

Frame size threading. Rather than adding a parameter to DSLNode.build, the frame size rides on ResourceStore, which is already threaded through every build call. Sequence publishes its format's dimensions before building children and restores the outer value afterwards, so nested sequences do not leak frame sizes. This keeps DSLNode.build's signature stable, which matters because #36 is making that protocol public.

Nothing is emitted unless asked for. An unstyled, unpositioned title serializes byte-identically to today; bold is written only when true; fontSize collapses whole numbers (63, not 63.0). The feature-pair structural diff against a real Final Cut export passes unchanged.

Also fills in AdjustTransform's missing DTD attributes — enabled, rotation, anchor — which were silently dropped. Note the DTD declares enabled first, so CodingKeys order is enabled, position, scale, rotation, anchor.

Ordering guard

Position resolution rewrites title elements on the way out, and the diff engine's inventory is order-blind (a reordered spine reports zero loss). PositionedSpineOrderingTests asserts a [title, transition, title] spine survives in order after resolution, at both the model and serialized-XML level.

Note for reviewers

Both DTD-validation tests export at 1.14 deliberately. At the 1.13 default these documents are also invalid for an unrelated reason — the default smart collections emit match-analysis-type, which 1.13 does not declare (#41, fixed in #42) — and that failure would mask the regressions these tests exist to catch.

x and y are added to SwiftLint's identifier_name exclusions, alongside the existing id/no.

Verification

• FCPKIT_REQUIRE_DTD=1 swift test — 68 XCTest + 49/12/37 Swift Testing, all passing
• fcpxml-diff schema-completeness --fail-if-total-exceeds 0 — no structural loss
• swift-format lint and swiftlint — clean

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d32a80e2-f4bd-4b4e-9835-2c3bc05d0e1a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[leogdion]

Follow-up: adversarial review found a crash

Pushed 17b1924 after re-reviewing this branch. Three bugs, all sharing a root cause — unvalidated Double→String conversion, and an Optional treated as "use a default" rather than "cannot resolve".

1. Crash (the one that matters). fontSize and the position formatter both did an unguarded Int(value) after a whole-number check. 1e21 is a whole number:

Title("Hi").fontSize(1e21)
→ Fatal error: Double value cannot be converted to Int because the result would be greater than Int.max
→ exited with unexpected signal code 5

Infinity and NaN take the same path. Reproduced in a real test run, not reasoned about. Fixed with a shared decimalString(_:) that falls back to the plain description outside Int's range or when non-finite. A library must not take the host process down on user input.

2. .position(.top, inset: 80) with no format silently dropped the inset, emitting "0 50". An inset is in points; converting to percent-of-height needs the frame height. The absolute .position(x:y:) path already threw missingFrameSize in exactly this situation — so the design already agreed it's unresolvable, and the alignment path just failed silently instead. Now it throws too. Zero-inset alignments still never throw.

3. .position(.center, inset: 100) emitted a pointless adjust-transform with "0 0" — breaking the "centred titles emit nothing" invariant while ignoring the inset. .center has no direction to inset along, so it now always resolves to no transform.

Also corrected a misleading test comment: the fixture whose position="-17.8241 7.77778" the formula test reproduces uses FFVideoFormat3840x2160p24, not 1080p as I'd written. The unit is percent-of-height and therefore scale-invariant, so the test passed for the right reason — but the stated provenance would have misled whoever derives from it next.

Gate after the fixes: 68 XCTest + 49/12/42 Swift Testing (was 37), schema-completeness clean, 0 lint violations.

One design question I did not change

Zero-inset .leading resolves to -88.889 and .top to 50 — the frame edges. That puts a centred title's centre on the edge, so half of it sits off-screen. Defensible (it's literally the alignment point) but arguably not what .top should mean in a SwiftUI-shaped API, where it usually means "inside the top edge". Left as-is since changing it is a design call, not a bug fix — flagging it for your judgement.

[leogdion]

1. Make it a rule never to have global functions
2. Is there something here to use https://developer.apple.com/documentation/foundation/decimal/formatstyle

[leogdion]

Done in aec1ed6, on both points.

1. Global function. Now AttributeValue.decimal(_:), file renamed to match. Applied module-wide — storyItems/anchoredItem(s) on #43 and softPromote on #42 got the same treatment, so FCPKitDSL has no top-level functions left.

2. Decimal.FormatStyle — I tested it, and it's the wrong tool here. It does collapse whole numbers natively, which is the one thing it would buy us, but three problems:

locale    Decimal(string: "63.5").formatted(.number.grouping(.never))
en_US  →  63.5
de_DE  →  63,5      ← invalid FCPXML
fr_FR  →  63,5

• Locale-aware. That comma would produce invalid FCPXML on any non-English machine. This is the decisive one — these are machine-readable attribute values, not display strings. Now pinned by decimalStringsUseAPeriodRegardlessOfLocale so nobody swaps a locale-aware formatter back in.
• Rounds to six fractional digits. 7.777777777 → 7.777778. Fine for a font size, silently lossy for a position component.
• Decimal(Double.infinity) traps, so it doesn't even solve the crash that motivated the helper.

All three verified by running them rather than reasoning about them. The rationale is in the doc comment so this doesn't get re-litigated later.

If you'd still prefer it for the font-size case specifically — where precision and range are not really at issue — say the word and I'll split the two call sites. My read is that one formatter with one rule is easier to reason about than two.

[leogdion]

Done in 517a1c2 — went with the String init.

internal init(fcpxmlValue value: Double)

Both call sites produce a String from a Double, which is what an init is for, and it keeps the conversion attached to the type being produced rather than hanging a domain-specific property off Double. Call sites read as String(fcpxmlValue: fontSize).

One thing worth flagging: the argument label is load-bearing. String(_: Double) already exists via LosslessStringConvertible, so an unlabelled init would have quietly overloaded it — anyone writing String(someDouble) would have silently got the FCPXML formatter instead of the stdlib one. I verified the labelled version coexists cleanly: String(63.5) still routes to the stdlib and returns "63.5".

Behaviour unchanged — same guards, same fallbacks, same tests, and the exported deck is byte-identical.

[leogdion]

Typed throws adopted (1920647)

Per your note on the other PRs, FCPKitDSL now uses throws(BuildError) end to end — DSLNode.build through Document.export. This branch had no namespace enums to remove, so typed throws is the whole change here.

It was well-scoped rather than a stretch: every throw site in the module already raised BuildError and nothing else, and the module calls no throwing FCPKit API. So there were no catch sites to rewrite, and callers now get a concrete error type instead of any Error.

Two things worth knowing:

It caught things I'd have missed. Typing ModelSequence.init immediately surfaced that ResourceStore.format and the entire Layout packing chain were still untyped — the compiler walked the call graph for me.

Closures need explicit annotations. try map { … } does not infer the thrown type, so propagating closures need try map { x throws(BuildError) in … }. In Title.build the flatMap/map chain resolving a deferred position is rewritten as a plain if let — types cleanly and reads better than an annotated chain.

One API narrowing, deliberate: an external conformer declaring plain throws no longer satisfies DSLNode.build. That is a hard compile error, not a warning. DSLNode is public as of #43 and already carries a - Warning: that its signature evolves until 1.0, so this is in-policy — but it is a narrowing and worth knowing before merge.

Typed throws for the remaining modules is filed as #47 — FCPKit is a much bigger job (47 throwing declarations across six error types) and needs a design call first.

Gate: 68 XCTest + 49/12/43 Swift Testing, 0 lint violations, no structural loss.