Add check permissions state independent of request - #30
Conversation
|
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! |
0a97e1e to
296af1d
Compare
|
deva666
left a comment
deva666
left a comment
There was a problem hiding this comment.
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.
should we rather here return emptyFlow() ?
| override fun finish() { | ||
| super.finish() | ||
| viewModel.channel.close() | ||
| viewModel.closeAllChannels() |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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!
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! |
…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 # Conflicts: # examples/src/main/java/com/markodevcic/samples/MainActivity.kt
- rename permissionState() to checkPermissionState() - refactor getRequestId() and satisfy SonarQube + update Readme for version 3.1.0
- make closeAllChannels method private in PekoViewModel - remove onFinish method from PekoActivity - remove redundant ReceiveChannels from PermissionRequester - remove redundant close chanels - replace PermissionResult.Cancelled with emptyFlow() in PermissionRequester for empty permissions
- rename PermissionState to PermissionGtoup to make PermissionState free fot the publik API - rename PermissionStateBuilder interface to PermissionGrouper to reflect its job - rename PermissionState variables to PermissionGroup
+ add PermissionState sealed class - update example and checkPermissionState() to handle states with new sealed class PermissionState
bb45b13 to
6d6d8b9
Compare
6d6d8b9 to
94546e3
Compare
Evaluate permission groups when each Flow is collected, rather than when it is created, so emitted results reflect current permission state. Ensure request and state-check channels are closed and the transient activity is finished even when collection is cancelled.\n\nRepair the request test to drive the NativeActivity callback channel and add coverage for deferred evaluation, cancellation cleanup, and PermissionState delivery.
2 participants
No description provided.