Drop ranking update events when another one is already queued.
Fixes: 207737509
Test: atest NotificationListenerTest
Change-Id: I5c082de30b7b40fc72c14dd3087df0853eb62531
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
index 5437ce6..d6125ce 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java
@@ -25,7 +25,6 @@
import android.app.NotificationManager;
import android.content.ComponentName;
import android.content.Context;
-import android.os.Handler;
import android.os.RemoteException;
import android.os.UserHandle;
import android.service.notification.StatusBarNotification;
@@ -35,9 +34,13 @@
import com.android.systemui.statusbar.dagger.StatusBarModule;
import com.android.systemui.statusbar.phone.NotificationListenerWithPlugins;
import com.android.systemui.statusbar.phone.StatusBar;
+import com.android.systemui.util.time.SystemClock;
import java.util.ArrayList;
+import java.util.Deque;
import java.util.List;
+import java.util.concurrent.ConcurrentLinkedDeque;
+import java.util.concurrent.Executor;
/**
* This class handles listening to notification updates and passing them along to
@@ -47,23 +50,31 @@
public class NotificationListener extends NotificationListenerWithPlugins {
private static final String TAG = "NotificationListener";
private static final boolean DEBUG = StatusBar.DEBUG;
+ private static final long MAX_RANKING_DELAY_MILLIS = 500L;
private final Context mContext;
private final NotificationManager mNotificationManager;
- private final Handler mMainHandler;
+ private final SystemClock mSystemClock;
+ private final Executor mMainExecutor;
private final List<NotificationHandler> mNotificationHandlers = new ArrayList<>();
private final ArrayList<NotificationSettingsListener> mSettingsListeners = new ArrayList<>();
+ private final Deque<RankingMap> mRankingMapQueue = new ConcurrentLinkedDeque<>();
+ private final Runnable mDispatchRankingUpdateRunnable = this::dispatchRankingUpdate;
+ private long mSkippingRankingUpdatesSince = -1;
+
/**
* Injected constructor. See {@link StatusBarModule}.
*/
public NotificationListener(
Context context,
NotificationManager notificationManager,
- @Main Handler mainHandler) {
+ SystemClock systemClock,
+ @Main Executor mainExecutor) {
mContext = context;
mNotificationManager = notificationManager;
- mMainHandler = mainHandler;
+ mSystemClock = systemClock;
+ mMainExecutor = mainExecutor;
}
/** Registers a listener that's notified when notifications are added/removed/etc. */
@@ -89,7 +100,7 @@
return;
}
final RankingMap currentRanking = getCurrentRanking();
- mMainHandler.post(() -> {
+ mMainExecutor.execute(() -> {
// There's currently a race condition between the calls to getActiveNotifications() and
// getCurrentRanking(). It's possible for the ranking that we store here to not contain
// entries for every notification in getActiveNotifications(). To prevent downstream
@@ -119,7 +130,7 @@
final RankingMap rankingMap) {
if (DEBUG) Log.d(TAG, "onNotificationPosted: " + sbn);
if (sbn != null && !onPluginNotificationPosted(sbn, rankingMap)) {
- mMainHandler.post(() -> {
+ mMainExecutor.execute(() -> {
processForRemoteInput(sbn.getNotification(), mContext);
for (NotificationHandler handler : mNotificationHandlers) {
@@ -134,7 +145,7 @@
int reason) {
if (DEBUG) Log.d(TAG, "onNotificationRemoved: " + sbn + " reason: " + reason);
if (sbn != null && !onPluginNotificationRemoved(sbn, rankingMap)) {
- mMainHandler.post(() -> {
+ mMainExecutor.execute(() -> {
for (NotificationHandler handler : mNotificationHandlers) {
handler.onNotificationRemoved(sbn, rankingMap, reason);
}
@@ -151,12 +162,49 @@
public void onNotificationRankingUpdate(final RankingMap rankingMap) {
if (DEBUG) Log.d(TAG, "onRankingUpdate");
if (rankingMap != null) {
+ // Add the ranking to the queue, then run dispatchRankingUpdate() on the main thread
RankingMap r = onPluginRankingUpdate(rankingMap);
- mMainHandler.post(() -> {
- for (NotificationHandler handler : mNotificationHandlers) {
- handler.onNotificationRankingUpdate(r);
+ mRankingMapQueue.addLast(r);
+ // Maintaining our own queue and always posting the runnable allows us to guarantee the
+ // relative ordering of all events which are dispatched, which is important so that the
+ // RankingMap always has exactly the same elements that are current, per add/remove
+ // events.
+ mMainExecutor.execute(mDispatchRankingUpdateRunnable);
+ }
+ }
+
+ /**
+ * This method is (and must be) the sole consumer of the RankingMap queue. After pulling an
+ * object off the queue, it checks if the queue is empty, and only dispatches the ranking update
+ * if the queue is still empty.
+ */
+ private void dispatchRankingUpdate() {
+ if (DEBUG) Log.d(TAG, "dispatchRankingUpdate");
+ RankingMap r = mRankingMapQueue.pollFirst();
+ if (r == null) {
+ Log.wtf(TAG, "mRankingMapQueue was empty!");
+ }
+ if (!mRankingMapQueue.isEmpty()) {
+ final long now = mSystemClock.elapsedRealtime();
+ if (mSkippingRankingUpdatesSince == -1) {
+ mSkippingRankingUpdatesSince = now;
+ }
+ final long timeSkippingRankingUpdates = now - mSkippingRankingUpdatesSince;
+ if (timeSkippingRankingUpdates < MAX_RANKING_DELAY_MILLIS) {
+ if (DEBUG) {
+ Log.d(TAG, "Skipping dispatch of onNotificationRankingUpdate() -- "
+ + mRankingMapQueue.size() + " more updates already in the queue.");
}
- });
+ return;
+ }
+ if (DEBUG) {
+ Log.d(TAG, "Proceeding with dispatch of onNotificationRankingUpdate() -- "
+ + mRankingMapQueue.size() + " more updates already in the queue.");
+ }
+ }
+ mSkippingRankingUpdatesSince = -1;
+ for (NotificationHandler handler : mNotificationHandlers) {
+ handler.onNotificationRankingUpdate(r);
}
}
@@ -165,7 +213,7 @@
String pkgName, UserHandle user, NotificationChannel channel, int modificationType) {
if (DEBUG) Log.d(TAG, "onNotificationChannelModified");
if (!onPluginNotificationChannelModified(pkgName, user, channel, modificationType)) {
- mMainHandler.post(() -> {
+ mMainExecutor.execute(() -> {
for (NotificationHandler handler : mNotificationHandlers) {
handler.onNotificationChannelModified(pkgName, user, channel, modificationType);
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java
index 8e2f88c..d574cda 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java
@@ -168,9 +168,10 @@
static NotificationListener provideNotificationListener(
Context context,
NotificationManager notificationManager,
- @Main Handler mainHandler) {
+ SystemClock systemClock,
+ @Main Executor mainExecutor) {
return new NotificationListener(
- context, notificationManager, mainHandler);
+ context, notificationManager, systemClock, mainExecutor);
}
/** */
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationListenerWithPlugins.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationListenerWithPlugins.java
index 4651e8446..c68d39b 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationListenerWithPlugins.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationListenerWithPlugins.java
@@ -66,11 +66,7 @@
@Override
public RankingMap getCurrentRanking() {
- RankingMap currentRanking = super.getCurrentRanking();
- for (NotificationListenerController plugin : mPlugins) {
- currentRanking = plugin.getCurrentRanking(currentRanking);
- }
- return currentRanking;
+ return onPluginRankingUpdate(super.getCurrentRanking());
}
public void onPluginConnected() {
@@ -120,8 +116,11 @@
return false;
}
- public RankingMap onPluginRankingUpdate(RankingMap rankingMap) {
- return getCurrentRanking();
+ protected RankingMap onPluginRankingUpdate(RankingMap rankingMap) {
+ for (NotificationListenerController plugin : mPlugins) {
+ rankingMap = plugin.getCurrentRanking(rankingMap);
+ }
+ return rankingMap;
}
@Override
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationListenerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationListenerTest.java
index afbe668..8c5f04f 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationListenerTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationListenerTest.java
@@ -16,27 +16,30 @@
package com.android.systemui.statusbar;
-import static org.mockito.ArgumentMatchers.any;
+import static com.google.common.truth.Truth.assertThat;
+
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import android.app.Notification;
import android.app.NotificationManager;
-import android.os.Handler;
import android.os.UserHandle;
import android.service.notification.NotificationListenerService.Ranking;
import android.service.notification.NotificationListenerService.RankingMap;
import android.service.notification.StatusBarNotification;
import android.testing.AndroidTestingRunner;
-import android.testing.TestableLooper;
import androidx.test.filters.SmallTest;
import com.android.systemui.SysuiTestCase;
import com.android.systemui.statusbar.NotificationListener.NotificationHandler;
+import com.android.systemui.util.concurrency.FakeExecutor;
+import com.android.systemui.util.time.FakeSystemClock;
import org.junit.Before;
import org.junit.Test;
@@ -46,7 +49,6 @@
@SmallTest
@RunWith(AndroidTestingRunner.class)
-@TestableLooper.RunWithLooper
public class NotificationListenerTest extends SysuiTestCase {
private static final String TEST_PACKAGE_NAME = "test";
private static final int TEST_UID = 0;
@@ -54,6 +56,8 @@
@Mock private NotificationHandler mNotificationHandler;
@Mock private NotificationManager mNotificationManager;
+ private final FakeSystemClock mFakeSystemClock = new FakeSystemClock();
+ private final FakeExecutor mFakeExecutor = new FakeExecutor(mFakeSystemClock);
private NotificationListener mListener;
private StatusBarNotification mSbn;
private RankingMap mRanking = new RankingMap(new Ranking[0]);
@@ -65,7 +69,8 @@
mListener = new NotificationListener(
mContext,
mNotificationManager,
- new Handler(TestableLooper.get(this).getLooper()));
+ mFakeSystemClock,
+ mFakeExecutor);
mSbn = new StatusBarNotification(TEST_PACKAGE_NAME, TEST_PACKAGE_NAME, 0, null, TEST_UID, 0,
new Notification(), UserHandle.CURRENT, null, 0);
@@ -75,23 +80,67 @@
@Test
public void testNotificationAddCallsAddNotification() {
mListener.onNotificationPosted(mSbn, mRanking);
- TestableLooper.get(this).processAllMessages();
+ mFakeExecutor.runAllReady();
verify(mNotificationHandler).onNotificationPosted(mSbn, mRanking);
}
@Test
public void testNotificationRemovalCallsRemoveNotification() {
mListener.onNotificationRemoved(mSbn, mRanking);
- TestableLooper.get(this).processAllMessages();
+ mFakeExecutor.runAllReady();
verify(mNotificationHandler).onNotificationRemoved(eq(mSbn), eq(mRanking), anyInt());
}
@Test
public void testRankingUpdateCallsNotificationRankingUpdate() {
mListener.onNotificationRankingUpdate(mRanking);
- TestableLooper.get(this).processAllMessages();
- // RankingMap may be modified by plugins.
- verify(mNotificationHandler).onNotificationRankingUpdate(any());
+ assertThat(mFakeExecutor.runAllReady()).isEqualTo(1);
+ verify(mNotificationHandler).onNotificationRankingUpdate(eq(mRanking));
+ }
+
+ @Test
+ public void testRankingUpdateMultipleTimesCallsNotificationRankingUpdateOnce() {
+ // GIVEN multiple notification ranking updates
+ RankingMap ranking1 = mock(RankingMap.class);
+ RankingMap ranking2 = mock(RankingMap.class);
+ RankingMap ranking3 = mock(RankingMap.class);
+ mListener.onNotificationRankingUpdate(ranking1);
+ mListener.onNotificationRankingUpdate(ranking2);
+ mListener.onNotificationRankingUpdate(ranking3);
+
+ // WHEN executor runs with multiple updates in the queue
+ assertThat(mFakeExecutor.numPending()).isEqualTo(3);
+ assertThat(mFakeExecutor.runAllReady()).isEqualTo(3);
+
+ // VERIFY that only the last ranking actually gets handled
+ verify(mNotificationHandler, never()).onNotificationRankingUpdate(eq(ranking1));
+ verify(mNotificationHandler, never()).onNotificationRankingUpdate(eq(ranking2));
+ verify(mNotificationHandler).onNotificationRankingUpdate(eq(ranking3));
+ verifyNoMoreInteractions(mNotificationHandler);
+ }
+
+ @Test
+ public void testRankingUpdateWillCallAgainIfQueueIsSlow() {
+ // GIVEN multiple notification ranking updates
+ RankingMap ranking1 = mock(RankingMap.class);
+ RankingMap ranking2 = mock(RankingMap.class);
+ RankingMap ranking3 = mock(RankingMap.class);
+ mListener.onNotificationRankingUpdate(ranking1);
+ mListener.onNotificationRankingUpdate(ranking2);
+ mListener.onNotificationRankingUpdate(ranking3);
+
+ // WHEN executor runs with a 1-second gap between handling events 1 and 2
+ assertThat(mFakeExecutor.numPending()).isEqualTo(3);
+ assertThat(mFakeExecutor.runNextReady()).isTrue();
+ // delay a second, which empties the executor
+ mFakeSystemClock.advanceTime(1000);
+ assertThat(mFakeExecutor.numPending()).isEqualTo(0);
+
+ // VERIFY that both event 2 and event 3 are called
+ verify(mNotificationHandler, never()).onNotificationRankingUpdate(eq(ranking1));
+ verify(mNotificationHandler).onNotificationRankingUpdate(eq(ranking2));
+ verify(mNotificationHandler).onNotificationRankingUpdate(eq(ranking3));
+ verifyNoMoreInteractions(mNotificationHandler);
}
@Test