Fix color picking accuracy - #189
Merged
Merged
Conversation
Picked colors were off by 1-3 values per channel because colors were stored in the display's ICC profile color space, causing lossy conversions. Now colors are always stored in sRGB and only converted to the user's preferred color space at display/copy time. Fixes #187 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to fix small per-channel drift in picked colors by normalizing/storing picked colors in sRGB (rather than the active display/ICC profile), and by ensuring the on-screen overlay reflects the same normalized color.
Changes:
- Normalize eyedropper stored colors to sRGB during initialization and when setting colors from sampling / system color picker.
- Normalize the overlay’s displayed color/text to sRGB so overlay and stored value align.
- Remove the Defaults color-space observer that previously mutated stored colors on preference changes.
Comments suppressed due to low confidence (1)
Pika/Services/Eyedroppers.swift:154
- In
start(), the overlay is rendered fromnormalizedColor(sRGB), but the stored value is set viaself.set(selectedColor), which converts again insideset(_:). To guarantee the overlay text/color and the stored color match exactly (and avoid any double-conversion rounding drift), pass the already-normalized sRGB color intoset(_:)instead.
let normalizedColor = selectedColor.usingColorSpace(.sRGB)!
if Defaults[.showColorOverlay] {
let colorText = normalizedColor.toFormat(
format: Defaults[.colorFormat], style: Defaults[.copyFormat]
)
let cursorPosition = NSEvent.mouseLocation
self.overlayWindow.show(
colorText: colorText,
pickedColor: normalizedColor,
nearCursor: cursorPosition,
duration: Defaults[.colorOverlayDuration]
)
}
self.set(selectedColor)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Replace force-unwraps with safe unwraps and fallbacks in set(), colorDidChange(), and start() - Fix closestVector and toRGB8BitArray to use sRGB consistently so closest color name lookup stays stable across color space changes - Pass normalizedColor to set() in start() to avoid double conversion - Add tests for sRGB normalization stability and round-trip accuracy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test incorrectly expected toHexString() to produce the same output regardless of color space preference. Since toHexString() converts to the user's preferred color space for display, changing the preference changes the output — that's correct behavior. Updated the test to verify the actual invariant: a P3 color normalized to sRGB round-trips through RGBA components without channel drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
EyedropperButton observed colorFormat and copyFormat but not colorSpace, so toFormat() (which reads Defaults[.colorSpace] internally) would not trigger a re-render when the user changed the color space in preferences. Adding @default(.colorSpace) makes SwiftUI invalidate the view on change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NSSecureCodingKey is not compatible with @default property wrapper. Use @State seeded from Defaults[.colorSpace] and onReceive with Defaults.publisher to update it when the preference changes, which triggers SwiftUI to re-render the color format text. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in body Defaults.publisher doesn't emit for NSSecureCodingKey, and the colorSpace state was never read in the body so SwiftUI had no reason to re-render. Switch to UserDefaults.didChangeNotification (which reliably fires) and explicitly convert the color via usingColorSpace(colorSpace) before formatting, so the state is actually read during render. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
Fixes #187
Test plan
#0E1829block in Figma) and verify Pika reports the exact hex value🤖 Generated with Claude Code