Background fill for text boxes (#78) - #80
Conversation
|
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 |
|
Author update — took the naming note and did the rename, plus the interop.
|
|
Author update — render pass reviewed. Thanks for running it. I looked at the exports rather than take "seems perfect" at face value, and the fill itself is correct — but the deck surfaced two real defects, one of which is a blocker for #66. What #78 got right
So the shape-style fork, the TSD nesting, and Defect 1 — multi-paragraph formatting is dropped (filed as #81)Slide 1's code panel should be near-white text in Menlo 44. It rendered near-black in the template's proportional face — Slide 4 in the same deck, same run, got its white text. The difference is paragraph count: slide 4's box is one paragraph, slide 1's is three. Root cause, from the archive: Keynote wants one style run per paragraph; we write one at index 0. The minted style is correct — only the run table is wrong. This is the same bug behind the #64 monospace failure, which is why It also means #37's whole-item formatting verification only ever covered the single-paragraph case. Defect 2 — my acceptance deck's geometry is wrongSlide 2 authored three 460pt boxes at x=100/640/1180. They rendered ~535 and ~830 wide, so the blue box swallowed the "NOT filled" one — which now sits on blue and reads as filled, the opposite of what the slide is meant to prove. Slide 1's panel also fills ~650pt against an authored 320. That is #52 (placeholder text laid out at master width, ignoring the authored frame) applying to the fill's extent too — pre-existing, not caused by this PR, but it makes my deck misleading. I'll rework it to one box per slide so the frames can't collide. What I'd like to do
The fill itself I'd call render-verified. Say the word if you'd rather I fold #81 into this branch instead. |
| Deck { | ||
| Slide { | ||
| TextBox("Panel") | ||
| .background(TextColor(red: 0.2, green: 0.2, blue: 0.2)) |
There was a problem hiding this comment.
why is the type called TextColor instead of just Color? Look at Color in SwiftUI for guidance. Support other Color types by OS
| /// `KeynoteKit` enum that shadows the module name — so a downstream target | ||
| /// importing both cannot spell `KeynoteKit.Color` to disambiguate. | ||
| /// `KeynoteKitSwiftUI` uses this alias. | ||
| public typealias KeynoteColor = Color |
|
Author — alias removed. It took removing something else too, so flagging the chain. The alias existed because bare That enum held exactly one member: /// Placeholder version, replaced when the write path lands.
public static let version = "0.1.0"The write path landed a while back, so the placeholder outlived its own doc comment, and nothing but its own test referenced it. Removing it makes module qualification work normally — and removes the need for the alias entirely. Callers now write the standard spelling: KeynoteKit.Color(swiftUIColor) // was: KeynoteColor(swiftUIColor)
nsColor.keynoteColor // unchangedAlso repointed the "module is linkable" test at a real public type rather than a version constant — a better check regardless. This is a public API removal, deliberately before v0.1.0 tags. If you'd rather keep a
|
Adds `TextBox.background(_:)` — a solid fill behind the text, the code-panel treatment the demo deck (#56) needs. Without it the only workaround is a dark PNG behind the box via `.image` + `zIndex`, which across 15-20 slides of varying code length means hand-sizing every panel and matching none of them exactly. The fill rides the shape-style fork that already exists for vertical alignment and columns: same type-2025 variation, same guard, same six registration edges. It writes `TSD.FillArchive` into the TSD-level property bag (`style.super.shapeProperties`) rather than the TSWP-level one that carries alignment and columns. I planned to split `overrideCount` into independent TSD and TSWP counters — the existing code writes both from one variable, which looked like an artifact of the TSD bag always being empty. That was wrong, and the archive says so. Surveyed every `TSWP.ShapeStyleArchive` in `build_action_B.key`: all 29 satisfy `overrideCount == super.overrideCount`. The decisive case is variation 2651764, which sets a TSD fill *plus* TSWP vertical alignment and padding and writes 4/4 — not the 1/3 a per-bag reading predicts. So the count is a total across both bags, mirrored into both fields, and a single counter is correct. Recorded in `research/findings/text_columns.md`, where the original wording invited the misreading. That survey also explains why unfilled theme boxes draw nothing: their fill is present but *colorless* (`fill.hasColor == false`). - `swift test` — 74 tests green, including the golden differential, which is the real proof that fill-free decks are unperturbed - `swift run AcceptanceDecks` — `background_fill.key` writes and self-checks - `LINT_MODE=STRICT ./Scripts/lint.sh` — exit 0 Structural only. Whether Keynote *draws* the fill, and draws it behind the text rather than over it, needs the human render pass. `ShapeStyleTests` outgrew the 225-line limit, so its archive-walk helpers moved to a shared `ShapeStyleProbe` and the fill cases to their own suite — restructured rather than exempted, per the standing directive. Refs #78 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`TextColor` was named when text color was the only color in the DSL. #78 made it fill shapes too, so the name is now actively wrong. Renamed to `Color` and modelled on SwiftUI's, per review feedback. ## Color - `opacity` rather than `alpha`, matching SwiftUI's vocabulary - `init(white:opacity:)` convenience for greys - `.black` / `.white` / `.clear` - `opacity(_:)` modifier returning a copy - components clamped to `0...1` on init — the archive stores `Float`s that Keynote reads as sRGB, where out-of-range values render undefined rather than erroring Named colors stop at those three deliberately. KeynoteKit authors decks rather than draws UI; a full palette would be noise. ## Platform interop `CGColor` lands in core (CoreGraphics is conflict-free). AppKit, UIKit, and SwiftUI live in a new `KeynoteKitSwiftUI` target. That split is forced, not stylistic: AppKit's conformances collide with `KeynoteKit`'s `Never: SlideContent` — the slide DSL's primitive terminator — and importing it anywhere in the core module makes `Never.body` ambiguous and the module stops compiling. Splitting also keeps core linking nothing but swift-protobuf, so authoring still works on Linux, Windows, and Android. Everything converts to **sRGB**, since that is what the archive stores. The `CGColor`, `NSColor`, and `UIColor` bridges are failable: a pattern color has no components, and cross-space conversion can fail. Failing visibly beats writing a color that renders differently than the caller intended. The SwiftUI bridge needed the linear-to-sRGB transfer function — `Color.Resolved` exposes *linear* components while the archive stores sRGB-encoded ones, so passing them straight through renders visibly washed out (mid-grey would land at 0.216 instead of 0.5). `midGreyRoundTrips` guards exactly that. A `KeynoteColor` typealias lives in core because `KeynoteKit` declares a `KeynoteKit` enum that shadows the module name, so downstream targets importing both modules cannot spell `KeynoteKit.Color` to disambiguate. ## Verification - `swift test` — 80 in KeynoteKit (was 74) + a new 5-test bridge suite - `LINT_MODE=STRICT ./Scripts/lint.sh` — exit 0 Refs #78 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The render pass confirmed the fill itself — painted behind the text, exact colors, opt-in, alpha compositing correctly. But two slides were proving the wrong thing. **Side-by-side boxes overlap.** Slide 2 authored three 460pt boxes at x=100/640/1180; they rendered ~535 and ~830 wide, so the blue box swallowed the "NOT filled" one — which then sat on blue and read as filled, the exact opposite of the slide's point. That is #52 (placeholder laid out at the master's width, ignoring the authored frame) applying to the fill's extent. Now one box per slide wherever a fill is under test, stacked vertically with a wide gap. The composition slide splits into separate alignment and columns slides for the same reason. Five slides instead of four. **Slide 1 is now single-paragraph on purpose.** Its multi-line code sample rendered dark-on-dark because item-level `.foregroundColor` and `.font` are silently dropped on multi-paragraph boxes — filed as #81, and the same root cause as the #64 monospace failure. Keeping the multi-line sample would have failed this slide for a reason unrelated to the fill. The comment records that the multi-line version should come back when #81 lands, since that is the shape the demo deck actually needs. Refs #78, #52, #81 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round two's slide 2 still failed. Stacking the filled and unfilled boxes vertically with a 580pt gap was not enough — the fill ignores the authored 300pt height and paints ~760pt, so the unfilled box still landed inside the red and read as filled. Same #52 layout behavior as the width, which I had assumed was horizontal-only. One box per slide now: slide 2 filled, slide 3 the identical box unfilled. Six slides. Refs #78, #52 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per review. The alias existed because bare `Color` is ambiguous when a
target imports both `KeynoteKit` and `SwiftUI`, and the obvious
disambiguation — `KeynoteKit.Color` — did not compile.
The reason it did not compile was `public enum KeynoteKit`, which shadowed
the module name. That enum held exactly one member:
/// Placeholder version, replaced when the write path lands.
public static let version = "0.1.0"
The write path landed some time ago, so the placeholder outlived its own doc
comment, and nothing but its own test referenced it. Removing it makes
module qualification work normally, which removes the need for the alias.
Callers now write `KeynoteKit.Color(swiftUIColor)` — the standard spelling —
instead of learning a bespoke alias.
The "module is linkable" test now asserts against a real public type rather
than a version constant, which is a better check anyway.
Public API removal, deliberately before v0.1.0 tags.
`swift test` 80 + 5 green, `LINT_MODE=STRICT` exit 0, all 12 acceptance
decks regenerate clean.
Refs #78
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase integration. #82 landed `MultiParagraphFormattingContent` and its tests while this branch was open; they use `TextColor`, which this branch renames to `Color`. Mechanical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8491147 to
c540f4b
Compare
Adds
TextBox.background(_:)— a solid fill behind the text, so the demo deck (#56) can put code on a panel instead of on the slide background.Branches off
v0.1.xdirectly. #65 will stack on this branch, since both addTextBox.swiftmodifiers (PARALLEL-WORKTREES.md:267). When you merge this, retarget the #65 PR tov0.1.x.The API
Reuses
TextColorrather than minting a near-duplicate type. It's named for text but is a plain sRGB struct, and #78 only asks for a solid color — flagging it in case you'd rather rename it to something neutral before the API is public.The one thing worth reviewing
I planned to split
overrideCountinto independent TSD and TSWP counters. The existing code writes both from one variable, which read like an artifact of the TSD bag always being empty. That was wrong, and I only caught it by checking the archive rather than trusting the plan.Surveyed every
TSWP.ShapeStyleArchiveinbuild_action_B.key— all 29 satisfyoverrideCount == super.overrideCount. Decisive case: variation 2651764 sets a TSD fill plus TSWP vertical alignment and padding, and writes 4/4, not the 1/3 a per-bag reading predicts. So it's a shared total mirrored into both fields, and the single counter that was already there is correct.research/findings/text_columns.mdgains that finding — the original wording ("the TSD-level super carries a present-but-empty bag and the sameoverrideCount") is what invited the misreading, and it's no longer true now that fills populate that bag.Side finding: theme text boxes carry a present-but-colorless fill (
fill.hasColor == false), which is why an unfilled placeholder draws no background even though a fill message exists.Scope
Solid color only. Gradients, image fills, stroke, and fill on
.imageare out per the issue. Corner radius was listed as "optional if cheap" — it isn't: it needs its own archive investigation, so I left it out rather than guess.Verification
swift test— 74 tests green, includingGoldenDifferentialTests, which is the real proof that fill-free decks are unperturbedswift run AcceptanceDecks—background_fill.keywrites and self-checks (records decode, both SIGTRAP invariants hold)LINT_MODE=STRICT ./Scripts/lint.sh— exit 0Note on the byte-unchanged criterion: I first wrote it as whole-file byte identity and it failed — every
write(to:)mints fresh UUIDs, so two writes of the same deck already differ. That's pre-existing, not caused by this change. The test now asserts the fork's property bags and override count are unchanged, and the golden differential covers the cross-version claim.Render checklist (yours)
xcrun swift run AcceptanceDecks /path/to/out open /path/to/out/background_fill.key # copy first — Keynote autosaves in placeRefs #78
🤖 Generated with Claude Code