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

[camera] Add CameraInfo class and clean CameraX plugin #6345

Merged
auto-submit merged 14 commits into from
Sep 13, 2022
Merged

[camera] Add CameraInfo class and clean CameraX plugin #6345

auto-submit merged 14 commits into from
Sep 13, 2022

Conversation

@camsim99
Copy link
Contributor

@camsim99 camsim99 commented Aug 31, 2022 •
edited
Loading

Adds CameraInfo class wrapper to CameraX plugin and removes unnecessary code/refactors some code to prepare for the implementation of availableCameras().

Part of flutter/flutter#111125.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

camsim99
camsim99 commented Aug 31, 2022
View reviewed changes
...oid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraAndroidCameraxPlugin.java
*/
public CameraAndroidCameraxPlugin() {}

void setUp(BinaryMessenger binaryMessenger, Context context) {
Copy link
Contributor Author

@camsim99 camsim99 Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context is not currently being used I noticed but I will need it in a follow up PR as per: https://github.com/flutter/plugins/pull/6307/files#diff-f037f7d2d2a92c6e1b775e086f9da043c0a77dfbb1209c63adf41b0b70899ed2R27-R40

@camsim99 camsim99 marked this pull request as ready for review August 31, 2022 20:48
bparrishMines
bparrishMines reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. Added some comments for my first pass through.

packages/camera/camera_android_camerax/android/build.gradle Outdated

dependencies {
// CameraX core library using the camera2 implementation must use same version number.
def camerax_version = "1.2.0-alpha04"
Copy link
Contributor

@bparrishMines bparrishMines Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cirrus warning is complaining that this is not version 1.2.0-beta01. Is there a reason you chose this version?

Copy link
Contributor Author

@camsim99 camsim99 Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the latest at the time I wrote it. I'll change it to the latest!

camsim99 and others added 4 commits September 1, 2022 15:27
@camsim99 @bparrishMines
Update packages/camera/camera_android_camerax/CHANGELOG.md
4e00d61 
Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
@camsim99 @bparrishMines
Update packages/camera/camera_android_camerax/lib/src/camera_info.dart
d9b8344 
Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
@camsim99
Address review
38ba357
@camsim99
Bump androidx.test.core and add comment about 33
d50e7f2
bparrishMines
bparrishMines reviewed Sep 6, 2022
View reviewed changes
packages/camera/camera_android_camerax/example/pubspec.yaml
#
# For details regarding fonts from package dependencies,
# see https://flutter.dev/custom-fonts/#from-packages

Copy link
Contributor

@bparrishMines bparrishMines Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline

Copy link
Contributor Author

@camsim99 camsim99 Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a new line

Copy link
Contributor

@bparrishMines bparrishMines Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think you're right. I wonder why this is indicating that a newline doesn't exist.

...oid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraAndroidCameraxPlugin.java Outdated

void setUp(BinaryMessenger binaryMessenger, Context context) {
// Set up instance manager.
instanceManager = InstanceManager.open(identifier -> {});
Copy link
Contributor

@bparrishMines bparrishMines Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback method should call

new JavaObjectFlutterApi(binaryMessenger).dispose(identifier, reply -> {});

Copy link
Contributor Author

@camsim99 camsim99 Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies -- you commented on this before and I meant to ask you about it, but I see what you're saying now!

packages/camera/camera_android_camerax/pigeons/camerax_library.dart Outdated
)
@HostApi(dartHostTestHandler: 'TestJavaObjectHostApi')
abstract class JavaObjectHostApi {
void dispose(int instanceId);
Copy link
Contributor

@bparrishMines bparrishMines Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit(optional): the term identifier has personally worked better for us than instanceId. (since instanceId is close to instance.). We changed the term starting in the iOS implementation of the webview_flutter plugin.

bparrishMines
bparrishMines reviewed Sep 9, 2022
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're also missing the JavaObjectHostApiImpl.java and its setup?

packages/camera/camera_android_camerax/lib/src/java_object.dart Outdated

/// Global instance of [InstanceManager].
static final InstanceManager globalInstanceManager = InstanceManager(
onWeakReferenceRemoved: (_) {},
Copy link
Contributor

@bparrishMines bparrishMines Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

onWeakReferenceRemoved: (int identifier) {
  JavaObjectHostApiImpl().dispose(identifier);
},

Copy link
Contributor Author

@camsim99 camsim99 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right! Thanks for pointing that out!

bparrishMines
bparrishMines approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a last nit

...oid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraAndroidCameraxPlugin.java
.dispose(identifier, reply -> {});
});

// Set up Host APIs.
Copy link
Contributor

@bparrishMines bparrishMines Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing GeneratedCameraXLibrary.JavaObjectHostApi.setup(...)

Copy link
Contributor Author

@camsim99 camsim99 Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Ok I think I'm getting the pattern!

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 13, 2022
@auto-submit auto-submit bot merged commit c7a5781 into flutter:main Sep 13, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 14, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2022
@engine-flutter-autoroll
c7a578125 [camera] Add CameraInfo class and clean CameraX plugin (flu…
829c9f2 
…tter/plugins#6345)
adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022
@camsim99
[camera] Add CameraInfo class and clean CameraX plugin (flutter#6345)
2f6d4a3
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@camsim99 @mauricioluz
[camera] Add CameraInfo class and clean CameraX plugin (flutter#6345)
15dbfaa
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bparrishMines bparrishMines bparrishMines approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@camsim99 @bparrishMines