From 14e0c6bd135693b8ae799424daf5292811361913 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Sun, 29 Dec 2019 21:13:42 -0500 Subject: [PATCH] Revert "Closes #5284: Adds progress bar to downloads" This reverts commit fb3847f8c955140cd0e6318ecd79daf16a8edb1c. Reverting due to failing CI builds. --- .../downloads/AbstractFetchDownloadService.kt | 151 ++++++------------ .../feature/downloads/DownloadNotification.kt | 17 +- .../AbstractFetchDownloadServiceTest.kt | 14 +- 3 files changed, 67 insertions(+), 115 deletions(-) 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 888386c6da1..c5226cf6436 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 @@ -22,7 +22,6 @@ import android.os.IBinder import android.os.ParcelFileDescriptor import android.provider.MediaStore import android.widget.Toast -import androidx.annotation.GuardedBy import androidx.annotation.VisibleForTesting import androidx.core.app.NotificationManagerCompat import androidx.core.content.FileProvider @@ -31,10 +30,6 @@ import androidx.localbroadcastmanager.content.LocalBroadcastManager import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Job -import kotlinx.coroutines.MainScope -import kotlinx.coroutines.cancel -import kotlinx.coroutines.delay -import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.concept.fetch.Client @@ -67,8 +62,6 @@ import kotlin.random.Random @Suppress("TooManyFunctions", "LargeClass") abstract class AbstractFetchDownloadService : Service() { - private val notificationUpdateScope = MainScope() - protected abstract val httpClient: Client @VisibleForTesting internal val broadcastManager by lazy { LocalBroadcastManager.getInstance(this) } @@ -81,7 +74,7 @@ abstract class AbstractFetchDownloadService : Service() { var job: Job? = null, @Volatile var state: DownloadState, var currentBytesCopied: Long = 0, - @GuardedBy("context") var status: DownloadJobStatus, + @Volatile var status: DownloadJobStatus, var foregroundServiceId: Int = 0, var downloadDeleted: Boolean = false ) @@ -99,7 +92,6 @@ abstract class AbstractFetchDownloadService : Service() { internal val broadcastReceiver by lazy { object : BroadcastReceiver() { - @Suppress("LongMethod") override fun onReceive(context: Context, intent: Intent?) { val downloadId = intent?.extras?.getLong(DownloadNotification.EXTRA_DOWNLOAD_ID) ?: return @@ -107,17 +99,13 @@ abstract class AbstractFetchDownloadService : Service() { when (intent.action) { ACTION_PAUSE -> { - synchronized(context) { - currentDownloadJobState.status = DownloadJobStatus.PAUSED - } + currentDownloadJobState.status = DownloadJobStatus.PAUSED currentDownloadJobState.job?.cancel() emitNotificationPauseFact() } ACTION_RESUME -> { - synchronized(context) { - currentDownloadJobState.status = DownloadJobStatus.ACTIVE - } + currentDownloadJobState.status = DownloadJobStatus.ACTIVE currentDownloadJobState.job = CoroutineScope(IO).launch { startDownloadJob(downloadId) @@ -130,9 +118,8 @@ abstract class AbstractFetchDownloadService : Service() { NotificationManagerCompat.from(context).cancel( currentDownloadJobState.foregroundServiceId ) - synchronized(context) { - currentDownloadJobState.status = DownloadJobStatus.CANCELLED - } + currentDownloadJobState.status = DownloadJobStatus.CANCELLED + currentDownloadJobState.job?.cancel() currentDownloadJobState.job = CoroutineScope(IO).launch { @@ -147,10 +134,7 @@ abstract class AbstractFetchDownloadService : Service() { NotificationManagerCompat.from(context).cancel( currentDownloadJobState.foregroundServiceId ) - - synchronized(context) { - currentDownloadJobState.status = DownloadJobStatus.ACTIVE - } + currentDownloadJobState.status = DownloadJobStatus.ACTIVE currentDownloadJobState.job = CoroutineScope(IO).launch { startDownloadJob(downloadId) @@ -198,63 +182,12 @@ abstract class AbstractFetchDownloadService : Service() { startDownloadJob(download.id) } - notificationUpdateScope.launch { - while (isActive) { - delay(PROGRESS_UPDATE_INTERVAL) - updateDownloadNotification() - } - } - return super.onStartCommand(intent, flags, startId) } - /*** - * Android rate limits notifications being sent, so we must send them on a delay so that - * notifications are not dropped - */ - private fun updateDownloadNotification() { - synchronized(context) { - for (download in downloadJobs.values) { - // Dispatch the corresponding notification based on the current status - val notification = when (download.status) { - DownloadJobStatus.ACTIVE -> { - DownloadNotification.createOngoingDownloadNotification( - context, - download.state, - download.currentBytesCopied - ) - } - - DownloadJobStatus.PAUSED -> { - DownloadNotification.createPausedDownloadNotification(context, download.state) - } - - DownloadJobStatus.COMPLETED -> { - DownloadNotification.createDownloadCompletedNotification(context, download.state) - } - - DownloadJobStatus.FAILED -> { - DownloadNotification.createDownloadFailedNotification(context, download.state) - } - - DownloadJobStatus.CANCELLED -> { return } - } - - NotificationManagerCompat.from(context).notify( - download.foregroundServiceId, - notification - ) - - if (download.status != DownloadJobStatus.ACTIVE) { - sendDownloadNotInProgress(download.state.id, download.status) - } - } - } - } - override fun onTaskRemoved(rootIntent: Intent?) { // If the task gets removed (app gets swiped away in the task switcher) our process and this - // service gets killed. In this situation we remove the notification since the download will + // servies gets killed. In this situation we remove the notification since the download will // stop and cannot be controlled via the notification anymore (until we persist enough data // to resume downloads from a new process). @@ -271,18 +204,40 @@ abstract class AbstractFetchDownloadService : Service() { downloadJobs.values.forEach { it.job?.cancel() } - - notificationUpdateScope.cancel() } internal fun startDownloadJob(downloadId: Long) { val currentDownloadJobState = downloadJobs[downloadId] ?: return - try { + val notification = try { performDownload(currentDownloadJobState.state) + when (currentDownloadJobState.status) { + DownloadJobStatus.PAUSED -> { + DownloadNotification.createPausedDownloadNotification(context, currentDownloadJobState.state) + } + + DownloadJobStatus.ACTIVE -> { + currentDownloadJobState.status = DownloadJobStatus.COMPLETED + DownloadNotification.createDownloadCompletedNotification(context, currentDownloadJobState.state) + } + + DownloadJobStatus.FAILED -> { + DownloadNotification.createDownloadFailedNotification(context, currentDownloadJobState.state) + } + + else -> return + } } catch (e: IOException) { currentDownloadJobState.status = DownloadJobStatus.FAILED + DownloadNotification.createDownloadFailedNotification(context, currentDownloadJobState.state) } + + NotificationManagerCompat.from(context).notify( + currentDownloadJobState.foregroundServiceId, + notification + ) + + sendDownloadCompleteBroadcast(downloadId, currentDownloadJobState.status) } internal fun deleteDownloadingFile(downloadState: DownloadState) { @@ -302,10 +257,21 @@ abstract class AbstractFetchDownloadService : Service() { context.registerReceiver(broadcastReceiver, filter) } + private fun displayOngoingDownloadNotification(download: DownloadState) { + val ongoingDownloadNotification = DownloadNotification.createOngoingDownloadNotification( + context, + download + ) + + NotificationManagerCompat.from(context).notify( + downloadJobs[download.id]?.foregroundServiceId ?: 0, + ongoingDownloadNotification + ) + } + @Suppress("ComplexCondition") internal fun performDownload(download: DownloadState) { - val currentBytes = downloadJobs[download.id]?.currentBytesCopied ?: 0L - val isResumingDownload = currentBytes > 0L + val isResumingDownload = downloadJobs[download.id]?.currentBytesCopied ?: 0L > 0L val headers = MutableHeaders() if (isResumingDownload) { @@ -329,6 +295,8 @@ abstract class AbstractFetchDownloadService : Service() { val newDownloadState = download.withResponse(response.headers, inStream) downloadJobs[download.id]?.state = newDownloadState + displayOngoingDownloadNotification(newDownloadState) + useFileStream(newDownloadState, isResumingDownload) { outStream -> copyInChunks(downloadJobs[download.id]!!, inStream, outStream) } @@ -337,21 +305,14 @@ abstract class AbstractFetchDownloadService : Service() { } } - /** - * Updates the status of an ACTIVE download to completed or failed based on bytes copied - */ internal fun verifyDownload(download: DownloadJobState) { - synchronized(context) { - if (download.status == DownloadJobStatus.ACTIVE && - download.currentBytesCopied < download.state.contentLength ?: 0) { - download.status = DownloadJobStatus.FAILED - } else if (download.status == DownloadJobStatus.ACTIVE) { - download.status = DownloadJobStatus.COMPLETED - } + if (download.status == DownloadJobStatus.ACTIVE && + download.currentBytesCopied < download.state.contentLength ?: 0) { + download.status = DownloadJobStatus.FAILED } } - internal fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream) { + private fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream) { // To ensure that we copy all files (even ones that don't have fileSize, we must NOT check < fileSize while (downloadJobState.status == DownloadJobStatus.ACTIVE) { val data = ByteArray(CHUNK_SIZE) @@ -368,9 +329,9 @@ abstract class AbstractFetchDownloadService : Service() { /** * Informs [mozilla.components.feature.downloads.manager.FetchDownloadManager] that a download - * is no longer in progress due to being paused, completed, or failed + * has been completed. */ - private fun sendDownloadNotInProgress(downloadID: Long, status: DownloadJobStatus) { + private fun sendDownloadCompleteBroadcast(downloadID: Long, status: DownloadJobStatus) { val download = downloadJobs[downloadID]?.state ?: return val intent = Intent(ACTION_DOWNLOAD_COMPLETE) @@ -496,12 +457,6 @@ 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 - * user is tapping a button, their press will be cancelled. - */ - private const val PROGRESS_UPDATE_INTERVAL = 750L const val EXTRA_DOWNLOAD = "mozilla.components.feature.downloads.extras.DOWNLOAD" const val EXTRA_DOWNLOAD_STATUS = "mozilla.components.feature.downloads.extras.DOWNLOAD_STATUS" 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 119c3784ef4..40fce67a78a 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 @@ -30,34 +30,23 @@ internal object DownloadNotification { private const val NOTIFICATION_CHANNEL_ID = "mozac.feature.downloads.generic" private const val LEGACY_NOTIFICATION_CHANNEL_ID = "Downloads" - private const val PERCENTAGE_MULTIPLIER = 100 internal const val EXTRA_DOWNLOAD_ID = "downloadId" /** * Build the notification to be displayed while the download service is active. */ - fun createOngoingDownloadNotification( - context: Context, - downloadState: DownloadState, - bytesCopied: Long - ): Notification { + fun createOngoingDownloadNotification(context: Context, downloadState: DownloadState): Notification { val channelId = ensureChannelExists(context) val fileSizeText = (downloadState.contentLength?.toMegabyteString() ?: "") - val isIndeterminate = downloadState.contentLength == null - val contentText = if (isIndeterminate) { - fileSizeText - } else { - "${PERCENTAGE_MULTIPLIER * bytesCopied / downloadState.contentLength!!}%" - } return NotificationCompat.Builder(context, channelId) .setSmallIcon(R.drawable.mozac_feature_download_ic_ongoing_download) .setContentTitle(downloadState.fileName) - .setContentText(contentText) + .setContentText(fileSizeText) .setColor(ContextCompat.getColor(context, R.color.mozac_feature_downloads_notification)) .setCategory(NotificationCompat.CATEGORY_PROGRESS) - .setProgress(downloadState.contentLength?.toInt() ?: 0, bytesCopied.toInt(), isIndeterminate) + .setProgress(1, 0, true) .setOngoing(true) .addAction(getPauseAction(context, downloadState.id)) .addAction(getCancelAction(context, downloadState.id)) 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 b8de71db006..6b9b187ed2a 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 @@ -68,9 +68,6 @@ class AbstractFetchDownloadServiceTest { doReturn(broadcastManager).`when`(service).broadcastManager doReturn(testContext).`when`(service).context - doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) - doNothing().`when`(service).copyInChunks(any(), any(), any()) - service.downloadJobs.values.forEach { it.job?.cancel() } } @Test @@ -83,6 +80,7 @@ class AbstractFetchDownloadServiceTest { Response.Body(mock()) ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download) @@ -226,6 +224,7 @@ class AbstractFetchDownloadServiceTest { Response.Body(mock()) ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download) @@ -250,6 +249,7 @@ class AbstractFetchDownloadServiceTest { assertEquals(NOTIFICATION, pauseFact.item) } + service.downloadJobs[providedDownload.value.id]?.job?.join() assertEquals(PAUSED, service.downloadJobs[providedDownload.value.id]?.status) } @@ -263,6 +263,7 @@ class AbstractFetchDownloadServiceTest { Response.Body(mock()) ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download) @@ -315,6 +316,7 @@ class AbstractFetchDownloadServiceTest { .fetch(Request("https://example.com/file.txt")) doReturn(resumeResponse).`when`(client) .fetch(Request("https://example.com/file.txt", headers = MutableHeaders("Range" to "bytes=1-"))) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download) @@ -341,6 +343,8 @@ class AbstractFetchDownloadServiceTest { assertEquals(NOTIFICATION, resumeFact.item) } + service.downloadJobs[providedDownload.value.id]?.job?.join() + assertEquals(ACTIVE, service.downloadJobs[providedDownload.value.id]?.status) verify(service).startDownloadJob(providedDownload.value.id) } @@ -355,6 +359,7 @@ class AbstractFetchDownloadServiceTest { Response.Body(mock()) ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download) @@ -381,6 +386,8 @@ class AbstractFetchDownloadServiceTest { assertEquals(NOTIFICATION, tryAgainFact.item) } + service.downloadJobs[providedDownload.value.id]?.job?.join() + assertEquals(ACTIVE, service.downloadJobs[providedDownload.value.id]?.status) verify(service).startDownloadJob(providedDownload.value.id) } @@ -395,6 +402,7 @@ class AbstractFetchDownloadServiceTest { Response.Body(mock()) ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) + doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) val downloadIntent = Intent("ACTION_DOWNLOAD").apply { putDownloadExtra(download)