- Notifications
You must be signed in to change notification settings - Fork9.7k
Implement Android WebView api with pigeon (Dart portion)#4435
Implement Android WebView api with pigeon (Dart portion)#4435bparrishMines merged 11 commits intoflutter:masterfrom
Conversation
bparrishMines commentedOct 19, 2021
@mvanbeusekom@cyanglaz This PR is mostly documentation and code generation. Could you just take a look at: |
bparrishMines commentedOct 19, 2021
@gaaclarke Could you take a look at the pigeon api?: |
mvanbeusekom left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good overall, however I have one remark/ question:
I notice that it is common to put multiple classes into the same file. Personally I always like to split out classes into their own files as much as possible. Keeping multiple classes in the same file makes it more difficult to oversee the whole solution and to find the code I need (maybe this is personal and I should just learn to live with it). Another reason is that it is much harder to enforce certain design principles. In Dart it is perfectly legal to access private class members when both classes are in the same file. This would allow people to call into classes directly where they are not supposed to, creating dependency violations (a good example in my opinion would be the circular dependency between theWebView class and theWebSettings class).
What is the policy within the Flutter team regarding this? I see this happening quite often, although not everywhere (google_maps_flutter_platform_interface is a good example which splits everything into separate files).
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
gaaclarke commentedOct 19, 2021
Beautiful, lgtm. Hey, I don't understand why you aren't also generating and using the java code? I don't imagine it's much useful to just have the Dart code. |
bparrishMines commentedOct 19, 2021
@mvanbeusekom According toEffective Dart, it seems to be up to personal preference and whatever is the current style of a plugin. I don't think we have a repo wide policy, but I prefer putting related classes in the same file due to
As a side note: I also had the problem of finding the code I needed when coding, but I started using |
bparrishMines commentedOct 19, 2021
@gaaclarke My goal was to do the entire Java side in the next PR because it will probably be another 1000 lines of code to review. It would also be hard to add any running code now because I think putting the plugin in a state that uses method channels and pigeon would put the plugin in a weird state that could cause bugs. |
mvanbeusekom left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
* master: Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442)
* master: Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442)
…ideo_src_on_same_controller* commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33': (537 commits) [video_player] Initialize player when size and duration become available (flutter#4438) [webview_flutter] Implement zoom enabled for ios and android (flutter#4417) Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453) Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442) [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) ...# Conflicts:#packages/video_player/video_player/pubspec.yaml#packages/video_player/video_player_web/lib/video_player_web.dart#packages/video_player/video_player_web/pubspec.yaml
Uh oh!
There was an error while loading.Please reload this page.
Implements theAndroid WebView library that is currently supported with pigeon.
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel onDiscord.