From 1003535f8127b68b2b45688d8d098842b680da46 Mon Sep 17 00:00:00 2001 From: Kate Glazko Date: Mon, 13 Jul 2020 12:31:44 -0700 Subject: [PATCH] For #7673: Move DownloadProgress and Status to DownloadState --- .../browser/session/engine/EngineObserver.kt | 3 + .../state/state/content/DownloadState.kt | 35 +++ .../downloads/AbstractFetchDownloadService.kt | 153 +++++++------ .../feature/downloads/DownloadNotification.kt | 23 +- .../manager/AndroidDownloadManager.kt | 5 +- .../downloads/manager/DownloadManager.kt | 4 +- .../downloads/manager/FetchDownloadManager.kt | 5 +- .../AbstractFetchDownloadServiceTest.kt | 201 ++++++++++-------- .../downloads/DownloadDialogFragmentTest.kt | 6 +- .../downloads/DownloadNotificationTest.kt | 69 ++++-- .../SimpleDownloadDialogFragmentTest.kt | 2 +- .../manager/AndroidDownloadManagerTest.kt | 15 +- .../manager/FetchDownloadManagerTest.kt | 19 +- docs/changelog.md | 4 + 14 files changed, 321 insertions(+), 223 deletions(-) diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt index 97fea9eb981..b51c479abd5 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt @@ -20,6 +20,7 @@ import mozilla.components.browser.state.action.MediaAction import mozilla.components.browser.state.action.TrackingProtectionAction import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.state.content.FindResultState +import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.HitResult @@ -203,6 +204,8 @@ internal class EngineObserver( fileName, contentType, fileSize, + 0, + INITIATED, userAgent, Environment.DIRECTORY_DOWNLOADS ) diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt index b324963673d..018c16ec726 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt @@ -16,6 +16,8 @@ import kotlin.random.Random * @property fileName A canonical filename for this download. * @property contentType Content type (MIME type) to indicate the media type of the download. * @property contentLength The file size reported by the server. + * @property currentBytesCopied The number of current bytes copied. + * @property status The current status of the download. * @property userAgent The user agent to be used for the download. * @property destinationDirectory The matching destination directory for this type of download. * @property filePath The file path the file was saved at. @@ -32,6 +34,8 @@ data class DownloadState( val fileName: String? = null, val contentType: String? = null, val contentLength: Long? = null, + val currentBytesCopied: Long = 0, + val status: Status = Status.INITIATED, val userAgent: String? = null, val destinationDirectory: String = Environment.DIRECTORY_DOWNLOADS, val referrerUrl: String? = null, @@ -41,4 +45,35 @@ data class DownloadState( ) : Parcelable { val filePath: String get() = Environment.getExternalStoragePublicDirectory(destinationDirectory).path + "/" + fileName + + /** + * Status that represents every state that a download can be in. + */ + enum class Status { + /** + * Indicates that the download is in the first state after creation but not yet [DOWNLOADING]. + */ + INITIATED, + /** + * Indicates that an [INITIATED] download is now actively being downloaded. + */ + DOWNLOADING, + /** + * Indicates that the download that has been [DOWNLOADING] has been paused. + */ + PAUSED, + /** + * Indicates that the download that has been [DOWNLOADING] has been cancelled. + */ + CANCELLED, + /** + * Indicates that the download that has been [DOWNLOADING] has moved to failed because + * something unexpected has happened. + */ + FAILED, + /** + * Indicates that the [DOWNLOADING] download has been completed. + */ + COMPLETED + } } diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt index d3852eb1d8b..c871d9e64e9 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt @@ -26,7 +26,6 @@ import android.os.ParcelFileDescriptor import android.provider.MediaStore import android.widget.Toast import android.webkit.MimeTypeMap -import androidx.annotation.GuardedBy import androidx.annotation.VisibleForTesting import androidx.core.app.NotificationManagerCompat import androidx.core.content.FileProvider @@ -41,18 +40,20 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.content.DownloadState.Status import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.state.content.DownloadState.Status.DOWNLOADING +import mozilla.components.browser.state.state.content.DownloadState.Status.CANCELLED +import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED +import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED +import mozilla.components.browser.state.state.content.DownloadState.Status.PAUSED +import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.Headers.Names.CONTENT_RANGE import mozilla.components.concept.fetch.Headers.Names.RANGE import mozilla.components.concept.fetch.MutableHeaders import mozilla.components.concept.fetch.Request -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.CANCELLED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.COMPLETED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.ACTIVE -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.FAILED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED import mozilla.components.feature.downloads.DownloadNotification.NOTIFICATION_DOWNLOAD_GROUP_ID import mozilla.components.feature.downloads.ext.addCompletedDownload import mozilla.components.feature.downloads.ext.isScheme @@ -87,10 +88,13 @@ abstract class AbstractFetchDownloadService : Service() { private val notificationUpdateScope = MainScope() protected abstract val httpClient: Client + @VisibleForTesting internal val broadcastManager by lazy { LocalBroadcastManager.getInstance(this) } + @VisibleForTesting internal val context: Context get() = this + @VisibleForTesting internal var compatForegroundNotificationId: Int = COMPAT_DEFAULT_FOREGROUND_ID private val logger = Logger("AbstractFetchDownloadService") @@ -102,8 +106,6 @@ abstract class AbstractFetchDownloadService : Service() { internal data class DownloadJobState( var job: Job? = null, @Volatile var state: DownloadState, - var currentBytesCopied: Long = 0, - @GuardedBy("context") var status: DownloadJobStatus, var foregroundServiceId: Int = 0, var downloadDeleted: Boolean = false, var notifiedStopped: Boolean = false, @@ -130,30 +132,22 @@ abstract class AbstractFetchDownloadService : Service() { } } - internal fun setDownloadJobStatus(downloadJobState: DownloadJobState, status: DownloadJobStatus) { + internal fun setDownloadJobStatus(downloadJobState: DownloadJobState, status: Status) { synchronized(context) { - if (status == DownloadJobStatus.ACTIVE) { downloadJobState.notifiedStopped = false } - downloadJobState.status = status + if (status == DOWNLOADING) { + downloadJobState.notifiedStopped = false + } + + updateDownloadState(downloadJobState.state.copy(status = status)) } } - internal fun getDownloadJobStatus(downloadJobState: DownloadJobState): DownloadJobStatus { + internal fun getDownloadJobStatus(downloadJobState: DownloadJobState): Status { synchronized(context) { - return downloadJobState.status + return downloadJobState.state.status } } - /** - * Status of an ongoing download - */ - enum class DownloadJobStatus { - ACTIVE, - PAUSED, - CANCELLED, - FAILED, - COMPLETED - } - internal val broadcastReceiver by lazy { object : BroadcastReceiver() { @Suppress("LongMethod") @@ -171,7 +165,7 @@ abstract class AbstractFetchDownloadService : Service() { } ACTION_RESUME -> { - setDownloadJobStatus(currentDownloadJobState, ACTIVE) + setDownloadJobStatus(currentDownloadJobState, DOWNLOADING) currentDownloadJobState.job = CoroutineScope(IO).launch { startDownloadJob(currentDownloadJobState) @@ -200,7 +194,7 @@ abstract class AbstractFetchDownloadService : Service() { ACTION_TRY_AGAIN -> { removeNotification(context, currentDownloadJobState) currentDownloadJobState.lastNotificationUpdate = System.currentTimeMillis() - setDownloadJobStatus(currentDownloadJobState, ACTIVE) + setDownloadJobStatus(currentDownloadJobState, DOWNLOADING) currentDownloadJobState.job = CoroutineScope(IO).launch { startDownloadJob(currentDownloadJobState) @@ -220,12 +214,13 @@ abstract class AbstractFetchDownloadService : Service() { context = context, filePath = currentDownloadJobState.state.filePath, contentType = currentDownloadJobState.state.contentType - )) { + ) + ) { val fileExt = MimeTypeMap.getFileExtensionFromUrl( - currentDownloadJobState.state.filePath.toString() + currentDownloadJobState.state.filePath.toString() ) val errorMessage = applicationContext.getString( - R.string.mozac_feature_downloads_open_not_supported1, fileExt + R.string.mozac_feature_downloads_open_not_supported1, fileExt ) Toast.makeText(applicationContext, errorMessage, Toast.LENGTH_SHORT).show() @@ -257,11 +252,12 @@ abstract class AbstractFetchDownloadService : Service() { // Create a new job and add it, with its downloadState to the map val downloadJobState = DownloadJobState( - state = download, - foregroundServiceId = foregroundServiceId, - status = ACTIVE + state = download.copy(status = DOWNLOADING), + foregroundServiceId = foregroundServiceId ) + store.dispatch(DownloadAction.UpdateQueuedDownloadAction(downloadJobState.state)) + downloadJobState.job = CoroutineScope(IO).launch { startDownloadJob(downloadJobState) } @@ -302,7 +298,7 @@ abstract class AbstractFetchDownloadService : Service() { // Dispatch the corresponding notification based on the current status updateDownloadNotification(uiStatus, download) - if (uiStatus != ACTIVE) { + if (uiStatus != DOWNLOADING) { sendDownloadStopped(download) } } @@ -314,9 +310,9 @@ abstract class AbstractFetchDownloadService : Service() { * from another thread, causing inconsistencies in the ui. */ @VisibleForTesting - internal fun updateDownloadNotification(latestUIStatus: DownloadJobStatus, download: DownloadJobState) { + internal fun updateDownloadNotification(latestUIStatus: Status, download: DownloadJobState) { val notification = when (latestUIStatus) { - ACTIVE -> DownloadNotification.createOngoingDownloadNotification(context, download) + DOWNLOADING -> DownloadNotification.createOngoingDownloadNotification(context, download) PAUSED -> DownloadNotification.createPausedDownloadNotification(context, download) FAILED -> DownloadNotification.createDownloadFailedNotification(context, download) COMPLETED -> { @@ -328,6 +324,7 @@ abstract class AbstractFetchDownloadService : Service() { download.lastNotificationUpdate = System.currentTimeMillis() null } + INITIATED -> null } notification?.let { @@ -390,22 +387,22 @@ abstract class AbstractFetchDownloadService : Service() { internal fun addToDownloadSystemDatabaseCompat(download: DownloadState) { if (SDK_INT < Build.VERSION_CODES.Q) { val fileName = download.fileName - ?: throw IllegalStateException("A fileName for a download is required") + ?: throw IllegalStateException("A fileName for a download is required") val file = File(download.filePath) // addCompletedDownload can't handle any non http(s) urls val url = if (!download.isScheme(listOf("http", "https"))) null else download.url.toUri() context.addCompletedDownload( - title = fileName, - description = fileName, - isMediaScannerScannable = true, - mimeType = download.contentType ?: "*/*", - path = file.absolutePath, - length = download.contentLength ?: file.length(), - // Only show notifications if our channel is blocked - showNotification = !DownloadNotification.isChannelEnabled(context), - uri = url, - referer = download.referrerUrl?.toUri() + title = fileName, + description = fileName, + isMediaScannerScannable = true, + mimeType = download.contentType ?: "*/*", + path = file.absolutePath, + length = download.contentLength ?: file.length(), + // Only show notifications if our channel is blocked + showNotification = !DownloadNotification.isChannelEnabled(context), + uri = url, + referer = download.referrerUrl?.toUri() ) } } @@ -454,7 +451,8 @@ abstract class AbstractFetchDownloadService : Service() { internal fun updateNotificationGroup(): Notification? { return if (SDK_INT >= Build.VERSION_CODES.N) { val downloadList = downloadJobs.values.toList() - val notificationGroup = DownloadNotification.createDownloadGroupNotification(context, downloadList) + val notificationGroup = + DownloadNotification.createDownloadGroupNotification(context, downloadList) NotificationManagerCompat.from(context).apply { notify(NOTIFICATION_DOWNLOAD_GROUP_ID, notificationGroup) } @@ -465,7 +463,8 @@ abstract class AbstractFetchDownloadService : Service() { } internal fun createCompactForegroundNotification(downloadJobState: DownloadJobState): Notification { - val notification = DownloadNotification.createOngoingDownloadNotification(context, downloadJobState) + val notification = + DownloadNotification.createOngoingDownloadNotification(context, downloadJobState) compatForegroundNotificationId = downloadJobState.foregroundServiceId NotificationManagerCompat.from(context).apply { notify(compatForegroundNotificationId, notification) @@ -500,7 +499,9 @@ abstract class AbstractFetchDownloadService : Service() { previousDownload = downloadJobs.values.firstOrNull { it.foregroundServiceId == compatForegroundNotificationId } - downloadJobState.foregroundServiceId to createCompactForegroundNotification(downloadJobState) + downloadJobState.foregroundServiceId to createCompactForegroundNotification( + downloadJobState + ) } startForeground(notificationId, notification) @@ -513,7 +514,7 @@ abstract class AbstractFetchDownloadService : Service() { * it always deletes the previous notification :( */ previousDownload?.let { - updateDownloadNotification(previousDownload.status, it) + updateDownloadNotification(previousDownload.state.status, it) } } @@ -530,7 +531,7 @@ abstract class AbstractFetchDownloadService : Service() { * the foreground notification id, when the previous one gets a state that * is likely to be dismissed. */ - val status = download.status + val status = download.state.status val foregroundId = download.foregroundServiceId val isSelectedForegroundId = compatForegroundNotificationId == foregroundId val needNewForegroundNotification = when (status) { @@ -544,7 +545,7 @@ abstract class AbstractFetchDownloadService : Service() { stopForeground(false) // Now we need to find a new foreground notification, if needed. - val newSelectedForegroundDownload = downloadJobs.values.firstOrNull { it.status == ACTIVE } + val newSelectedForegroundDownload = downloadJobs.values.firstOrNull { it.state.status == DOWNLOADING } newSelectedForegroundDownload?.let { setForegroundNotification(it) } @@ -558,11 +559,11 @@ abstract class AbstractFetchDownloadService : Service() { @Suppress("ComplexCondition") internal fun performDownload(currentDownloadJobState: DownloadJobState) { val download = currentDownloadJobState.state - val isResumingDownload = currentDownloadJobState.currentBytesCopied > 0L + val isResumingDownload = currentDownloadJobState.state.currentBytesCopied > 0L val headers = MutableHeaders() if (isResumingDownload) { - headers.append(RANGE, "bytes=${currentDownloadJobState.currentBytesCopied}-") + headers.append(RANGE, "bytes=${currentDownloadJobState.state.currentBytesCopied}-") } val request = Request(download.url.sanitizeURL(), headers = headers) @@ -574,7 +575,7 @@ abstract class AbstractFetchDownloadService : Service() { if (response.status != PARTIAL_CONTENT_STATUS && response.status != OK_STATUS || (isResumingDownload && !response.headers.contains(CONTENT_RANGE))) { // We experienced a problem trying to fetch the file, send a failure notification - currentDownloadJobState.currentBytesCopied = 0 + currentDownloadJobState.state = currentDownloadJobState.state.copy(currentBytesCopied = 0) setDownloadJobStatus(currentDownloadJobState, FAILED) logger.debug("Unable to fetching Download for ${currentDownloadJobState.state.url} status FAILED") return @@ -596,11 +597,12 @@ abstract class AbstractFetchDownloadService : Service() { * Updates the status of an ACTIVE download to completed or failed based on bytes copied */ internal fun verifyDownload(download: DownloadJobState) { - if (getDownloadJobStatus(download) == DownloadJobStatus.ACTIVE && - download.currentBytesCopied < download.state.contentLength ?: 0) { + if (getDownloadJobStatus(download) == DOWNLOADING && + download.state.currentBytesCopied < download.state.contentLength ?: 0 + ) { setDownloadJobStatus(download, FAILED) logger.error("verifyDownload for ${download.state.url} FAILED") - } else if (getDownloadJobStatus(download) == DownloadJobStatus.ACTIVE) { + } else if (getDownloadJobStatus(download) == DOWNLOADING) { setDownloadJobStatus(download, COMPLETED) /** * In cases when we don't get the file size provided initially, we have to @@ -608,31 +610,37 @@ abstract class AbstractFetchDownloadService : Service() { */ val fileSizeNotFound = download.state.contentLength == null || download.state.contentLength == 0L if (fileSizeNotFound) { - val newState = download.state.copy(contentLength = download.currentBytesCopied) + val newState = download.state.copy(contentLength = download.state.currentBytesCopied) updateDownloadState(newState) } - logger.debug("verifyDownload for ${download.state.url} ${download.status}") + logger.debug("verifyDownload for ${download.state.url} ${download.state.status}") } } private fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream) { val data = ByteArray(CHUNK_SIZE) logger.debug("starting copyInChunks ${downloadJobState.state.url}" + - " currentBytesCopied ${downloadJobState.currentBytesCopied}") + " currentBytesCopied ${downloadJobState.state.currentBytesCopied}") // To ensure that we copy all files (even ones that don't have fileSize, we must NOT check < fileSize - while (getDownloadJobStatus(downloadJobState) == DownloadJobStatus.ACTIVE) { + while (getDownloadJobStatus(downloadJobState) == DOWNLOADING) { val bytesRead = inStream.read(data) // If bytesRead is -1, there's no data left to read from the stream if (bytesRead == -1) { break } - downloadJobState.currentBytesCopied += bytesRead + val newState = downloadJobState.state.copy( + currentBytesCopied = + downloadJobState.state.currentBytesCopied + bytesRead + ) + updateDownloadState(newState) outStream.write(data, 0, bytesRead) } - logger.debug("Finishing copyInChunks ${downloadJobState.state.url} " + - "currentBytesCopied ${downloadJobState.currentBytesCopied}") + logger.debug( + "Finishing copyInChunks ${downloadJobState.state.url} " + + "currentBytesCopied ${downloadJobState.state.currentBytesCopied}" + ) } /** @@ -687,13 +695,17 @@ abstract class AbstractFetchDownloadService : Service() { download: DownloadState, append: Boolean ): DownloadState { - if (append) { return download } + if (append) { + return download + } return download.fileName?.let { - download.copy(fileName = DownloadUtils.uniqueFileName( - Environment.getExternalStoragePublicDirectory(download.destinationDirectory), - it - )) + download.copy( + fileName = DownloadUtils.uniqueFileName( + Environment.getExternalStoragePublicDirectory(download.destinationDirectory), + it + ) + ) } ?: download } @@ -780,6 +792,7 @@ abstract class AbstractFetchDownloadService : Service() { private const val CHUNK_SIZE = 4 * 1024 private const val PARTIAL_CONTENT_STATUS = 206 private const val OK_STATUS = 200 + /** * This interval was decided on by balancing the limit of the system (200ms) and allowing * users to press buttons on the notification. If a new notification is presented while a diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadNotification.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadNotification.kt index c16338c42e5..c896cedb8e2 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadNotification.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadNotification.kt @@ -18,6 +18,12 @@ import androidx.core.app.NotificationManagerCompat import androidx.core.app.NotificationManagerCompat.IMPORTANCE_NONE import androidx.core.content.ContextCompat import androidx.core.content.getSystemService +import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED +import mozilla.components.browser.state.state.content.DownloadState.Status.DOWNLOADING +import mozilla.components.browser.state.state.content.DownloadState.Status.PAUSED +import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED +import mozilla.components.browser.state.state.content.DownloadState.Status.CANCELLED +import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_CANCEL import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_DISMISS import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_OPEN @@ -25,11 +31,6 @@ import mozilla.components.feature.downloads.AbstractFetchDownloadService.Compani import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_RESUME import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_TRY_AGAIN import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobState -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.CANCELLED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.COMPLETED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.ACTIVE -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.FAILED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED import kotlin.random.Random @Suppress("LargeClass", "TooManyFunctions") @@ -48,7 +49,7 @@ internal object DownloadNotification { context: Context, notifications: List ): Notification { - val allDownloadsHaveFinished = notifications.all { it.status != ACTIVE } + val allDownloadsHaveFinished = notifications.all { it.state.status != DOWNLOADING } val icon = if (allDownloadsHaveFinished) { R.drawable.mozac_feature_download_ic_download_complete } else { @@ -78,7 +79,7 @@ internal object DownloadNotification { downloadJobState: DownloadJobState ): Notification { val downloadState = downloadJobState.state - val bytesCopied = downloadJobState.currentBytesCopied + val bytesCopied = downloadJobState.state.currentBytesCopied val channelId = ensureChannelExists(context) val isIndeterminate = downloadState.contentLength == null @@ -279,7 +280,7 @@ internal fun NotificationCompat.Builder.setCompatGroup(groupKey: String): Notifi @VisibleForTesting internal fun DownloadJobState.getProgress(): String { - val bytesCopied = currentBytesCopied + val bytesCopied = state.currentBytesCopied val isIndeterminate = state.contentLength == null || bytesCopied == 0L return if (isIndeterminate) { "" @@ -290,8 +291,8 @@ internal fun DownloadJobState.getProgress(): String { @VisibleForTesting internal fun DownloadJobState.getStatusDescription(context: Context): String { - return when (this.status) { - ACTIVE -> { + return when (this.state.status) { + DOWNLOADING -> { getProgress() } @@ -307,6 +308,6 @@ internal fun DownloadJobState.getStatusDescription(context: Context): String { context.getString(R.string.mozac_feature_downloads_failed_notification_text2) } - CANCELLED -> "" + CANCELLED, INITIATED -> "" } } diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt index cc3034a88dc..443462486ab 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt @@ -19,6 +19,7 @@ import androidx.core.content.getSystemService import androidx.core.net.toUri import androidx.core.util.set import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.content.DownloadState.Status import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Headers.Names.COOKIE @@ -107,9 +108,9 @@ class AndroidDownloadManager( val downloadID = intent.getLongExtra(EXTRA_DOWNLOAD_ID, -1) val download = store.state.queuedDownloads[downloadID] val downloadStatus = intent.getSerializableExtra(AbstractFetchDownloadService.EXTRA_DOWNLOAD_STATUS) - as AbstractFetchDownloadService.DownloadJobStatus + as Status - if (downloadStatus == AbstractFetchDownloadService.DownloadJobStatus.COMPLETED) { + if (downloadStatus == Status.COMPLETED) { store.dispatch(DownloadAction.RemoveQueuedDownloadAction(downloadID)) } diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/DownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/DownloadManager.kt index cdb2ed60e85..327a9e3f6fe 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/DownloadManager.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/DownloadManager.kt @@ -5,11 +5,11 @@ package mozilla.components.feature.downloads.manager import android.content.Context +import mozilla.components.browser.state.state.content.DownloadState.Status import mozilla.components.browser.state.state.content.DownloadState -import mozilla.components.feature.downloads.AbstractFetchDownloadService import mozilla.components.support.ktx.android.content.isPermissionGranted -typealias onDownloadStopped = (DownloadState, Long, AbstractFetchDownloadService.DownloadJobStatus) -> Unit +typealias onDownloadStopped = (DownloadState, Long, Status) -> Unit interface DownloadManager { diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt index 016c96d6c99..50b8e720faa 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt @@ -17,6 +17,7 @@ import android.os.Build.VERSION.SDK_INT import android.os.Build.VERSION_CODES.P import androidx.localbroadcastmanager.content.LocalBroadcastManager import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.content.DownloadState.Status import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.store.BrowserStore import mozilla.components.feature.downloads.AbstractFetchDownloadService @@ -103,9 +104,9 @@ class FetchDownloadManager( val downloadID = intent.getLongExtra(EXTRA_DOWNLOAD_ID, -1) val download = store.state.queuedDownloads[downloadID] val downloadStatus = intent.getSerializableExtra(EXTRA_DOWNLOAD_STATUS) - as AbstractFetchDownloadService.DownloadJobStatus + as Status - if (downloadStatus == AbstractFetchDownloadService.DownloadJobStatus.COMPLETED) { + if (downloadStatus == Status.COMPLETED) { store.dispatch(DownloadAction.RemoveQueuedDownloadAction(downloadID)) } if (download != null) { diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt index e81a750d842..a402cd55155 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt @@ -27,6 +27,7 @@ import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.state.content.DownloadState.Status.DOWNLOADING import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.MutableHeaders @@ -38,11 +39,6 @@ import mozilla.components.feature.downloads.AbstractFetchDownloadService.Compani import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_TRY_AGAIN import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.PROGRESS_UPDATE_INTERVAL import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobState -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.ACTIVE -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.CANCELLED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.COMPLETED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.FAILED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED import mozilla.components.feature.downloads.DownloadNotification.NOTIFICATION_DOWNLOAD_GROUP_ID import mozilla.components.feature.downloads.facts.DownloadsFacts.Items.NOTIFICATION import mozilla.components.support.base.facts.Action @@ -50,6 +46,7 @@ import mozilla.components.support.base.facts.processor.CollectionProcessor import mozilla.components.support.test.any import mozilla.components.support.test.argumentCaptor import mozilla.components.support.test.ext.joinBlocking +import mozilla.components.support.test.libstate.ext.waitUntilIdle import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext import org.junit.After @@ -163,123 +160,133 @@ class AbstractFetchDownloadServiceTest { fun `verifyDownload sets the download to failed if it is not complete`() = runBlocking { val downloadState = DownloadState( url = "mozilla.org/mozilla.txt", - contentLength = 50L + contentLength = 50L, + currentBytesCopied = 5, + status = DOWNLOADING ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 5, - status = ACTIVE, foregroundServiceId = 1, downloadDeleted = false ) service.verifyDownload(downloadJobState) - assertEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + verify(service).setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + verify(service).updateDownloadState(downloadState.copy(status = DownloadState.Status.FAILED)) } @Test fun `verifyDownload does NOT set the download to failed if it is paused`() = runBlocking { val downloadState = DownloadState( url = "mozilla.org/mozilla.txt", - contentLength = 50L + contentLength = 50L, + currentBytesCopied = 5, + status = DownloadState.Status.PAUSED ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 5, - status = PAUSED, foregroundServiceId = 1, downloadDeleted = false ) service.verifyDownload(downloadJobState) - assertEquals(PAUSED, service.getDownloadJobStatus(downloadJobState)) + verify(service, times(0)).setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + verify(service, times(0)).updateDownloadState(downloadState.copy(status = DownloadState.Status.FAILED)) } @Test fun `verifyDownload does NOT set the download to failed if it is complete`() = runBlocking { val downloadState = DownloadState( url = "mozilla.org/mozilla.txt", - contentLength = 50L + contentLength = 50L, + currentBytesCopied = 50, + status = DOWNLOADING ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 50, - status = ACTIVE, foregroundServiceId = 1, downloadDeleted = false ) service.verifyDownload(downloadJobState) - assertNotEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + verify(service, times(0)).setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + verify(service, times(0)).updateDownloadState(downloadState.copy(status = DownloadState.Status.FAILED)) } @Test fun `verifyDownload does NOT set the download to failed if it is cancelled`() = runBlocking { val downloadState = DownloadState( url = "mozilla.org/mozilla.txt", - contentLength = 50L + contentLength = 50L, + currentBytesCopied = 50, + status = DownloadState.Status.CANCELLED ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 50, - status = CANCELLED, foregroundServiceId = 1, downloadDeleted = false ) service.verifyDownload(downloadJobState) - assertNotEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + verify(service, times(0)).setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + verify(service, times(0)).updateDownloadState(downloadState.copy(status = DownloadState.Status.FAILED)) } @Test fun `verifyDownload does NOT set the download to failed if it is status COMPLETED`() = runBlocking { val downloadState = DownloadState( url = "mozilla.org/mozilla.txt", - contentLength = 50L + contentLength = 50L, + currentBytesCopied = 50, + status = DownloadState.Status.COMPLETED ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 50, - status = COMPLETED, foregroundServiceId = 1, downloadDeleted = false ) service.verifyDownload(downloadJobState) - assertNotEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + verify(service, times(0)).setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + verify(service, times(0)).updateDownloadState(downloadState.copy(status = DownloadState.Status.FAILED)) } @Test fun `verify that a COMPLETED download contains a file size`() { - val downloadState = DownloadState(url = "mozilla.org/mozilla.txt", contentLength = 0L) + val downloadState = DownloadState(url = "mozilla.org/mozilla.txt", + contentLength = 0L, + currentBytesCopied = 50, + status = DOWNLOADING + ) val downloadJobState = DownloadJobState( job = null, state = downloadState, - currentBytesCopied = 50, - status = ACTIVE, foregroundServiceId = 1, downloadDeleted = false ) + browserStore.dispatch(DownloadAction.QueueDownloadAction(downloadState)).joinBlocking() service.downloadJobs[downloadJobState.state.id] = downloadJobState service.verifyDownload(downloadJobState) + browserStore.waitUntilIdle() assertEquals(downloadJobState.state.contentLength, service.downloadJobs[downloadJobState.state.id]!!.state.contentLength) + assertEquals(downloadJobState.state.contentLength, browserStore.state.queuedDownloads.values.first().contentLength) } @Test @@ -318,7 +325,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - assertEquals(PAUSED, service.getDownloadJobStatus(downloadJobState)) + assertEquals(DownloadState.Status.PAUSED, service.getDownloadJobStatus(downloadJobState)) } @Test @@ -391,8 +398,11 @@ class AbstractFetchDownloadServiceTest { // Simulate a pause var downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - downloadJobState.currentBytesCopied = 1 - service.setDownloadJobStatus(downloadJobState, PAUSED) + val newState = downloadJobState.state.copy(currentBytesCopied = 1) + service.downloadJobs[newState.id]?.state = newState + browserStore.dispatch(DownloadAction.UpdateQueuedDownloadAction(newState)) + + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.PAUSED) service.downloadJobs[providedDownload.value.state.id]?.job?.cancel() val resumeIntent = Intent(ACTION_RESUME).apply { @@ -409,10 +419,10 @@ class AbstractFetchDownloadServiceTest { } downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - assertEquals(ACTIVE, service.getDownloadJobStatus(downloadJobState)) + assertEquals(DOWNLOADING, service.getDownloadJobStatus(downloadJobState)) // Make sure the download job is completed (break out of copyInChunks) - service.setDownloadJobStatus(downloadJobState, PAUSED) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.PAUSED) service.downloadJobs[providedDownload.value.state.id]?.job?.join() @@ -443,7 +453,7 @@ class AbstractFetchDownloadServiceTest { // Simulate a failure var downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, FAILED) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) service.downloadJobs[providedDownload.value.state.id]?.job?.cancel() val tryAgainIntent = Intent(ACTION_TRY_AGAIN).apply { @@ -460,10 +470,10 @@ class AbstractFetchDownloadServiceTest { } downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - assertEquals(ACTIVE, service.getDownloadJobStatus(downloadJobState)) + assertEquals(DOWNLOADING, service.getDownloadJobStatus(downloadJobState)) // Make sure the download job is completed (break out of copyInChunks) - service.setDownloadJobStatus(downloadJobState, PAUSED) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.PAUSED) service.downloadJobs[providedDownload.value.state.id]?.job?.join() @@ -493,7 +503,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - assertEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + assertEquals(DownloadState.Status.FAILED, service.getDownloadJobStatus(downloadJobState)) } @Test @@ -507,7 +517,7 @@ class AbstractFetchDownloadServiceTest { ) val transformedDownload = service.makeUniqueFileNameIfNecessary(download, false) - assertNotEquals(download, transformedDownload) + assertNotEquals(download.fileName, transformedDownload.fileName) } @Test @@ -547,8 +557,8 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, ACTIVE) - assertEquals(ACTIVE, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DOWNLOADING) + assertEquals(DOWNLOADING, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -573,11 +583,11 @@ class AbstractFetchDownloadServiceTest { @Test fun `sets the notification foreground in devices that support notification group`() = runBlocking { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) val notification = mock() @@ -593,11 +603,11 @@ class AbstractFetchDownloadServiceTest { @Test @Config(sdk = [Build.VERSION_CODES.M]) fun `sets the notification foreground in devices that DO NOT support notification group`() { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", + fileName = "file.txt", status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) val notification = mock() @@ -613,11 +623,11 @@ class AbstractFetchDownloadServiceTest { @Test @Config(sdk = [Build.VERSION_CODES.M]) fun createCompactForegroundNotification() { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) assertEquals(0, shadowNotificationService.size()) @@ -668,11 +678,11 @@ class AbstractFetchDownloadServiceTest { @Test @Config(sdk = [Build.VERSION_CODES.M]) fun `updateNotificationGroup will do nothing on devices that do not support notificaiton groups`() = runBlocking { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) service.downloadJobs[1L] = downloadState @@ -685,11 +695,11 @@ class AbstractFetchDownloadServiceTest { @Test fun `removeDownloadJob will update the background notification if there are other pending downloads`() { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) service.downloadJobs[1L] = downloadState @@ -706,11 +716,11 @@ class AbstractFetchDownloadServiceTest { @Test fun `removeDownloadJob will stop the service if there are none pending downloads`() { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, - foregroundServiceId = Random.nextInt(), - status = ACTIVE + foregroundServiceId = Random.nextInt() ) doNothing().`when`(service).stopForeground(false) @@ -739,14 +749,14 @@ class AbstractFetchDownloadServiceTest { @Config(sdk = [Build.VERSION_CODES.M]) fun `updateForegroundNotification will select a new foreground notification`() { val downloadState1 = DownloadJobState( - state = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt"), - foregroundServiceId = Random.nextInt(), - status = COMPLETED + state = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DownloadState.Status.COMPLETED), + foregroundServiceId = Random.nextInt() ) val downloadState2 = DownloadJobState( - state = DownloadState(id = 2, url = "https://example.com/file.txt", fileName = "file.txt"), - foregroundServiceId = Random.nextInt(), - status = ACTIVE + state = DownloadState(id = 2, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING), + foregroundServiceId = Random.nextInt() ) service.compatForegroundNotificationId = downloadState1.foregroundServiceId @@ -764,14 +774,14 @@ class AbstractFetchDownloadServiceTest { @Config(sdk = [Build.VERSION_CODES.M]) fun `updateForegroundNotification will NOT select a new foreground notification`() { val downloadState1 = DownloadJobState( - state = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt"), - foregroundServiceId = Random.nextInt(), - status = ACTIVE + state = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING), + foregroundServiceId = Random.nextInt() ) val downloadState2 = DownloadJobState( - state = DownloadState(id = 2, url = "https://example.com/file.txt", fileName = "file.txt"), - foregroundServiceId = Random.nextInt(), - status = ACTIVE + state = DownloadState(id = 2, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING), + foregroundServiceId = Random.nextInt() ) service.compatForegroundNotificationId = downloadState1.foregroundServiceId @@ -809,8 +819,8 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, PAUSED) - assertEquals(PAUSED, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.PAUSED) + assertEquals(DownloadState.Status.PAUSED, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -841,8 +851,8 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, COMPLETED) - assertEquals(COMPLETED, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.COMPLETED) + assertEquals(DownloadState.Status.COMPLETED, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -872,8 +882,8 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, FAILED) - assertEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.FAILED) + assertEquals(DownloadState.Status.FAILED, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -904,8 +914,8 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, CANCELLED) - assertEquals(CANCELLED, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.CANCELLED) + assertEquals(DownloadState.Status.CANCELLED, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -929,7 +939,7 @@ class AbstractFetchDownloadServiceTest { verify(service).performDownload(providedDownload.capture()) val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - assertEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) + assertEquals(DownloadState.Status.FAILED, service.getDownloadJobStatus(downloadJobState)) } @Test @@ -957,6 +967,7 @@ class AbstractFetchDownloadServiceTest { browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.registerNotificationActionsReceiver() service.onStartCommand(downloadIntent, 0, 0) + service.downloadJobs.values.forEach { assertTrue(it.job!!.isActive) } val providedDownload = argumentCaptor() @@ -982,8 +993,9 @@ class AbstractFetchDownloadServiceTest { @Test fun `updateDownloadState must update the download state in the store and in the downloadJobs`() { - val download = DownloadState("https://example.com/file.txt", "file1.txt") - val downloadJob = DownloadJobState(state = mock(), status = ACTIVE) + val download = DownloadState("https://example.com/file.txt", "file1.txt", + status = DOWNLOADING) + val downloadJob = DownloadJobState(state = mock()) val mockStore = mock() val service = spy(object : AbstractFetchDownloadService() { override val httpClient = client @@ -1020,7 +1032,7 @@ class AbstractFetchDownloadServiceTest { val providedDownload = argumentCaptor() verify(service).performDownload(providedDownload.capture()) - service.downloadJobs[download.id]?.status = PAUSED + service.setDownloadJobStatus(service.downloadJobs[download.id]!!, DownloadState.Status.PAUSED) // Advance the clock so that the poller posts a notification. testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -1034,11 +1046,11 @@ class AbstractFetchDownloadServiceTest { @Test fun `clearAllDownloadsNotificationsAndJobs cancels all running jobs and remove all notifications`() = runBlocking { - val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") + val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt", + status = DOWNLOADING) val downloadState = DownloadJobState( state = download, foregroundServiceId = Random.nextInt(), - status = ACTIVE, job = CoroutineScope(IO).launch { while (true) { } } ) @@ -1105,17 +1117,18 @@ class AbstractFetchDownloadServiceTest { val download = DownloadState( url = "http://www.mozilla.org", fileName = "example.apk", - destinationDirectory = folder.root.path + destinationDirectory = folder.root.path, + status = DownloadState.Status.COMPLETED ) val service = spy(object : AbstractFetchDownloadService() { override val httpClient = client override val store = browserStore }) - val downloadJobState = DownloadJobState(state = download, status = COMPLETED) + val downloadJobState = DownloadJobState(state = download) doReturn(testContext).`when`(service).context - service.updateDownloadNotification(COMPLETED, downloadJobState) + service.updateDownloadNotification(DownloadState.Status.COMPLETED, downloadJobState) verify(service).addToDownloadSystemDatabaseCompat(any()) } @@ -1195,8 +1208,8 @@ class AbstractFetchDownloadServiceTest { val cancelledDownloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(cancelledDownloadJobState, CANCELLED) - assertEquals(CANCELLED, service.getDownloadJobStatus(cancelledDownloadJobState)) + service.setDownloadJobStatus(cancelledDownloadJobState, DownloadState.Status.CANCELLED) + assertEquals(DownloadState.Status.CANCELLED, service.getDownloadJobStatus(cancelledDownloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) // The additional notification is the summary one (the notification group). assertEquals(1, shadowNotificationService.size()) @@ -1214,8 +1227,8 @@ class AbstractFetchDownloadServiceTest { val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! - service.setDownloadJobStatus(downloadJobState, COMPLETED) - assertEquals(COMPLETED, service.getDownloadJobStatus(downloadJobState)) + service.setDownloadJobStatus(downloadJobState, DownloadState.Status.COMPLETED) + assertEquals(DownloadState.Status.COMPLETED, service.getDownloadJobStatus(downloadJobState)) testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) // one of the notifications it is the group notification only for devices the support it assertEquals(2, shadowNotificationService.size()) @@ -1223,7 +1236,7 @@ class AbstractFetchDownloadServiceTest { @Test fun `keeps track of how many seconds have passed since the last update to a notification`() = runBlocking { - val downloadJobState = DownloadJobState(state = mock(), status = ACTIVE) + val downloadJobState = DownloadJobState(state = mock()) val oneSecond = 1000L downloadJobState.lastNotificationUpdate = System.currentTimeMillis() @@ -1243,7 +1256,7 @@ class AbstractFetchDownloadServiceTest { @Test fun `is a notification under the time limit for updates`() = runBlocking { - val downloadJobState = DownloadJobState(state = mock(), status = ACTIVE) + val downloadJobState = DownloadJobState(state = mock()) val oneSecond = 1000L downloadJobState.lastNotificationUpdate = System.currentTimeMillis() @@ -1257,7 +1270,7 @@ class AbstractFetchDownloadServiceTest { @Test fun `try to update a notification`() = runBlocking { - val downloadJobState = DownloadJobState(state = mock(), status = ACTIVE) + val downloadJobState = DownloadJobState(state = mock()) val oneSecond = 1000L downloadJobState.lastNotificationUpdate = System.currentTimeMillis() diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadDialogFragmentTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadDialogFragmentTest.kt index e05b030a259..68a3d3994af 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadDialogFragmentTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadDialogFragmentTest.kt @@ -23,9 +23,9 @@ class DownloadDialogFragmentTest { fun setup() { dialog = object : DownloadDialogFragment() {} download = DownloadState( - "http://ipv4.download.thinkbroadband.com/5MB.zip", - "5MB.zip", "application/zip", 5242880, - "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" + "http://ipv4.download.thinkbroadband.com/5MB.zip", + "5MB.zip", "application/zip", 5242880, + userAgent = "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) } diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadNotificationTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadNotificationTest.kt index 11a99b77578..eccdcef456e 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadNotificationTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadNotificationTest.kt @@ -9,12 +9,6 @@ import androidx.core.app.NotificationCompat import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobState -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.FAILED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.ACTIVE -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.CANCELLED -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.COMPLETED -import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals @@ -29,9 +23,9 @@ class DownloadNotificationTest { fun getProgress() { val downloadJobState = DownloadJobState( job = null, - state = DownloadState(url = "mozilla.org/mozilla.txt", contentLength = 100L), - currentBytesCopied = 10, - status = ACTIVE, + state = DownloadState(url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.DOWNLOADING), foregroundServiceId = 1, downloadDeleted = false ) @@ -65,23 +59,58 @@ class DownloadNotificationTest { val pausedText = testContext.getString(R.string.mozac_feature_downloads_paused_notification_text) val completedText = testContext.getString(R.string.mozac_feature_downloads_completed_notification_text2) val failedText = testContext.getString(R.string.mozac_feature_downloads_failed_notification_text2) - var downloadJobState = DownloadJobState(state = mock(), status = ACTIVE) + var downloadJobState = DownloadJobState( + job = null, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.DOWNLOADING), + foregroundServiceId = 1, + downloadDeleted = false + ) assertEquals(downloadJobState.getProgress(), downloadJobState.getStatusDescription(testContext)) - downloadJobState = DownloadJobState(state = mock(), status = PAUSED) + downloadJobState = DownloadJobState( + job = null, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.PAUSED), + foregroundServiceId = 1, + downloadDeleted = false + ) assertEquals(pausedText, downloadJobState.getStatusDescription(testContext)) - downloadJobState = DownloadJobState(state = mock(), status = COMPLETED) + downloadJobState = DownloadJobState( + job = null, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.COMPLETED), + foregroundServiceId = 1, + downloadDeleted = false + ) assertEquals(completedText, downloadJobState.getStatusDescription(testContext)) - downloadJobState = DownloadJobState(state = mock(), status = FAILED) + downloadJobState = DownloadJobState( + job = null, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.FAILED), + foregroundServiceId = 1, + downloadDeleted = false + ) assertEquals(failedText, downloadJobState.getStatusDescription(testContext)) - downloadJobState = DownloadJobState(state = mock(), status = CANCELLED) + downloadJobState = DownloadJobState( + job = null, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.CANCELLED), + foregroundServiceId = 1, + downloadDeleted = false + ) assertEquals("", downloadJobState.getStatusDescription(testContext)) } @@ -90,17 +119,17 @@ class DownloadNotificationTest { fun getDownloadSummary() { val download1 = DownloadJobState( job = null, - state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L), - currentBytesCopied = 10, - status = ACTIVE, + state = DownloadState(fileName = "mozilla.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 10, + status = DownloadState.Status.DOWNLOADING), foregroundServiceId = 1, downloadDeleted = false ) val download2 = DownloadJobState( job = null, - state = DownloadState(fileName = "mozilla2.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L), - currentBytesCopied = 20, - status = ACTIVE, + state = DownloadState(fileName = "mozilla2.txt", url = "mozilla.org/mozilla.txt", contentLength = 100L, + currentBytesCopied = 20, + status = DownloadState.Status.DOWNLOADING), foregroundServiceId = 1, downloadDeleted = false ) diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/SimpleDownloadDialogFragmentTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/SimpleDownloadDialogFragmentTest.kt index 50944ab9810..30588dac903 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/SimpleDownloadDialogFragmentTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/SimpleDownloadDialogFragmentTest.kt @@ -40,7 +40,7 @@ class SimpleDownloadDialogFragmentTest { download = DownloadState( "http://ipv4.download.thinkbroadband.com/5MB.zip", "5MB.zip", "application/zip", 5242880, - "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" + userAgent = "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) dialog = SimpleDownloadDialogFragment.newInstance() } diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt index 8e4f3e54273..51f6f15fa38 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt @@ -24,7 +24,6 @@ import org.mockito.ArgumentMatchers.anyString import org.mockito.Mockito.verify import org.mockito.Mockito.verifyZeroInteractions import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD_STATUS -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus import mozilla.components.support.test.libstate.ext.waitUntilIdle import org.junit.Assert.assertEquals @@ -40,7 +39,7 @@ class AndroidDownloadManagerTest { download = DownloadState( "http://ipv4.download.thinkbroadband.com/5MB.zip", "", "application/zip", 5242880, - "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" + userAgent = "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) store = BrowserStore() downloadManager = AndroidDownloadManager(testContext, store) @@ -98,7 +97,7 @@ class AndroidDownloadManagerTest { @Test fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() { var downloadCompleted = false - var downloadStatus: DownloadJobStatus? = null + var downloadStatus: DownloadState.Status? = null val downloadWithFileName = download.copy(fileName = "5MB.zip") grantPermissions() @@ -117,12 +116,12 @@ class AndroidDownloadManagerTest { notifyDownloadCompleted(id) assertTrue(downloadCompleted) - assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertEquals(DownloadState.Status.COMPLETED, downloadStatus) } @Test fun `sendBroadcast with completed download removes queued download from store`() { - var downloadStatus: DownloadJobStatus? = null + var downloadStatus: DownloadState.Status? = null val downloadWithFileName = download.copy(fileName = "5MB.zip") grantPermissions() @@ -139,7 +138,7 @@ class AndroidDownloadManagerTest { notifyDownloadCompleted(id) store.waitUntilIdle() - assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertEquals(DownloadState.Status.COMPLETED, downloadStatus) assertTrue(store.state.queuedDownloads.isEmpty()) } @@ -165,14 +164,14 @@ class AndroidDownloadManagerTest { private fun notifyDownloadFailed(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, id) - intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.FAILED) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadState.Status.FAILED) testContext.sendBroadcast(intent) } private fun notifyDownloadCompleted(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, id) - intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.COMPLETED) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadState.Status.COMPLETED) testContext.sendBroadcast(intent) } diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt index 94fdc9ba266..6dfd6baa45a 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt @@ -26,7 +26,6 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.verify -import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus import mozilla.components.support.test.libstate.ext.waitUntilIdle import org.junit.Assert.assertTrue import org.junit.Assert.assertNull @@ -51,7 +50,7 @@ class FetchDownloadManagerTest { download = DownloadState( "http://ipv4.download.thinkbroadband.com/5MB.zip", "", "application/zip", 5242880, - "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" + userAgent = "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) downloadManager = FetchDownloadManager(testContext, store, MockDownloadService::class, broadcastManager) } @@ -135,7 +134,7 @@ class FetchDownloadManagerTest { @Test fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() { var downloadStopped = false - var downloadStatus: DownloadJobStatus? = null + var downloadStatus: DownloadState.Status? = null val downloadWithFileName = download.copy(fileName = "5MB.zip") grantPermissions() @@ -153,12 +152,12 @@ class FetchDownloadManagerTest { notifyDownloadCompleted(id) assertTrue(downloadStopped) - assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertEquals(DownloadState.Status.COMPLETED, downloadStatus) } @Test fun `sendBroadcast with completed download removes queued download from store`() { - var downloadStatus: DownloadJobStatus? = null + var downloadStatus: DownloadState.Status? = null val downloadWithFileName = download.copy(fileName = "5MB.zip") grantPermissions() @@ -175,14 +174,14 @@ class FetchDownloadManagerTest { notifyDownloadCompleted(id) store.waitUntilIdle() - assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertEquals(DownloadState.Status.COMPLETED, downloadStatus) assertTrue(store.state.queuedDownloads.isEmpty()) } @Test fun `onReceive properly gets download object form sendBroadcast`() { var downloadStopped = false - var downloadStatus: DownloadJobStatus? = null + var downloadStatus: DownloadState.Status? = null var downloadName = "" var downloadSize = 0L val downloadWithFileName = download.copy(fileName = "5MB.zip", contentLength = 5L) @@ -203,20 +202,20 @@ class FetchDownloadManagerTest { assertTrue(downloadStopped) assertEquals("5MB.zip", downloadName) assertEquals(5L, downloadSize) - assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertEquals(DownloadState.Status.COMPLETED, downloadStatus) } private fun notifyDownloadFailed(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(EXTRA_DOWNLOAD_ID, id) - intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.FAILED) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadState.Status.FAILED) broadcastManager.sendBroadcast(intent) } private fun notifyDownloadCompleted(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(EXTRA_DOWNLOAD_ID, id) - intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.COMPLETED) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadState.Status.COMPLETED) broadcastManager.sendBroadcast(intent) } diff --git a/docs/changelog.md b/docs/changelog.md index 33199eaf352..ed5d016d875 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -12,6 +12,10 @@ permalink: /changelog/ * [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt) * [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt) +* **feature-downloads** + * ⚠️ **This is a breaking change**: Removed the following properties from `DownloadJobState` in `AbstractFetchDownloadService` and added to `DownloadState`: `DownloadJobStatus` (now renamed to `DownloadStatus`) and `currentBytesCopied`. These properties can now be read from `DownloadState`. + * ⚠️ **This is a breaking change**: Removed the enum class `DownloadJobStatus` from `AbstractFetchDownloadService` and moved into `DownloadState`, and removed the `ACTIVE` state while introducing two new states called `INITIATED` and `DOWNLOADING`. + # 52.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v51.0.0...v52.0.0)