packages icon indicating copy to clipboard operation
packages copied to clipboard

[camera_android] Fix camera android deprecation warning for CamcorderProfile.get()

Open navaronbracke opened this issue 2 years ago • 6 comments

This PR was recreated from https://github.com/flutter/plugins/pull/7062 in flutter/plugins

This PR adds a @TargetApi() annotation and moves a @SuppressWarnings("deprecation") annotation to the right place.

List which issues are fixed by this PR. You must list at least one issue. https://github.com/flutter/flutter/issues/94999

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy. N/A

Pre-launch Checklist

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

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

navaronbracke avatar Feb 23 '23 07:02 navaronbracke

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Feb 23 '23 07:02 flutter-dashboard[bot]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Feb 23 '23 07:02 google-cla[bot]

@stuartmorgan I'll recap my findings about the use of --fatal-warnings in the build-examples tooling to act as a test for this PR (and in the long term, for all plugins).

If I build the example with a release build: cd packages/camera/camera_android/example && flutter run --release I get the following logs:

Build Logs
navaronbracke@MacBook-Pro-van-Navaron example % flutter run --release
Launching lib/main.dart on motorola one in release mode...
w: Runtime JAR files in the classpath should have the same version. These files were found in the classpath:
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-jdk8/1.5.30/5fd47535cc85f9e24996f939c2de6583991481b0/kotlin-stdlib-jdk8-1.5.30.jar (version 1.5)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-jdk7/1.5.30/525f5a7fa6d7790a571c07dd24214ed2dda352fe/kotlin-stdlib-jdk7-1.5.30.jar (version 1.5)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.7.10/d2abf9e77736acc4450dc4a3f707fa2c10f5099d/kotlin-stdlib-1.7.10.jar (version 1.7)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-common/1.7.10/bac80c520d0a9e3f3673bc2658c6ed02ef45a76a/kotlin-stdlib-common-1.7.10.jar (version 1.7)
w: Some runtime JAR files in the classpath have an incompatible version. Consider removing them from the classpath
e: /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/androidx.annotation/annotation/1.5.0/857678d6b4ca7b28571ef7935c668bdb57e15027/annotation-1.5.0.jar!/META-INF/annotation.kotlin_module: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.7.1, expected version is 1.5.1.
e: /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.7.10/d2abf9e77736acc4450dc4a3f707fa2c10f5099d/kotlin-stdlib-1.7.10.jar!/META-INF/kotlin-stdlib.kotlin_module: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.7.1, expected version is 1.5.1.
e: /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-common/1.7.10/bac80c520d0a9e3f3673bc2658c6ed02ef45a76a/kotlin-stdlib-common-1.7.10.jar!/META-INF/kotlin-stdlib-common.kotlin_module: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.7.1, expected version is 1.5.1.
w: Runtime JAR files in the classpath should have the same version. These files were found in the classpath:
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-jdk8/1.5.30/5fd47535cc85f9e24996f939c2de6583991481b0/kotlin-stdlib-jdk8-1.5.30.jar (version 1.5)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-jdk7/1.5.30/525f5a7fa6d7790a571c07dd24214ed2dda352fe/kotlin-stdlib-jdk7-1.5.30.jar (version 1.5)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.7.10/d2abf9e77736acc4450dc4a3f707fa2c10f5099d/kotlin-stdlib-1.7.10.jar (version 1.7)
    /Users/navaronbracke/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-common/1.7.10/bac80c520d0a9e3f3673bc2658c6ed02ef45a76a/kotlin-stdlib-common-1.7.10.jar (version 1.7)
w: Some runtime JAR files in the classpath have an incompatible version. Consider removing them from the classpath
No issues found.
Running Gradle task 'assembleRelease'...                           61.6s
✓  Built build/app/outputs/flutter-apk/app-release.apk (7.4MB).
Installing build/app/outputs/flutter-apk/app-release.apk...      2,946ms

Flutter run key commands.
h List all available interactive commands.
c Clear the screen
q Quit (terminate the application on the device).

