Throttle all notify/cancel notification mgr calls

app/src/main/java/org/tasks/notifications/NotificationLimiter.java

@@ -29,6 +29,12 @@ class NotificationLimiter {
     return evicted;
   }
 
+  synchronized void remove(Iterable<Long> ids) {
+    for (Long id : ids) {
+      remove(id);
+    }
+  }
+
   synchronized void remove(long id) {
     if (id == NotificationManager.SUMMARY_NOTIFICATION_ID) {
       summary = false;

app/src/main/java/org/tasks/notifications/NotificationManager.kt

@@ -5,7 +5,6 @@ import android.app.PendingIntent
 import android.content.Context
 import android.content.Intent
 import androidx.core.app.NotificationCompat
-import androidx.core.app.NotificationManagerCompat
 import com.todoroo.andlib.utility.AndroidUtilities
 import com.todoroo.andlib.utility.DateUtilities
 import com.todoroo.astrid.reminders.ReminderService
@@ -38,10 +37,9 @@ class NotificationManager @Inject constructor(
         private val taskDao: TaskDao,
         private val locationDao: LocationDao,
         private val localBroadcastManager: LocalBroadcastManager,
-        private val reminderService: ReminderService) {
-    private val notificationManagerCompat = NotificationManagerCompat.from(context)
+        private val reminderService: ReminderService,
+        private val notificationManager: ThrottledNotificationManager) {
     private val colorProvider = ColorProvider(context, preferences)
-    private val throttle = Throttle(NOTIFICATIONS_PER_SECOND, tag = "NOTIFY")
     private val queue = NotificationLimiter(MAX_NOTIFICATIONS)
 
     @SuppressLint("CheckResult")
@@ -56,11 +54,9 @@ class NotificationManager @Inject constructor(
     @SuppressLint("CheckResult")
     suspend fun cancel(ids: Iterable<Long>) {
         for (id in ids) {
-            throttle.run {
-                notificationManagerCompat.cancel(id.toInt())
-                queue.remove(id)
-            }
+            notificationManager.cancel(id.toInt())
         }
+        queue.remove(ids)
         notificationDao.deleteAll(ids.toList())
         notifyTasks(emptyList(), alert = false, nonstop = false, fiveTimes = false)
     }
@@ -69,7 +65,7 @@ class NotificationManager @Inject constructor(
         val notifications = notificationDao.getAllOrdered()
         if (cancelExisting) {
             for (notification in notifications) {
-                notificationManagerCompat.cancel(notification.taskId.toInt())
+                notificationManager.cancel(notification.taskId.toInt())
             }
         }
         if (preferences.bundleNotifications() && notifications.size > 1) {
@@ -150,7 +146,7 @@ class NotificationManager @Inject constructor(
         for (notification in notifications) {
             val builder = getTaskNotification(notification)
             if (builder == null) {
-                notificationManagerCompat.cancel(notification.taskId.toInt())
+                notificationManager.cancel(notification.taskId.toInt())
                 notificationDao.delete(notification.taskId)
             } else {
                 builder
@@ -206,7 +202,7 @@ class NotificationManager @Inject constructor(
             cancel(evicted)
         }
         for (i in 0 until ringTimes) {
-            throttle.run { notificationManagerCompat.notify(notificationId.toInt(), notification) }
+            notificationManager.notify(notificationId.toInt(), notification)
         }
     }
 
@@ -365,7 +361,7 @@ class NotificationManager @Inject constructor(
     }
 
     private fun cancelSummaryNotification() {
-        notificationManagerCompat.cancel(SUMMARY_NOTIFICATION_ID)
+        notificationManager.cancel(SUMMARY_NOTIFICATION_ID)
     }
 
     companion object {
@@ -377,7 +373,5 @@ class NotificationManager @Inject constructor(
         const val EXTRA_NOTIFICATION_ID = "extra_notification_id"
         const val SUMMARY_NOTIFICATION_ID = 0
         private const val GROUP_KEY = "tasks"
-        private const val NOTIFICATIONS_PER_SECOND = 4
     }
-
 }

app/src/main/java/org/tasks/notifications/Throttle.kt

@@ -1,33 +1,37 @@
 package org.tasks.notifications
 
-import kotlinx.coroutines.*
+import kotlinx.coroutines.runBlocking
 import org.tasks.time.DateTimeUtils.currentTimeMillis
 import timber.log.Timber
+import java.util.concurrent.Executor
 import java.util.concurrent.Executors.newSingleThreadExecutor
 
 internal class Throttle constructor(
         ratePerPeriod: Int,
         private val periodMillis: Long = 1000,
         private val tag: String = "",
-        private val scope: CoroutineScope =
-                CoroutineScope(newSingleThreadExecutor().asCoroutineDispatcher() + SupervisorJob()),
-        private val sleeper: suspend (Long) -> Unit = { delay(it) }) {
+        private val executor: Executor = newSingleThreadExecutor(),
+        private val sleeper: (Long) -> Unit = { Thread.sleep(it) }) {
     private val throttle: LongArray = LongArray(ratePerPeriod)
     private var oldest = 0
 
     @Synchronized
-    fun run(runnable: suspend () -> Unit): Job = scope.launch {
-        val sleep = throttle[oldest] - (currentTimeMillis() - periodMillis)
-        if (sleep > 0) {
-            Timber.v("$tag: Throttled for ${sleep}ms")
-            sleeper.invoke(sleep)
+    fun run(runnable: suspend () -> Unit) {
+        executor.execute {
+            val sleep = throttle[oldest] - (currentTimeMillis() - periodMillis)
+            if (sleep > 0) {
+                Timber.v("$tag: Throttled for ${sleep}ms")
+                sleeper.invoke(sleep)
+            }
+            try {
+                runBlocking {
+                    runnable.invoke()
+                }
+            } catch (e: Exception) {
+                Timber.e(e)
+            }
+            throttle[oldest] = currentTimeMillis()
+            oldest = (oldest + 1) % throttle.size
         }
-        try {
-            runnable.invoke()
-        } catch (e: Exception) {
-            Timber.e(e)
-        }
-        throttle[oldest] = currentTimeMillis()
-        oldest = (oldest + 1) % throttle.size
     }
 }

app/src/main/java/org/tasks/notifications/ThrottledNotificationManager.kt

@@ -0,0 +1,32 @@
+package org.tasks.notifications
+
+import android.app.Notification
+import android.content.Context
+import androidx.core.app.NotificationManagerCompat
+import dagger.hilt.android.qualifiers.ApplicationContext
+import java.util.concurrent.Executors.newSingleThreadExecutor
+import javax.inject.Inject
+
+class ThrottledNotificationManager @Inject constructor(
+        @ApplicationContext val context: Context
+) {
+    private val notificationManagerCompat = NotificationManagerCompat.from(context)
+    private val executor = newSingleThreadExecutor()
+    private val throttle = Throttle(NOTIFICATIONS_PER_SECOND, executor = executor, tag = "NOTIFY")
+
+    fun cancel(id: Int) {
+        executor.execute {
+            notificationManagerCompat.cancel(id)
+        }
+    }
+
+    fun notify(id: Int, notification: Notification) {
+        throttle.run {
+            notificationManagerCompat.notify(id, notification)
+        }
+    }
+
+    companion object {
+        private const val NOTIFICATIONS_PER_SECOND = 4
+    }
+}

app/src/test/java/org/tasks/notifications/ThrottleTest.kt

@@ -1,12 +1,12 @@
 package org.tasks.notifications
 
+import androidx.work.impl.utils.SynchronousExecutor
 import kotlinx.coroutines.ExperimentalCoroutinesApi
-import kotlinx.coroutines.test.runBlockingTest
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
-import org.tasks.SuspendFreeze
+import org.tasks.Freeze
 import org.tasks.time.DateTimeUtils
 
 @ExperimentalCoroutinesApi
@@ -20,8 +20,8 @@ class ThrottleTest {
     }
 
     @Test
-    fun dontThrottle() = runBlockingTest {
-        throttle = Throttle(3, scope = this) { sleep.add(it) }
+    fun dontThrottle() {
+        throttle = Throttle(3, executor = SynchronousExecutor()) { sleep.add(it) }
         val now = DateTimeUtils.currentTimeMillis()
         runAt(now)
         runAt(now)
@@ -31,8 +31,8 @@ class ThrottleTest {
     }
 
     @Test
-    fun throttleForOneMillisecond() = runBlockingTest {
-        throttle = Throttle(3, scope = this) { sleep.add(it) }
+    fun throttleForOneMillisecond() {
+        throttle = Throttle(3, executor = SynchronousExecutor()) { sleep.add(it) }
         val now = DateTimeUtils.currentTimeMillis()
         runAt(now)
         runAt(now)
@@ -42,8 +42,8 @@ class ThrottleTest {
     }
 
     @Test
-    fun throttleForOneSecond() = runBlockingTest {
-        throttle = Throttle(3, scope = this) { sleep.add(it) }
+    fun throttleForOneSecond() {
+        throttle = Throttle(3, executor = SynchronousExecutor()) { sleep.add(it) }
         val now = DateTimeUtils.currentTimeMillis()
         runAt(now)
         runAt(now)
@@ -53,8 +53,8 @@ class ThrottleTest {
     }
 
     @Test
-    fun throttleMultiple() = runBlockingTest {
-        throttle = Throttle(3, scope = this) { sleep.add(it) }
+    fun throttleMultiple() {
+        throttle = Throttle(3, executor = SynchronousExecutor()) { sleep.add(it) }
         val now = DateTimeUtils.currentTimeMillis()
         runAt(now)
         runAt(now + 200)
@@ -65,7 +65,7 @@ class ThrottleTest {
         assertEquals(arrayListOf(300L, 450L), sleep)
     }
 
-    private suspend fun runAt(millis: Long) {
-        SuspendFreeze.freezeAt(millis) { throttle.run {} }
+    private fun runAt(millis: Long) {
+        Freeze.freezeAt(millis) { throttle.run {} }
     }
 }