Merge "Reduce log spam when RankingMap is missing rankings for non-cancelled entries." into tm-qpr-dev

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java

@@ -89,6 +89,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.In
 import com.android.systemui.statusbar.notification.collection.notifcollection.InternalNotifUpdater;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLogger;
+import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLoggerKt;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifDismissInterceptor;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifEvent;
 import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender;
@@ -110,6 +111,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Queue;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
@@ -157,6 +159,8 @@ public class NotifCollection implements Dumpable {
     private final List<NotifLifetimeExtender> mLifetimeExtenders = new ArrayList<>();
     private final List<NotifDismissInterceptor> mDismissInterceptors = new ArrayList<>();
 
+    private Set<String> mNotificationsWithoutRankings = Collections.emptySet();
+
     private Queue<NotifEvent> mEventQueue = new ArrayDeque<>();
 
     private boolean mAttached = false;
@@ -560,6 +564,7 @@ public class NotifCollection implements Dumpable {
     }
 
     private void applyRanking(@NonNull RankingMap rankingMap) {
+        ArrayMap<String, NotificationEntry> currentEntriesWithoutRankings = null;
         for (NotificationEntry entry : mNotificationSet.values()) {
             if (!isCanceled(entry)) {
 
@@ -581,10 +586,21 @@ public class NotifCollection implements Dumpable {
                         }
                     }
                 } else {
-                    mLogger.logRankingMissing(entry, rankingMap);
+                    if (currentEntriesWithoutRankings == null) {
+                        currentEntriesWithoutRankings = new ArrayMap<>();
+                    }
+                    currentEntriesWithoutRankings.put(entry.getKey(), entry);
                 }
             }
         }
+        NotifCollectionLoggerKt.maybeLogInconsistentRankings(
+                mLogger,
+                mNotificationsWithoutRankings,
+                currentEntriesWithoutRankings,
+                rankingMap
+        );
+        mNotificationsWithoutRankings = currentEntriesWithoutRankings == null
+                ? Collections.emptySet() : currentEntriesWithoutRankings.keySet();
         mEventQueue.add(new RankingAppliedEvent());
     }
 
@@ -836,6 +852,11 @@ public class NotifCollection implements Dumpable {
                         entries,
                         true,
                         "\t\t"));
+
+        pw.println("\n\tmNotificationsWithoutRankings: " + mNotificationsWithoutRankings.size());
+        for (String key : mNotificationsWithoutRankings) {
+            pw.println("\t * : " + key);
+        }
     }
 
     private final BatchableNotificationHandler mNotifHandler = new BatchableNotificationHandler() {

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt

@@ -196,16 +196,32 @@ class NotifCollectionLogger @Inject constructor(
         })
     }
 
