googlesamples / easypermissions

Simplify Android M system permissions
https://firebaseopensource.com/projects/googlesamples/easypermissions/
Apache License 2.0
9.87k stars 1.46k forks source link

Put in defense for issue 278 #290

Closed samtstern closed 5 years ago

samtstern commented 5 years ago

I have no idea how the Intent could return null for that extra, but just in case I think it's better to use the default dialog than to crash.

samtstern commented 5 years ago

@VMadalin what do you think?

vmadalin commented 5 years ago

@samtstern Thank you for add me, and for the changes. I reviewed this issue and is very strange, basically because this happens on this onPermissionDenied invocation on display the AppSettingsDialog by:

            AppSettingsDialog.Builder(this).build().show()

Under the hood this method just start an activity (AppSettingsDialogHolderActivity) what finally show the dialog (AppSettingsDialog). Apparently is impossible to don't obtain correctly the extra intent (AppSettingsDialog.EXTRA_APP_SETTINGS), because it is set only here:

public static Intent createShowDialogIntent(Context context, AppSettingsDialog dialog) {
        Intent intent = new Intent(context, AppSettingsDialogHolderActivity.class);
        intent.putExtra(AppSettingsDialog.EXTRA_APP_SETTINGS, dialog);
        return intent;
    }

Maybe in some strange cases the context is destroyed, after finished the Activity, Fragment just in middle of permission flow, similar to async method like callbacks.

IMHO the changes are nice to avoid the crash, but losing all configured alert like title, message, etc.. displaying a default alert. This is not bad taking in consideration this was added for an unexpected behaviour.

For the next steps, maybe it's a good point to avoid create an activity just for display a dialog. On the kotlin refactor for example I simplified this logic

samtstern commented 5 years ago

@VMadalin I agree I have no idea how the extras could get lost! It shouldn't be possible...