-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Conversation
@@ -0,0 +1,554 @@ | |||
// Copyright 2013 The Flutter Authors. All rights reserved. |
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.
This is an exact copy from camera_android
.
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.
I'm assuming that camera_android
just copied this from camera
. Would it be better to use the latest copy of this from camera
instead?
@@ -0,0 +1,84 @@ | |||
// Copyright 2013 The Flutter Authors. All rights reserved. |
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.
This is an exact copy from camera_android
.
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.
Same as above
@@ -2,43 +2,1003 @@ | |||
// Use of this source code is governed by a BSD-style license that can be | |||
// found in the LICENSE file. | |||
|
|||
import 'dart:async'; |
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.
This is an exact copy from camera_android
, except some functionality was deleted for currently unsupported operations. My thought is that we can just fill them back in as we implement things. I left TODOs in those spots.
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.
Same as above.
@visibleForTesting | ||
CameraSelector? backCameraSelector = CameraSelector.getDefaultBackCamera(); |
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.
@gmackall I ended up refactoring this, primarily for testing reasons. Initializing it like this would cause us to have to initialize this in every test, so I apologize for noting this as an option.
To do this, I modified availableCameras()
to use a helper method to create CameraSelector
s that is stubbed in the mock version of AndroidCameraCameraX
that I created for testing. Let me know what you think!
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.
I think that makes sense!
It means we will have to recreate those camera selectors for each call to availableCameras, but I don't think there is much reason to repeatedly make calls to availableCameras.
It also looks like it makes the testing much cleaner.
ProcessCameraProvider, | ||
CameraSelector, | ||
CameraInfo, | ||
@GenerateNiceMocks(<MockSpec<Object>>[ |
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.
@stuartmorgan @bparrishMines Generate mocks for BuildContext
this way has caused me to run into this error: https://cirrus-ci.com/task/6299387630977024?logs=unit_test#L203. This passes for me locally, so I'm wondering if y'all have any insight as to what might be happening with the CI?
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.
It looks like the error is about a typedef that was added on master recently, so it's not on stable. You can probably just generate the mocks on stable
.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
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.
LGTM!
The only change I would consider is changing the _bindPreviewToLifecycle (and unbind) into helpers that can handle all of the potential use cases (i.e. a _bindUseCaseToLifecycle).
But I think that change is probably easier to make once we have more use cases to work with and it is clearer what assertions are shared.
@@ -0,0 +1,554 @@ | |||
// Copyright 2013 The Flutter Authors. All rights reserved. |
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.
I'm assuming that camera_android
just copied this from camera
. Would it be better to use the latest copy of this from camera
instead?
@@ -0,0 +1,84 @@ | |||
// Copyright 2013 The Flutter Authors. All rights reserved. |
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.
Same as above
@@ -2,43 +2,1003 @@ | |||
// Use of this source code is governed by a BSD-style license that can be | |||
// found in the LICENSE file. | |||
|
|||
import 'dart:async'; |
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.
Same as above.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/camera_selector.dart
Outdated
Show resolved
Hide resolved
/// The [Camera] instance returned by the [processCameraProvider] when a [UseCase] is | ||
/// bound to the lifecycle of the camera it manages. | ||
@visibleForTesting | ||
Camera? camera; |
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.
nit: It may be worth moving all the @visibleForTesting
fields and methods to a separate class and pass that class as an optional parameter in a constructor. Calling it something like AndroidCameraXProxy
. This could help organize the code a bit. You can decide which is easier to use.
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.
I would honestly find this kind of confusing, especially since this would involve some of the major use case instances. Are you open to reconsidering this later on? I could see it helping organizationally if there ends up being a lot of different fields (especially booleans and things that aren't necessarily CameraX objects) that need to be visible or there are a bunch more methods we'll need to create to mock.
@gmackall I agree! I thought the same thing. |
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// TODO(a14n): remove this import once Flutter 3.1 or later reaches stable (including flutter/flutter#104231) |
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.
fluter 3.7 is in stable and that pull request has landed.
try { | ||
await cameraController.initialize(); | ||
await Future.wait(<Future<Object?>>[ | ||
// The exposure mode is currently not supported on the web. |
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.
Consider breaking this out into a method _isExposureModeSupported
and using the method to encapsulate that logic.
.getMinZoomLevel() | ||
.then((double value) => _minAvailableZoom = value), | ||
]); | ||
} on CameraException catch (e) { |
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.
I think the above block has a subtle programming error related to how async code and thrown exceptions work. In my own dart programming I avoid then.
From reading this section https://dart.dev/guides/libraries/futures-error-handling#error-handling-within-then I think that if you get an error thrown during getMaxExposureOffset, getMaxZoomLevel or getMinZoom level it will not be caught by your code below if the async execution has left this method.
Instead you either need to ensure each of your then methods have their own catch error or alternatively you can use await. Also with the way this is currently structured I think there is a possibility that max available zoom level is not updated when the value is used.
]); | ||
} on CameraException catch (e) { | ||
switch (e.code) { | ||
case 'CameraAccessDenied': |
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.
Where do these cases come from? Can we use a static constant instead of inline strings?
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.
The code is thrown from the platform side, and yes we can use a string constant instead of inline strings.
Though the latest recommendation is to translate the code
into subclasses (e.g.CameraAccessDeniedException
)
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.
Though the latest recommendation is to translate the
code
into subclasses (e.g.CameraAccessDeniedException
)
That's not what it says; the recommendation is to have a plugin-specific exception with some kind of structured definition of specific cases. Subclasses are a possible approach, but the examples it gives are enums or other constants.
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.
Ah I read this part wrong:
plugin-specific Exception class
This is just referring to CameraException
, not CameraAccessDeniedException
.
Thanks for the correction!
} | ||
|
||
void onFlashModeButtonPressed() { | ||
if (_flashModeControlRowAnimationController.value == 1) { |
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.
if there are only 2 possible values for this and the below lines why are we comparing against an int instead of a boolean?
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.
Oh I see. We can probably just check if the animation is finished or not instead.
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.
I think code that is clearly about animation logic makes more sense than what I am reading now.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Show resolved
Hide resolved
static const int LENS_FACING_FRONT = 0; | ||
/// | ||
/// See https://developer.android.com/reference/androidx/camera/core/CameraSelector#LENS_FACING_FRONT(). | ||
static const int lensFacingFront = 0; |
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.
Why do not we have a copy of lens facing unknown?
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.
We aren't using it now, but we will need that later on, so I added it!
@reidbaker Regarding the comments you left on the example app, I agree with them, so I plan to make a cleanup PR to update the camera example apps across the board. I just coped the one from |
* 3c7d54bba [camerax] Implement camera preview (flutter/plugins#7112) * 48f50b4f1 [image_picker] Update NSPhotoLibraryUsageDescription description in README (flutter/plugins#6589) * eea17c996 Migrate these tests to the "new" API. (flutter/plugins#7189) * 190c6d916 Roll Flutter from 298d8c7 to 0be7c3f (21 revisions) (flutter/plugins#7194) * c0d4e8041 [google_sign_in] Endorses next web package. (flutter/plugins#7191) * cc4eaba0f [google_maps]: Bump org.mockito:mockito-core (flutter/plugins#7099) * 717a8bfef [image_picker]: Bump org.mockito:mockito-core (flutter/plugins#7097) * 8a09d8c13 [lifecycle]: Bump org.mockito:mockito-core (flutter/plugins#7096) * 40377a12a [in_app_pur]: Bump org.mockito:mockito-core (flutter/plugins#7094) * 6a4bbf1df [url_launcher]: Bump org.mockito:mockito-core (flutter/plugins#7098) * 96ab5cd12 Update codeowners (flutter/plugins#7188) * 00d5855cc Add missing CODEOWNER (flutter/plugins#7016) * c3e9d1ba3 [webview_flutter] Adds examples of accessing platform-specific features for each class (flutter/plugins#7089) * 1f7b57917 Roll Flutter from 0be7c3f to 33e4d21 (5 revisions) (flutter/plugins#7196) * 1acaf55c2 [plugins] Disables the AndroidGradlePluginVersion issue ID in all android packages (flutter/plugins#7045) * 132d9c77d [espresso] Update some dependencies (flutter/plugins#7195)
Implements Flutter camera API methods related building, pausing, and resuming a preview.
In order to implement this, I also needed to implement methods to create an initialize the camera, as well as callback methods to send information related to camera initialization, the device orientation changing, and native camera errors.
I also added back the example app used in the other plugins, as it will be needed for integration testing and easier to verify feature parity is ultimately reached.
Fixes flutter/flutter#112658.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).