-    fun logRankingMissing(entry: NotificationEntry, rankingMap: RankingMap) {
+    fun logMissingRankings(
+        newlyInconsistentEntries: List<NotificationEntry>,
+        totalInconsistent: Int,
+        rankingMap: RankingMap
+    ) {
         buffer.log(TAG, WARNING, {
-            str1 = entry.logKey
+            int1 = totalInconsistent
+            int2 = newlyInconsistentEntries.size
+            str1 = newlyInconsistentEntries.joinToString { it.logKey ?: "null" }
         }, {
-            "Ranking update is missing ranking for $str1"
+            "Ranking update is missing ranking for $int1 entries ($int2 new): $str1"
+        })
+        buffer.log(TAG, DEBUG, {
+            str1 = rankingMap.orderedKeys.map { logKey(it) ?: "null" }.toString()
+        }, {
+            "Ranking map contents: $str1"
         })
-        buffer.log(TAG, DEBUG, {}, { "Ranking map contents:" })
-        for (entry in rankingMap.orderedKeys) {
-            buffer.log(TAG, DEBUG, { str1 = logKey(entry) }, { "  $str1" })
     }
+
+    fun logRecoveredRankings(newlyConsistentKeys: List<String>) {
+        buffer.log(TAG, INFO, {
+            int1 = newlyConsistentKeys.size
+            str1 = newlyConsistentKeys.joinToString { logKey(it) ?: "null" }
+        }, {
+            "Ranking update now contains rankings for $int1 previously inconsistent entries: $str1"
+        })
     }
 
     fun logRemoteExceptionOnNotificationClear(entry: NotificationEntry, e: RemoteException) {
@@ -346,4 +362,29 @@ class NotifCollectionLogger @Inject constructor(
     }
 }
 
+fun maybeLogInconsistentRankings(
+    logger: NotifCollectionLogger,
+    oldKeysWithoutRankings: Set<String>,
+    newEntriesWithoutRankings: Map<String, NotificationEntry>?,
+    rankingMap: RankingMap
+) {
+    if (oldKeysWithoutRankings.isEmpty() && newEntriesWithoutRankings == null) return
+    val newlyConsistent: List<String> = oldKeysWithoutRankings
+        .mapNotNull { key ->
+            key.takeIf { key !in (newEntriesWithoutRankings ?: emptyMap()) }
+                .takeIf { key in rankingMap.orderedKeys }
+        }.sorted()
+    if (newlyConsistent.isNotEmpty()) {
+        logger.logRecoveredRankings(newlyConsistent)
+    }
+    val newlyInconsistent: List<NotificationEntry> = newEntriesWithoutRankings
+        ?.mapNotNull { (key, entry) ->
+            entry.takeIf { key !in oldKeysWithoutRankings }
+        }?.sortedBy { it.key } ?: emptyList()
+    if (newlyInconsistent.isNotEmpty()) {
+        val totalInconsistent: Int = newEntriesWithoutRankings?.size ?: 0
+        logger.logMissingRankings(newlyInconsistent, totalInconsistent, rankingMap)
+    }
+}
+
 private const val TAG = "NotifCollection"

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLoggerTest.kt

@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.android.systemui.statusbar.notification.collection.notifcollection
+
+import android.service.notification.NotificationListenerService.RankingMap
+import android.testing.AndroidTestingRunner
+import android.testing.TestableLooper.RunWithLooper
+import androidx.test.filters.SmallTest
+import com.android.systemui.SysuiTestCase
+import com.android.systemui.statusbar.notification.collection.NotificationEntry
+import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder
+import com.android.systemui.util.mockito.eq
+import com.android.systemui.util.mockito.mock
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.Mockito.verify
+import org.mockito.Mockito.verifyNoMoreInteractions
+
+@SmallTest
+@RunWith(AndroidTestingRunner::class)
+@RunWithLooper
+class NotifCollectionLoggerTest : SysuiTestCase() {
+    private val logger: NotifCollectionLogger = mock()
+    private val entry1: NotificationEntry = NotificationEntryBuilder().setId(1).build()
+    private val entry2: NotificationEntry = NotificationEntryBuilder().setId(2).build()
+
+    private fun mapOfEntries(vararg entries: NotificationEntry): Map<String, NotificationEntry> =
+        entries.associateBy { it.key }
+
+    private fun rankingMapOf(vararg entries: NotificationEntry): RankingMap =
+        RankingMap(entries.map { it.ranking }.toTypedArray())
+
+    @Test
+    fun testMaybeLogInconsistentRankings_logsNewlyInconsistentRanking() {
+        val rankingMap = rankingMapOf(entry1)
+        maybeLogInconsistentRankings(
+            logger = logger,
+            oldKeysWithoutRankings = emptySet(),
+            newEntriesWithoutRankings = mapOfEntries(entry2),
+            rankingMap = rankingMap
+        )
+        verify(logger).logMissingRankings(
+            newlyInconsistentEntries = eq(listOf(entry2)),
+            totalInconsistent = eq(1),
+            rankingMap = eq(rankingMap),
+        )
+        verifyNoMoreInteractions(logger)
+    }
+
+    @Test
+    fun testMaybeLogInconsistentRankings_doesNotLogAlreadyInconsistentRanking() {
+        maybeLogInconsistentRankings(
+            logger = logger,
+            oldKeysWithoutRankings = setOf(entry2.key),
+            newEntriesWithoutRankings = mapOfEntries(entry2),
+            rankingMap = rankingMapOf(entry1)
+        )
+        verifyNoMoreInteractions(logger)
+    }
+
+    @Test
+    fun testMaybeLogInconsistentRankings_logsWhenRankingIsAdded() {
+        maybeLogInconsistentRankings(
+            logger = logger,
+            oldKeysWithoutRankings = setOf(entry2.key),
+            newEntriesWithoutRankings = mapOfEntries(),
+            rankingMap = rankingMapOf(entry1, entry2)
+        )
+        verify(logger).logRecoveredRankings(
+            newlyConsistentKeys = eq(listOf(entry2.key)),
+        )
+        verifyNoMoreInteractions(logger)
+    }
+
+    @Test
+    fun testMaybeLogInconsistentRankings_doesNotLogsWhenEntryIsRemoved() {
+        maybeLogInconsistentRankings(
+            logger = logger,
+            oldKeysWithoutRankings = setOf(entry2.key),
+            newEntriesWithoutRankings = mapOfEntries(),
+            rankingMap = rankingMapOf(entry1)
+        )
+        verifyNoMoreInteractions(logger)
+    }
+}