Skip to content
This repository has been archived by the owner on Feb 4, 2024. It is now read-only.

Fix permission request subjects being not complete #319

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

samuolis
Copy link

  • Fix issue, when permission request is started, but then app put to background and resumed

    • Expected:
      • Request could be started again
    • Existant:
      • Request cant be started, because, there is not finished requests in queue
  • So now all requests could be finished with cancelation exception and requests cleared from list

@mrArtCore
Copy link

Hello!
Do you want to throw Error when app goes to the background?
IMHO if you want to stop your permissions request when the app set to onPause() just dispose your request. It'll be enough, won't it?

Sorry but I just can't get it. I mean why it should throw?

@samuolis
Copy link
Author

Hello!
Do you want to throw Error when app goes to the background?
IMHO if you want to stop your permissions request when the app set to onPause() just dispose your request. It'll be enough, won't it?

Sorry but I just can't get it. I mean why it should throw?

When application goes to background android permission dialog disappears and if you are not specifically disposing request, it cant be started, so if it cant be started after some action, library should handle it. Only way to end this dispose was to throw error to subject.

@neworld
Copy link

neworld commented Mar 20, 2020

The library should cancel all requests the user is not able to complete. Otherwise, there could unexpected memory leaks if the user of the library is not aware of such situations. This is especially true if you are subscribing to RxPermissions inside ViewModel, which does not have such states at all.

@mrArtCore
Copy link

When application goes to background android permission dialog disappears and if you are not specifically disposing request, it cant be started, so if it cant be started after some action, library should handle it. Only way to end this dispose was to throw error to subject.

I have never seen and never get into such scenario. How could I reproduce this case?

@mrArtCore
Copy link

The library should cancel all requests the user is not able to complete. Otherwise, there could unexpected memory leaks if the user of the library is not aware of such situations.

  • cancel request and throw an error are two big differences. How to reproduce the scenario of memory leaks? Also in RX approach clients/subscribers initiate data flow. And clients/subscribers responsible to cancel flow (dispose subscription) whenever they don't want or don't able to consume.
    If you request permission in an appropriate UI state - all should be ok.

...This is especially true if you are subscribing to RxPermissions inside ViewModel, which does not have such states at all.

  • I don't think it is a good idea to request permission in your viewModel. ViewModel shouldn't get a link to an activity context. It will leak. And it's bad design to pass activity context there. Avoid using any context inside ViewModel so that you are always safe and able to unit test it.

@neworld
Copy link

neworld commented Mar 20, 2020

cancel request and throw an error are two big differences.

Throwable is used pretty often as a canceling event. For example Object.await() uses exception to interrupt waiting for the state. Or job cancelation at kotlinx.coroutines. But yeah, non-throwable could be feasible solution as well.

How to reproduce the scenario of memory leaks? Also in RX approach clients/subscribers initiate data flow. And clients/subscribers responsible to cancel flow (dispose subscription) whenever they don't want or don't able to consume.

For example use permissions is services which could live longer than single fragment:

@PerActivityScope
class LocationService @Inject constructor (activity: Activity) {
  fun getLocation(): Single<Location> {
      rxPermissions.request(...)
          .flatMap { ... }
         .....
  }
}

I don't think it is a good idea to request permission in your viewModel. ViewModel shouldn't get a link to an activity context. It will leak. And it's bad design to pass activity context there. Avoid using any context inside ViewModel so that you are always safe and able to unit test it.

I suppose the original ViewModel idea states the same. However, in reality, it is very hard to maintain code with many bidirectional communication points between fragment and ViewModel. And believe me, in a large codebase it became unmanageable spaghetti pretty fast.

@GediminasZukas
Copy link

When application goes to background android permission dialog disappears and if you are not specifically disposing request, it cant be started, so if it cant be started after some action, library should handle it. Only way to end this dispose was to throw error to subject.

I have never seen and never get into such scenario. How could I reproduce this case?

You can reproduce the issue in such scenario:

  1. Have activity with android:launchMode="singleTask"
  2. Have a fragment inside that Activity with UI button which triggers permission request flow

And then:

button click -> permissions dialog appears -> don't do any clicks on dialog and put app to the background via "Home" button click -> click on application icon -> app resumes -> click on UI button again -> permission dialog won't appear anymore and user can't perform intended action.

There is already similar issues created about this behaviour, for example: vanniktech/RxPermission#66 (it is of course different library but behaviour is the same in this library).

@hoanghiephui
Copy link

okey

1 similar comment
@hoanghiephui
Copy link

okey

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants