Merge "Fix race condition in NotificationStatsLogger to avoid dupe logs" into main

3 files changed, 119 insertions, 69 deletions

diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt
index 12d122b2fe1d..80e9e36862dd 100644
--- a/packages/SystemUI/multivalentTests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt
+++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt
@@ -170,6 +170,41 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
         }
 
     @Test
+    fun onNotificationVisibilityChanged_thenShadeNotInteractive_noDuplicateLogs() =
+        testScope.runTest {
+            // GIVEN a visible Notifications is reported
+            val (ranks, locations) = fakeNotificationMaps("key0")
+            val callable = Callable { locations }
+            underTest.onNotificationLocationsChanged(callable, ranks)
+            runCurrent()
+            clearInvocations(mockStatusBarService, mockNotificationListenerService)
+
+            // WHEN the same Notification becomins invisible
+            val emptyCallable = Callable { emptyMap<String, Int>() }
+            underTest.onNotificationLocationsChanged(emptyCallable, ranks)
+            // AND notifications become non interactible
+            underTest.onLockscreenOrShadeNotInteractive(emptyList())
+            runCurrent()
+
+            // THEN visibility changes are reported
+            verify(mockStatusBarService)
+                .onNotificationVisibilityChanged(eq(emptyArray()), visibilityArrayCaptor.capture())
+            val noLongerVisible = visibilityArrayCaptor.value
+            assertThat(noLongerVisible).hasLength(1)
+            assertThat(noLongerVisible[0]).apply {
+                isKeyEqualTo("key0")
+                isRankEqualTo(0)
+                notVisible()
+                isInMainArea()
+                isCountEqualTo(1)
+            }
+
+            // AND nothing else is logged
+            verifyNoMoreInteractions(mockStatusBarService)
+            verifyNoMoreInteractions(mockNotificationListenerService)
+        }
+
+    @Test
     fun onNotificationListUpdated_itemsChangedPositions_nothingLogged() =
         testScope.runTest {
             // GIVEN some visible Notifications are reported
@@ -253,14 +288,14 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     activeNotificationModel(
                         key = "key0",
                         uid = 0,
-                        packageName = "com.android.first"
+                        packageName = "com.android.first",
                     ),
                     activeNotificationModel(
                         key = "key1",
                         uid = 1,
-                        packageName = "com.android.second"
+                        packageName = "com.android.second",
                     ),
-                )
+                ),
             )
             runCurrent()
 
@@ -286,7 +321,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
 
@@ -296,7 +331,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     /* key = */ "key",
                     /* userAction = */ true,
                     /* expanded = */ true,
-                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal
+                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal,
                 )
         }
 
@@ -308,7 +343,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
             clearInvocations(mockStatusBarService)
@@ -318,7 +353,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
 
@@ -334,7 +369,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_BOTTOM_STACK_HIDDEN,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
 
@@ -350,7 +385,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_GONE,
-                isUserAction = false
+                isUserAction = false,
             )
             runCurrent()
 
@@ -366,7 +401,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     /* key = */ "key",
                     /* userAction = */ false,
                     /* expanded = */ true,
-                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal
+                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal,
                 )
         }
 
@@ -378,7 +413,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_GONE,
-                isUserAction = false
+                isUserAction = false,
             )
             runCurrent()
             // AND we open the shade, so we log its events
@@ -404,7 +439,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     /* key = */ "key",
                     /* userAction = */ false,
                     /* expanded = */ true,
-                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal
+                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal,
                 )
         }
 
@@ -416,7 +451,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = false,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = false
+                isUserAction = false,
             )
             runCurrent()
 
@@ -433,7 +468,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
 
@@ -443,7 +478,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     /* key = */ "key",
                     /* userAction = */ true,
                     /* expanded = */ true,
-                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal
+                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal,
                 )
 
             // AND the Notification is expanded again
@@ -451,7 +486,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key",
                 isExpanded = false,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
 
@@ -461,7 +496,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                     /* key = */ "key",
                     /* userAction = */ true,
                     /* expanded = */ false,
-                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal
+                    NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal,
                 )
         }
 
