Skip to content

MS-154 camera permission improvement#699

Merged
luhmirin-s merged 2 commits into
mainfrom
feature/MS-154-camera-permission-improvement
Jun 13, 2024
Merged

MS-154 camera permission improvement#699
luhmirin-s merged 2 commits into
mainfrom
feature/MS-154-camera-permission-improvement

Conversation

@luhmirin-s
Copy link
Copy Markdown
Contributor

@luhmirin-s luhmirin-s commented Apr 24, 2024

Added an extra state in "LiveFeedbackFragment" to ask for permission if not granted before.

PS. The old version with the separate alert screen has been scrapped as overly complex.

@cla-bot cla-bot Bot added the ... label Apr 24, 2024
@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch 3 times, most recently from 1a6ebad to 6490dbe Compare April 24, 2024 11:53
@luhmirin-s luhmirin-s marked this pull request as ready for review April 24, 2024 12:44
@luhmirin-s
Copy link
Copy Markdown
Contributor Author

The coverage is as good as it gets since the "uncovered" lines should have been excluded in the first place.

@luhmirin-s luhmirin-s requested review from a team, BurningAXE, TristramN, alex-vt, alexandr-simprints, meladRaouf and ybourgery and removed request for a team April 24, 2024 12:46
@alexandr-simprints
Copy link
Copy Markdown
Contributor

alexandr-simprints commented Apr 25, 2024

Would it be possible to record a video of how the permission handling currently looks like? Considering that these changes aim to improve UI, I'd like us to handle both Deny and Deny, Don't ask again states individually. For that matter, utilizing the already existing PermissionStatus enum comes in handy. By using it, it becomes possible to render the screen content depending on its value.

Figma example:
image

The state of the permission response can be summarized in the following way:

  • Granted: proceed with the camera preview
  • Deny: display the action button and show the rationale to the user, explaining that the camera access is required in order to render preview. When the button is pressed, it displays the system permission dialog once again.
  • Deny, don't ask again: display the action button and show the rationale to the user, explaining that they need to provide the camera access in settings. When the button is pressed, it navigates the user to the app's settings page.

@luhmirin-s
Copy link
Copy Markdown
Contributor Author

For now, I just replicated our flow in the fingerprint connection with the fullscreen alert in case the initial dialog is rejected. There is a lot of room for improvement.

Screen_recording_20240425_142527.mp4

@BurningAXE
Copy link
Copy Markdown
Contributor

For now, I just replicated our flow in the fingerprint connection with the fullscreen alert in case the initial dialog is rejected. There is a lot of room for improvement.

Screen_recording_20240425_142527.mp4

In Fingerprint the alert is dismissed when you tap the settings button so when you come back you don't have to press retry. Permissions are checked in the Fragment's onResume() so if user didn't grant the permission, alert would be displayed again. Not a big deal but nice to have.

Copy link
Copy Markdown
Contributor

@BurningAXE BurningAXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much boilerplate needed to add a new alert :(


AlertError.ACTION_RETRY, AlertContract.ALERT_BUTTON_PRESSED_BACK -> {
shouldRequestPermissions = true
// viewModel.recapture()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be deleted?

@luhmirin-s
Copy link
Copy Markdown
Contributor Author

So much boilerplate needed to add a new alert :(

In this case, the boilerplate is more due to the navigation to the settings (i.e. out of the app) and the camera lifecycle handling.

@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch from 6490dbe to 762b648 Compare April 25, 2024 12:55
@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch from 762b648 to 8ded625 Compare May 9, 2024 07:28
@alexandr-simprints
Copy link
Copy Markdown
Contributor

Could you please verify that the following scenarios would not disturb the UX:

  1. Access granted
  2. Access Denied, Retry is clicked
  3. Access Denied, Don't Ask Again is selected, Retry is clicked
  4. Access Denied, Don't Ask Again is selected, Open Settings is clicked, User returns without giving the permissions
  5. Access Denied, Don't Ask Again is selected, Open Settings is clicked, User returns with providing the permissions

@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch 2 times, most recently from 17608b9 to 14ef74d Compare May 22, 2024 14:35
@luhmirin-s
Copy link
Copy Markdown
Contributor Author

Simple deny/allow permission flow:

Screen_recording_20240523_144111.mp4

Flow with "Don't ask again" and closing settings with and without providing permission:

Screen_recording_20240523_144232.mp4

As you can the alert is shown the first time the screen is opened without permission and with the "Don't ask again" flag. Once the alert if permission is not provided, the exit form will be shown on the next retry. This allows us to force the user into either giving permission or explicitly exiting the flow, IMO this is the clearest way to handle it.

@luhmirin-s luhmirin-s requested a review from BurningAXE May 28, 2024 06:47
@alexandr-simprints
Copy link
Copy Markdown
Contributor

Once the alert if permission is not provided, the exit form will be shown on the next retry.

This doesn't seem like the best user experience decision. I was incredibly confused while watching the video and seeing the exit form displayed after hitting the Retry button. I've realized it was intentional only after reading the comment afterward.

As mentioned above, I'm still in favor of simplifying the flow for users by using the same screen with a single permission button and rationale text.
image

I find the current solution unnecessary visually complex.

@BurningAXE
Copy link
Copy Markdown
Contributor

I agree with Alex that showing the exit form without user requesting it might be confusing. I'd keep the user on the same screen asking for permissions until they either give permissions or press the back button to explicitly go to the exit form. IMO this is handled pretty well for fingerprint and we should copy the behaviour to face.

@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch from 14ef74d to f8a1995 Compare June 3, 2024 08:37
@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch 2 times, most recently from 0d7f7fc to 6fcb3b3 Compare June 5, 2024 13:10
@luhmirin-s
Copy link
Copy Markdown
Contributor Author

@alexandr-simprints @BurningAXE Redid the screen with significant simplification. No more separate alert screens and juggling navigation.

@luhmirin-s luhmirin-s force-pushed the feature/MS-154-camera-permission-improvement branch from 6fcb3b3 to c78164c Compare June 5, 2024 13:19
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
61.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@alexandr-simprints
Copy link
Copy Markdown
Contributor

@luhmirin-s could you please share a video similar to how you did it last time?

@luhmirin-s
Copy link
Copy Markdown
Contributor Author

Screen_recording_20240606_165721.mp4

@luhmirin-s luhmirin-s merged commit 35d2047 into main Jun 13, 2024
@luhmirin-s luhmirin-s deleted the feature/MS-154-camera-permission-improvement branch June 13, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants