Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: The permission workflow for the camera should now be OK #2763

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Aug 9, 2022

Hi everyone,

I have fixed many issues on this PR, but the main change is that it should work far better on iOS.
Basically:

  • The PermissionListener was created twice -> singleton
  • shouldShowRequestRationale always returns false on iOS, so the code now takes care of this
  • (Really) ensure only one permission request can be asked at a given time

This PR will participate to improve #2741

@g123k g123k self-assigned this Aug 9, 2022
@g123k g123k requested a review from a team as a code owner August 9, 2022 17:52
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2022

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 7.16%. Comparing base (2ea0da3) to head (f4eaec2).
Report is 2150 commits behind head on develop.

Files with missing lines Patch % Lines
...ages/smooth_app/lib/helpers/permission_helper.dart 0.00% 9 Missing ⚠️
packages/smooth_app/lib/pages/scan/scan_page.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #2763      +/-   ##
==========================================
- Coverage     8.86%   7.16%   -1.70%     
==========================================
  Files          161     218      +57     
  Lines         6623   10568    +3945     
==========================================
+ Hits           587     757     +170     
- Misses        6036    9811    +3775     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon
Copy link
Member

@g123k merging if that's ok

@g123k
Copy link
Collaborator Author

g123k commented Aug 11, 2022

@g123k merging if that's ok

For me, it is :p

@teolemon
Copy link
Member

I'd very much like to ship that quickly in order to have a clearer view on scan Issues

@g123k
Copy link
Collaborator Author

g123k commented Aug 11, 2022

It should only change for iOS users, on Android, it should behave like before (which was OK)

@teolemon
Copy link
Member

Which was not ok, I have several videos on my Pixel @g123k ?

@g123k
Copy link
Collaborator Author

g123k commented Aug 11, 2022

Mmm could you please post one? Or pink an existing issue?

@teolemon
Copy link
Member

screen-20220809-105500.mp4

@g123k
Copy link
Collaborator Author

g123k commented Aug 11, 2022

I can't reproduce it, so let's say it's ok

@teolemon
Copy link
Member

ok, let's merge and see

@teolemon teolemon merged commit b28e905 into openfoodfacts:develop Aug 11, 2022
@g123k g123k deleted the fix_ios_camera_permission branch September 2, 2022 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants