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

Stop using AsyncTask #218

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Stop using AsyncTask #218

merged 3 commits into from
Aug 28, 2023

Conversation

ddohler
Copy link
Contributor

@ddohler ddohler commented Aug 18, 2023

Overview

AsyncTask has been deprecated, so this stops using it.

Demo

Here we are running on a Nexus 4 (2012) on Android SDK 22
Screenshot from 2023-08-18 15-01-19

And on a Pixel 5 (2020) running Android SDK 33
Screenshot from 2023-08-18 15-05-00

Notes

  • I had read some indications that Fix RecyclerView errors #215 might be being caused by trying to set up the views inside of an async function and thought we might be able to address it as part of this task. However, switching away from AsyncTask didn't solve the issue, and I didn't really see any indication that it impacted the PlacesListActivity at all. Since that part of this task went significantly quicker than estimated, I spent some extra time trying to see if I could separately diagnose and fix the RecyclerView errors, but I'm not a good enough Android debugger yet to easily track down what is going wrong. It still doesn't seem to cause any user-visible issues, so it didn't seem to be worth digging in too deeply and I left it as-is.
  • I discovered that running automated tests seems to have broken at some point in the past, but it turns out we only have a single example test that runs anyway, so I don't think it's necessary to fix it unless we plan to add substantive testing at some point.
  • Assigned @rachelekm because you should be at least partly spun up. There is no time pressure on this project any longer and I'll be out for a couple days right after you get back, so take your time!

Testing Instructions

  • As with the past several PRs, fire up the app and click around. Confirm that everything continues to work the same way that it always has.

Closes #213

@@ -83,4 +90,12 @@ EventDao provideEventDao(GpgDatabase db) {
AttractionFlagDao provideAttractionFlagDao(GpgDatabase db) {
return db.attractionFlagDao();
}

@Singleton
Copy link
Contributor Author

@ddohler ddohler Aug 18, 2023

Choose a reason for hiding this comment

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

So what's going on here is that we use Dagger for dependency injection (sidenote: a dependency injection library called Dagger strikes me as...kind of grim). We need to create just a single instance of both ExecutorService and Handler at the top level of the app so that they're available to any parts of the app that need them, but the place where we need them, DestinationRepository, is nested many, many levels down from the top. If we did this by hand, we'd have to pass an instance of ExecutorService down through every nested constructor call in the application to get it down to the level where it's actually used.

What Dagger does is the @Singleton decorator tells it to know about this resource and to make sure that only one of it ever exists, and the @Provides decorator tells it that if some other part of the app asks for one of these things, to get it from here.

Then, in places where we need one or more parameters that Dagger knows about, we can add the @Inject decorator and Dagger will inspect the parameters to the constructor, compare them to the different resources that it knows about, and, assuming it finds a match, it'll auto-generate all the code to pass that resource all the way down to where it gets used so that we don't write it by hand.

You can see this at work in DestinationRepository.java. I don't 100% understand what heuristics it uses to figure out which resource to attach to which constructor parameters (does it use the type signature alone or does it also parse the name somehow?) but it seems to be working, so that's good enough for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really helpful context, thank you!

Copy link
Collaborator

@rachelekm rachelekm left a comment

Choose a reason for hiding this comment

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

Great job! My java knowledge is very limited (read: none) but your comments provided helpful context and these changes make sense. I was able to successfully get the app running and click around and did not see any regressions!
On a Pixel 3 running Android SDK 33:
Screenshot 2023-08-23 at 1 46 52 PM

Screenshot 2023-08-23 at 1 46 35 PM

@@ -83,4 +90,12 @@ EventDao provideEventDao(GpgDatabase db) {
AttractionFlagDao provideAttractionFlagDao(GpgDatabase db) {
return db.attractionFlagDao();
}

@Singleton
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really helpful context, thank you!

@ddohler
Copy link
Contributor Author

ddohler commented Aug 28, 2023

Thanks for reviewing, @rachelekm ! I'm going to go ahead and merge now.

@ddohler ddohler merged commit fa5f45d into develop Aug 28, 2023
@ddohler ddohler deleted the feature/dpd/async-task-deprecation branch August 28, 2023 14:39
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.

Migrate away from deprecated AsyncTask
2 participants