Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensuring the fragment isAdded before to continue with the AsyncTask. #11360

Conversation

develric
Copy link
Contributor

@develric develric commented Feb 21, 2020

This PR is related to some consideration made during #11343 review (see here for more context)

With the following call path in ReaderPostListFragment:

LoadTagsTask -> onPostExecute -> onFilterCriteriasLoaded -> setCurrentFilter(mFilterListener.onRecallSelection())

it can happen that the onRecallSelection (who calls requireActivity) could be called when the fragment is not added so resulting in a crash. This PR tries to mitigate that possibility checking that the fragment is attached to activity in onPostExecute

To test

Was not able to trigger the issue so not fully sure how to test. At least smoke test the reader to check that we did not introduce issues.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@malinajirka malinajirka merged commit a8f972e into issue/ia-reader-feature-flag-removal Feb 21, 2020
@malinajirka malinajirka deleted the issue/11343-pr-avoid-issue-when-fragment-not-attached branch February 21, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants