[camera] Refactor Camera/Android implementation to allow reuse of Camera functionality #1949
Conversation
|
@bparrishMines This is the canonical PR for my work. Please ignore the CL. |
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
Is there a way to get the diffs for the code that was moved to separate files? It would be easier for me to review if I knew where the changes were. Did you only copy paste their functionality?
Otherwise, I will probably just manually test it. Especially since this would only be a refactor in the short run.
Also, note that we have been working on a way to do the long-term refactor in a few weeks.
packages/camera/CHANGELOG.md
Outdated
| @@ -1,3 +1,7 @@ | |||
| ## 0.5.3 | |||
|
|
|||
| * Massive refactoring of Android Camera functionality to allow reuse of Camera class. | |||
There was a problem hiding this comment.
I don't believe refactors require a version bump if no new features were added. If someone accessing this from pub.dev they would notice a difference on the dart side.
There was a problem hiding this comment.
So we don't publish this at all? That's fine by me. Reverting.
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
LGTM pending tests and one comment
I also tested the camera example app and firebase_ml_vision example app that uses a live preview.
|
|
||
| public static void registerWith(Registrar registrar) { | ||
| if (registrar.activity() == null || Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { |
There was a problem hiding this comment.
Did you mean to remove this? I believe this check is needed when you are embedding the plugin in an Android app.
There was a problem hiding this comment.
Yes, I meant to remove it. In general, it is a bad idea to not register plugins. Activity is technically not needed when this plugin is registered, so why require it?
We should probably remove the version check as well. Often apps (Dart) do not check the presence of activity or SDK version before calling a plugin. To make it as user friendly as possible, plugins should degrade gracefully and return proper errors.
If you don't register the plugin altogether, you get a catastrophic failure that says "channel name not found" which is not indicative of the real problem.
There was a problem hiding this comment.
The version check was added in this PR and was only added so a developer could still compile the plugin with sdk < 21. To do this, one would need to add a specific line in their manifest to ignore that this plugin requires 21+.
|
What did you mean by "pending tests"? Do you mean manual tests? |
|
I just meant the formatting and build tests. |
…era functionality (flutter#1949)
3 participants
Purpose of the PR is two-fold: