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

Prefer faster networks for downloads on watch #881

Merged
merged 4 commits into from
Apr 18, 2023

Conversation

mchowning
Copy link
Contributor

Description

This PR uses the horologist network-awareness library to prefer faster networks when we download media. Without this, the watch defaults to using slower network connections (i.e., Bluetooth). More info here.

There was some trickiness integrating this because this wraps the Call.Factory which we create in the repository module. I initially tried using the library directly there, but the problem is that the network-awareness library (1) uses watch features (or at least claims it does in its manifest file) and (2) has min SDK of 26. Since the phone app uses our repository module, both of these requirements created issues.

To work around this, I have created an annotation in the repository module to provide the relevant Call.Factory, but the
@Provides methods for those annotations are in the application modules (phone, automotive, and wear). I'm keeping the actual construction of the non-wear dependencies in the repository module—the application module is really only responsible for identifying which dependency to inject.

I had to do something similar with the Request.Builder because this library creates an extension method (.requestType(...) that needs to be set with an object from the library.

I'm only doing this for downloads currently, but I imagine I'll be doing something similar for some of our other okhttp requests in the future, so let me know any issues you see with this pattern.

This is a bit confusing from the DI perspective, but it completely avoids having any horologist dependency in our repository module, which feels like a nice win. With that said, let me know if you think this is too messy and not worth it.

Testing Instructions

  1. Install the app on a physical watch that is paired to a phone by Bluetooth and connected to a wifi network
  2. Go to the episode details screen and download an episode
  3. Check the logs for the watch, you should see a message along the lines of HTTPS request media-download Wifi reflecting that Wifi was used for the download
  4. Alter the code to prefer Bluetooth connections for downloads like this.
  5. Reinstall the app with the altered code
  6. Download an episode again
  7. Check the logs for the watch, you should see a message along the lines of HTTPS request media-download BT reflecting that Bluetooth was used for the download

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@mchowning mchowning added the [Area] Wear OS watch app label Apr 14, 2023
Comment on lines +24 to +25
@DownloadOkHttpClient
fun downloadOkHttpClient(): OkHttpClient {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am providing this in the repository module because the wear app needs the OkHttpClient to create a NetworkSelectingCallFactory which serves as the Call.Factory for downloads (@DownloadCallFactory) in the watch app. The phone and automotive apps just return this directly with their @DownloadCallFactory providers.

@DownloadRequestBuilder
fun downloadRequestBuilder(): Request.Builder =
Request.Builder()
.requestType(RequestType.MediaRequest.DownloadRequest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This RequestType comes from the horologist library and setting it on the Request.Builder for wear requests is the only reason I need to inject it separately for the wear vs phone/automotive apps.

Comment on lines +22 to +23
check(requestType != RequestType.UnknownRequest) {
"Unknown request type. Failing on debug builds."
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like a good way to make sure we don't use these networking rules but forget to set a request type on the request.


@Module
@InstallIn(SingletonComponent::class)
object AutomotiveAppModule {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a duplicate fo the AppModule. We could create a new module shared only by the phone and wear apps to avoid this duplication, but that felt like it would be overcomplicating things. Maybe later something like that may make sense (I kind of doubt it though).

Comment on lines +72 to +73
@DownloadCallFactory private val callFactory: Call.Factory,
@DownloadRequestBuilder private val requestBuilderProvider: Provider<Request.Builder>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on which app is running (phone/automotive/wear), these dependencies will be provided by the @DownloadCallFactory and @DownloadRequestBuilder provide methods from the relevant app (i.e., from AppModule, AutomotiveAppModule or WearNetworkModule).

Request.Builder()
.requestType(RequestType.MediaRequest.DownloadRequest)

// FIXME update the provide methods below this point
Copy link
Contributor Author

@mchowning mchowning Apr 14, 2023

Choose a reason for hiding this comment

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

I put this cleanup in a follow-up PR.

@mchowning mchowning force-pushed the update/wear-network-call-factory branch from cbfb0df to d7ff7c6 Compare April 14, 2023 01:02
@mchowning mchowning marked this pull request as ready for review April 14, 2023 01:02
@mchowning mchowning requested a review from a team as a code owner April 14, 2023 01:02
@mchowning mchowning mentioned this pull request Apr 14, 2023
4 tasks
@ashiagr
Copy link
Contributor

ashiagr commented Apr 14, 2023

Hey @mchowning! 👋

Hats off for putting out this PR, it must have been tricky to figure out dependency injection like this and also support it with tests. 🕵️

While everything that you described makes sense, I still see HTTPS request media-download Wifi in logs even after applying the patch and making sure that watch is paired to the phone using Bluetooth and connected to a WiFi network. I might be missing something in my setup. Maybe we can sync later today.

@mchowning
Copy link
Contributor Author

I still see HTTPS request media-download Wifi in logs even after applying the patch and making sure that watch is paired to the phone using Bluetooth and connected to a WiFi network.

Interesting. I assume you're using a physical watch device paired to a physical phone in your testing. I just retested this, and I'm still seeing the expected behavior on my watch. 🤔

Perhaps you could try dropping a breakpoint or log message on the changed line and see (1) that the code is actually getting to that point and, if it is (2) what networks are actually available (i.e., maybe the watch is reporting that Bluetooth isn't available for some reason).

@ashiagr
Copy link
Contributor

ashiagr commented Apr 18, 2023

I assume you're using a physical watch device paired to a physical phone in your testing.

Yes, I was using a physical watch, Bluetooth paired with a physical mobile device, and connected to wifi.

Perhaps you could try dropping a breakpoint or log message on the changed line and see (1) that the code is actually getting to that point and, if it is (2) what networks are actually available (i.e., maybe the watch is reporting that Bluetooth isn't available for some reason).

Also added a breakpoint, the code actually hit that point

I'm not sure what was wrong, I factory reset my watch and tried it all over again, 😅 this time it worked 🥳

Screenshot 2023-04-18 at 12 44 47 PM

Great job! I'm approving the PR now.

@ashiagr ashiagr merged commit 1950fed into main Apr 18, 2023
@ashiagr ashiagr deleted the update/wear-network-call-factory branch April 18, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants