Skip to content

Pass alpha value from the color panel to the embedder. #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 30, 2018
Merged

Pass alpha value from the color panel to the embedder. #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2307c44
[macos] Pass alpha value from the color panel to the embedder.
franciscojma86 Nov 29, 2018
ddf7d61
Pass alpha value from the color panel to the embedder.
franciscojma86 Nov 29, 2018
1e23487
Address review comments
franciscojma86 Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions example/flutter_app/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ class _MyHomePage extends StatelessWidget {
if (!colorPanel.showing) {
colorPanel.show((color) {
_AppState.of(context).setPrimaryColor(color);
});
// Setting the primary color to a non-opaque color raises an exception.
}, showAlpha: false);
}
}

Expand Down Expand Up @@ -212,9 +213,9 @@ class FileChooserTestWidget extends StatelessWidget {
onPressed: () {
file_chooser.showSavePanel((result, paths) {
Scaffold.of(context).showSnackBar(SnackBar(
content: Text(_resultTextForFileChooserOperation(
_FileChooserType.save, result, paths)),
));
content: Text(_resultTextForFileChooserOperation(
_FileChooserType.save, result, paths)),
));
}, suggestedFileName: 'save_test.txt');
},
),
Expand All @@ -223,9 +224,9 @@ class FileChooserTestWidget extends StatelessWidget {
onPressed: () {
file_chooser.showOpenPanel((result, paths) {
Scaffold.of(context).showSnackBar(SnackBar(
content: Text(_resultTextForFileChooserOperation(
_FileChooserType.open, result, paths)),
));
content: Text(_resultTextForFileChooserOperation(
_FileChooserType.open, result, paths)),
));
}, allowsMultipleSelection: true);
},
),
Expand Down
1 change: 1 addition & 0 deletions plugins/color_panel/common/channel_constants.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const char kHideColorPanelMethod[] = "ColorPanel.Hide";
const char kColorSelectedCallbackMethod[] = "ColorPanel.ColorSelectedCallback";
const char kClosedCallbackMethod[] = "ColorPanel.ClosedCallback";

const char kColorComponentAlphaKey[] = "alpha";
const char kColorComponentRedKey[] = "red";
const char kColorComponentGreenKey[] = "green";
const char kColorComponentBlueKey[] = "blue";
Expand Down
8 changes: 5 additions & 3 deletions plugins/color_panel/common/channel_constants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ extern const char kClosedCallbackMethod[];
// The argument to show an opacity modifier on the panel. Default is true.
extern const char kColorPanelShowAlpha[];

// Keys for the RGB color JSON object sent to kColorPanelCallback.
// The values should be numbers between 0 and 1.
// Keys for the ARGB color JSON object sent to kColorPanelCallback.
// The values should be numbers between 0 and 1. Native color pickers on
// macOS and Linux always return a value of 1 on the Alpha channel if the
// opacity slider is not shown.
extern const char kColorComponentAlphaKey[];
extern const char kColorComponentRedKey[];
extern const char kColorComponentGreenKey[];
extern const char kColorComponentBlueKey[];

} // namespace plugins_color_panel

#endif // PLUGINS_COLOR_PANEL_COMMON_CHANNEL_CONSTANTS_H_
3 changes: 2 additions & 1 deletion plugins/color_panel/lib/color_panel.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const String _kColorPanelShowAlpha = 'ColorPanel.ShowAlpha';
const String _kRedKey = 'red';
const String _kGreenKey = 'green';
const String _kBlueKey = 'blue';
const String _kAlphaKey = 'alpha';

const MethodChannel _platformChannel =
const MethodChannel(_kColorPanelChannel, const JSONMethodCodec());
Expand Down Expand Up @@ -109,7 +110,7 @@ class ColorPanel {
methodCall.arguments.cast<String, dynamic>();
if (arg != null) {
_callback(Color.fromARGB(
255,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. plugins should be explicitly required to send back 255 for alpha when the param is false, or
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the slider is not shown, alpha is always 1. Added a comment in channel_constants.h

_colorComponentFloatToInt(arg[_kAlphaKey]),
_colorComponentFloatToInt(arg[_kRedKey]),
_colorComponentFloatToInt(arg[_kGreenKey]),
_colorComponentFloatToInt(arg[_kBlueKey])));
Expand Down
12 changes: 6 additions & 6 deletions plugins/color_panel/linux/src/color_panel_plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ class ColorPanelPlugin::ColorPanel {
}
}

// Converts a color from RGBA to RGB in the form of a JSON object.
// Converts a color from ARGB to a JSON object.
//
// The format of the message is intended for platform consumption. The
// conversion assumes that the background color will be black.
// The format of the message is intended for platform consumption.
static Json::Value GdkColorToArgs(const GdkRGBA *color) {
Json::Value result;
result[kColorComponentRedKey] = color->red * color->alpha;
result[kColorComponentGreenKey] = color->green * color->alpha;
result[kColorComponentBlueKey] = color->blue * color->alpha;
result[kColorComponentAlphaKey] = color->alpha;
result[kColorComponentRedKey] = color->red;
result[kColorComponentGreenKey] = color->green;
result[kColorComponentBlueKey] = color->blue;
return result;
}

Expand Down
11 changes: 7 additions & 4 deletions plugins/color_panel/macos/FLEColorPanelPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ - (void)handleMethodCall:(FLEMethodCall *)call result:(FLEMethodResult)result {
if ([call.methodName isEqualToString:@(plugins_color_panel::kShowColorPanelMethod)]) {
if ([call.arguments isKindOfClass:[NSDictionary class]]) {
BOOL showAlpha =
[[call.arguments valueForKey:@(plugins_color_panel::kColorPanelShowAlpha)] boolValue];
[[call.arguments valueForKey:@(plugins_color_panel::kColorPanelShowAlpha)] boolValue];
[self showColorPanelWithAlpha:showAlpha];
} else {
NSLog(@"Malformed call for %@. Expected an NSDictionary but got %@",
Expand Down Expand Up @@ -112,9 +112,12 @@ - (void)selectedColorDidChange {
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

- (NSDictionary *)dictionaryWithColor:(NSColor *)color {
NSMutableDictionary *result = [NSMutableDictionary dictionary];
result[@(plugins_color_panel::kColorComponentRedKey)] = @(color.redComponent);
result[@(plugins_color_panel::kColorComponentGreenKey)] = @(color.greenComponent);
result[@(plugins_color_panel::kColorComponentBlueKey)] = @(color.blueComponent);
// TODO: Consider being able to pass other type of color space (Gray scale, CMYK, etc).
NSColor *rgbColor = [color colorUsingColorSpace:[NSColorSpace genericRGBColorSpace]];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result[@(plugins_color_panel::kColorComponentAlphaKey)] = @(rgbColor.alphaComponent);
result[@(plugins_color_panel::kColorComponentRedKey)] = @(rgbColor.redComponent);
result[@(plugins_color_panel::kColorComponentGreenKey)] = @(rgbColor.greenComponent);
result[@(plugins_color_panel::kColorComponentBlueKey)] = @(rgbColor.blueComponent);
return result;
}

Expand Down