fix: parse hexadecimal and 8-bit .colorset color components (#146)#468
Open
arulagarwal wants to merge 3 commits into
Open
fix: parse hexadecimal and 8-bit .colorset color components (#146)#468arulagarwal wants to merge 3 commits into
arulagarwal wants to merge 3 commits into
Conversation
…ls#146 Adds three asset-catalog fixtures that encode the same color (#04F188) via each of Xcode's numeric "Input Method" variants: HexColor (8-bit hexadecimal, 0x04/0xF1/0x88), FloatColor (floating point), and IntColor (8-bit 0-255, 4/241/136). Used by the issue skiptools#146 regression tests and reachable via Bundle.module in SkipUITests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kiptools#146) Custom colors only parsed correctly when the .colorset "Input Method" was "Floating Point". With "8-bit Hexadecimal" (e.g. "0xF1") the transpiled Kotlin read channels via toDoubleOrNull -> null, so every channel fell back to 0.0 and the color rendered pure black; "8-bit (0-255)" (e.g. "148") parsed as 148.0 and clamped to white. The issue asks to handle all Input Method variants. Channels now route through a new parseColorComponent helper that covers all three: an 0x/0X/# prefix is parsed as an 8-bit hex byte (via Kotlin's toIntOrNull(radix:), since Swift's Int(_:radix:) does not transpile); a bare integer with no decimal point is treated as an 8-bit (0-255) value; both are normalized by /255. Anything else is parsed as floating point, so existing decimal colorsets are unchanged. Hex is detected by prefix rather than by Double(_:) failing, because Swift parses Double("0xF1") as 241.0 while Kotlin's toDoubleOrNull is null -- the prefix check keeps both platforms identical (and is why skiptools#146 was Android-only). Malformed/empty input preserves the previous defaults. parseColorComponent is a free function rather than a static method on the Decodable ColorComponents struct: a static member breaks Skip's reflection-based JSON decoding of ColorSet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s#146) Adds DISABLEDtestHexColorset, DISABLEDtestFloatColorset, and DISABLEDtestIntColorset, which render the three fixtures and assert each produces #04F188 (all Input Methods agree, and none is the pre-fix black). They are DISABLED-prefixed because decoding a bundled component .colorset throws a kotlin-reflect IllegalAccessException under the Robolectric unit runner (it works on a real device, where skiptools#146 was reported); run them on an emulator/device. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thank you for your pull request and welcome to the Skip community. We require contributors to sign our contributor license agreement (CLA), and we don't seem to have the user(s) @arulagarwal on file. In order for us to review and merge your code, for each noted user please add your GitHub username to Skip's .clabot file |
Author
|
@cla-bot check |
|
recheck |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Xcode's Asset Catalog lets each
.colorsetchannel use a different "Input Method". SkipUI's Android colorset parser assumed the Floating Point method, where each channel is a 0–1 decimal string like"0.945". When a color is authored with the 8-bit Hexadecimal method, Xcode emits the same JSON shape but encodes each channel as a hex byte string like"0xF1". On the transpiled (Android) path these are read withtoDoubleOrNull, which returnsnull, so every channel falls back to0.0and the color renders pure black (#146). It is not a crash — JSON decoding succeeds and nothing is logged; the only symptom is wrong pixels.This affects Android only: on Apple platforms
Color(_:bundle:)defers to SwiftUI's own asset loader, so the defect lives entirely in the#if SKIPcolorset code inColor.swift.What
ColorComponentsnow routes each channel (red/green/blue/alpha) through a new file-levelparseColorComponent(_:defaultValue:)helper that covers all three of Xcode's numeric Input Methods: an0x/0X/#prefix is parsed as an 8-bit hex byte (via Kotlin'stoIntOrNull(radix:), since Swift'sInt(_:radix:)does not transpile); a bare integer with no decimal point is treated as an 8-bit (0–255) value; anything else is parsed as a floating-point string, so existing decimal colorsets are byte-for-byte unchanged. Hex and 0–255 values are normalized to 0–1 by/255. Malformed or empty input preserves the previous defaults (0.0for RGB,1.0for alpha), so nothing can regress. It is a free function rather than astaticmethod on theDecodableColorComponentsstruct, because adding a static member breaks Skip's reflection-based JSON decoding ofColorSet.The hex prefix is detected explicitly rather than relying on
Double(_:)to reject hex, because Swift parsesDouble("0xF1")as241.0while the transpiled Kotlin"0xF1".toDoubleOrNull()isnull. A "tryDoublefirst" approach would therefore behave differently across platforms (and is precisely why this bug is Android-only); prefix detection keeps Swift and Kotlin identical.The 8-bit (0–255) decimal method (e.g.
"148") is distinguished from floating point by the absence of a decimal point — Xcode writes floats as"1.000"and 8-bit as bare integers — so it's handled too, since the issue asks to cover all Input Method variants. (Grayscale colorsets, which usewhite/alphacomponents instead of RGB, are a separate pre-existing gap not addressed here.)Before / After evidence
parseColorComponentwas exercised over 13 cases spanning all three Input Methods (floating point,0x/0X/#hex, lowercase hex, 8-bit148/255/4, empty/nil, malformed):swift test --filter ColorTests→ 2 tests, 0 failures (native build clean; existing tests unaffected — the template's required check).skip test --filter ColorTests(transpiled Kotlin + Robolectric) → succeeded, confirming the fix compiles/transpiles correctly and the suite stays green. The three new render tests (DISABLEDtestHexColorset/DISABLEDtestFloatColorset/DISABLEDtestIntColorset) assert all three encodings render#04F188; they areDISABLED-prefixed because decoding a bundled component.colorsetthrows a kotlin-reflect error under the Robolectric unit runner (it works on a real device) — run them on an emulator/device.Acceptance criteria
DISABLEDtestHexColorset/DISABLEDtestFloatColorset/DISABLEDtestIntColorset, one per Input Method); the parsing algorithm is also covered by a standalone 13-case logic checkswift test2/2;skip teston Robolectric succeeded)Color.swift/ColorTestsconventions)Closes #146
Skip Pull Request Checklist:
swift testColorComponents, draft theparseColorComponenthelper, and scaffold the fixtures and tests. Manual verification: I read and understood every line, empirically confirmed theDouble("0xF1")Swift-vs-Kotlin parsing difference that drove the prefix-first design, traced the#if SKIPcode path, and ranswift testandskip test(Robolectric) plus a standalone 10-case logic check to confirm the before/after behavior. The localskip testrun also surfaced two transpilation issues I then fixed: Swift'sInt(_:radix:)does not transpile (switched to Kotlin'stoIntOrNull(radix:)), and astatichelper on theDecodablestruct brokeColorSetdecoding (moved it to a free function).