The deprecation messages no longer appear (which is the goal of this PR), but the following messages from Gradle are seen as warnings (Also notice they start with w:), which would become fatal with the flag enabled:

w: Runtime JAR files in the classpath should have the same version.
These files were found in the classpath:
<list of files with classpath issues>

Those messages are actually dependency problems (as the dependencies and the project itself use different version of the Kotlin Standard Libary, hence the warnings) and shouldn't be fatal? I'm not sure if there is a way to lower the severity of this category of messages (to an info category of sorts), so that we can rely on --fatal-warnings as intended.

navaronbracke avatar Feb 23 '23 08:02 navaronbracke

I'm exploring an alternate solution for making deprecation warnings errors now; if that pans out I'll have a PR soon to enable it with an initial exclusion list, and then this PR will be able to include removing camera_android from the exclusion list.

stuartmorgan-g avatar Feb 24 '23 16:02 stuartmorgan-g

I'm exploring an alternate solution for making deprecation warnings errors now; if that pans out I'll have a PR soon to enable it with an initial exclusion list, and then this PR will be able to include removing camera_android from the exclusion list.

This is posted as https://github.com/flutter/packages/pull/3293

Once that lands, this can be covered by just enabling this: https://github.com/flutter/packages/pull/3293/files#diff-c40bf8281656da0025c18216f38b473c0f0c018ccce9b04c57cf25f8a391b91fR38-R39

stuartmorgan-g avatar Feb 27 '23 18:02 stuartmorgan-g

@camsim99 after enabling the javac options I get a bunch of raw_types warnings. When I specify HashMap<String, CameraFeature<Object>> cameraFeatures; to get rid of those, I then get warnings like

/Users/navaronbracke/Documents/packages/packages/camera/camera_android/android/src/main/java/io/flutter/plugins/camera/features/CameraFeatures.java:94: error: incompatible types: CameraFeature<Object> cannot be converted to AutoFocusFeature
    return (AutoFocusFeature) featureMap.get(AUTO_FOCUS);
                                            ^
/Users/navaronbracke/Documents/packages/packages/camera/camera_android/android/src/main/java/io/flutter/plugins/camera/features/CameraFeatures.java:103: error: incompatible types: AutoFocusFeature cannot be converted to CameraFeature<Object>
    this.featureMap.put(AUTO_FOCUS, autoFocus);

for each camera feature. Do you know how to resolve those warnings? Those are the last ones I need to resolve. Should we just get rid of the internal HashMap (and only provide a map for the one for-loop that needs one)?

navaronbracke avatar Mar 03 '23 14:03 navaronbracke

It looks like there are a ton more warnings, unfortunately. If you're willing to work through all of them, that would be awesome; if not I can add some fixes here in the next week or two since I'm working on the packages where it's still disabled.

Yes, I plan on addressing those warnings. I just need a bit of guidance for the change relating to the various CameraFeature getters that are now complaining.

navaronbracke avatar Mar 04 '23 16:03 navaronbracke

I'm definitely not a Java expert, but I think for a use case like CameraFeatures's featureMap you need CameraFeatures<?> rather than CameraFeatures<Object>.

stuartmorgan-g avatar Mar 04 '23 16:03 stuartmorgan-g

For the fallthrough warnings in the two methods, I suppressed them because a comment in the code stated that the falllthrough is deliberate.

navaronbracke avatar Mar 04 '23 17:03 navaronbracke

@stuartmorgan I think I got everything sorted.

For the @SuppressWarning("try"), it was referring to the unused variable in the try-with-resources. Since the tests still pass without that try block, I removed the block, so the suppression is not needed.

navaronbracke avatar Mar 06 '23 13:03 navaronbracke

Removing needs tests as in follow up commits tests were added to verify the change.

CaseyHillers avatar Mar 06 '23 18:03 CaseyHillers

What release is this fix available in?

bartekpacia avatar Mar 21 '23 16:03 bartekpacia

What release is this fix available in?

You can look at the pubspec changes in a PR to know what version it is in.

stuartmorgan-g avatar Mar 21 '23 16:03 stuartmorgan-g