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

[camera] Improving handling when camera permissions are not granted. #2848

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

panmari
Copy link
Contributor

@panmari panmari commented Jun 25, 2020

Description

If camera permissions are not granted, the plugin currently throws an exception in Java:

E/MethodChannel#plugins.flutter.io/camera(22476): Failed to handle method call
E/MethodChannel#plugins.flutter.io/camera(22476): java.lang.ClassCastException: android.hardware.camera2.CameraAccessException cannot be cast to java.lang.RuntimeException
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugins.camera.MethodCallHandlerImpl.handleException(MethodCallHandlerImpl.java:172)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugins.camera.MethodCallHandlerImpl.lambda$onMethodCall$0$MethodCallHandlerImpl(MethodCallHandlerImpl.java:66)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugins.camera.-$$Lambda$MethodCallHandlerImpl$OMU5dV7VCKXKBT37_ThIybqlHuo.onResult(Unknown Source:6)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugins.camera.CameraPermissions.requestPermissions(CameraPermissions.java:49)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugins.camera.MethodCallHandlerImpl.onMethodCall(MethodCallHandlerImpl.java:57)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler.onMessage(MethodChannel.java:226)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.embedding.engine.dart.DartMessenger.handleMessageFromDart(DartMessenger.java:85)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at io.flutter.embedding.engine.FlutterJNI.handlePlatformMessage(FlutterJNI.java:631)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at android.os.MessageQueue.nativePollOnce(Native Method)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at android.os.MessageQueue.next(MessageQueue.java:336)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at android.os.Looper.loop(Looper.java:174)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at android.app.ActivityThread.main(ActivityThread.java:7356)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at java.lang.reflect.Method.invoke(Native Method)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
E/MethodChannel#plugins.flutter.io/camera(22476): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

That's because the code attempts to cast a CameraAccessException to a RuntimeException. I think that part of the code is executed unintentionally: what's desired at this point is to fully handle the exception in Java and pass it to dart, i.e. don't throw it again.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

The intent here is to catch the exception and pass it to dart code, no
need to raise it afterwards.

An additional complication here is that CameraAccessException does
extend from RuntimeException.
@panmari
Copy link
Contributor Author

panmari commented Jun 29, 2020

@bparrishMines any concerns?

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

Thanks for the contribution! I changed the CHANGELOG exception to be more specific.

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 change does still need a test. It should be possible to revoke camera permissions for a test and then verify you don't see the cast exception when opening the camera. Some code we already have for controlling camera permissions on Android: https://github.com/flutter/plugins/blob/master/packages/camera/example/test_driver/camera_e2e_test.dart

@panmari
Copy link
Contributor Author

panmari commented Jul 8, 2020

Thanks for taking a look! I'm not quite sure how to create a test here, can you help me with these two things?

  1. Would the test live in the package or in the example, like the e2e test you linked?
  2. How would I formulate the expectation of this test? The error currently doesn't crash the app, it only prints the failure I quote in my initial comment above. Would I need to run the test with some flag to make these type of errors fatal?

@panmari
Copy link
Contributor Author

panmari commented Jul 14, 2020

@bparrishMines do you have any advice?

@bparrishMines
Copy link
Contributor

@cyanglaz Do you know of a way to properly test this? It looks like we grant permissions before running the test and revoke permissions at the end. I don't think the e2e provides a way to revoke permission in the middle of the test. I'm considering merging this without tests.

@cyanglaz
Copy link
Contributor

@bparrishMines I'm not sure either. Maybe we can use a flutter driver test instead of e2e to test this?
@dnfield might know how to test this with e2e.

@panmari
Copy link
Contributor Author

panmari commented Jul 27, 2020

@dnfield Dan, do you have an idea how to create a test?

@panmari
Copy link
Contributor Author

panmari commented Aug 5, 2020

@bparrishMines any guidance on how to progress here?

@panmari
Copy link
Contributor Author

panmari commented Aug 11, 2020

@bparrishMines any suggestion on how to resolve the situation? I still would love to add a test, I'm just not sure how to write it.

@bparrishMines
Copy link
Contributor

I don't believe we currently have a good way to test this, but the code looks good and this needs to be fixed. Merging

@bparrishMines bparrishMines merged commit 1c9529a into flutter:master Aug 11, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants