[camerax] Update README with plugin overview #3891
Conversation
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
In webview_flutter, we added what lines to run to generate new apis and mocks. See: https://pub.dev/packages/webview_flutter_android#contributing. These were added as a response to feedback from external contributors.
| currently uses to implement `camera`. Each of these classes uses an `InstanceManager` | ||
| (implementation in `instance_manager.dart`) to manage objects that are created by | ||
| the plugin implementation that map to objects of the same type created on the native | ||
| side. This plugin uses [`pigeon`][12] to communicate with native code, so each of |
There was a problem hiding this comment.
Here is where you could include the location of the pigeon file as well as the exact command line to run. See: https://pub.dev/packages/webview_flutter_android#contributing
There was a problem hiding this comment.
Ideally we should actually move that to a CONTRIBUTING.md; it's not relevant to plugin clients.
There was a problem hiding this comment.
Oh, this is in the section you already suggested moving. Sorry, I wasn't looking at the full context when I replied at first.
| currently uses to implement `camera`. Each of these classes uses an `InstanceManager` | ||
| (implementation in `instance_manager.dart`) to manage objects that are created by | ||
| the plugin implementation that map to objects of the same type created on the native | ||
| side. This plugin uses [`pigeon`][12] to communicate with native code, so each of |
There was a problem hiding this comment.
Oh, this is in the section you already suggested moving. Sorry, I wasn't looking at the full context when I replied at first.
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
LGTM with a couple nits
| @@ -0,0 +1,46 @@ | |||
| # Contributing to camera\_android\_camerax | |||
|
|
|||
| ## Plugin structure | |||
There was a problem hiding this comment.
not a blocker but I think this should start with an overview of the architecture @bparrishMines designed. Maybe linking the design doc or if not that then a 1 paragraph summary describing the structure.
| regenerate the communication layer by running | ||
| `dart run pigeon --input pigeons/camerax_library.dart` from the plugin root. | ||
|
|
||
| On the native side in `android/src/main/java/io/flutter/plugins/camerax/`, you'll |
There was a problem hiding this comment.
Consider saying java/kotlin or android. Native means enough different things to different people that we should minimize the usage when a more accurate word choice is available.
| `dart run pigeon --input pigeons/camerax_library.dart` from the plugin root. | ||
|
|
||
| On the native side in `android/src/main/java/io/flutter/plugins/camerax/`, you'll | ||
| find the Host API and Flutter API implementations of the same classes wrapped with |
There was a problem hiding this comment.
It would probably be helpful to articulate what host api and Flutter api mean in this context of native (or java ;) code.
| in the native code map to objects created on the Dart side, and thus, are also | ||
| managed by an `InstanceManager` (implementation in `InstanceManager.java`). | ||
|
|
||
| If you need to access any Android classes to contribute to this plugin, you should |
There was a problem hiding this comment.
I understand what this line through 30 is saying but I think it mixes architecture and instructions in a way that muddles what the author should use. If the architecture philosophy is include above then I think this can be shortened to "If CameraX or other Android classes do not have a duplicately named implementation in lib/src then follow the for more detail on how to add them.
| you need are not wrapped or you need to access any methods not wrapped in a class, | ||
| you must take the additional steps to wrap them to maintain the structure of this plugin. | ||
|
|
||
| For more information on the approach of wrapping native libraries for plugins, please |
There was a problem hiding this comment.
Having read though this I think a bunch of the information only makes sense after understanding the architecture. A plain english version should be at the top along with a link to the design document.
There was a problem hiding this comment.
Oh and @bparrishMines to help people ask questions and spread this pattern or even just write about it you should come up with a unique name for the pattern.
| @@ -0,0 +1,46 @@ | |||
| # Contributing to camera\_android\_camerax | |||
|
|
|||
| ## Plugin structure | |||
| described above to add them. | ||
|
|
||
| For more information on the approach of wrapping native libraries for plugins, | ||
| please see the [design document][2] or contact [@bparrishmines][6] on any |
There was a problem hiding this comment.
@bparrishMines is mentioning you like this okay with you?
There was a problem hiding this comment.
We don't typically include references to our github accounts in the plugin. I think having https://github.com/flutter/packages/blob/main/CODEOWNERS should suffice. It also makes it easier to maintain since I will be added to all camera plugin PRs anyways.
Alternatively you could link to the ecosystem discord channel instead. Then anyone who is able to answer a question will be notified.
There was a problem hiding this comment.
I like both of those suggestions. In general it is a good idea not try not to encourage contacting a person. It is not great for the person or the one reaching out. (imagine someone 12h timezone offset from @bparrishMines 3 years from now)
camsim99
added
the
autosubmit
flutter/packages@4800d65...1f91710 2023-05-09 [email protected] [camerax] Update README with plugin overview (flutter/packages#3891) 2023-05-09 [email protected] Fix nullability in docs (flutter/packages#3876) 2023-05-08 [email protected] [tools] Check version ranges when pathifying deps (flutter/packages#3943) 2023-05-08 [email protected] [quick_actions] Update minimum iOS version to 11 (flutter/packages#3946) 2023-05-08 [email protected] [shared_preferences] Update minimum iOS version to 11 (flutter/packages#3945) 2023-05-08 [email protected] [path_provider] Update minimum iOS version to 11 (flutter/packages#3944) 2023-05-08 [email protected] Adding setup for TestInstanceManagerHostApi in zoom and exposure tests (flutter/packages#3947) 2023-05-08 [email protected] Roll Flutter from 43ac23b to 4ed1c92 (16 revisions) (flutter/packages#3942) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Updates the README of the `camera_android_camerax` plugin with an overview and instructions as to how to use the plugin. Also adds `CONTRIBUTING.md` for further information on how to contribute to the plugin.
4 participants
Updates the README of the
camera_android_cameraxplugin with an overview and instructions as to how to use the plugin. Also addsCONTRIBUTING.mdfor further information on how to contribute to the plugin.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).