-
-
Notifications
You must be signed in to change notification settings - Fork 857
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
Refactor Permission Handling for Improved Readability #1164
Conversation
PermissionCallbacks
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michaelnabil230,
This is a really interesting addition that we would love to support. I have two requests that I hope you could help update:
- The different callbacks currently accept a function of type
VoidCallback
, I can imagine that some developers would like to run asynchronous tasks in side the callback (e.g. fetching a location once location permission is granted). I think it would be beneficial to update these callbacks to support aFuture<void>
(or possible aFutureOr<void>
) method. - With the current changes there are two methods to start the permission request,
request
andask
. I think this might lead to confusion as it will be possible to register callbacks and then userequest
which doesn't use the registered callbacks. Since this is not a breaking change, why not add the callbacks to the currentPermissionAction
extension and call them from therequest
method?
Co-authored-by: Maurits van Beusekom <maurits@baseflow.com>
Thank you for your instruction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michaelnabil230,
I have noticed one more issue that I feel should be addressed.
Thanks for the update! It's awesome to hear that the code is ready to merge. Let's proceed with the merging process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for this contribution.
P.S. maybe you could create another PR updating the documentation and explain this alternative route, so people are informed about this new feature.
This PR introduces a new style for checking permission status.
Before:
After:
In this updated code, the permission status checks have been replaced with a more concise style that leverages the
PermissionCallbacks
extension. This change simplifies the code and makes it more readable.Pre-launch Checklist
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is does not need version changes.CHANGELOG.md
to add a description of the change.///
).main
.dart format .
and committed any changes.flutter analyze
and fixed any errors.