RxPermissions icon indicating copy to clipboard operation
RxPermissions copied to clipboard

Fix permission request subjects being not complete

Open samuolis opened this issue 4 years ago • 9 comments

  • 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

samuolis avatar Mar 19 '20 16:03 samuolis

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?

mrArtCore avatar Mar 19 '20 20:03 mrArtCore

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.

samuolis avatar Mar 20 '20 06:03 samuolis

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.

neworld avatar Mar 20 '20 09:03 neworld

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 avatar Mar 20 '20 20:03 mrArtCore

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.

mrArtCore avatar Mar 20 '20 21:03 mrArtCore

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.

neworld avatar Mar 20 '20 22:03 neworld

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: https://github.com/vanniktech/RxPermission/issues/66 (it is of course different library but behaviour is the same in this library).

GediminasZukas avatar Mar 24 '20 08:03 GediminasZukas

okey

hoanghiephui avatar Mar 27 '20 03:03 hoanghiephui

okey

hoanghiephui avatar Mar 27 '20 08:03 hoanghiephui