Screen rotation while showing Rationale dialog invalidates PermissionListener

nsk90 commented 3 years ago

Expected behaviour

PermissionListener passed to withListener() function is triggered after permission request.

Actual behaviour

PermissionListener is invalidated on screen rotation and its callbacks are not triggered.

Steps to reproduce

Rotate screen while showing dialog from onPermissionRationaleShouldBeShown callback.

Version of the library

6.2.2

I suppose that using ViewModel for DexterActivity should fix the problem, but it requires jetpack dependency.

traviszim commented 3 years ago

Expected behaviour

PermissionListener passed to withListener() function is triggered after permission request.

Actual behaviour

PermissionListener is invalidated on screen rotation and its callbacks are not triggered.

Steps to reproduce

Rotate screen while showing dialog from onPermissionRationaleShouldBeShown callback.

Version of the library

6.2.2

I suppose that using ViewModel for DexterActivity should fix the problem, but it requires jetpack dependency.

pedrovgs commented 3 years ago

@nsk90 what do you mean by invalidated? Could you describe the issue from the user point of view?

nsk90 commented 3 years ago

what do you mean by invalidated?

I mean it is replaced with default one, and so it is not triggered.

  void onActivityDestroyed(Activity oldActivity) {
    if (activity == oldActivity) {
      activity = null;
      isRequestingPermission.set(false);
      rationaleAccepted.set(false);
      isShowingNativeDialog.set(false);
      listener = EMPTY_LISTENER; // as I see this line invalidates my listener
    }
  }

Could you describe the issue from the user point of view?

from the user point of view is depends on what my app is doing in onPermissionRationaleShouldBeShown()

here are some cases:

in onPermissionRationaleShouldBeShown() I show AlertDialog with ok button. Screen rotation closes dialog and default (replaced) listener shows permission request.
in onPermissionRationaleShouldBeShown() I show DialogFragment with ok button. DialogFragment supports orientation changes so it is not closed by screen rotation. Permission request is shown above DialogFragment by the same reason as before.
in onPermissionRationaleShouldBeShown() I show AlertDialog with ok button. And in onPermissionGranted() I do some action which user must see (show some notification for example). In this case user will see notification if he clicks ok on rationale dialog and grands permission. But he will not see notification if he will rotate screen while rationale dialog is shown and grand permission. There is not way to fix this with DialogFragment.

I wrote that in my opinion ViewModel for DexterActivity should help, now I can add that there is a simpler option "headless fragment". I hope this will help to support screen rotations (configuration changes) correctly.

pedrovgs commented 3 years ago

I don't know right now what's the best fix for this issue. I need to review it in detail before changing anything. I'm also checking how to reproduce the issue manually and the sample app we provide seems to work properly. If you open the sample app, tap on the audio permission and waits for the dialog to be shown you can rotate the screen and the library works as expected. Maybe I'm missing something...could you try it out? If you can reproduce the error in our sample project it could be great if you create a branch reproducing the error. As a workaround, you could use configure your activity configChanges to avoid restoring the activity when the device screen rotates.

nsk90 commented 3 years ago

Audio button in sample app does not use Rationale dialog. Use CAMERA button.

Steps to reproduce on Sample app.

press CAMERA button
see rationale dialog
rotate screen
accept or reject permission.

Take a look at SampleActivity.showPermissionRationale() method it is called from SamplePermissionListener. There is an AlertDialog which calls token.cancelPermissionRequest(); and token.continuePermissionRequest(); (in 3 places) you can place breakpoints or logs on this calls to see that no one of them is called if you rotate screen while dialog is up. In this condition permission is actually requested (made call to token.continuePermissionRequest();) from BaseMultiplePermissionsListener (default). To see that listener is "invalidated" (replaced with default one) put breakpoints or logs to onPermissionGranted() and onPermissionDenied() methods in SamplePermissionListener. This methods will not be triggered if you rotate screen while rationale dialog is up.

The problem is just hidden by default listener which is calling token.continuePermissionRequest();.

pedrovgs commented 3 years ago

I'm afraid this is not easy to fix at all. When the device is rotated the OS recreates the activity and this means we lose every reference to the original listener. As far as I've reviewing there is no way we can get a reference of any listener pointing at the new activity. I'm all ears in case any of you are able to find a fix for this, right now I can't any possible solution but blocking screen orientation which is not a solution at all.

nsk90 commented 3 years ago

Storing a reference to the listener in a Fragment with setRetainInstance(true) does not help?

pedrovgs commented 3 years ago

That would help if you are using Dexter but that wouldn't fix the issue and it would not be valid for anybody using activities.

nsk90 commented 3 years ago

Looks like I explained not correctly.

I meant, to use such a fragment (with setRetainInstance(true)) inside DexterActivity. Move Dexter.onActivityReady(this); and Dexter.onActivityDestroyed(this); calls to that fragment.

I expect this will extend lifetime of references stored in Dexter class and they will be retained across configuration changes.

pedrovgs commented 3 years ago

@nsk90 I think that's not going to work because the listener would hold a reference to a destroyed activity. Even if we save the listener in a static property the activity of the developer who uses dexter will be recreated and the listener we saved would point at a dead activity :(

deinlandel commented 3 years ago

This is a serious bug. At least make some kind of broadcast so we can check that rights were granted or means to cancel this dialog after rotation.

pedrovgs commented 3 years ago

Indeed! It's a serious bug @deinlandel. Keep in mind, I'm the only developer actively working on this project for years and I have to maintain the other company repositories. https://github.com/Karumi/KotlinSnapshot and https://github.com/Karumi/Shot are just two examples. I'm afraid I only have 12 days per year to work on our open-source projects and two hands. If you think this is a serious bug and it is blocking you I'd really appreciate it if you can have the time to help us and collaborate. If you don't have the time, I guess you can use only portrait rotation whenever you are requesting this permission. There are also other libraries you can use if you need it.

deinlandel commented 3 years ago

@pedrovgs That's okay, life often leaves us without time for our side projects. Although please at least write in library's readme that it's unmantained because you don't have time to maintain it. So new users would know that they should not expect any updates and bug fixes.

Karumi / Dexter