Fix crash when user wants to use the camera #1400
Conversation
…when sending attachment to a room (#1395)
|
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
bmarty
changed the title
| ConfigureRoomView( | ||
| state = state, | ||
| modifier = modifier, | ||
| onBackPressed = this::navigateUp, | ||
| onRoomCreated = this::onRoomCreated, | ||
| onOpenSystemSettings = { context.openAppSettingsPage() } |
There was a problem hiding this comment.
Maybe this can be moved into PermissionsPresenter itself so each client of PermissionsPresenter doesn't have to manually add it?
There was a problem hiding this comment.
yes, a bit simpler: dd5d67d
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #1400 +/- ##
===========================================
+ Coverage 57.97% 58.01% +0.03%
===========================================
Files 1126 1128 +2
Lines 29794 29853 +59
Branches 6052 6060 +8
===========================================
+ Hits 17274 17318 +44
- Misses 9931 9936 +5
- Partials 2589 2599 +10
☔ View full report in Codecov by Sentry. |
| ) : Presenter<ConfigureRoomState> { | ||
|
|
||
| private val cameraPermissionPresenter: PermissionsPresenter = permissionsPresenterFactory.create(android.Manifest.permission.CAMERA) | ||
| private var pendingPermissionRequest = false |
There was a problem hiding this comment.
Maybe this should be a remember { mutableStateOf() } ?
Moreover, perhaps this could also be hoisted to PermissionsPresenter to simplify the client side impementation.
| @@ -182,6 +185,10 @@ class ConfigureRoomPresenterTests { | |||
| // From camera | |||
| val uriFromCamera = Uri.parse(AN_URI_FROM_CAMERA) | |||
| fakePickerProvider.givenResult(uriFromCamera) | |||
| assertThat(newState.cameraPermissionState.permissionGranted).isFalse() | |||
There was a problem hiding this comment.
Shouldn't we also test that when permission is not granted we catually call into the PermissionsPresenter to request it?
There was a problem hiding this comment.
You're right, there is a missing test. I will ad it.
| @@ -65,7 +65,7 @@ class NotificationsOptInPresenter @AssistedInject constructor( | |||
| if (notificationsPermissionsState.permissionGranted) { | |||
| callback.onNotificationsOptInFinished() | |||
| } else { | |||
| notificationsPermissionsState.eventSink(PermissionsEvents.OpenSystemDialog) | |||
| notificationsPermissionsState.eventSink(PermissionsEvents.AskPermissionToUser) | |||
There was a problem hiding this comment.
In similar android contexts this is usually called "RequestPermissions".
|
@julioromano should be fine now, the CI should turn green. |
|
Kudos, SonarCloud Quality Gate passed!
|
Type of change
Content
Camera permission is not required when using the external app, except when the Android Manifest contains the CAMERA permission. And this permission has been added when we integrated Element Call.
This PR ensures that the CAMERA permission is provided when user want to:
Also contains some cleanup on the codebase, and some new localazy strings.
At some point we will need to revisit how we manage the permissions, since this is also done for the location permission, in the dedicated module.
Motivation and context
Fixes #1395
Screenshots / GIFs
Tests
Tested devices
Checklist