-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[camera] Migrate to new embedding #2114
[camera] Migrate to new embedding #2114
Conversation
…ded new example Activity that uses new embedding - still needs lots of refactoring and tests.
…er EventSink behavior from the Camera class to help simplify test dependencies.
…tion of callbacks and exceptions. All Result responses are now handled by the CameraPlugin, itself.
…om taking pictures, from video recording.
…ontextCompat, permission, and ActivityPluginBinding dependencies.
…ssions into the implementation to make it easier to fake a CameraPermissions in tests.
…PluginProtocol, which resulted in the simplest possible implementation of CameraPlugin. Also added copyright to new files.
…PluginProtocol, hitting 30% code coverage of the class at this point.
… CameraPluginProtocol because they involve channel comms, then added unit tests for ChannelCameraPreviewDisplay and ChannelCameraImageStream to keep us at 100% coverage for the protocol.
…ced to just a CameraSystem that doesn't know about Android stuff.
…, CameraPluginProtocol (97%), CameraDetails (50%), CameraPlugin (12%), the rest 0%.
| } | ||
|
|
||
| @Override | ||
| public void onMethodCall(@NonNull MethodCall call, @NonNull final Result result) { |
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.
Can we find a way to not duplicate all this code?
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.
Sure, but it looks like this PR probably won't merge because @bparrishMines has other plans for the plugin. I would recommend that each plugin dev do whatever is most appropriate in terms of replication as they migrate.
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.
Would be useful to have a sample PR/commit that shows only the diff that's relevant to properly extending a plugin to the new embedder.
The camera refactoring plans are longer term, we will want to extend the existing plugin to support the new embedder much sooner, and I won't tie doing that to the big refactoring planned.
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.
Ok. I'll put up a PR that does the minimum possible work and shares code.
Migrates the camera plugin to the new embedding and adds an example activity exercising it. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See #2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, going a little beyond #2118: - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a separate file than to try and manage that logic all in one place. Other than the above refactor, the original logic is untouched.
|
Closing since the camera plugin has been migrated. |
Migrates the camera plugin to the new embedding and adds an example activity exercising it. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See flutter#2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, going a little beyond flutter#2118: - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a separate file than to try and manage that logic all in one place. Other than the above refactor, the original logic is untouched.
Migrates the camera plugin to the new embedding and adds an example activity exercising it. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See flutter#2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, going a little beyond flutter#2118: - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a separate file than to try and manage that logic all in one place. Other than the above refactor, the original logic is untouched.
Migrates the camera plugin to the new embedding and adds an example activity exercising it. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See flutter#2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, going a little beyond flutter#2118: - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a separate file than to try and manage that logic all in one place. Other than the above refactor, the original logic is untouched.
Migrates the Camera plugin to the new Android embedding.
A doc exists internally that corresponds to this PR. If you'd like to review this PR, I would recommend reading that doc as well, to better understand the intentions.
This change serves the following purposes:
For demonstration purposes, I have not yet setup the new and old plugins to share implementation details. This is for developer clarity when using this PR as a guide. Once the plugin migration effort is underway, I plan to delete as much of the old plugin as possible, pointing the old plugin to the new plugin's implementation. By pointing the old plugin to the new implementation, instead of the other way around, we can keep all the new tests that I've written.
The architecture of the Camera plugin is not 100% where I want it to be. There are still some references that prevent ideal segmentation points. However, this is probably 90% to the finish line and a reasonable place to pause for the purpose of helping others migrate 1P plugins.