@@ -473,14 +508,14 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key1",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
             underTest.onNotificationExpansionChanged(
                 key = "key2",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
             clearInvocations(mockStatusBarService)
@@ -500,14 +535,14 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
                 key = "key1",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
             underTest.onNotificationExpansionChanged(
                 key = "key2",
                 isExpanded = true,
                 location = ExpandableViewState.LOCATION_MAIN_AREA,
-                isUserAction = true
+                isUserAction = true,
             )
             runCurrent()
             clearInvocations(mockStatusBarService)
@@ -535,10 +570,15 @@ class NotificationStatsLoggerTest : SysuiTestCase() {
 
 private class NotificationVisibilitySubject(private val visibility: NotificationVisibility) {
     fun isKeyEqualTo(key: String) = assertThat(visibility.key).isEqualTo(key)
+
     fun isRankEqualTo(rank: Int) = assertThat(visibility.rank).isEqualTo(rank)
+
     fun isCountEqualTo(count: Int) = assertThat(visibility.count).isEqualTo(count)
+
     fun isVisible() = assertThat(this.visibility.visible).isTrue()
+
     fun notVisible() = assertThat(this.visibility.visible).isFalse()
+
     fun isInMainArea() =
         assertThat(this.visibility.location)
             .isEqualTo(NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA)
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt
index c8c798d00a06..5689230f6bed 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt
@@ -19,6 +19,7 @@ package com.android.systemui.statusbar.notification.stack.ui.view
 import android.service.notification.NotificationListenerService
 import androidx.annotation.VisibleForTesting
 import com.android.app.tracing.coroutines.TrackTracer
+import com.android.app.tracing.coroutines.launchTraced as launch
 import com.android.internal.statusbar.IStatusBarService
 import com.android.internal.statusbar.NotificationVisibility
 import com.android.systemui.dagger.SysUISingleton
@@ -33,8 +34,9 @@ import java.util.concurrent.ConcurrentHashMap
 import javax.inject.Inject
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.Job
-import com.android.app.tracing.coroutines.launchTraced as launch
+import kotlinx.coroutines.channels.BufferOverflow
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.consumeEach
 import kotlinx.coroutines.withContext
 
 @VisibleForTesting const val UNKNOWN_RANK = -1
@@ -49,32 +51,56 @@ constructor(
     private val notificationPanelLogger: NotificationPanelLogger,
     private val statusBarService: IStatusBarService,
 ) : NotificationStatsLogger {
-    private val lastLoggedVisibilities = mutableMapOf<String, VisibilityState>()
-    private var logVisibilitiesJob: Job? = null
-
     private val expansionStates: MutableMap<String, ExpansionState> =
         ConcurrentHashMap<String, ExpansionState>()
     @VisibleForTesting
     val lastReportedExpansionValues: MutableMap<String, Boolean> =
         ConcurrentHashMap<String, Boolean>()
 
+    private val visibilityLogger =
+        Channel<VisibilityAction>(capacity = 2, onBufferOverflow = BufferOverflow.DROP_OLDEST)
+
+    init {
+        applicationScope.launch { consumeVisibilityActions() }
+    }
+
+    private suspend fun consumeVisibilityActions() {
+        val lastLoggedVisibilities = mutableMapOf<String, VisibilityState>()
+
+        visibilityLogger.consumeEach { action ->
+            val newVisibilities =
+                when (action) {
+                    is VisibilityAction.Change -> action.visibilities
+                    is VisibilityAction.Clear -> emptyMap()
+                }
+
+            val newlyVisible = newVisibilities - lastLoggedVisibilities.keys
+            val noLongerVisible = lastLoggedVisibilities - newVisibilities.keys
+
+            maybeLogVisibilityChanges(newlyVisible, noLongerVisible, action.activeCount)
+            updateExpansionStates(newlyVisible, noLongerVisible)
+            TrackTracer.instantForGroup("Notifications", "Active", action.activeCount)
+            TrackTracer.instantForGroup("Notifications", "Visible", newVisibilities.size)
+
+            lastLoggedVisibilities.clear()
+            lastLoggedVisibilities.putAll(newVisibilities)
+        }
+    }
+
     override fun onNotificationLocationsChanged(
         locationsProvider: Callable<Map<String, Int>>,
         notificationRanks: Map<String, Int>,
     ) {
-        if (logVisibilitiesJob?.isActive == true) {
-            return
-        }
-
-        logVisibilitiesJob =
-            startLogVisibilitiesJob(
-                newVisibilities =
+        visibilityLogger.trySend(
+            VisibilityAction.Change(
+                visibilities =
                     combine(
                         visibilities = locationsProvider.call(),
-                        rankingsMap = notificationRanks
+                        rankingsMap = notificationRanks,
                     ),
-                activeNotifCount = notificationRanks.size,
+                activeCount = notificationRanks.size,
             )
+        )
     }
 
     override fun onNotificationExpansionChanged(
@@ -125,7 +151,7 @@ constructor(
                     /* expanded = */ expansionState.isExpanded,
                     /* notificationLocation = */ expansionState.location
                         .toNotificationLocation()
-                        .ordinal
+                        .ordinal,
                 )
             }
         }
@@ -138,7 +164,7 @@ constructor(
             withContext(bgDispatcher) {
                 notificationPanelLogger.logPanelShown(
                     isOnLockScreen,
-                    activeNotifications.toNotificationProto()
+                    activeNotifications.toNotificationProto(),
                 )
             }
         }
@@ -147,11 +173,7 @@ constructor(
     override fun onLockscreenOrShadeNotInteractive(
         activeNotifications: List<ActiveNotificationModel>
     ) {
-        logVisibilitiesJob =
-            startLogVisibilitiesJob(
-                newVisibilities = emptyMap(),
-                activeNotifCount = activeNotifications.size
-            )
+        visibilityLogger.trySend(VisibilityAction.Clear(activeCount = activeNotifications.size))
     }
 
     override fun onNotificationRemoved(key: String) {
@@ -167,29 +189,12 @@ constructor(
 
     private fun combine(
         visibilities: Map<String, Int>,
-        rankingsMap: Map<String, Int>
+        rankingsMap: Map<String, Int>,
     ): Map<String, VisibilityState> =
         visibilities.mapValues { entry ->
             VisibilityState(entry.key, entry.value, rankingsMap[entry.key] ?: UNKNOWN_RANK)
         }
 
-    private fun startLogVisibilitiesJob(
-        newVisibilities: Map<String, VisibilityState>,
-        activeNotifCount: Int,
-    ) =
-        applicationScope.launch {
-            val newlyVisible = newVisibilities - lastLoggedVisibilities.keys
-            val noLongerVisible = lastLoggedVisibilities - newVisibilities.keys
-
-            maybeLogVisibilityChanges(newlyVisible, noLongerVisible, activeNotifCount)
-            updateExpansionStates(newlyVisible, noLongerVisible)
-            TrackTracer.instantForGroup("Notifications", "Active", activeNotifCount)
-            TrackTracer.instantForGroup("Notifications", "Visible", newVisibilities.size)
-
-            lastLoggedVisibilities.clear()
-            lastLoggedVisibilities.putAll(newVisibilities)
-        }
-
     private suspend fun maybeLogVisibilityChanges(
         newlyVisible: Map<String, VisibilityState>,
         noLongerVisible: Map<String, VisibilityState>,
@@ -205,7 +210,7 @@ constructor(
         val noLongerVisibleAr =
             noLongerVisible.mapToNotificationVisibilitiesAr(
                 visible = false,
-                count = activeNotifCount
+                count = activeNotifCount,
             )
 
         withContext(bgDispatcher) {
@@ -218,7 +223,7 @@ constructor(
 
     private fun updateExpansionStates(
         newlyVisible: Map<String, VisibilityState>,
-        noLongerVisible: Map<String, VisibilityState>
+        noLongerVisible: Map<String, VisibilityState>,
     ) {
         expansionStates.forEach { (key, expansionState) ->
             if (newlyVisible.contains(key)) {
@@ -241,11 +246,16 @@ constructor(
         }
     }
 
-    private data class VisibilityState(
-        val key: String,
-        val location: Int,
-        val rank: Int,
-    )
+    private sealed class VisibilityAction(open val activeCount: Int) {
+        data class Change(
+            val visibilities: Map<String, VisibilityState>,
+            override val activeCount: Int,
+        ) : VisibilityAction(activeCount)
+
+        data class Clear(override val activeCount: Int) : VisibilityAction(activeCount)
+    }
+
+    private data class VisibilityState(val key: String, val location: Int, val rank: Int)
 
     private data class ExpansionState(
         val key: String,
@@ -278,7 +288,7 @@ constructor(
                     /* rank = */ state.rank,
                     /* count = */ count,
                     /* visible = */ visible,
-                    /* location = */ state.location.toNotificationLocation()
+                    /* location = */ state.location.toNotificationLocation(),
                 )
             }
             .toTypedArray()
diff --git a/packages/SystemUI/tests/utils/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerKosmos.kt b/packages/SystemUI/tests/utils/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerKosmos.kt
index de52155dce79..f91c2f620bb1 100644
--- a/packages/SystemUI/tests/utils/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerKosmos.kt
+++ b/packages/SystemUI/tests/utils/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerKosmos.kt
@@ -26,7 +26,7 @@ import com.android.systemui.statusbar.notification.logging.notificationPanelLogg
 
 val Kosmos.notificationStatsLogger by Fixture {
     NotificationStatsLoggerImpl(
-        applicationScope = testScope,
+        applicationScope = testScope.backgroundScope,
         bgDispatcher = testDispatcher,
         statusBarService = statusBarService,
         notificationListenerService = notificationListenerService,