Add support for updating existing notifications

app/core/src/main/java/com/fsck/k9/notification/NewMailNotificationController.kt

@@ -27,7 +27,9 @@ internal class NewMailNotificationController(
     fun addNewMailNotification(account: Account, message: LocalMessage, silent: Boolean) {
         val notificationData = newMailNotificationManager.addNewMailNotification(account, message, silent)
 
-        processNewMailNotificationData(notificationData)
+        if (notificationData != null) {
+            processNewMailNotificationData(notificationData)
+        }
     }
 
     fun removeNewMailNotifications(

app/core/src/main/java/com/fsck/k9/notification/NewMailNotificationManager.kt

@@ -38,10 +38,10 @@ internal class NewMailNotificationManager(
         )
     }
 
-    fun addNewMailNotification(account: Account, message: LocalMessage, silent: Boolean): NewMailNotificationData {
+    fun addNewMailNotification(account: Account, message: LocalMessage, silent: Boolean): NewMailNotificationData? {
         val content = contentCreator.createFromMessage(account, message)
 
-        val result = notificationRepository.addNotification(account, content, timestamp = now())
+        val result = notificationRepository.addNotification(account, content, timestamp = now()) ?: return null
 
         val singleNotificationData = createSingleNotificationData(
             account = account,

app/core/src/main/java/com/fsck/k9/notification/NotificationDataStore.kt

@@ -2,7 +2,6 @@ package com.fsck.k9.notification
 
 import com.fsck.k9.Account
 import com.fsck.k9.controller.MessageReference
-import com.fsck.k9.core.BuildConfig
 
 internal const val MAX_NUMBER_OF_NEW_MESSAGE_NOTIFICATIONS = 8
 
@@ -30,15 +29,45 @@ internal class NotificationDataStore {
     }
 
     @Synchronized
-    fun addNotification(account: Account, content: NotificationContent, timestamp: Long): AddNotificationResult {
+    fun addNotification(account: Account, content: NotificationContent, timestamp: Long): AddNotificationResult? {
         val notificationData = getNotificationData(account)
         val messageReference = content.messageReference
 
-        if (BuildConfig.DEBUG && notificationData.contains(messageReference)) {
+        val activeNotification = notificationData.activeNotifications.firstOrNull { notificationHolder ->
-            throw AssertionError("Notification for message $messageReference already exists")
+            notificationHolder.content.messageReference == messageReference
+        }
+        val inactiveNotification = notificationData.inactiveNotifications.firstOrNull { inactiveNotificationHolder ->
+            inactiveNotificationHolder.content.messageReference == messageReference
         }
 
-        return if (notificationData.isMaxNumberOfActiveNotificationsReached) {
+        return if (activeNotification != null) {
+            val newActiveNotification = activeNotification.copy(content = content)
+            val notificationHolder = activeNotification.copy(
+                content = content
+            )
+
+            val operations = emptyList<NotificationStoreOperation>()
+
+            val newActiveNotifications = notificationData.activeNotifications
+                .replace(activeNotification, newActiveNotification)
+            val newNotificationData = notificationData.copy(
+                activeNotifications = newActiveNotifications
+            )
+            notificationDataMap[account.uuid] = newNotificationData
+
+            AddNotificationResult.newNotification(newNotificationData, operations, notificationHolder)
+        } else if (inactiveNotification != null) {
+            val newInactiveNotification = inactiveNotification.copy(content = content)
+            val newInactiveNotifications = notificationData.inactiveNotifications
+                .replace(inactiveNotification, newInactiveNotification)
+
+            val newNotificationData = notificationData.copy(
+                inactiveNotifications = newInactiveNotifications
+            )
+            notificationDataMap[account.uuid] = newNotificationData
+
+            null
+        } else if (notificationData.isMaxNumberOfActiveNotificationsReached) {
             val lastNotificationHolder = notificationData.activeNotifications.last()
             val inactiveNotificationHolder = lastNotificationHolder.toInactiveNotificationHolder()
 
@@ -175,14 +204,15 @@ internal class NotificationDataStore {
         throw AssertionError("getNewNotificationId() called with no free notification ID")
     }
 
-    private fun NotificationData.contains(messageReference: MessageReference): Boolean {
-        return activeNotifications.any { it.content.messageReference == messageReference } ||
-            inactiveNotifications.any { it.content.messageReference == messageReference }
-    }
-
     private fun NotificationHolder.toInactiveNotificationHolder() = InactiveNotificationHolder(timestamp, content)
 
     private fun InactiveNotificationHolder.toNotificationHolder(notificationId: Int): NotificationHolder {
         return NotificationHolder(notificationId, timestamp, content)
     }
+
+    private fun <T> List<T>.replace(old: T, new: T): List<T> {
+        return map { element ->
+            if (element === old) new else element
+        }
+    }
 }

app/core/src/main/java/com/fsck/k9/notification/NotificationRepository.kt

@@ -37,8 +37,8 @@ internal class NotificationRepository(
     }
 
     @Synchronized
-    fun addNotification(account: Account, content: NotificationContent, timestamp: Long): AddNotificationResult {
+    fun addNotification(account: Account, content: NotificationContent, timestamp: Long): AddNotificationResult? {
-        return notificationDataStore.addNotification(account, content, timestamp).also { result ->
+        return notificationDataStore.addNotification(account, content, timestamp)?.also { result ->
             persistNotificationDataStoreChanges(
                 account = account,
                 operations = result.notificationStoreOperations,

app/core/src/test/java/com/fsck/k9/notification/NewMailNotificationManagerTest.kt

@@ -49,6 +49,7 @@ class NewMailNotificationManagerTest {
 
         val result = manager.addNewMailNotification(account, message, silent = false)
 
+        assertNotNull(result)
         assertThat(result.singleNotificationData.first().content).isEqualTo(
             NotificationContent(
                 messageReference = createMessageReference("msg-1"),
@@ -85,6 +86,7 @@ class NewMailNotificationManagerTest {
 
         val result = manager.addNewMailNotification(account, messageTwo, silent = false)
 
+        assertNotNull(result)
         assertThat(result.singleNotificationData.first().content).isEqualTo(
             NotificationContent(
                 messageReference = createMessageReference("msg-2"),
@@ -121,6 +123,7 @@ class NewMailNotificationManagerTest {
 
         val result = manager.addNewMailNotification(account, message, silent = false)
 
+        assertNotNull(result)
         val notificationId = NotificationIds.getSingleMessageNotificationId(account, index = 0)
         assertThat(result.cancelNotificationIds).isEqualTo(listOf(notificationId))
         assertThat(result.singleNotificationData.first().notificationId).isEqualTo(notificationId)
@@ -196,6 +199,7 @@ class NewMailNotificationManagerTest {
             messageUid = "msg-2"
         )
         val dataTwo = manager.addNewMailNotification(account, messageTwo, silent = true)
+        assertNotNull(dataTwo)
         val notificationIdTwo = dataTwo.singleNotificationData.first().notificationId
         val messageThree = addMessageToNotificationContentCreator(
             sender = "Alice",

app/core/src/test/java/com/fsck/k9/notification/NotificationDataStoreTest.kt

@@ -3,12 +3,8 @@ package com.fsck.k9.notification
 import com.fsck.k9.Account
 import com.fsck.k9.RobolectricTest
 import com.fsck.k9.controller.MessageReference
-import com.fsck.k9.core.BuildConfig
 import com.google.common.truth.Truth.assertThat
 import kotlin.test.assertNotNull
-import org.junit.Assert.fail
-import org.junit.Assume.assumeFalse
-import org.junit.Assume.assumeTrue
 import org.junit.Test
 
 private const val ACCOUNT_UUID = "1-2-3"
@@ -26,6 +22,7 @@ class NotificationDataStoreTest : RobolectricTest() {
 
         val result = notificationDataStore.addNotification(account, content, TIMESTAMP)
 
+        assertNotNull(result)
         assertThat(result.shouldCancelNotification).isFalse()
 
         val holder = result.notificationHolder
@@ -48,6 +45,7 @@ class NotificationDataStoreTest : RobolectricTest() {
 
         val result = notificationDataStore.addNotification(account, createNotificationContent("9"), TIMESTAMP)
 
+        assertNotNull(result)
         assertThat(result.shouldCancelNotification).isTrue()
         assertThat(result.cancelNotificationId).isEqualTo(NotificationIds.getSingleMessageNotificationId(account, 0))
     }
@@ -107,19 +105,23 @@ class NotificationDataStoreTest : RobolectricTest() {
     fun testNewMessagesCount() {
         val contentOne = createNotificationContent("1")
         val resultOne = notificationDataStore.addNotification(account, contentOne, TIMESTAMP)
+        assertNotNull(resultOne)
         assertThat(resultOne.notificationData.newMessagesCount).isEqualTo(1)
 
         val contentTwo = createNotificationContent("2")
         val resultTwo = notificationDataStore.addNotification(account, contentTwo, TIMESTAMP)
+        assertNotNull(resultTwo)
         assertThat(resultTwo.notificationData.newMessagesCount).isEqualTo(2)
     }
 
     @Test
     fun testIsSingleMessageNotification() {
         val resultOne = notificationDataStore.addNotification(account, createNotificationContent("1"), TIMESTAMP)
+        assertNotNull(resultOne)
         assertThat(resultOne.notificationData.isSingleMessageNotification).isTrue()
 
         val resultTwo = notificationDataStore.addNotification(account, createNotificationContent("2"), TIMESTAMP)
+        assertNotNull(resultTwo)
         assertThat(resultTwo.notificationData.isSingleMessageNotification).isFalse()
     }
 
@@ -128,37 +130,58 @@ class NotificationDataStoreTest : RobolectricTest() {
         val content = createNotificationContent("1")
         val addResult = notificationDataStore.addNotification(account, content, TIMESTAMP)
 
+        assertNotNull(addResult)
         assertThat(addResult.notificationData.activeNotifications.first()).isEqualTo(addResult.notificationHolder)
     }
 
     @Test
-    fun `adding notification for same message twice should throw in debug build`() {
+    fun `adding notification for message with active notification should update notification`() {
-        assumeTrue(BuildConfig.DEBUG)
-
         val content1 = createNotificationContent("1")
         val content2 = createNotificationContent("1")
-        notificationDataStore.addNotification(account, content1, TIMESTAMP)
 
-        try {
+        val resultOne = notificationDataStore.addNotification(account, content1, TIMESTAMP)
-            notificationDataStore.addNotification(account, content2, TIMESTAMP)
+        val resultTwo = notificationDataStore.addNotification(account, content2, TIMESTAMP)
-            fail("Exception expected")
+
-        } catch (e: AssertionError) {
+        assertNotNull(resultOne)
-            assertThat(e).hasMessageThat().matches("Notification for message .+ already exists")
+        assertNotNull(resultTwo)
+        assertThat(resultTwo.notificationData.activeNotifications).hasSize(1)
+        assertThat(resultTwo.notificationData.activeNotifications.first().content).isSameInstanceAs(content2)
+        assertThat(resultTwo.notificationStoreOperations).isEmpty()
+        with(resultTwo.notificationHolder) {
+            assertThat(notificationId).isEqualTo(resultOne.notificationHolder.notificationId)
+            assertThat(timestamp).isEqualTo(resultOne.notificationHolder.timestamp)
+            assertThat(content).isSameInstanceAs(content2)
         }
+        assertThat(resultTwo.shouldCancelNotification).isFalse()
     }
 
     @Test
-    fun `adding notification for same message twice should add another notification in release build`() {
+    fun `adding notification for message with inactive notification should update notificationData`() {
-        assumeFalse(BuildConfig.DEBUG)
+        notificationDataStore.addNotification(account, createNotificationContent("1"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("2"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("3"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("4"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("5"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("6"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("7"), TIMESTAMP)
+        notificationDataStore.addNotification(account, createNotificationContent("8"), TIMESTAMP)
+        val latestNotificationContent = createNotificationContent("9")
+        notificationDataStore.addNotification(account, latestNotificationContent, TIMESTAMP)
+        val content = createNotificationContent("1")
 
-        val content1 = createNotificationContent("1")
+        val resultOne = notificationDataStore.addNotification(account, content, TIMESTAMP)
-        val content2 = createNotificationContent("1")
 
-        val addResult1 = notificationDataStore.addNotification(account, content1, TIMESTAMP)
+        assertThat(resultOne).isNull()
-        val addResult2 = notificationDataStore.addNotification(account, content2, TIMESTAMP)
 
-        assertThat(addResult1.notificationHolder.notificationId)
+        val resultTwo = notificationDataStore.removeNotifications(account) {
-            .isNotEqualTo(addResult2.notificationHolder.notificationId)
+            listOf(latestNotificationContent.messageReference)
+        }
+
+        assertNotNull(resultTwo)
+        val notificationHolder = resultTwo.notificationData.activeNotifications.first { notificationHolder ->
+            notificationHolder.content.messageReference == content.messageReference
+        }
+        assertThat(notificationHolder.content).isSameInstanceAs(content)
     }
 
     private fun createAccount(): Account {