From ef2ef6c909f28a494f812019ee672db4531725be Mon Sep 17 00:00:00 2001 From: Ned Burns Date: Wed, 2 Jan 2019 16:48:08 -0500 Subject: [PATCH] Change onRemoteEntry() to only fire when entries are removed Conceptually, this should be a paired match with onNotificationAdded(). Previously we were firing this method even if the notification was no longer present (already removed) or if it hadn't been added yet (was in pending). Test: atest Change-Id: I613f60aa8cf4e1aeb7bb13ff5883a221c9b623c6 --- ...ForegroundServiceNotificationListener.java | 7 +--- .../statusbar/NotificationMediaManager.java | 10 ++---- .../NotificationRemoteInputManager.java | 5 +-- .../NotificationAlertingManager.java | 8 ++--- .../NotificationEntryListener.java | 8 +---- .../NotificationEntryManager.java | 17 ++++++---- .../logging/NotificationLogger.java | 10 ++---- .../NotificationGroupAlertTransferHelper.java | 5 +-- .../phone/StatusBarNotificationPresenter.java | 8 ++--- .../ForegroundServiceControllerTest.java | 2 +- .../NotificationEntryManagerTest.java | 32 ++++++++++++++++--- .../logging/NotificationLoggerTest.java | 5 --- ...ificationGroupAlertTransferHelperTest.java | 3 +- 13 files changed, 54 insertions(+), 66 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java index 151a6b0131103..b0b7e6c88984a 100644 --- a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java +++ b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java @@ -61,14 +61,9 @@ public class ForegroundServiceNotificationListener { @Override public void onEntryRemoved( NotificationData.Entry entry, - String key, - StatusBarNotification old, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { - if (entry != null && !lifetimeExtended) { - removeNotification(entry.notification); - } + removeNotification(entry.notification); } }); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java index 1bf101c00711f..e59bc2a82c4cf 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java @@ -37,7 +37,6 @@ import android.media.session.PlaybackState; import android.os.Handler; import android.os.Trace; import android.os.UserHandle; -import android.service.notification.StatusBarNotification; import android.util.Log; import android.view.View; import android.widget.ImageView; @@ -157,15 +156,10 @@ public class NotificationMediaManager implements Dumpable { notificationEntryManager.addNotificationEntryListener(new NotificationEntryListener() { @Override public void onEntryRemoved( - @Nullable Entry entry, - String key, - StatusBarNotification old, + Entry entry, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { - if (!lifetimeExtended) { - onNotificationRemoved(key); - } + onNotificationRemoved(entry.key); } }); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationRemoteInputManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationRemoteInputManager.java index 886d99eeff176..1ab9c5c2b4a2a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationRemoteInputManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationRemoteInputManager.java @@ -254,13 +254,10 @@ public class NotificationRemoteInputManager implements Dumpable { @Override public void onEntryRemoved( @Nullable NotificationData.Entry entry, - String key, - StatusBarNotification old, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { if (removedByUser && entry != null) { - onPerformRemoveNotification(entry, key); + onPerformRemoveNotification(entry, entry.key); } } }); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationAlertingManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationAlertingManager.java index 7b42dd901d728..2bb0d5ce9161f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationAlertingManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationAlertingManager.java @@ -20,7 +20,6 @@ import static com.android.systemui.statusbar.NotificationRemoteInputManager.FORC import static com.android.systemui.statusbar.notification.row.NotificationInflater.FLAG_CONTENT_VIEW_AMBIENT; import static com.android.systemui.statusbar.notification.row.NotificationInflater.FLAG_CONTENT_VIEW_HEADS_UP; -import android.annotation.Nullable; import android.app.Notification; import android.service.notification.StatusBarNotification; import android.util.Log; @@ -83,13 +82,10 @@ public class NotificationAlertingManager { @Override public void onEntryRemoved( - @Nullable NotificationData.Entry entry, - String key, - StatusBarNotification old, + NotificationData.Entry entry, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { - stopAlerting(key); + stopAlerting(entry.key); } }); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryListener.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryListener.java index 1d06ce031f2be..2f60f115ed530 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryListener.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryListener.java @@ -71,20 +71,14 @@ public interface NotificationEntryListener { * because the developer retracted it). * @param entry notification data entry that was removed. Null if no entry existed for the * removed key at the time of removal. - * @param key key of notification that was removed - * @param old StatusBarNotification of the notification before it was removed * @param visibility logging data related to the visibility of the notification at the time of * removal, if it was removed by a user action. Null if it was not removed by * a user action. - * @param lifetimeExtended true if something is artificially extending how long the notification * @param removedByUser true if the notification was removed by a user action */ default void onEntryRemoved( - @Nullable NotificationData.Entry entry, - String key, - StatusBarNotification old, + NotificationData.Entry entry, @Nullable NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java index 2d316508c8cf2..b679eb3225ca2 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java @@ -153,6 +153,12 @@ public class NotificationEntryManager implements return mNotificationRowBinder; } + // TODO: Remove this once we can always use a mocked row binder in our tests + @VisibleForTesting + void setRowBinder(NotificationRowBinder notificationRowBinder) { + mNotificationRowBinder = notificationRowBinder; + } + public void setUpWithPresenter(NotificationPresenter presenter, NotificationListContainer listContainer, HeadsUpManager headsUpManager) { @@ -309,7 +315,6 @@ public class NotificationEntryManager implements abortExistingInflation(key); - StatusBarNotification old = null; boolean lifetimeExtended = false; if (entry != null) { @@ -342,12 +347,12 @@ public class NotificationEntryManager implements // Let's remove the children if this was a summary handleGroupSummaryRemoved(key); - old = removeNotificationViews(key, ranking); - } - } + removeNotificationViews(key, ranking); - for (NotificationEntryListener listener : mNotificationEntryListeners) { - listener.onEntryRemoved(entry, key, old, visibility, lifetimeExtended, removedByUser); + for (NotificationEntryListener listener : mNotificationEntryListeners) { + listener.onEntryRemoved(entry, visibility, removedByUser); + } + } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/logging/NotificationLogger.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/logging/NotificationLogger.java index 610d3003f0e93..43048a2c087e1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/logging/NotificationLogger.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/logging/NotificationLogger.java @@ -15,7 +15,6 @@ */ package com.android.systemui.statusbar.notification.logging; -import android.annotation.Nullable; import android.content.Context; import android.os.Handler; import android.os.RemoteException; @@ -168,14 +167,11 @@ public class NotificationLogger implements StateListener { entryManager.addNotificationEntryListener(new NotificationEntryListener() { @Override public void onEntryRemoved( - @Nullable NotificationData.Entry entry, - String key, - StatusBarNotification old, + NotificationData.Entry entry, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { - if (removedByUser && visibility != null && entry != null) { - logNotificationClear(key, entry.notification, visibility); + if (removedByUser && visibility != null) { + logNotificationClear(entry.key, entry.notification, visibility); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelper.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelper.java index 3839ed5d5d88f..af3257ae423ea 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelper.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelper.java @@ -220,15 +220,12 @@ public class NotificationGroupAlertTransferHelper implements OnHeadsUpChangedLis @Override public void onEntryRemoved( @Nullable Entry entry, - String key, - StatusBarNotification old, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { // Removes any alerts pending on this entry. Note that this will not stop any inflation // tasks started by a transfer, so this should only be used as clean-up for when // inflation is stopped and the pending alert no longer needs to happen. - mPendingAlerts.remove(key); + mPendingAlerts.remove(entry.key); } }; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java index b9372e8935adc..fb3157a128d67 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java @@ -196,14 +196,10 @@ public class StatusBarNotificationPresenter implements NotificationPresenter, @Override public void onEntryRemoved( @Nullable Entry entry, - String key, - StatusBarNotification old, NotificationVisibility visibility, - boolean lifetimeExtended, boolean removedByUser) { - if (!lifetimeExtended) { - StatusBarNotificationPresenter.this.onNotificationRemoved(key, old); - } + StatusBarNotificationPresenter.this.onNotificationRemoved( + entry.key, entry.notification); if (removedByUser) { maybeEndAmbientPulse(); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceControllerTest.java index 199a3a62d72bc..f8912bcca3020 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceControllerTest.java @@ -392,7 +392,7 @@ public class ForegroundServiceControllerTest extends SysuiTestCase { private void entryRemoved(StatusBarNotification notification) { mEntryListener.onEntryRemoved(new NotificationData.Entry(notification), - null, null, null, false, false); + null, false); } private void entryAdded(StatusBarNotification notification, int importance) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java index 57c9d29b24f46..6197341acda54 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java @@ -121,6 +121,7 @@ public class NotificationEntryManagerTest extends SysuiTestCase { @Mock private MetricsLogger mMetricsLogger; @Mock private SmartReplyController mSmartReplyController; @Mock private RowInflaterTask mAsyncInflationTask; + @Mock private NotificationRowBinder mMockedRowBinder; private NotificationData.Entry mEntry; private StatusBarNotification mSbn; @@ -310,8 +311,8 @@ public class NotificationEntryManagerTest extends SysuiTestCase { verify(mListContainer).cleanUpViewStateForEntry(mEntry); verify(mPresenter).updateNotificationViews(); - verify(mEntryListener).onEntryRemoved(mEntry, mSbn.getKey(), mSbn, - null, false /* lifetimeExtended */, false /* removedByUser */); + verify(mEntryListener).onEntryRemoved( + mEntry, null, false /* removedByUser */); verify(mRow).setRemoved(); assertNull(mEntryManager.getNotificationData().get(mSbn.getKey())); @@ -335,8 +336,31 @@ public class NotificationEntryManagerTest extends SysuiTestCase { assertNotNull(mEntryManager.getNotificationData().get(mSbn.getKey())); verify(extender).setShouldManageLifetime(mEntry, true /* shouldManage */); - verify(mEntryListener).onEntryRemoved(mEntry, mSbn.getKey(), null, - null, true /* lifetimeExtended */, false /* removedByUser */); + verify(mEntryListener, never()).onEntryRemoved( + mEntry, null, false /* removedByUser */); + } + + @Test + public void testRemoveNotification_onEntryRemoveNotFiredIfEntryDoesntExist() { + com.android.systemui.util.Assert.isNotMainThread(); + + mEntryManager.removeNotification("not_a_real_key", mRankingMap); + + verify(mEntryListener, never()).onEntryRemoved( + mEntry, null, false /* removedByUser */); + } + + @Test + public void testRemoveNotification_whilePending() throws InterruptedException { + com.android.systemui.util.Assert.isNotMainThread(); + + mEntryManager.setRowBinder(mMockedRowBinder); + + mEntryManager.addNotification(mSbn, mRankingMap); + mEntryManager.removeNotification(mSbn.getKey(), mRankingMap); + + verify(mEntryListener, never()).onEntryRemoved( + mEntry, null, false /* removedByUser */); } @Test diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/logging/NotificationLoggerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/logging/NotificationLoggerTest.java index 983ca837b6396..afdeb62a8f28d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/logging/NotificationLoggerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/logging/NotificationLoggerTest.java @@ -159,11 +159,6 @@ public class NotificationLoggerTest extends SysuiTestCase { verify(mBarService, times(1)).onNotificationVisibilityChanged(any(), any()); } - @Test - public void testHandleNullEntryOnEntryRemoved() { - mNotificationEntryListener.onEntryRemoved(null, "foobar", null, null, false, false); - } - private class TestableNotificationLogger extends NotificationLogger { TestableNotificationLogger(NotificationListener notificationListener, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelperTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelperTest.java index 56af40054316d..79695fd6d38d2 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelperTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/NotificationGroupAlertTransferHelperTest.java @@ -237,8 +237,7 @@ public class NotificationGroupAlertTransferHelperTest extends SysuiTestCase { mGroupManager.onEntryAdded(summaryEntry); mGroupManager.onEntryAdded(childEntry); - mNotificationEntryListener.onEntryRemoved(childEntry, childEntry.key, null, null, - false, false); + mNotificationEntryListener.onEntryRemoved(childEntry, null, false); assertFalse(mGroupAlertTransferHelper.isAlertTransferPending(childEntry)); }