updated source code #10610
updated source code #10610
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the webview_flutter_wkwebview package to use UIColor for setting background colors instead of an integer representation, which is a good improvement for type safety and correctness. However, I've found a critical issue in the Dart code where Color properties are accessed incorrectly (.r, .g, etc. instead of .red, .green, etc.) and not normalized for the UIColor constructor. This will cause compilation errors. I've provided suggestions to fix this in the implementation and the tests. Additionally, a test case for setBackgroundColor was commented out, which I've suggested to re-enable with the necessary fixes.
| unawaited( | ||
| webView.setBackgroundColor( | ||
| UIColor( | ||
| red: transparent.r, | ||
| green: transparent.g, | ||
| blue: transparent.b, | ||
| alpha: transparent.a, | ||
| ), | ||
| ), | ||
| ); | ||
| unawaited( | ||
| webView.scrollView.setBackgroundColor( | ||
| params.backgroundColor?.toARGB32(), | ||
| UIColor( | ||
| red: backgroundColor.r, | ||
| green: backgroundColor.g, | ||
| blue: backgroundColor.b, | ||
| alpha: backgroundColor.a, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The Color class does not have r, g, b, and a properties. The correct properties are red, green, blue, and alpha, which are integers from 0 to 255. These need to be converted to double values between 0.0 and 1.0 for the UIColor constructor.
| unawaited( | |
| webView.setBackgroundColor( | |
| UIColor( | |
| red: transparent.r, | |
| green: transparent.g, | |
| blue: transparent.b, | |
| alpha: transparent.a, | |
| ), | |
| ), | |
| ); | |
| unawaited( | |
| webView.scrollView.setBackgroundColor( | |
| params.backgroundColor?.toARGB32(), | |
| UIColor( | |
| red: backgroundColor.r, | |
| green: backgroundColor.g, | |
| blue: backgroundColor.b, | |
| alpha: backgroundColor.a, | |
| ), | |
| ), | |
| ); | |
| unawaited( | |
| webView.setBackgroundColor( | |
| UIColor( | |
| red: transparent.red / 255.0, | |
| green: transparent.green / 255.0, | |
| blue: transparent.blue / 255.0, | |
| alpha: transparent.alpha / 255.0, | |
| ), | |
| ), | |
| ); | |
| unawaited( | |
| webView.scrollView.setBackgroundColor( | |
| UIColor( | |
| red: backgroundColor.red / 255.0, | |
| green: backgroundColor.green / 255.0, | |
| blue: backgroundColor.blue / 255.0, | |
| alpha: backgroundColor.alpha / 255.0, | |
| ), | |
| ), | |
| ); |
| _webView.setBackgroundColor( | ||
| UIColor( | ||
| red: transparent.r, | ||
| green: transparent.g, | ||
| blue: transparent.b, | ||
| alpha: transparent.a, | ||
| ), | ||
| ), | ||
| // This method must be called last. | ||
| _webView.scrollView.setBackgroundColor(color.toARGB32()), | ||
| _webView.scrollView.setBackgroundColor( | ||
| UIColor(red: color.r, green: color.g, blue: color.b, alpha: color.a), | ||
| ), |
There was a problem hiding this comment.
The Color class does not have r, g, b, and a properties. The correct properties are red, green, blue, and alpha, which are integers from 0 to 255. These need to be converted to double values between 0.0 and 1.0 for the UIColor constructor.
| _webView.setBackgroundColor( | |
| UIColor( | |
| red: transparent.r, | |
| green: transparent.g, | |
| blue: transparent.b, | |
| alpha: transparent.a, | |
| ), | |
| ), | |
| // This method must be called last. | |
| _webView.scrollView.setBackgroundColor(color.toARGB32()), | |
| _webView.scrollView.setBackgroundColor( | |
| UIColor(red: color.r, green: color.g, blue: color.b, alpha: color.a), | |
| ), | |
| _webView.setBackgroundColor( | |
| UIColor( | |
| red: transparent.red / 255.0, | |
| green: transparent.green / 255.0, | |
| blue: transparent.blue / 255.0, | |
| alpha: transparent.alpha / 255.0, | |
| ), | |
| ), | |
| // This method must be called last. | |
| _webView.scrollView.setBackgroundColor( | |
| UIColor(red: color.red / 255.0, green: color.green / 255.0, blue: color.blue / 255.0, alpha: color.alpha / 255.0), | |
| ), |
| PigeonOverrides.uIColor_new = | ||
| ({ | ||
| required double red, | ||
| required double green, | ||
| required double blue, | ||
| required double alpha, | ||
| dynamic observeValue, | ||
| }) { | ||
| if (red == Colors.transparent.r && | ||
| green == Colors.transparent.g && | ||
| blue == Colors.transparent.b && | ||
| alpha == Colors.transparent.a) { | ||
| return transparentUiColor; | ||
| } else if (red == Colors.red.r && | ||
| green == Colors.red.g && | ||
| blue == Colors.red.b && | ||
| alpha == Colors.red.a) { | ||
| return redUiColor; | ||
| } | ||
|
|
||
| return UIColor.pigeon_detached(); | ||
| }; |
There was a problem hiding this comment.
The Color class does not have r, g, b, and a properties. The correct properties are red, green, blue, and alpha, which are integers from 0 to 255. The test mock for UIColor.new should be updated to use these properties and normalize them to double values between 0.0 and 1.0.
PigeonOverrides.uIColor_new =
({
required double red,
required double green,
required double blue,
required double alpha,
dynamic observeValue,
}) {
if (red == Colors.transparent.red / 255.0 &&
green == Colors.transparent.green / 255.0 &&
blue == Colors.transparent.blue / 255.0 &&
alpha == Colors.transparent.alpha / 255.0) {
return transparentUiColor;
} else if (red == Colors.red.red / 255.0 &&
green == Colors.red.green / 255.0 &&
blue == Colors.red.blue / 255.0 &&
alpha == Colors.red.alpha / 255.0) {
return redUiColor;
}
return UIColor.pigeon_detached();
};
Fixes flutter/flutter#178870
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3