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

Use a lifecycle aware async task instead of a loader to open collection #4972

Merged
merged 2 commits into from
Sep 17, 2018

Conversation

timrae
Copy link
Member

@timrae timrae commented Sep 16, 2018

Fixes

Fixes #4938 and is related to #4937 and #4939
Supersedes #4950

Approach

The new lifecycle support library makes it pretty easy to build lifecycle aware components, and Loaders have now been deprecated in favor of that library. So instead of using an AsyncTaskLoader as the base class, I've used an AsyncTask and added some simple logic to ensure that onCollectionLoaded() is not called when the Activity is stopped.

I was originally considering to use the new ViewModels plus LiveData approach, but quickly realized that's overkill for the simple task we're trying to achieve here.

In the future I think we should probably refactor the guts of this into a more general LifecycleAwareAsyncTask and use that more liberally in our code base.

How Has This Been Tested?

I did some basic sanity checks on the simulator with a phone and tablet profile to make sure that all Activities are starting up correctly and that no crashes occur when the Activity is paused or stopped while the collection is loading.

Learning (optional, can help others)

https://developer.android.com/topic/libraries/architecture/lifecycle

This was pretty good as well:
https://developer.android.com/topic/libraries/architecture/guide.html

timrae and others added 2 commits September 16, 2018 22:26
Loaders have now been deprecated in favor of the lifecycle support library
@ankidroid ankidroid deleted a comment Sep 16, 2018
@ankidroid ankidroid deleted a comment Sep 16, 2018
@ankidroid ankidroid deleted a comment Sep 16, 2018
@mikehardy
Copy link
Member

Looks pretty good to me and my test run of your branch also seemed to work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants