Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@matthew-carroll
Copy link
Contributor

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:

  • Migrate to the new embedding
  • Demonstrate how a plugin migrates to the new embedding
  • Demonstrate an improved architecture that separates Flutter comms, from logical behavior, from Android details. This is done for testing purposes and for developer comprehension
  • Add tests. At the time of putting up the PR, unit test coverage has gone from 0% to 50% lines of code for the plugin. This includes nearly 100% coverage for 2 of the most significant files.

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.

…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.
…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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mklim mklim mentioned this pull request Oct 8, 2019
13 tasks
mklim pushed a commit that referenced this pull request Oct 15, 2019
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.
@collinjackson collinjackson changed the title Migrate camera plugin to new embedding [camera] Migrate camera plugin to new embedding Oct 24, 2019
@collinjackson collinjackson changed the title [camera] Migrate camera plugin to new embedding [camera] Migrate to new embedding Oct 24, 2019
@mklim
Copy link
Contributor

mklim commented Oct 31, 2019

Closing since the camera plugin has been migrated.

@mklim mklim closed this Oct 31, 2019
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
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.
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
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.
Akachu pushed a commit to Akachu/flutter_camera that referenced this pull request Apr 27, 2020
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants