[camerax] Wrap classes to implement resolution configuration for video capture #4620
Conversation
| /** | ||
| * Host API implementation for {@link QualitySelector}. | ||
| * | ||
| * <p>This class may handle instantiating and adding native object instances that are attached to a |
There was a problem hiding this comment.
extreme nit: I had to read this sentence a couple of times to understand it, maybe break it up? Also maybe remove "may"?
Also applies to other instances of this comment
There was a problem hiding this comment.
After further discussion, it seems like this comment is coming from the generator for the ...HostApiImpls, correct? Feel free to ignore if so.
There was a problem hiding this comment.
Ah yes this is coming from the generator.
packages/camera/camera_android_camerax/lib/src/fallback_strategy.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/quality_selector.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/quality_selector.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/quality_selector.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/quality_selector.dart
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/quality_selector.dart
Outdated
Show resolved
Hide resolved
| /// camera represented by [cameraInfo]. | ||
| Future<ResolutionInfo> getResolutionFromInstance( | ||
| CameraInfo cameraInfo, VideoQualityConstraint quality) async { | ||
| final int? cameraInfoIdentifier = instanceManager.getIdentifier(cameraInfo); |
There was a problem hiding this comment.
Should we put a check after this that the identifier isn't null?
This pattern of int? foo = instanceManager.getIdentifier(bar) followed by //do something with foo! is pretty common in the ...FromInstance methods, but in every other case bar is passed in with this from the Wrapped class to the _WrappedHostApi, so we're pretty sure it exists.
But in this case, the bar is a different class entirely (CameraInfo, instead of QualitySelector), so it seems significantly more likely that someone makes the mistake of using an object that isn't actually in the instance manager.
There was a problem hiding this comment.
That's a really good point! Added a check below
| QualitySelector.fromOrderedList( | ||
| {BinaryMessenger? binaryMessenger, | ||
| InstanceManager? instanceManager, | ||
| required this.qualityList, |
There was a problem hiding this comment.
Do you think it would be good to add a check here that the list isn't empty?
We verify this on the Java side, but the error would end up being the result of code written in dart (i.e. a use of this constructor), so it may be easier to debug if the verification instead comes from the dart constructor.
It would be duplicated code, though, so I'm open to not making this change.
There was a problem hiding this comment.
Added an assert which I think is a happy medium since it will help developers debug.
|
A couple of comments on the dart code, but mostly documentation suggestions. Mostly looks good though! |
…gy.dart Co-authored-by: Gray Mackall <[email protected]>
…r.dart Co-authored-by: Gray Mackall <[email protected]>
…r.dart Co-authored-by: Gray Mackall <[email protected]>
…r.dart Co-authored-by: Gray Mackall <[email protected]>
…r.dart Co-authored-by: Gray Mackall <[email protected]>
…r.dart Co-authored-by: Gray Mackall <[email protected]>
….dart Co-authored-by: Gray Mackall <[email protected]>
...id_camerax/android/src/main/java/io/flutter/plugins/camerax/FallbackStrategyHostApiImpl.java
Outdated
Show resolved
Hide resolved
...oid_camerax/android/src/main/java/io/flutter/plugins/camerax/QualitySelectorHostApiImpl.java
Outdated
Show resolved
Hide resolved
packages/camera/camera_android_camerax/lib/src/fallback_strategy.dart
Outdated
Show resolved
Hide resolved
…o/flutter/plugins/camerax/FallbackStrategyHostApiImpl.java Co-authored-by: Gray Mackall <[email protected]>
…o/flutter/plugins/camerax/QualitySelectorHostApiImpl.java Co-authored-by: Gray Mackall <[email protected]>
…gy.dart Co-authored-by: Gray Mackall <[email protected]>
camsim99
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/packages/4620, due to - The status or check suite version_check has failed. Please fix the issues identified (or deflake) before re-applying this label. |
camsim99
added
the
autosubmit
flutter/packages@84218b9...9bf208f 2023-08-16 [email protected] Manual roll Flutter from 9b6945b to f0e7c51 (63 revisions) (flutter/packages#4719) 2023-08-15 [email protected] Update palette_generator_test.dart (flutter/packages#4720) 2023-08-15 [email protected] [pigeon] Fix typo in README.md (flutter/packages#4707) 2023-08-15 [email protected] [camerax] Explicitly remove READ_EXTERNAL_STORAGE permission (flutter/packages#4716) 2023-08-15 [email protected] [webview_flutter_platform_interface] Adds support for retrieving user agent (flutter/packages#4563) 2023-08-15 [email protected] [camerax] Wrap classes to implement resolution configuration for video capture (flutter/packages#4620) 2023-08-15 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.3 to 2.21.4 (flutter/packages#4706) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Wraps classes to implement resolution configuration for video capture. No functionality changes.
Part of flutter/flutter#120462.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).