Pass alpha value from the color panel to the embedder. #159
Conversation
example/flutter_app/lib/main.dart
Outdated
| _primaryColor = color; | ||
| // Force 255 on the alpha channel to avoid crashes when setting the | ||
| // ThemeData colors. | ||
| _primaryColor = Color.fromARGB(255, color.red, color.green, color.blue); |
There was a problem hiding this comment.
This would be clearer as color.withAlpha(255)
But if alpha isn't supported, wouldn't it make more sense to have the example up just pass false to the color picker?
There was a problem hiding this comment.
Yep, removed alpha slider from example
| @@ -26,5 +26,6 @@ const char kClosedCallbackMethod[] = "ColorPanel.ClosedCallback"; | |||
| const char kColorComponentRedKey[] = "red"; | |||
| const char kColorComponentGreenKey[] = "green"; | |||
| const char kColorComponentBlueKey[] = "blue"; | |||
| const char kColorComponentAlphaKey[] = "alpha"; | |||
There was a problem hiding this comment.
Move up (see comment in header).
| @@ -109,7 +110,7 @@ class ColorPanel { | |||
| methodCall.arguments.cast<String, dynamic>(); | |||
| if (arg != null) { | |||
| _callback(Color.fromARGB( | |||
| 255, | |||
There was a problem hiding this comment.
If false is passed for showing alpha, do both the macOS and Linux implementations send back 255 so that this will behave correctly when it's false?
It seems like either:
- plugins should be explicitly required to send back 255 for alpha when the param is false, or
- they should be required to not send an alpha at all, and this code check for the existence of an alpha key and use 255 if it's not present
Either way, channel_constants.h should document it.
There was a problem hiding this comment.
When the slider is not shown, alpha is always 1. Added a comment in channel_constants.h
| result[@(plugins_color_panel::kColorComponentGreenKey)] = @(color.greenComponent); | ||
| result[@(plugins_color_panel::kColorComponentBlueKey)] = @(color.blueComponent); | ||
| // Consider being able to pass other type of color space (Gray scale, CMYK, etc). | ||
| NSColor *rgbColor = [color colorUsingColorSpace:[NSColorSpace genericRGBColorSpace]]; |
There was a problem hiding this comment.
Why is this necessary now? Is it just to handle asking for the alpha when showing alpha is false (in which case, a conditional would handle it) or are there other cases that are broken?
Messing with color space makes me nervous, since it's not clear to me that this is actually going to give the same color the user intended to pick in all configurations. (It may be, but I'm not even close to an expert on color spaces; I just know enough to know that it's easy to get them wrong.)
There was a problem hiding this comment.
This prevents crashes if the NSColor passed in is not an rgb color.
I believe this converts the color to the proper color space (generic) for data representation. However, if you want the color to display accurately it should be converted to NSCalibratedRGBColorSpace (https://stackoverflow.com/questions/37307113/swift-why-nscolor-becomes-lighter-when-rendered).
#0000FF -> NSColor.NSCalibratedRGBColorSpace -> screen (shows #0000FF)
I'm not 100% sure about this next part but I'm assuming:
screen (shows #0000FF) -> NSColor.genericRGBColorSpace -> #0000FF
screen (shows #0000FF) -> no changes -> #0F3FFB
I couldn't find any information online that explains/proves the reverse conversion. Though it should be relatively easy to pull up xcode to double check this.
There was a problem hiding this comment.
Yeah, there was actually a bug. If you choose a color from the Gray scale slider or CYMK, it raises an exception. As @izackp mentions, it prevents the exception, and it corresponds to the calibrated space. Per the genericRGBColorSpace documentation:
// NSColorSpace corresponding to Cocoa color space name NSCalibratedRGBColorSpace
| @@ -112,9 +112,12 @@ - (void)selectedColorDidChange { | |||
| */ | |||
There was a problem hiding this comment.
I can't seem to add a review comment above this context line, so arbitrarily adding here: while you are here, please fix the indentation on line 38 (should be indented 4 more). I had a specific comment about this when I approved the previous change (thus the mention of re-running clang-format) but apparently didn't save it so it wasn't part of the approval comment.
There was a problem hiding this comment.
Yeah not sure what happened there. I saw your comment and ran the clang-format. Might've undone the change by mistake. Anyway, it's done now :)
franciscojma86
force-pushed
the
color
branch
from
fab115a to
faea7fd
Compare
franciscojma86
force-pushed
the
color
branch
from
faea7fd to
1e23487
Compare
|
Comments addressed, PTAL! |
No description provided.