tbruyelle / RxPermissions

Android runtime permissions powered by RxJava2
Apache License 2.0
10.49k stars 1.31k forks source link

Fix permission request subjects being not complete #319

Open samuolis opened 4 years ago

samuolis commented 4 years ago
mrArtCore commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

cancel request and throw an error are two big differences.

Throwable is used pretty often as a canceling event. For example [Object.await()](https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait()) 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 commented 4 years ago

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).

hoanghiephui commented 4 years ago

okey

hoanghiephui commented 4 years ago

okey