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

Put in defense for issue 278 #290

Merged
merged 1 commit into from
Sep 23, 2019
Merged

Put in defense for issue 278 #290

merged 1 commit into from
Sep 23, 2019

Conversation

samtstern
Copy link
Contributor

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.

Change-Id: If0e5dbc415baa13bba95935e2516a961e42c69d9
@samtstern
Copy link
Contributor Author

@vmadalin what do you think?

@vmadalin
Copy link

@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
Copy link
Contributor Author

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

@samtstern samtstern merged commit 43e63a5 into master Sep 23, 2019
@samtstern samtstern added this to the 3.0.1 milestone Sep 23, 2019
vmadalin added a commit to vmadalin/easypermissions-ktx that referenced this pull request May 4, 2021
* Put in defense for issue 278 (googlesamples#290)

* Updating project to support Android Studio 4.0 (googlesamples#307)

* Updating module 'app' to Java 8 syntax (googlesamples#308)

* Changed getContext() to requireContext(), to ensure it is not null. (googlesamples#309)

* Add link to easypermissions-ktx

* Move from JCenter to Maven Central (googlesamples#323)

* Add the @IntRange annotation to requestCode to prevent invalid input (googlesamples#325)

Co-authored-by: gaopengfei <[email protected]>

* Move to GitHub Actions (googlesamples#326)

* Start migrating project to maven central

* Fix build

* Update with correct maven central links

Co-authored-by: Sam Stern <[email protected]>
Co-authored-by: Fung <[email protected]>
Co-authored-by: Fung <[email protected]>
Co-authored-by: coder1024 <[email protected]>
Co-authored-by: gaopengfei <[email protected]>
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.

2 participants