fix(REN-7): expand #RGB shorthand to #RRGGBB in hex-color decoders#235
fix(REN-7): expand #RGB shorthand to #RRGGBB in hex-color decoders#235kostub wants to merge 3 commits into
Conversation
… hex decoding Add decoder-level XCTest cases for UIColor(HexString)/NSColor(HexString): - #f00/#0f0/#00f/#fff/#000 (3-digit shorthand, currently failing) - #ff0000/#00ff00/#0000ff/#44aa99 (6-digit baseline, must stay passing) Tests fail before the fix; they prove the decoder bug is real. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
UIColor+HexString.m and NSColor+HexString.m both scanned the post-'#'
string as a raw hex int, so "#f00" was read as 0x000F00 (near-black green)
instead of pure red. Fix: strip '#', then if the remaining string is
exactly 3 chars duplicate each nibble ("f00" → "ff0000") before scanning.
6-digit paths are unaffected.
Coordinates with REN-2 / PR #230 which made the parser accept #RGB as
grammatically valid and deferred the decoder fix to this task (REN-7).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds support for expanding 3-digit CSS shorthand hex colors (e.g., #RGB to #RRGGBB) in both NSColor and UIColor categories, and introduces a comprehensive suite of unit tests to verify the implementation. The review feedback suggests optimizing the string expansion logic in both categories by replacing stringWithFormat: with stringWithCharacters:length: using a local stack array, which avoids the overhead of parsing format strings at runtime.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (hex.length == 3) { | ||
| unichar r = [hex characterAtIndex:0]; | ||
| unichar g = [hex characterAtIndex:1]; | ||
| unichar b = [hex characterAtIndex:2]; | ||
| hex = [NSString stringWithFormat:@"%C%C%C%C%C%C", r, r, g, g, b, b]; | ||
| } |
There was a problem hiding this comment.
Using stringWithFormat: with multiple %C format specifiers introduces unnecessary overhead because it requires parsing the format string at runtime. Since we are just duplicating characters to form a 6-character string, we can use stringWithCharacters:length: with a local stack array. This is significantly faster and more memory-efficient.
| if (hex.length == 3) { | |
| unichar r = [hex characterAtIndex:0]; | |
| unichar g = [hex characterAtIndex:1]; | |
| unichar b = [hex characterAtIndex:2]; | |
| hex = [NSString stringWithFormat:@"%C%C%C%C%C%C", r, r, g, g, b, b]; | |
| } | |
| if (hex.length == 3) { | |
| unichar r = [hex characterAtIndex:0]; | |
| unichar g = [hex characterAtIndex:1]; | |
| unichar b = [hex characterAtIndex:2]; | |
| unichar chars[6] = {r, r, g, g, b, b}; | |
| hex = [NSString stringWithCharacters:chars length:6]; | |
| } |
| if (hex.length == 3) { | ||
| unichar r = [hex characterAtIndex:0]; | ||
| unichar g = [hex characterAtIndex:1]; | ||
| unichar b = [hex characterAtIndex:2]; | ||
| hex = [NSString stringWithFormat:@"%C%C%C%C%C%C", r, r, g, g, b, b]; | ||
| } |
There was a problem hiding this comment.
Using stringWithFormat: with multiple %C format specifiers introduces unnecessary overhead because it requires parsing the format string at runtime. Since we are just duplicating characters to form a 6-character string, we can use stringWithCharacters:length: with a local stack array. This is significantly faster and more memory-efficient.
| if (hex.length == 3) { | |
| unichar r = [hex characterAtIndex:0]; | |
| unichar g = [hex characterAtIndex:1]; | |
| unichar b = [hex characterAtIndex:2]; | |
| hex = [NSString stringWithFormat:@"%C%C%C%C%C%C", r, r, g, g, b, b]; | |
| } | |
| if (hex.length == 3) { | |
| unichar r = [hex characterAtIndex:0]; | |
| unichar g = [hex characterAtIndex:1]; | |
| unichar b = [hex characterAtIndex:2]; | |
| unichar chars[6] = {r, r, g, g, b, b}; | |
| hex = [NSString stringWithCharacters:chars length:6]; | |
| } |
|
EM-REVIEW v1 REN-7 hex-color shorthand fix — reviewVerdict: APPROVE WITH ONE IMPORTANT NOTE (non-blocking). The decode logic is correct on both platforms and tests pass (301 via `swift test`). One real gap: the new test file is not wired into the iOS Xcode project, so the iOS code path this PR fixes has no executing automated coverage. Correctness — looks right
Platform parity — confirmedThe edits to `UIColor+HexString.m` and `NSColor+HexString.m` are character-for-character identical in the changed region. Good — this was the easy thing to get half-right. IMPORTANT: iOS Xcode project never compiles/runs the new test`iosMath.xcodeproj` uses explicit file references (no `fileSystemSynchronized` groups — count is 0). All 4 pre-existing test files (`MTMathListBuilderTest`, `MTMathListTest`, `MTTypesetterTest`, `MTFontManagerTest`) are registered in the iosMathTests target's Sources build phase. `MTColorDecoderTest.m` is the only `.m` on disk with zero references in the pbxproj. Consequence: `xcodebuild test -project iosMath.xcodeproj` (the iOS test command in CLAUDE.md, and what CI likely runs) silently excludes these tests. `swift test` runs them, but on macOS that only exercises the `NSColor` / `#if !TARGET_OS_IPHONE` branch. The `UIColor` decoder fix and the `#if TARGET_OS_IPHONE` `getRed:green:blue:alpha:` test branch are therefore never executed by any green build — the primary-platform fix is untested in practice. Fix: add `MTColorDecoderTest.m` to the iosMathTests target in `iosMath.xcodeproj` (PBXBuildFile + PBXFileReference + group child + Sources phase, mirroring the existing 4 entries). (MacOSMath.xcodeproj has no test target, so nothing needed there.) Minor / out of scope
No blocking issues. Recommend wiring the test into the iOS project before merge so the iOS fix is actually guarded. |
The iOS Xcode test target excluded MTColorDecoderTest.m, so only the macOS swift test path exercised it. Add the PBXFileReference, PBXBuildFile, test group child, and Sources build-phase entries so the UIColor decoder fix gets executing coverage on iOS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the IMPORTANT review item: |
Summary
Fixes REN-7 (issues.md#L172): CSS 3-digit color shorthand
#f00was silently decoded to a near-black green (rgb(0, 15, 0)) instead of pure red (rgb(255, 0, 0)).Root cause: Both
UIColor+HexString.mandNSColor+HexString.mpassed the post-#string straight toscanHexInt, so"f00"was read as0x000F00. The mask0xFF0000 >> 16then gave0x00for red.Fix (~8 LOC per file, 16 total): Strip the leading
#, then if the remaining string is exactly 3 chars, duplicate each nibble ("f00"→"ff0000") before scanning. 6-digit paths are unaffected.Coordination with REN-2 / PR #230: PR #230 (
MTMathListBuilder.m readColor) made the parser accept#RGB(length 4 with#) as grammatically valid and left a code comment that correct rendering of#RGBwas deferred to REN-7 (this task). After this PR,\color{#f00}xboth parses and renders correctly.Changes
iosMath/render/UIColor+HexString.m— expand 3-digit shorthand beforescanHexIntiosMath/render/NSColor+HexString.m— identical edit (platform parity)iosMathTests/MTColorDecoderTest.m— new TDD test file with 9 cases:#f00,#0f0,#00f,#fff,#000)Test plan
swift test— 301 tests, 0 failures (includes newMTColorDecoderTest)#f00→r=0 g=15before fix,r=255 g=0after)xcodebuild test -scheme iosMath) — coversUIColorpathxcodebuild test -scheme MacOSMath) — coversNSColorpath🤖 Generated with Claude Code