-
Notifications
You must be signed in to change notification settings - Fork 224
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
Changes from all commits
8159262
03a6e42
a017f5e
d7ff7c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package au.com.shiftyjelly.pocketcasts.di | ||
|
||
import android.content.Context | ||
import android.net.ConnectivityManager | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadCallFactory | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadOkHttpClient | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadRequestBuilder | ||
import dagger.Module | ||
import dagger.Provides | ||
import dagger.hilt.InstallIn | ||
import dagger.hilt.android.qualifiers.ApplicationContext | ||
import dagger.hilt.components.SingletonComponent | ||
import okhttp3.Call | ||
import okhttp3.OkHttpClient | ||
import okhttp3.Request | ||
import javax.inject.Singleton | ||
|
||
@Module | ||
@InstallIn(SingletonComponent::class) | ||
object AppModule { | ||
|
||
@Provides | ||
fun connectivityManager(@ApplicationContext application: Context): ConnectivityManager = | ||
application.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager | ||
|
||
@Provides | ||
@Singleton | ||
@DownloadCallFactory | ||
fun downloadCallFactory( | ||
@DownloadOkHttpClient phoneCallFactory: OkHttpClient, | ||
): Call.Factory = phoneCallFactory | ||
|
||
@Provides | ||
@DownloadRequestBuilder | ||
fun downloadRequestBuilder(): Request.Builder = Request.Builder() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package au.com.shiftyjelly.pocketcasts.di | ||
|
||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadCallFactory | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadOkHttpClient | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadRequestBuilder | ||
import dagger.Module | ||
import dagger.Provides | ||
import dagger.hilt.InstallIn | ||
import dagger.hilt.components.SingletonComponent | ||
import okhttp3.Call | ||
import okhttp3.OkHttpClient | ||
import okhttp3.Request | ||
import javax.inject.Singleton | ||
|
||
@Module | ||
@InstallIn(SingletonComponent::class) | ||
object AutomotiveAppModule { | ||
|
||
@Provides | ||
@Singleton | ||
@DownloadCallFactory | ||
fun downloadCallFactory( | ||
@DownloadOkHttpClient phoneCallFactory: OkHttpClient, | ||
): Call.Factory = phoneCallFactory | ||
|
||
@Provides | ||
@DownloadRequestBuilder | ||
fun downloadRequestBuilder(): Request.Builder = Request.Builder() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package au.com.shiftyjelly.pocketcasts.repositories.di | ||
|
||
import javax.inject.Qualifier | ||
|
||
/** | ||
* Annotation for providing the Call.Factory used for downloads. The provides method | ||
* for this annotation must be provided in the relevant application module because | ||
* the Call.Factory is different for Wear. | ||
*/ | ||
@Qualifier | ||
@Retention(AnnotationRetention.BINARY) | ||
annotation class DownloadCallFactory | ||
|
||
/** | ||
* Annotation for providing the OkhttpClient for download calls. | ||
*/ | ||
@Qualifier | ||
@Retention(AnnotationRetention.BINARY) | ||
annotation class DownloadOkHttpClient | ||
|
||
/** | ||
* Annotation for providing the Request.Builder used for download calls. The provides method | ||
* for this annotation must be provided in the relevant application module because | ||
* the Request.Builder is different for Wear. | ||
*/ | ||
@Qualifier | ||
@Retention(AnnotationRetention.BINARY) | ||
annotation class DownloadRequestBuilder |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,9 @@ import dagger.Module | |
import dagger.Provides | ||
import dagger.hilt.InstallIn | ||
import dagger.hilt.components.SingletonComponent | ||
import okhttp3.Dispatcher | ||
import okhttp3.OkHttpClient | ||
import java.util.concurrent.TimeUnit | ||
import javax.inject.Singleton | ||
|
||
@Module | ||
|
@@ -15,4 +18,19 @@ class RepositoryProviderModule { | |
@Provides | ||
@Singleton | ||
fun provideTokenHandler(syncAccountManager: SyncAccountManager): TokenHandler = syncAccountManager | ||
|
||
@Provides | ||
@Singleton | ||
@DownloadOkHttpClient | ||
fun downloadOkHttpClient(): OkHttpClient { | ||
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
val dispatcher = Dispatcher().apply { | ||
maxRequestsPerHost = 5 | ||
} | ||
return OkHttpClient.Builder() | ||
.dispatcher(dispatcher) | ||
.connectTimeout(30, TimeUnit.SECONDS) | ||
.writeTimeout(30, TimeUnit.SECONDS) | ||
.readTimeout(30, TimeUnit.SECONDS) | ||
.build() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ import au.com.shiftyjelly.pocketcasts.models.entity.Playable | |
import au.com.shiftyjelly.pocketcasts.models.entity.UserEpisode | ||
import au.com.shiftyjelly.pocketcasts.models.type.EpisodeStatusEnum | ||
import au.com.shiftyjelly.pocketcasts.preferences.Settings.NotificationId | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadCallFactory | ||
import au.com.shiftyjelly.pocketcasts.repositories.di.DownloadRequestBuilder | ||
import au.com.shiftyjelly.pocketcasts.repositories.download.DownloadManager | ||
import au.com.shiftyjelly.pocketcasts.repositories.download.DownloadProgressUpdate | ||
import au.com.shiftyjelly.pocketcasts.repositories.download.ResponseValidationResult | ||
|
@@ -32,10 +34,9 @@ import io.reactivex.ObservableEmitter | |
import kotlinx.coroutines.runBlocking | ||
import kotlinx.coroutines.rx2.await | ||
import okhttp3.Call | ||
import okhttp3.Dispatcher | ||
import okhttp3.Callback | ||
import okhttp3.HttpUrl.Companion.toHttpUrlOrNull | ||
import okhttp3.MediaType | ||
import okhttp3.OkHttpClient | ||
import okhttp3.Request | ||
import okhttp3.Response | ||
import timber.log.Timber | ||
|
@@ -52,8 +53,11 @@ import java.io.RandomAccessFile | |
import java.net.SocketException | ||
import java.net.SocketTimeoutException | ||
import java.net.UnknownHostException | ||
import java.util.concurrent.TimeUnit | ||
import javax.inject.Provider | ||
import javax.net.ssl.SSLHandshakeException | ||
import kotlin.coroutines.resume | ||
import kotlin.coroutines.resumeWithException | ||
import kotlin.coroutines.suspendCoroutine | ||
import au.com.shiftyjelly.pocketcasts.localization.R as LR | ||
|
||
private class UnderscoreInHostName : Exception("Download URL is invalid, as it contains an underscore in the hostname. Please contact the podcast author to resolve this.") | ||
|
@@ -64,7 +68,9 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
@Assisted params: WorkerParameters, | ||
var downloadManager: DownloadManager, | ||
var episodeManager: EpisodeManager, | ||
var userEpisodeManager: UserEpisodeManager | ||
var userEpisodeManager: UserEpisodeManager, | ||
@DownloadCallFactory private val callFactory: Call.Factory, | ||
@DownloadRequestBuilder private val requestBuilderProvider: Provider<Request.Builder> | ||
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
) : Worker(context, params) { | ||
|
||
companion object { | ||
|
@@ -107,18 +113,6 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
private var bytesDownloadedSoFar: Long = 0 | ||
private var bytesRemaining: Long = 0 | ||
|
||
private val okHttpClient by lazy { | ||
val dispatcher = Dispatcher() | ||
dispatcher.maxRequestsPerHost = 5 | ||
val builder = OkHttpClient.Builder() | ||
.dispatcher(dispatcher) | ||
.connectTimeout(30, TimeUnit.SECONDS) | ||
.writeTimeout(30, TimeUnit.SECONDS) | ||
.readTimeout(30, TimeUnit.SECONDS) | ||
|
||
builder.build() | ||
} | ||
|
||
override fun doWork(): Result { | ||
if (isStopped) { | ||
LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Cancelling execution of $episodeUUID download because we are already stopped") | ||
|
@@ -228,7 +222,7 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
emitter.onComplete() | ||
} | ||
} else { | ||
downloadFile(tempDownloadPath!!, okHttpClient, 1, emitter) | ||
downloadFile(tempDownloadPath!!, callFactory, 1, emitter) | ||
if (!emitter.isDisposed) { | ||
emitter.onComplete() | ||
} | ||
|
@@ -243,7 +237,7 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
} | ||
} | ||
|
||
private fun downloadFile(tempDownloadPath: String, httpClient: OkHttpClient, tryCount: Int, emitter: ObservableEmitter<DownloadProgressUpdate>) { | ||
private fun downloadFile(tempDownloadPath: String, httpClient: Call.Factory, tryCount: Int, emitter: ObservableEmitter<DownloadProgressUpdate>) { | ||
if (emitter.isDisposed || isStopped || pathToSaveTo == null) { | ||
return | ||
} | ||
|
@@ -269,7 +263,7 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
throw UnderscoreInHostName() | ||
} | ||
|
||
val requestBuilder = Request.Builder() | ||
val requestBuilder = requestBuilderProvider.get() | ||
.url(downloadUrl) | ||
.header("User-Agent", "Pocket Casts") | ||
|
||
|
@@ -294,7 +288,7 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
.header("Accept-Encoding", "identity") | ||
.build() | ||
call = httpClient.newCall(request) | ||
response = call.execute() | ||
response = call.blockingEnqueue() | ||
|
||
if (response.code != HTTP_RESUME_SUPPORTED) { | ||
LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Resuming ${episode.title} not supported, restarting download.") | ||
|
@@ -313,7 +307,7 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
if (response == null) { | ||
val request = requestBuilder.build() | ||
call = httpClient.newCall(request) | ||
response = call.execute() | ||
response = call.blockingEnqueue() | ||
} | ||
|
||
if (emitter.isDisposed || isStopped) { | ||
|
@@ -646,3 +640,22 @@ class DownloadEpisodeTask @AssistedInject constructor( | |
|
||
class DownloadFailed(val exception: Exception?, message: String, val retry: Boolean) : Exception(message) | ||
} | ||
|
||
/** | ||
* Have to use enqueue for high bandwidth requests on the watch app | ||
* See https://github.com/google/horologist/blob/7bd044a4766e379f85ee3f5a01272853eec3155d/network-awareness/src/main/java/com/google/android/horologist/networks/okhttp/impl/HighBandwidthCall.kt#L93-L92 | ||
*/ | ||
private fun Call.blockingEnqueue(): Response = | ||
runBlocking { | ||
suspendCoroutine { cont -> | ||
[email protected](object : Callback { | ||
override fun onFailure(call: Call, e: IOException) { | ||
cont.resumeWithException(e) | ||
} | ||
|
||
override fun onResponse(call: Call, response: Response) { | ||
cont.resume(response) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
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).