diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationListener.java index 5437ce63475ec..d6125ce7aa65c 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.NotificationChannel; 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.dagger.qualifiers.Main; 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 @@ import java.util.List; 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 mNotificationHandlers = new ArrayList<>(); private final ArrayList mSettingsListeners = new ArrayList<>(); + private final Deque 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 @@ public class NotificationListener extends NotificationListenerWithPlugins { 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 @@ public class NotificationListener extends NotificationListenerWithPlugins { 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 @@ public class NotificationListener extends NotificationListenerWithPlugins { 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 class NotificationListener extends NotificationListenerWithPlugins { 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 @@ public class NotificationListener extends NotificationListenerWithPlugins { 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 8e2f88c36cf4e..d574cdabced62 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 @@ public interface StatusBarDependenciesModule { 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 4651e84460593..c68d39b97355c 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 @@ public class NotificationListenerWithPlugins extends NotificationListenerService @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 @@ public class NotificationListenerWithPlugins extends NotificationListenerService 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 afbe668a4de18..8c5f04f92073e 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 @@ import org.mockito.MockitoAnnotations; @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 @@ public class NotificationListenerTest extends SysuiTestCase { @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 @@ public class NotificationListenerTest extends SysuiTestCase { 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 @@ public class NotificationListenerTest extends SysuiTestCase { @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