From c6484589878c89ea26d7a21df35dc10ce1ceed9e Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Mon, 18 May 2020 15:30:08 -0400 Subject: [PATCH] Closes #6893 Fulfill the foreground service contract on AbstractFetchDownloadService --- .../downloads/AbstractFetchDownloadService.kt | 28 +++++++------------ .../feature/downloads/DownloadNotification.kt | 15 +++++----- .../AbstractFetchDownloadServiceTest.kt | 23 +++------------ .../downloads/DownloadNotificationTest.kt | 4 +-- 4 files changed, 23 insertions(+), 47 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 1c658e931d9..7342ac07862 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 @@ -59,6 +59,7 @@ import mozilla.components.feature.downloads.facts.emitNotificationPauseFact import mozilla.components.feature.downloads.facts.emitNotificationCancelFact import mozilla.components.feature.downloads.facts.emitNotificationTryAgainFact import mozilla.components.feature.downloads.facts.emitNotificationOpenFact +import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.utils.DownloadUtils import java.io.File import java.io.FileOutputStream @@ -77,7 +78,8 @@ import kotlin.random.Random @Suppress("TooManyFunctions", "LargeClass") abstract class AbstractFetchDownloadService : Service() { - private var notificationUpdateScope = MainScope() + private val notificationUpdateScope = MainScope() + private val logger = Logger("AbstractFetchDownloadService") protected abstract val httpClient: Client @VisibleForTesting @@ -225,6 +227,7 @@ abstract class AbstractFetchDownloadService : Service() { override fun onCreate() { super.onCreate() + logger.info("onCreate") registerNotificationActionsReceiver() } @@ -247,15 +250,9 @@ abstract class AbstractFetchDownloadService : Service() { setForegroundNotification(downloadJobState) + logger.info("notificationUpdateScope.isActive ${notificationUpdateScope.isActive}") downloadJobs[download.id] = downloadJobState - // We are canceling the notificationUpdateScope, because we don't want to keep it - // running when we don't have more downloads to process. As a result, we need to - // create a new one when there are more downloads to process and the scope is canceled - if (!notificationUpdateScope.isActive) { - notificationUpdateScope = MainScope() - } - notificationUpdateScope.launch { while (isActive) { delay(PROGRESS_UPDATE_INTERVAL) @@ -323,12 +320,13 @@ abstract class AbstractFetchDownloadService : Service() { // service 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). - - clearAllDownloadsNotificationsAndJobs() + logger.info("onTaskRemoved") + stopSelf() } override fun onDestroy() { super.onDestroy() + logger.info("onDestroy") clearAllDownloadsNotificationsAndJobs() unregisterNotificationActionsReceiver() @@ -385,12 +383,9 @@ abstract class AbstractFetchDownloadService : Service() { } @VisibleForTesting - internal open fun removeDownloadJob(downloadJobState: DownloadJobState) { + internal fun removeDownloadJob(downloadJobState: DownloadJobState) { downloadJobs.remove(downloadJobState.state.id) if (downloadJobs.isEmpty()) { - stopForeground(false) - NotificationManagerCompat.from(context).cancel(getForegroundId()) - clearAllDownloadsNotificationsAndJobs() stopSelf() } else { updateForegroundNotificationIfNeeded(downloadJobState) @@ -445,14 +440,11 @@ abstract class AbstractFetchDownloadService : Service() { internal fun setForegroundNotification(downloadJobState: DownloadJobState) { var previousDownload: DownloadJobState? = null - // Ongoing notifications can't be deleted by calling startForeground - val isOngoing: ((DownloadJobStatus) -> Boolean) = { status -> status == ACTIVE || status == PAUSED } - val (notificationId, notification) = if (SDK_INT >= Build.VERSION_CODES.N) { NOTIFICATION_DOWNLOAD_GROUP_ID to updateNotificationGroup() } else { previousDownload = downloadJobs.values.firstOrNull { - it.foregroundServiceId == compatForegroundNotificationId && !isOngoing(it.status) + it.foregroundServiceId == compatForegroundNotificationId } downloadJobState.foregroundServiceId to createCompactForegroundNotification(downloadJobState) } 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 69f92c6df8c..c16338c42e5 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 @@ -54,15 +54,15 @@ internal object DownloadNotification { } else { R.drawable.mozac_feature_download_ic_ongoing_download } - val downloadSummary = getDownloadSummary(context, notifications) - val summaryLineArray = downloadSummary.split("\n") - val summaryLine1 = summaryLineArray.first() - val summaryLine2 = if (summaryLineArray.size == 2) summaryLineArray[1] else "" + val summaryList = getSummaryList(context, notifications) + val summaryLine1 = summaryList.first() + val summaryLine2 = if (summaryList.size == 2) summaryList[1] else "" return NotificationCompat.Builder(context, ensureChannelExists(context)) .setSmallIcon(icon) + .setColor(ContextCompat.getColor(context, R.color.mozac_feature_downloads_notification)) .setContentTitle(context.getString(R.string.mozac_feature_downloads_notification_channel)) - .setContentText(downloadSummary) + .setContentText(summaryList.joinToString("\n")) .setStyle(NotificationCompat.InboxStyle().addLine(summaryLine1).addLine(summaryLine2)) .setGroup(NOTIFICATION_GROUP_KEY) .setGroupSummary(true) @@ -167,9 +167,8 @@ internal object DownloadNotification { } @VisibleForTesting - internal fun getDownloadSummary(context: Context, notifications: List): String { - val summaryList = notifications.take(2) - return summaryList.joinToString("\n") { downloadState -> + internal fun getSummaryList(context: Context, notifications: List): List { + return notifications.take(2).map { downloadState -> "${downloadState.state.fileName} ${downloadState.getStatusDescription(context)}" } } 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 f520cb624f5..b6976a5f9ec 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 @@ -294,15 +294,6 @@ class AbstractFetchDownloadServiceTest { @Test fun `broadcastReceiver handles ACTION_CANCEL`() = runBlocking { - val service = spy(object : AbstractFetchDownloadService() { - override val httpClient = client - override fun removeDownloadJob(downloadJobState: DownloadJobState) = Unit - }) - - doReturn(broadcastManager).`when`(service).broadcastManager - doReturn(testContext).`when`(service).context - doNothing().`when`(service).useFileStream(any(), anyBoolean(), any()) - val download = DownloadState("https://example.com/file.txt", "file.txt") val response = Response( "https://example.com/file.txt", @@ -702,8 +693,6 @@ class AbstractFetchDownloadServiceTest { service.removeDownloadJob(downloadJobState = downloadState) assertTrue(service.downloadJobs.isEmpty()) - verify(service).stopForeground(false) - verify(service).clearAllDownloadsNotificationsAndJobs() verify(service).stopSelf() verify(service, times(0)).updateForegroundNotificationIfNeeded(downloadState) } @@ -993,8 +982,7 @@ class AbstractFetchDownloadServiceTest { // Now simulate onTaskRemoved. service.onTaskRemoved(null) - // Assert that all currently shown notifications are gone. - assertEquals(0, shadowNotificationService.size()) + verify(service).stopSelf() } @Test @@ -1030,7 +1018,7 @@ class AbstractFetchDownloadServiceTest { } @Test - fun `onTaskRemoved and onDestroy will remove all download notifications and jobs`() = runBlocking { + fun `onDestroy will remove all download notifications, jobs and will call unregisterNotificationActionsReceiver`() = runBlocking { val service = spy(object : AbstractFetchDownloadService() { override val httpClient = client }) @@ -1038,14 +1026,11 @@ class AbstractFetchDownloadServiceTest { doReturn(testContext).`when`(service).context service.registerNotificationActionsReceiver() - service.onTaskRemoved(null) - - service.registerNotificationActionsReceiver() - verify(service).clearAllDownloadsNotificationsAndJobs() service.onDestroy() - verify(service, times(2)).clearAllDownloadsNotificationsAndJobs() + verify(service).clearAllDownloadsNotificationsAndJobs() + verify(service).unregisterNotificationActionsReceiver() } @Test 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 3fc0d0b145a..11a99b77578 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 @@ -105,7 +105,7 @@ class DownloadNotificationTest { downloadDeleted = false ) - val summary = DownloadNotification.getDownloadSummary(testContext, listOf(download1, download2)) - assertEquals("mozilla.txt 10%\nmozilla2.txt 20%", summary) + val summary = DownloadNotification.getSummaryList(testContext, listOf(download1, download2)) + assertEquals(listOf("mozilla.txt 10%", "mozilla2.txt 20%"), summary) } } \ No newline at end of file