diff --git a/CHANGELOG.md b/CHANGELOG.md index dfa3aa734a6..4c1bd0a7e8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,10 +3,12 @@ * New Feature: * Suggest episodes to play in Automotive - ([#1362](https://github.com/Automattic/pocket-casts-android/pull/1362)). + ([#1362](https://github.com/Automattic/pocket-casts-android/pull/1362)) * Bug Fixes: * Avoid memory leak when opening Up Next queue ([#1397](https://github.com/Automattic/pocket-casts-android/pull/1397)) + * Improved the downloading of episode show notes + ([#1390](https://github.com/Automattic/pocket-casts-android/pull/1390)) 7.47 ----- diff --git a/modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt b/modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt index 4fc07f56c93..be47aa6c73b 100644 --- a/modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt +++ b/modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch +import retrofit2.HttpException import timber.log.Timber import java.io.IOException import javax.inject.Inject @@ -62,7 +63,13 @@ class PodcastRatingsViewModel try { ratingsManager.refreshPodcastRatings(uuid) } catch (e: Exception) { - Timber.e(e, "Failed to refresh podcast ratings") + val message = "Failed to refresh podcast ratings" + // don't report missing rating or network errors to Sentry + if (e is HttpException || e is IOException) { + Timber.i(e, message) + } else { + Timber.e(e, message) + } } } } diff --git a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt index 340706e6034..70460f7cd49 100644 --- a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt +++ b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt @@ -338,24 +338,13 @@ class DownloadManagerImpl @Inject constructor( .addTag(episode.uuid) .build() - val tasks = mutableListOf(downloadTask) - if (episode is PodcastEpisode) { - val cacheShowNotesData = Data.Builder() - .putString(UpdateShowNotesTask.INPUT_PODCAST_UUID, episode.podcastUuid) - .putString(UpdateShowNotesTask.INPUT_EPISODE_UUID, episode.uuid) - .build() - val cacheShowNotesTask = OneTimeWorkRequestBuilder() - .setInputData(cacheShowNotesData) - .addTag(episode.uuid) - .build() - tasks.add(cacheShowNotesTask) - } + UpdateShowNotesTask.enqueue(episode, constraints, context) episodeManager.updateDownloadTaskId(episode, downloadTask.id.toString()) WorkManager .getInstance(context) .beginWith(updateTask) - .then(tasks) + .then(downloadTask) .enqueue() } catch (storageException: StorageException) { launch(downloadsCoroutineContext) { diff --git a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt index eed0e0dda79..32cbdb650f6 100644 --- a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt +++ b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt @@ -5,6 +5,7 @@ import android.media.MediaExtractor import android.media.MediaFormat import android.system.ErrnoException import android.system.OsConstants +import android.util.Log import androidx.hilt.work.HiltWorker import androidx.work.Data import androidx.work.ForegroundInfo @@ -125,13 +126,13 @@ class DownloadEpisodeTask @AssistedInject constructor( this.episode = runBlocking { episodeManager.findEpisodeByUuid(episodeUUID!!) } ?: return Result.failure() if (this.episode.downloadTaskId != id.toString()) { - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.") + LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling. downloadTaskId: ${this.episode.downloadTaskId} id: $id.") return Result.failure() } if (this.episode.isArchived) { // In case the episode was archived again but the cancellation event hasn't come through yet - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Episode ${episode.title} is archived in download task. Cancelling.") + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Episode ${episode.title} is archived in download task. Cancelling.") return Result.success(Data.Builder().putBoolean(OUTPUT_CANCELLED, true).build()) } @@ -193,7 +194,7 @@ class DownloadEpisodeTask @AssistedInject constructor( runBlocking { val requirements = downloadManager.getRequirementsAndSetStatusAsync(episode) val message = e?.toString() ?: "No exception" - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Download stopped, will retry with $requirements ${episode.title} ${episode.uuid} - $message") + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Download stopped, will retry with $requirements ${episode.title} ${episode.uuid} - $message") } } @@ -211,7 +212,7 @@ class DownloadEpisodeTask @AssistedInject constructor( val message = if (downloadMessage.isNullOrBlank()) "Download Failed" else downloadMessage episodeManager.updateDownloadErrorDetails(episode, message) - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, e, "Download failed ${episode.title} ${episode.uuid} - $message") + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, e, "Download failed ${episode.title} ${episode.uuid} - $message") return outputData } @@ -510,7 +511,9 @@ class DownloadEpisodeTask @AssistedInject constructor( call?.cancel() } if (exception != null) { - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, exception, "Download failed %s", this.episode.downloadUrl ?: "") + // don't report issues such as timeouts to Sentry + val logPriority = if (exception is IOException) Log.INFO else Log.ERROR + LogBuffer.addLog(logPriority, LogBuffer.TAG_BACKGROUND_TASKS, exception, "Download failed %s", this.episode.downloadUrl ?: "") } if (!emitter.isDisposed) { @@ -575,13 +578,13 @@ class DownloadEpisodeTask @AssistedInject constructor( val message = if (statusCode == 404) ERROR_FILE_NOT_FOUND else String.format( ERROR_DOWNLOAD_MESSAGE, if (responseReason.isBlank()) "" else "(error $statusCode $responseReason)" ) - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid response returned for episode download. ${response.code} $responseReason ${response.request.url}") + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid response returned for episode download. ${response.code} $responseReason ${response.request.url}") return ResponseValidationResult(isValid = false, errorMessage = message) } // check the content type is valid response.header("Content-Type")?.let { contentType -> if (INVALID_CONTENT_TYPES.any { it == contentType }) { - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid content type returned for episode download. $contentType ${response.request.url}") + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid content type returned for episode download. $contentType ${response.request.url}") return ResponseValidationResult(isValid = false, errorMessage = ERROR_FAILED_EPISODE) } } diff --git a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt index 442c3885e2c..953549891bf 100644 --- a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt +++ b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt @@ -1,11 +1,19 @@ package au.com.shiftyjelly.pocketcasts.repositories.download.task import android.content.Context +import android.os.SystemClock +import android.util.Log import androidx.hilt.work.HiltWorker +import androidx.work.Constraints import androidx.work.CoroutineWorker +import androidx.work.Data +import androidx.work.ExistingWorkPolicy +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkManager import androidx.work.WorkerParameters +import au.com.shiftyjelly.pocketcasts.models.entity.BaseEpisode +import au.com.shiftyjelly.pocketcasts.models.entity.PodcastEpisode import au.com.shiftyjelly.pocketcasts.servers.ServerShowNotesManager -import au.com.shiftyjelly.pocketcasts.utils.SentryHelper import au.com.shiftyjelly.pocketcasts.utils.log.LogBuffer import dagger.assisted.Assisted import dagger.assisted.AssistedInject @@ -21,24 +29,46 @@ class UpdateShowNotesTask @AssistedInject constructor( private val showNotesManager: ServerShowNotesManager ) : CoroutineWorker(context, params) { companion object { + private const val TASK_NAME = "UpdateShowNotesTask" const val INPUT_PODCAST_UUID = "podcast_uuid" - const val INPUT_EPISODE_UUID = "episode_uuid" + + fun enqueue(episode: BaseEpisode, constraints: Constraints = Constraints.NONE, context: Context) { + if (episode !is PodcastEpisode) { + return + } + + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "$TASK_NAME - enqueued ${episode.uuid}") + val cacheShowNotesData = Data.Builder() + .putString(INPUT_PODCAST_UUID, episode.podcastUuid) + .build() + val workRequest = OneTimeWorkRequestBuilder() + .setInputData(cacheShowNotesData) + .addTag(episode.uuid) + .setConstraints(constraints) + .build() + WorkManager.getInstance(context).beginUniqueWork(TASK_NAME, ExistingWorkPolicy.APPEND_OR_REPLACE, workRequest).enqueue() + } } private val podcastUuid = inputData.getString(INPUT_PODCAST_UUID) ?: "" - private val episodeUuid = inputData.getString(INPUT_EPISODE_UUID) ?: "" override suspend fun doWork(): Result { + info("Worker started - podcast: $podcastUuid") + val startTime = SystemClock.elapsedRealtime() return try { - showNotesManager.downloadShowNotes(podcastUuid = podcastUuid, episodeUuid = episodeUuid) + showNotesManager.downloadToCacheShowNotes(podcastUuid = podcastUuid) + info("Worker completed - took ${SystemClock.elapsedRealtime() - startTime} ms") Result.success() } catch (e: Exception) { - LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, e, "Failed to update show notes") - if (e !is HttpException) { - SentryHelper.recordException(e) - } + info("Worker failed - took ${SystemClock.elapsedRealtime() - startTime} ms") + val logPriority = if (e is HttpException) Log.INFO else Log.ERROR + LogBuffer.addLog(logPriority, LogBuffer.TAG_BACKGROUND_TASKS, e, "Failed to update show notes") // Don't keep retrying if the download fails. The user can download the show notes when viewing them. Result.success() } } + + private fun info(message: String) { + LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "$TASK_NAME (Worker ID: $id) - $message") + } } diff --git a/modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt b/modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt index 20a980de148..b0aadb8dac1 100644 --- a/modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt +++ b/modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt @@ -86,4 +86,11 @@ class ServerShowNotesManager @Inject constructor( val response = podcastCacheServerManager.getShowNotes(podcastUuid = podcastUuid) return response.findEpisode(episodeUuid)?.showNotes } + + suspend fun downloadToCacheShowNotes(podcastUuid: String) { + if (podcastUuid.isBlank()) { + return + } + podcastCacheServerManager.getShowNotes(podcastUuid = podcastUuid) + } } diff --git a/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt b/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt index 33a78ff68b3..d3e92f04b98 100644 --- a/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt +++ b/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt @@ -3,6 +3,7 @@ package au.com.shiftyjelly.pocketcasts.utils.log import android.util.Log import au.com.shiftyjelly.pocketcasts.utils.FileUtil import timber.log.Timber +import timber.log.Timber.Forest.tag import java.io.File import java.io.FileWriter import java.io.IOException @@ -110,6 +111,10 @@ object LogBuffer { addLog(Log.INFO, tag, null, message, *args) } + fun i(tag: String, throwable: Throwable, message: String, vararg args: Any) { + addLog(Log.INFO, tag, throwable, message, *args) + } + fun w(tag: String, message: String, vararg args: Any) { addLog(Log.WARN, tag, null, message, *args) } @@ -127,7 +132,7 @@ object LogBuffer { } @Suppress("NAME_SHADOWING") - private fun addLog(priority: Int, tag: String, throwable: Throwable?, message: String?, vararg args: Any) { + fun addLog(priority: Int, tag: String, throwable: Throwable?, message: String?, vararg args: Any) { var message = message if (message != null && message.isEmpty()) { message = null