Layout primitives: VStack, HStack, ZStack, Spacer, padding (#65) - #84
Conversation
Authoring 15-20 demo slides with absolute `.position(x:y:)` on every drawable is brittle. Stacks make the deck writable. **No archive work.** `SlideDrawable` already carries absolute x/y and the surgeon already writes those numbers, so a stack is a pure source-level convenience: `Slide.init` walks the tree once and each drawable comes out carrying the position an author could have typed by hand. `items` stays the same flat list it always was, and nothing downstream — lowering, z-order, builds, the archive — knows layout exists. ## Shape - `LayoutNode` — leaf / stack / spacer / padded - `SlideLayout` — what the builder collects; `TextBox` and `Image` conform - `VStack` / `HStack` / `ZStack`, `Spacer`, `.padding(_:)`, `EdgeInsets` - `SlideItemsBuilder` now collects `any SlideLayout` so stacks nest ## Two decisions worth review **A leaf outside any stack keeps its authored position.** `resolve` takes an optional origin; `nil` means "nobody is positioning you". Without this, every existing deck would have been silently moved to (0,0) — the whole acceptance catalog. The regression test for it is the most valuable one here. **A top-level stack inherits the slide canvas (1920x1080) as bounds**, so `Spacer` works without an explicit `.frame()` — pinning content to the top and bottom of a slide is the common case. A spacer only collapses when nested inside an unframed parent, where there genuinely is no slack. ## Sizing Explicit child sizes only, per the issue. An unsized child contributes zero extent and siblings pile up at the same offset — documented and tested rather than silently surprising. Intrinsic measurement is #67, split out because it is coupled to #52. ## Verification `swift test` 95 green (13 new), `LINT_MODE=STRICT` exit 0, all 14 acceptance decks regenerate clean — the last of which is the real proof that existing decks are unaffected. Structural only; the stacks still need a human render pass. Refs #65 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
| /// issue #67, split out because it is coupled to #52 (Keynote lays | ||
| /// placeholder text out at the layout master's width, ignoring the authored | ||
| /// frame). | ||
| public indirect enum LayoutNode: Sendable { |
There was a problem hiding this comment.
why are we using enums instead of protocols?
There was a problem hiding this comment.
Converted both LayoutNode and SlideDrawable to protocols.
Worth flagging what I found: SlideDrawable was pre-existing public API, not new in this PR — 9 switch sites. Seven were pure forwarding (zIndex, builds, actions, magicIdentifier, authoredSize, positioned, resolvedPositions) and became requirements with no loss. Two needed real design work:
Lowering. Deck+Lowering switched to build an 18-field TextItem vs an 8-field ImageItem. That construction now lives on each type, next to the fields it reads. It's a separate package protocol LowerableDrawable, because the result type AuthoredSlide.DrawableItem is package-level and a public protocol can't require it. A drawable that isn't lowerable now throws rather than being silently dropped.
Magic Move. Deck+MagicMove switched over a pair of cases to tell a type mismatch from a content mismatch. That's now magicMoveIdentity, splitting kind from content so both failures stay distinguishable.
The archive boundary is unchanged. The surgeon still switches exhaustively on AuthoredSlide.DrawableItem, which stays a closed package enum — the writer genuinely must handle every case it can receive. Only the authoring surface opened.
LayoutNode is now a protocol with LeafNode/StackNode/SpacerNode/PaddedNode, each owning its resolve logic, which removed the resolveStack/resolveDepthStack static dispatch.
95 tests green including the golden differential and magic-move suites; all 14 acceptance decks regenerate clean, which is the real proof written output didn't change.
| /// Test support for the layout pass (#65): stacks resolve at build time, | ||
| /// so the only way to assert a stack laid out correctly is to read the | ||
| /// positions it produced. | ||
| package var resolvedPositions: [(Double, Double)] { |
There was a problem hiding this comment.
can the tuple be a new type?
There was a problem hiding this comment.
Now returns [LayoutPoint].
The nice side effect: LayoutPrimitivesTests was carrying a hand-rolled == on [(Double, Double)] purely because tuples aren't Equatable. That's deleted — LayoutPoint is Equatable, so the assertions compare directly.
Also took the access-level note from #85 here since it's the same declaration: resolvedPositions was the repo's only package test accessor on a public DSL type. It's internal now, with the test switched to @testable import — matching the 8 test files that already do, and matching Slide.items which it reads.
| /// number to advance by — so ``LayoutNode`` treats an unsized child as | ||
| /// zero-extent along the stack's axis and documents the consequence. | ||
| /// Intrinsic measurement is issue #67. | ||
| internal var authoredSize: (width: Double?, height: Double?) { |
There was a problem hiding this comment.
Create a new type for this tuple
There was a problem hiding this comment.
Now DrawableSize — width: Double?, height: Double?.
Worth noting there was already a LayoutSize struct in the same file as the origin: tuple, so resolve(in bounds: Size?, origin: (x: Double, y: Double)?) was taking a struct for one parameter and a tuple for the other in one signature. That inconsistency was the real defect, so I added LayoutPoint as LayoutSize's peer too.
DrawableSize is deliberately separate from LayoutSize rather than reusing it with optionals: nil in an axis means "inherit the template placeholder's extent," which is a different concept from a resolved extent of zero. LeafNode.size is where the two meet — it maps nil to 0 with a comment explaining that a stack has no number to advance by until #67.
Addresses all three review comments. ## SlideDrawable and LayoutNode are protocols Both converted. `SlideDrawable` was pre-existing public API with 9 switch sites; 7 were pure forwarding and became requirements with no loss. Two sites needed real design work: **Lowering.** `Deck+Lowering` switched to build an 18-field `TextItem` vs an 8-field `ImageItem`. That construction now lives on each type via `LowerableDrawable`, next to the fields it reads. It is a SEPARATE package protocol because the result type `AuthoredSlide.DrawableItem` is package-level and a public protocol cannot require it. A drawable that is not lowerable now THROWS rather than being dropped — a silently missing drawable is the class of bug the acceptance decks exist to catch, so it should not be silently possible. **Magic Move.** Validation switched over a PAIR of cases to distinguish a type mismatch from a content mismatch. That is now `SlideDrawable.magicMoveIdentity`, splitting kind from content so both failures stay distinguishable. Geometry stays excluded — differing geometry is the motion, not a mismatch. The archive boundary is untouched: the surgeon still switches exhaustively on `AuthoredSlide.DrawableItem`, which stays a closed package enum. Only the authoring surface opened. `LayoutNode` becomes a protocol with `LeafNode` / `StackNode` / `SpacerNode` / `PaddedNode`, each owning its own resolve logic. That removes the `resolveStack` / `resolveDepthStack` static dispatch entirely. ## Named types for the tuples `LayoutSize` existed already, and `resolve(in: Size?, origin: (x:,y:))` took a struct for one parameter and a tuple for the other in a single signature — that inconsistency was the real defect. Added `LayoutPoint` as its peer, plus `DrawableSize` for the optional-component `authoredSize`. `resolvedPositions` now returns `[LayoutPoint]`, which let the test DELETE its hand-rolled `==` on `[(Double, Double)]` — that existed only because tuples are not `Equatable`. ## Access levels `resolvedPositions` was the repo's only `package` test accessor on a public DSL type. Now `internal`, with `LayoutPrimitivesTests` switched to `@testable import` — matching the 8 test files that already do, and matching `Slide.items` which it reads. ## Verification `swift test` 95 green — including `GoldenDifferentialTests` and the magic move suite, the two most exposed to this change. All 14 acceptance decks regenerate clean, which is the real proof written output is unchanged. `LINT_MODE=STRICT` exit 0. Refs #65 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebased onto #84, which is what makes the conformance possible. ## CodeBlock is placeable directly ```swift CodeBlock(source) .background(CodeTheme.midnightBackground) .position(x: 100, y: 200) .frame(width: 1_700, height: 700) ``` Conforming to `SlideLayout` lets a block sit in a `Slide` builder next to a `TextBox` or a stack. The conformance alone is one line; the ergonomics needed the modifiers forwarded, so `position`/`frame`/`background`/`zIndex` return `CodeBlock`. They accumulate a `@Sendable (TextBox) -> TextBox` transform rather than mirroring each field, so adding a forwarded modifier stays one method and never drifts from `TextBox`. `textBox` remains public as the escape hatch for anything not forwarded. All three acceptance-deck call sites dropped it. ## SwiftHighlighter is a struct Follows the `private init` + `static let default` pattern the agent-notes directive prescribes for a helper that could grow configuration — language selection beyond Swift being the obvious one. Worth noting the convention is not one-sided: the same directive explicitly sanctions caseless enum for stateless helpers, and 18+ exist in `Sources/` including `package enum UUIDMapVerifier`. This one converts because the config trigger genuinely applies, not because the enum was wrong. ## Access levels `TextBox+Inspection` accessors are `internal`, not `package`. The repo's `package` declarations are whole-type lowering DTOs shared across targets, never per-property test accessors — these were the only exception. Tests reach them through `@testable import`, which works across the module boundary from `KeynoteKitSyntaxTests`. ## Verification `swift test` 95 core + 16 syntax green, `LINT_MODE=STRICT` exit 0, all 15 acceptance decks regenerate clean. Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebased onto #84, which is what makes the conformance possible. ## CodeBlock is placeable directly ```swift CodeBlock(source) .background(CodeTheme.midnightBackground) .position(x: 100, y: 200) .frame(width: 1_700, height: 700) ``` Conforming to `SlideLayout` lets a block sit in a `Slide` builder next to a `TextBox` or a stack. The conformance alone is one line; the ergonomics needed the modifiers forwarded, so `position`/`frame`/`background`/`zIndex` return `CodeBlock`. They accumulate a `@Sendable (TextBox) -> TextBox` transform rather than mirroring each field, so adding a forwarded modifier stays one method and never drifts from `TextBox`. `textBox` remains public as the escape hatch for anything not forwarded. All three acceptance-deck call sites dropped it. ## SwiftHighlighter is a struct Follows the `private init` + `static let default` pattern the agent-notes directive prescribes for a helper that could grow configuration — language selection beyond Swift being the obvious one. Worth noting the convention is not one-sided: the same directive explicitly sanctions caseless enum for stateless helpers, and 18+ exist in `Sources/` including `package enum UUIDMapVerifier`. This one converts because the config trigger genuinely applies, not because the enum was wrong. ## Access levels `TextBox+Inspection` accessors are `internal`, not `package`. The repo's `package` declarations are whole-type lowering DTOs shared across targets, never per-property test accessors — these were the only exception. Tests reach them through `@testable import`, which works across the module boundary from `KeynoteKitSyntaxTests`. ## Verification `swift test` 95 core + 16 syntax green, `LINT_MODE=STRICT` exit 0, all 15 acceptance decks regenerate clean. Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
SwiftUI-style stacks so the demo deck (#56) can be authored without absolute
.position(x:y:)on every drawable.Branches off
v0.1.x; no overlap with the #66 syntax lane.No archive work at all
SlideDrawablealready carries absolute x/y and the surgeon already writes those numbers. So a stack is a pure source-level convenience:Slide.initwalks the tree once and each drawable comes out carrying the position an author could have typed by hand.itemsstays the same flat list, and nothing downstream — lowering, z-order, builds, the archive — knows layout exists.Two decisions worth your review
1. A leaf outside any stack keeps its authored position.
resolvetakes an optional origin;nilmeans "nobody is positioning you."I got this wrong first: my initial version resolved every leaf at the passed origin, which would have silently moved every drawable in every existing deck to (0,0) — the entire acceptance catalog.
unstackedDrawablesKeepPositionsis the regression test, and the 14 acceptance decks regenerating clean is the real proof.2. A top-level stack inherits the slide canvas (1920×1080) as bounds, so
Spacerworks without an explicit.frame(). Pinning content to the top and bottom of a slide is the common case, and requiring a frame that always equals the canvas is noise. A spacer only collapses when nested inside an unframed parent, where there genuinely is no slack to divide. Both behaviours are tested.Sizing is explicit, per the issue
An unsized child contributes zero extent, so siblings pile up at the same offset. That's documented and tested (
unsizedChildAdvancesNothing) rather than silently surprising. Intrinsic measurement is #67, split out because it's coupled to #52 — which we now know first-hand affects fill extent in both dimensions, not just width.One API change to flag
SlideItemsBuildernow collectsany SlideLayoutinstead of[SlideDrawable].Slide { … }is unchanged for callers, but one existing test drove the builder directly and pattern-matched its result; it now resolves the nodes first. No other call site needed touching.Verification
swift test— 95 green (13 new)LINT_MODE=STRICT ./Scripts/lint.sh— exit 0swift run AcceptanceDecks— all 14 decks regenerate and self-checkStructural only. The stacks need a human render pass before I'd call them done — the resolved numbers are asserted, but whether a stacked slide looks right is your call.
Refs #65