-
Notifications
You must be signed in to change notification settings - Fork 11
Add check permissions state independent of request #30
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
base: master
Are you sure you want to change the base?
Add check permissions state independent of request #30
Conversation
…sting permission + naming: + note: PermissionRequester remains as it was, for backward compatibility. - NativeRequester -> NativeActivity - NativeRequesterFactory -> NativeActivityFactory - getRequesterAsync -> getActivityAsync - PermissionRequest -> PermissionState - PermissionRequestBuilder -> PermissionStateBuilder - PekoPermissionRequestBuilder -> PekoPermissionStateBuilder - PekoPermissionRequester -> PekoPermissionManager + permissionState() added to PermissionRequester. + nativeActivity cached to make available handling of permission requests and permission state checking Simultaneously. + As, NativeActivity is ot necessarily disposable, PermissionManager sends Channel and requestId dynamically. - channel in PekoViewModel replaced with map of request id and its channel. + PermissionResult now has data class NeverAskedOrDeniedPermanently for checking permissions without request.
+ example updated
|
Thanks for the PR! Had a brief look, seems like good work. Will continue when I find some more time. |
|
Thanks, Marko! Replace the random ID logic to satisfy the SonarQube quality gate Add a short README update explaining the new checkPermissionState() feature I will push the changes as part of the same PR to stay clean and reviewable. Appreciate your time and feedback! |
acc200c to
0a97e1e
Compare
|
I’ve pushed a few final updates: Renamed permissionState() → checkPermissionState() for clarity Updated the README with usage examples and a short note about v4.0.0 plans Refactored getRequestId() to simplify random ID generation and satisfy SonarQube I believe this wraps up the feature cleanly within the 3.x scope. Let me know if you'd like to adjust or trim anything in the README! |
- rename permissionState() to checkPermissionState() - refactor getRequestId() and satisfy SonarQube + update Readme for version 3.1.0
0a97e1e to
296af1d
Compare
|
deva666
left a comment
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.
Great stuff, thanks again for the PR! Can you please format the files, I see in multiple that spaces are missing. And what do you think about my breaking change comment?
|
|
||
| override fun checkPermissionsState(vararg permissions: String): Flow<PermissionResult> { | ||
| return if (permissions.isEmpty()) { | ||
| flowOf(PermissionResult.Cancelled) |
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.
should we rather here return emptyFlow() ?
| override fun finish() { | ||
| super.finish() | ||
| viewModel.channel.close() | ||
| viewModel.closeAllChannels() |
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.
Is viewModel.closeAllChannels going to be called multiple times? Once on this activity finish and again on viewModel.onCleared ... should we remove them from the map after closing?
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.
Great point —
You're right that closeAllChannels() is already handled inside onCleared().
I'll remove the finish() override and make closeAllChannels() private to prevent accidental misuse.
Thanks for catching that!
|
|
||
| override fun checkStateOfDeniedPermissions(permissions: Array<out String>,channel: Channel<PermissionResult>) { | ||
| if (permissions.isEmpty()){ | ||
| channel.trySend(PermissionResult.Cancelled) |
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.
Is Cancelled really the result we should send for empty permissions? Somehow an empty flow that completes immediately sounds more logical
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.
That’s fair —
I re-checked the call site and checkStateOfDeniedPermissions() is only invoked when permissionState.denied is non-empty.
So the permissions.isEmpty() check inside is unreachable in practice.
I’ll remove that block to keep things clean and lean.
| * Represents a permission, not granted nor denied | ||
| * @param permission, the permission which is untouched | ||
| */ | ||
| data class NeverAskedOrDeniedPermanently(val permission: String) : PermissionResult() |
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.
Could this be a breaking change, adding a new type to sealed class ... if somebody expects an exhaustive when then it will not compile anymore
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.
I totally get your concern — adding a new type to a sealed class can break existing when exhaustiveness.
In this case, though, NeverAskedOrDeniedPermanently is only returned from the new checkPermissionState() API — not from request().
This means existing usage patterns are untouched, and any dev using the new function will see this in the README and handle it accordingly.
So this sealed subclass is intentionally scoped to that case, doesn’t break any existing flows, and is backward compatible.
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.
Yes, but PermissionResult is exposed in the PermissionRequester. Calling the request function returns the PermissionResult and if some code has an exhaustive when check, adding this new type will break the compilation. Not a major issue, but this then should be a part of next major version update, not a minor one.
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.
So, to avoid introducing a breaking change in PermissionResult, I suggest the following:
If you're on board, we can introduce a new sealed class:
sealed class PermissionState {
data class Granted(val permission: String) : PermissionState()
data class NeedsRationale(val permission: String) : PermissionState()
data class NeverAskedOrDeniedPermanently(val permission: String) : PermissionState()
object Cancelled : PermissionState()
}This class will be used exclusively by the checkPermissionState() API, keeping PermissionResult stable and backward compatible.
In version 4.0.0, when internal tracking is added, we can split NeverAskedOrDeniedPermanently into two clear results: NotAsked and DeniedPermanently.
Let me know if that sounds good to you!
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.
Sounds good
Hi Marko — Really sorry for the delay in getting back to you. A few things pulled me away temporarily, but I’ve picked this back up and gone through all your feedback carefully. Thanks again for the thoughtful comments — they all make sense, and I’m preparing the updates now. Will push changes shortly! Also, Thanks for pointing out the reformatting issue! I formatted only the modified blocks to preserve Git blame and keep the original authorship intact. But it’s possible my formatting config differs slightly from yours — do you have a .editorconfig, .idea/codeStyle, or shared formatter setup you’d prefer I use? I’d be happy to run a full reformat based on that if you'd like. Let me know! |



No description provided.