Fix NotificationEntry mismatch crashes when tapping notification.
The root of these problems is holding onto a NotificationEntry instance and performing a dismiss or end to lifetime extension after a delay, during which apps or the system may cancel and re-post the given notification. The original fix to this by lifetime extending these notifications worked in the case where this closure around a NotificationEntry perfectly matched lifetime extension, but this was becoming a whack-a-mole of finding all the ways we might need to extend these notifications. Moreover, there was a bug in the implementation that used notification keys as the keys for the lifetime extension map, rather than NotificationEntry instances themselves, which caused b/227254780.
This CL introduces the concept of reporting future dismissals to the NotifCollection (via OnUserInteractionCallback) which allows the NotifCollection to respond to system server's notification removal events by preventing the eventual dismissal call from crashing. This design is not the ideal end-game (see b/232260346) but rather preserves the existing call order into NotifCollection while also bailing out before any calls are made that would cause a crash.
Test: atest ExpandableNotificationRowTest StatusBarNotificationActivityStarterTest NotifCollectionTest
Fixes: 230540148
Fixes: 227254780
Bug: 232260346
Change-Id: Ia66ae5ade3fbfdd0436d54dcbeef720618622716
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java
index bcd8e59..6085096 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java
@@ -18,9 +18,12 @@
import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL;
import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL_ALL;
+import static android.service.notification.NotificationListenerService.REASON_ASSISTANT_CANCEL;
import static android.service.notification.NotificationListenerService.REASON_CANCEL;
import static android.service.notification.NotificationListenerService.REASON_CANCEL_ALL;
import static android.service.notification.NotificationListenerService.REASON_CHANNEL_BANNED;
+import static android.service.notification.NotificationListenerService.REASON_CHANNEL_REMOVED;
+import static android.service.notification.NotificationListenerService.REASON_CLEAR_DATA;
import static android.service.notification.NotificationListenerService.REASON_CLICK;
import static android.service.notification.NotificationListenerService.REASON_ERROR;
import static android.service.notification.NotificationListenerService.REASON_GROUP_OPTIMIZATION;
@@ -36,9 +39,11 @@
import static android.service.notification.NotificationListenerService.REASON_UNAUTOBUNDLED;
import static android.service.notification.NotificationListenerService.REASON_USER_STOPPED;
+import static com.android.systemui.statusbar.notification.NotificationUtils.logKey;
import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.DISMISSED;
import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED;
import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED;
+import static com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLoggerKt.cancellationReasonDebugString;
import static java.util.Objects.requireNonNull;
@@ -99,6 +104,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
@@ -143,6 +149,7 @@
private final Map<String, NotificationEntry> mNotificationSet = new ArrayMap<>();
private final Collection<NotificationEntry> mReadOnlyNotificationSet =
Collections.unmodifiableCollection(mNotificationSet.values());
+ private final HashMap<String, FutureDismissal> mFutureDismissals = new HashMap<>();
@Nullable private CollectionReadyForBuildListener mBuildListener;
private final List<NotifCollectionListener> mNotifCollectionListeners = new ArrayList<>();
@@ -511,6 +518,7 @@
cancelDismissInterception(entry);
mEventQueue.add(new EntryRemovedEvent(entry, entry.mCancellationReason));
mEventQueue.add(new CleanUpEntryEvent(entry));
+ handleFutureDismissal(entry);
return true;
} else {
return false;
@@ -519,31 +527,32 @@
/**
* Get the group summary entry
- * @param group
+ * @param groupKey
* @return
*/
@Nullable
- public NotificationEntry getGroupSummary(String group) {
+ public NotificationEntry getGroupSummary(String groupKey) {
return mNotificationSet
.values()
.stream()
- .filter(it -> Objects.equals(it.getSbn().getGroup(), group))
+ .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey))
.filter(it -> it.getSbn().getNotification().isGroupSummary())
.findFirst().orElse(null);
}
/**
- * Checks if the entry is the only child in the logical group
- * @param entry
- * @return
+ * Checks if the entry is the only child in the logical group;
+ * it need not have a summary to qualify
+ *
+ * @param entry the entry to check
*/
public boolean isOnlyChildInGroup(NotificationEntry entry) {
- String group = entry.getSbn().getGroup();
+ String groupKey = entry.getSbn().getGroupKey();
return mNotificationSet.get(entry.getKey()) == entry
&& mNotificationSet
.values()
.stream()
- .filter(it -> Objects.equals(it.getSbn().getGroup(), group))
+ .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey))
.filter(it -> !it.getSbn().getNotification().isGroupSummary())
.count() == 1;
}
@@ -916,10 +925,139 @@
dispatchEventsAndRebuildList();
}
+ /**
+ * A method to alert the collection that an async operation is happening, at the end of which a
+ * dismissal request will be made. This method has the additional guarantee that if a parent
+ * notification exists for a single child, then that notification will also be dismissed.
+ *
+ * The runnable returned must be run at the end of the async operation to enact the cancellation
+ *
+ * @param entry the notification we want to dismiss
+ * @param cancellationReason the reason for the cancellation
+ * @param statsCreator the callback for generating the stats for an entry
+ * @return the runnable to be run when the dismissal is ready to happen
+ */
+ public Runnable registerFutureDismissal(NotificationEntry entry, int cancellationReason,
+ DismissedByUserStatsCreator statsCreator) {
+ FutureDismissal dismissal = mFutureDismissals.get(entry.getKey());
+ if (dismissal != null) {
+ mLogger.logFutureDismissalReused(dismissal);
+ return dismissal;
+ }
+ dismissal = new FutureDismissal(entry, cancellationReason, statsCreator);
+ mFutureDismissals.put(entry.getKey(), dismissal);
+ mLogger.logFutureDismissalRegistered(dismissal);
+ return dismissal;
+ }
+
+ private void handleFutureDismissal(NotificationEntry entry) {
+ final FutureDismissal futureDismissal = mFutureDismissals.remove(entry.getKey());
+ if (futureDismissal != null) {
+ futureDismissal.onSystemServerCancel(entry.mCancellationReason);
+ }
+ }
+
+ /** A single method interface that callers can pass in when registering future dismissals */
+ public interface DismissedByUserStatsCreator {
+ DismissedByUserStats createDismissedByUserStats(NotificationEntry entry);
+ }
+
+ /** A class which tracks the double dismissal events coming in from both the system server and
+ * the ui */
+ public class FutureDismissal implements Runnable {
+ private final NotificationEntry mEntry;
+ private final DismissedByUserStatsCreator mStatsCreator;
+ @Nullable
+ private final NotificationEntry mSummaryToDismiss;
+ private final String mLabel;
+
+ private boolean mDidRun;
+ private boolean mDidSystemServerCancel;
+
+ private FutureDismissal(NotificationEntry entry, @CancellationReason int cancellationReason,
+ DismissedByUserStatsCreator statsCreator) {
+ mEntry = entry;
+ mStatsCreator = statsCreator;
+ mSummaryToDismiss = fetchSummaryToDismiss(entry);
+ mLabel = "<FutureDismissal@" + Integer.toHexString(hashCode())
+ + " entry=" + logKey(mEntry)
+ + " reason=" + cancellationReasonDebugString(cancellationReason)
+ + " summary=" + logKey(mSummaryToDismiss)
+ + ">";
+ }
+
+ @Nullable
+ private NotificationEntry fetchSummaryToDismiss(NotificationEntry entry) {
+ if (isOnlyChildInGroup(entry)) {
+ String group = entry.getSbn().getGroupKey();
+ NotificationEntry summary = getGroupSummary(group);
+ if (summary != null && summary.isDismissable()) return summary;
+ }
+ return null;
+ }
+
+ /** called when the entry has been removed from the collection */
+ public void onSystemServerCancel(@CancellationReason int cancellationReason) {
+ Assert.isMainThread();
+ if (mDidSystemServerCancel) {
+ mLogger.logFutureDismissalDoubleCancelledByServer(this);
+ return;
+ }
+ mLogger.logFutureDismissalGotSystemServerCancel(this, cancellationReason);
+ mDidSystemServerCancel = true;
+ // TODO: Internally dismiss the summary now instead of waiting for onUiCancel
+ }
+
+ private void onUiCancel() {
+ mFutureDismissals.remove(mEntry.getKey());
+ final NotificationEntry currentEntry = getEntry(mEntry.getKey());
+ // generate stats for the entry before dismissing summary, which could affect state
+ final DismissedByUserStats stats = mStatsCreator.createDismissedByUserStats(mEntry);
+ // dismiss the summary (if it exists)
+ if (mSummaryToDismiss != null) {
+ final NotificationEntry currentSummary = getEntry(mSummaryToDismiss.getKey());
+ if (currentSummary == mSummaryToDismiss) {
+ mLogger.logFutureDismissalDismissing(this, "summary");
+ dismissNotification(mSummaryToDismiss,
+ mStatsCreator.createDismissedByUserStats(mSummaryToDismiss));
+ } else {
+ mLogger.logFutureDismissalMismatchedEntry(this, "summary", currentSummary);
+ }
+ }
+ // dismiss this entry (if it is still around)
+ if (mDidSystemServerCancel) {
+ mLogger.logFutureDismissalAlreadyCancelledByServer(this);
+ } else if (currentEntry == mEntry) {
+ mLogger.logFutureDismissalDismissing(this, "entry");
+ dismissNotification(mEntry, stats);
+ } else {
+ mLogger.logFutureDismissalMismatchedEntry(this, "entry", currentEntry);
+ }
+ }
+
+ /** called when the dismissal should be completed */
+ @Override
+ public void run() {
+ Assert.isMainThread();
+ if (mDidRun) {
+ mLogger.logFutureDismissalDoubleRun(this);
+ return;
+ }
+ mDidRun = true;
+ onUiCancel();
+ }
+
+ /** provides a debug label for this instance */
+ public String getLabel() {
+ return mLabel;
+ }
+ }
+
@IntDef(prefix = { "REASON_" }, value = {
REASON_NOT_CANCELED,
REASON_UNKNOWN,
REASON_CLICK,
+ REASON_CANCEL,
REASON_CANCEL_ALL,
REASON_ERROR,
REASON_PACKAGE_CHANGED,
@@ -937,6 +1075,9 @@
REASON_CHANNEL_BANNED,
REASON_SNOOZED,
REASON_TIMEOUT,
+ REASON_CHANNEL_REMOVED,
+ REASON_CLEAR_DATA,
+ REASON_ASSISTANT_CANCEL,
})
@Retention(RetentionPolicy.SOURCE)
public @interface CancellationReason {}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt
index b24d292..acb26a9 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt
@@ -93,7 +93,7 @@
mCoordinators.add(shadeEventCoordinator)
mCoordinators.add(viewConfigCoordinator)
mCoordinators.add(visualStabilityCoordinator)
- mCoordinators.add(activityLaunchAnimCoordinator)
+// mCoordinators.add(activityLaunchAnimCoordinator) // NOTE: will delete in followup CL
if (notifPipelineFlags.isSmartspaceDedupingEnabled()) {
mCoordinators.add(smartspaceDedupingCoordinator)
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java
index 3bd91b5..7dd3672 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java
@@ -18,17 +18,17 @@
import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL;
-import android.annotation.Nullable;
import android.os.SystemClock;
-import android.service.notification.NotificationListenerService;
import android.service.notification.NotificationStats;
+import androidx.annotation.NonNull;
+
import com.android.systemui.plugins.statusbar.StatusBarStateController;
import com.android.systemui.statusbar.notification.collection.NotifCollection;
+import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.collection.coordinator.VisualStabilityCoordinator;
import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats;
-import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager;
import com.android.systemui.statusbar.notification.collection.render.NotificationVisibilityProvider;
import com.android.systemui.statusbar.notification.row.OnUserInteractionCallback;
import com.android.systemui.statusbar.policy.HeadsUpManager;
@@ -43,54 +43,33 @@
private final HeadsUpManager mHeadsUpManager;
private final StatusBarStateController mStatusBarStateController;
private final VisualStabilityCoordinator mVisualStabilityCoordinator;
- private final GroupMembershipManager mGroupMembershipManager;
public OnUserInteractionCallbackImpl(
NotificationVisibilityProvider visibilityProvider,
NotifCollection notifCollection,
HeadsUpManager headsUpManager,
StatusBarStateController statusBarStateController,
- VisualStabilityCoordinator visualStabilityCoordinator,
- GroupMembershipManager groupMembershipManager
+ VisualStabilityCoordinator visualStabilityCoordinator
) {
mVisibilityProvider = visibilityProvider;
mNotifCollection = notifCollection;
mHeadsUpManager = headsUpManager;
mStatusBarStateController = statusBarStateController;
mVisualStabilityCoordinator = visualStabilityCoordinator;
- mGroupMembershipManager = groupMembershipManager;
}
- /**
- * Callback triggered when a user:
- * 1. Manually dismisses a notification {@see ExpandableNotificationRow}.
- * 2. Clicks on a notification with flag {@link android.app.Notification#FLAG_AUTO_CANCEL}.
- * {@see StatusBarNotificationActivityStarter}
- */
- @Override
- public void onDismiss(
- NotificationEntry entry,
- @NotificationListenerService.NotificationCancelReason int cancellationReason,
- @Nullable NotificationEntry groupSummaryToDismiss
- ) {
+ @NonNull
+ private DismissedByUserStats getDismissedByUserStats(NotificationEntry entry) {
int dismissalSurface = NotificationStats.DISMISSAL_SHADE;
if (mHeadsUpManager.isAlerting(entry.getKey())) {
dismissalSurface = NotificationStats.DISMISSAL_PEEK;
} else if (mStatusBarStateController.isDozing()) {
dismissalSurface = NotificationStats.DISMISSAL_AOD;
}
-
- if (groupSummaryToDismiss != null) {
- onDismiss(groupSummaryToDismiss, cancellationReason, null);
- }
-
- mNotifCollection.dismissNotification(
- entry,
- new DismissedByUserStats(
- dismissalSurface,
- DISMISS_SENTIMENT_NEUTRAL,
- mVisibilityProvider.obtain(entry, true))
- );
+ return new DismissedByUserStats(
+ dismissalSurface,
+ DISMISS_SENTIMENT_NEUTRAL,
+ mVisibilityProvider.obtain(entry, true));
}
@Override
@@ -100,19 +79,11 @@
SystemClock.uptimeMillis());
}
- /**
- * @param entry that is being dismissed
- * @return the group summary to dismiss along with this entry if this is the last entry in
- * the group. Else, returns null.
- */
+ @NonNull
@Override
- @Nullable
- public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) {
- String group = entry.getSbn().getGroup();
- if (mNotifCollection.isOnlyChildInGroup(entry)) {
- NotificationEntry summary = mNotifCollection.getGroupSummary(group);
- if (summary != null && summary.isDismissable()) return summary;
- }
- return null;
+ public Runnable registerFutureDismissal(@NonNull NotificationEntry entry,
+ @CancellationReason int cancellationReason) {
+ return mNotifCollection.registerFutureDismissal(
+ entry, cancellationReason, this::getDismissedByUserStats);
}
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java
index 8daf8be..103b14b 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java
@@ -18,12 +18,15 @@
import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL;
-import android.annotation.Nullable;
import android.service.notification.NotificationListenerService;
import android.service.notification.NotificationStats;
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+
import com.android.systemui.plugins.statusbar.StatusBarStateController;
import com.android.systemui.statusbar.notification.NotificationEntryManager;
+import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats;
import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager;
@@ -68,8 +71,7 @@
* along with this dismissal. If null, does not additionally
* dismiss any notifications.
*/
- @Override
- public void onDismiss(
+ private void onDismiss(
NotificationEntry entry,
@NotificationListenerService.NotificationCancelReason int cancellationReason,
@Nullable NotificationEntry groupSummaryToDismiss
@@ -106,14 +108,21 @@
* @return the group summary to dismiss along with this entry if this is the last entry in
* the group. Else, returns null.
*/
- @Override
@Nullable
- public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) {
+ private NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) {
if (mGroupMembershipManager.isOnlyChildInGroup(entry)) {
NotificationEntry groupSummary = mGroupMembershipManager.getLogicalGroupSummary(entry);
return groupSummary.isDismissable() ? groupSummary : null;
}
return null;
}
+
+ @Override
+ @NonNull
+ public Runnable registerFutureDismissal(@NonNull NotificationEntry entry,
+ @CancellationReason int cancellationReason) {
+ NotificationEntry groupSummaryToDismiss = getGroupSummaryToDismiss(entry);
+ return () -> onDismiss(entry, cancellationReason, groupSummaryToDismiss);
+ }
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt
index 7302de5..7e79367 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt
@@ -28,7 +28,9 @@
import com.android.systemui.log.dagger.NotificationLog
import com.android.systemui.statusbar.notification.collection.NotifCollection
import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason
+import com.android.systemui.statusbar.notification.collection.NotifCollection.FutureDismissal
import com.android.systemui.statusbar.notification.collection.NotificationEntry
+import com.android.systemui.statusbar.notification.logKey
import javax.inject.Inject
fun cancellationReasonDebugString(@CancellationReason reason: Int) =
@@ -36,6 +38,7 @@
-1 -> "REASON_NOT_CANCELED" // NotifCollection.REASON_NOT_CANCELED
NotifCollection.REASON_UNKNOWN -> "REASON_UNKNOWN"
NotificationListenerService.REASON_CLICK -> "REASON_CLICK"
+ NotificationListenerService.REASON_CANCEL -> "REASON_CANCEL"
NotificationListenerService.REASON_CANCEL_ALL -> "REASON_CANCEL_ALL"
NotificationListenerService.REASON_ERROR -> "REASON_ERROR"
NotificationListenerService.REASON_PACKAGE_CHANGED -> "REASON_PACKAGE_CHANGED"
@@ -53,6 +56,9 @@
NotificationListenerService.REASON_CHANNEL_BANNED -> "REASON_CHANNEL_BANNED"
NotificationListenerService.REASON_SNOOZED -> "REASON_SNOOZED"
NotificationListenerService.REASON_TIMEOUT -> "REASON_TIMEOUT"
+ NotificationListenerService.REASON_CHANNEL_REMOVED -> "REASON_CHANNEL_REMOVED"
+ NotificationListenerService.REASON_CLEAR_DATA -> "REASON_CLEAR_DATA"
+ NotificationListenerService.REASON_ASSISTANT_CANCEL -> "REASON_ASSISTANT_CANCEL"
else -> "unknown"
}
@@ -241,6 +247,81 @@
"ERROR suppressed due to initialization forgiveness: $str1"
})
}
+
+ fun logFutureDismissalReused(dismissal: FutureDismissal) {
+ buffer.log(TAG, INFO, {
+ str1 = dismissal.label
+ }, {
+ "Reusing existing registration: $str1"
+ })
+ }
+
+ fun logFutureDismissalRegistered(dismissal: FutureDismissal) {
+ buffer.log(TAG, DEBUG, {
+ str1 = dismissal.label
+ }, {
+ "Registered: $str1"
+ })
+ }
+
+ fun logFutureDismissalDoubleCancelledByServer(dismissal: FutureDismissal) {
+ buffer.log(TAG, WARNING, {
+ str1 = dismissal.label
+ }, {
+ "System server double cancelled: $str1"
+ })
+ }
+
+ fun logFutureDismissalDoubleRun(dismissal: FutureDismissal) {
+ buffer.log(TAG, WARNING, {
+ str1 = dismissal.label
+ }, {
+ "Double run: $str1"
+ })
+ }
+
+ fun logFutureDismissalAlreadyCancelledByServer(dismissal: FutureDismissal) {
+ buffer.log(TAG, DEBUG, {
+ str1 = dismissal.label
+ }, {
+ "Ignoring: entry already cancelled by server: $str1"
+ })
+ }
+
+ fun logFutureDismissalGotSystemServerCancel(
+ dismissal: FutureDismissal,
+ @CancellationReason cancellationReason: Int
+ ) {
+ buffer.log(TAG, DEBUG, {
+ str1 = dismissal.label
+ int1 = cancellationReason
+ }, {
+ "SystemServer cancelled: $str1 reason=${cancellationReasonDebugString(int1)}"
+ })
+ }
+
+ fun logFutureDismissalDismissing(dismissal: FutureDismissal, type: String) {
+ buffer.log(TAG, DEBUG, {
+ str1 = dismissal.label
+ str2 = type
+ }, {
+ "Dismissing $str2 for: $str1"
+ })
+ }
+
+ fun logFutureDismissalMismatchedEntry(
+ dismissal: FutureDismissal,
+ type: String,
+ latestEntry: NotificationEntry?
+ ) {
+ buffer.log(TAG, WARNING, {
+ str1 = dismissal.label
+ str2 = type
+ str3 = latestEntry.logKey
+ }, {
+ "Mismatch: current $str2 is $str3 for: $str1"
+ })
+ }
}
private const val TAG = "NotifCollection"
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
index d96590a..94848e8 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
@@ -350,8 +350,7 @@
notifCollection.get(),
headsUpManager,
statusBarStateController,
- visualStabilityCoordinator.get(),
- groupMembershipManagerLazy.get())
+ visualStabilityCoordinator.get())
: new OnUserInteractionCallbackImplLegacy(
entryManager,
visibilityProvider.get(),
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java
index ed69e06..af9d751 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java
@@ -17,7 +17,6 @@
package com.android.systemui.statusbar.notification.row;
import static android.app.Notification.Action.SEMANTIC_ACTION_MARK_CONVERSATION_AS_PRIORITY;
-import static android.os.UserHandle.USER_SYSTEM;
import static android.service.notification.NotificationListenerService.REASON_CANCEL;
import static com.android.systemui.statusbar.notification.row.NotificationContentView.VISIBLE_TYPE_HEADSUP;
@@ -34,8 +33,6 @@
import android.app.NotificationChannel;
import android.app.role.RoleManager;
import android.content.Context;
-import android.content.pm.PackageInfo;
-import android.content.pm.PackageManager;
import android.content.res.Configuration;
import android.content.res.Resources;
import android.graphics.Canvas;
@@ -51,7 +48,6 @@
import android.os.RemoteException;
import android.os.ServiceManager;
import android.os.Trace;
-import android.provider.Settings;
import android.service.notification.StatusBarNotification;
import android.util.ArraySet;
import android.util.AttributeSet;
@@ -112,7 +108,6 @@
import com.android.systemui.statusbar.notification.stack.NotificationChildrenContainer;
import com.android.systemui.statusbar.notification.stack.NotificationStackScrollLayout;
import com.android.systemui.statusbar.notification.stack.SwipeableView;
-import com.android.systemui.statusbar.phone.CentralSurfaces;
import com.android.systemui.statusbar.phone.KeyguardBypassController;
import com.android.systemui.statusbar.policy.HeadsUpManager;
import com.android.systemui.statusbar.policy.InflatedSmartReplyState;
@@ -1455,8 +1450,7 @@
dismiss(fromAccessibility);
if (mEntry.isDismissable()) {
if (mOnUserInteractionCallback != null) {
- mOnUserInteractionCallback.onDismiss(mEntry, REASON_CANCEL,
- mOnUserInteractionCallback.getGroupSummaryToDismiss(mEntry));
+ mOnUserInteractionCallback.registerFutureDismissal(mEntry, REASON_CANCEL).run();
}
}
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java
index 94c5507..98d4353 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java
@@ -16,8 +16,9 @@
package com.android.systemui.statusbar.notification.row;
-import android.service.notification.NotificationListenerService;
+import androidx.annotation.NonNull;
+import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
/**
@@ -26,29 +27,23 @@
public interface OnUserInteractionCallback {
/**
- * Handle a user interaction that triggers a notification dismissal. Called when a user clicks
- * on an auto-cancelled notification or manually swipes to dismiss the notification.
- *
- * @param entry notification being dismissed
- * @param cancellationReason reason for the cancellation
- * @param groupSummaryToDismiss group summary to dismiss with `entry`.
- */
- void onDismiss(
- NotificationEntry entry,
- @NotificationListenerService.NotificationCancelReason int cancellationReason,
- NotificationEntry groupSummaryToDismiss);
-
- /**
* Triggered after a user has changed the importance of the notification via its
* {@link NotificationGuts}.
*/
void onImportanceChanged(NotificationEntry entry);
-
/**
- * @param entry being dismissed by the user
- * @return group summary that should be dismissed along with `entry`. Can be null if no
- * relevant group summary exists or the group summary should not be dismissed with `entry`.
+ * Called once it is known that a dismissal will take place for the given reason.
+ * This returns a Runnable which MUST be invoked when the dismissal is ready to be completed.
+ *
+ * Registering for future dismissal is typically done before notifying the NMS that a
+ * notification was clicked or dismissed, but the local dismissal may happen later.
+ *
+ * @param entry the entry being cancelled
+ * @param cancellationReason the reason for the cancellation
+ * @return the runnable to call when the dismissal can happen
*/
- NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry);
+ @NonNull
+ Runnable registerFutureDismissal(@NonNull NotificationEntry entry,
+ @CancellationReason int cancellationReason);
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java
index af4c81b..eee7b2a 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java
@@ -380,22 +380,15 @@
final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true);
- // retrieve the group summary to remove with this entry before we tell NMS the
- // notification was clicked to avoid a race condition
- final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn());
- final NotificationEntry summaryToRemove = shouldAutoCancel
- ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null;
-
- // inform NMS that the notification was clicked
- mClickNotifier.onNotificationClick(notificationKey, nv);
-
- if (!canBubble && (shouldAutoCancel
+ if (!canBubble && (shouldAutoCancel(entry.getSbn())
|| mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey))) {
+ final Runnable locallyDismissNotification =
+ mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK);
// Immediately remove notification from visually showing.
// We have to post the removal to the UI thread for synchronization.
mMainThreadHandler.post(() -> {
final Runnable removeNotification = () -> {
- mOnUserInteractionCallback.onDismiss(entry, REASON_CLICK, summaryToRemove);
+ locallyDismissNotification.run();
if (!animate) {
// If we're animating, this would be invoked after the activity launch
// animation completes. Since we're not animating, the launch already
@@ -417,6 +410,9 @@
() -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry));
}
+ // inform NMS that the notification was clicked
+ mClickNotifier.onNotificationClick(notificationKey, nv);
+
mIsCollapsingToShowActivityOverLockscreen = false;
}
@@ -433,24 +429,14 @@
// will focus follow operation only after drag-and-drop that notification.
final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true);
- // retrieve the group summary to remove with this entry before we tell NMS the
- // notification was clicked to avoid a race condition
- final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn());
- final NotificationEntry summaryToRemove = shouldAutoCancel
- ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null;
-
String notificationKey = entry.getKey();
- // inform NMS that the notification was clicked
- mClickNotifier.onNotificationClick(notificationKey, nv);
-
- if (shouldAutoCancel || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(
- notificationKey)) {
+ if (shouldAutoCancel(entry.getSbn())
+ || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey)) {
+ final Runnable removeNotification =
+ mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK);
// Immediately remove notification from visually showing.
// We have to post the removal to the UI thread for synchronization.
mMainThreadHandler.post(() -> {
- final Runnable removeNotification = () ->
- mOnUserInteractionCallback.onDismiss(
- entry, REASON_CLICK, summaryToRemove);
if (mPresenter.isCollapsing()) {
// To avoid lags we're only performing the remove
// after the shade is collapsed
@@ -461,6 +447,9 @@
});
}
+ // inform NMS that the notification was clicked
+ mClickNotifier.onNotificationClick(notificationKey, nv);
+
mIsCollapsingToShowActivityOverLockscreen = false;
}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java
index 7068009..958d542 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java
@@ -32,6 +32,8 @@
import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED;
import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED;
+import static com.google.common.truth.Truth.assertThat;
+
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
@@ -47,6 +49,7 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import static java.util.Collections.singletonList;
@@ -180,13 +183,14 @@
@Test
public void testGetGroupSummary() {
- assertEquals(null, mCollection.getGroupSummary("group"));
- NotifEvent summary = mNoMan.postNotif(
- buildNotif(TEST_PACKAGE, 0)
- .setGroup(mContext, "group")
- .setGroupSummary(mContext, true));
+ final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 0)
+ .setGroup(mContext, "group")
+ .setGroupSummary(mContext, true);
+ final String groupKey = entryBuilder.build().getSbn().getGroupKey();
+ assertEquals(null, mCollection.getGroupSummary(groupKey));
+ NotifEvent summary = mNoMan.postNotif(entryBuilder);
- final NotificationEntry entry = mCollection.getGroupSummary("group");
+ final NotificationEntry entry = mCollection.getGroupSummary(groupKey);
assertEquals(summary.key, entry.getKey());
assertEquals(summary.sbn, entry.getSbn());
assertEquals(summary.ranking, entry.getRanking());
@@ -194,9 +198,9 @@
@Test
public void testIsOnlyChildInGroup() {
- NotifEvent notif1 = mNoMan.postNotif(
- buildNotif(TEST_PACKAGE, 1)
- .setGroup(mContext, "group"));
+ final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 1)
+ .setGroup(mContext, "group");
+ NotifEvent notif1 = mNoMan.postNotif(entryBuilder);
final NotificationEntry entry = mCollection.getEntry(notif1.key);
assertTrue(mCollection.isOnlyChildInGroup(entry));
@@ -1488,6 +1492,55 @@
}
@Test
+ public void testRegisterFutureDismissal() throws RemoteException {
+ // GIVEN a pipeline with one notification
+ NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag"));
+ NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key));
+ clearInvocations(mCollectionListener);
+
+ // WHEN registering a future dismissal, nothing happens right away
+ final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK,
+ NotifCollectionTest::defaultStats);
+ verifyNoMoreInteractions(mCollectionListener);
+
+ // WHEN finally dismissing
+ onDismiss.run();
+ verify(mStatusBarService).onNotificationClear(any(), anyInt(), eq(notifEvent.key),
+ anyInt(), anyInt(), any());
+ verifyNoMoreInteractions(mStatusBarService);
+ verifyNoMoreInteractions(mCollectionListener);
+ }
+
+ @Test
+ public void testRegisterFutureDismissalWithRetractionAndRepost() {
+ // GIVEN a pipeline with one notification
+ NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag"));
+ NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key));
+ clearInvocations(mCollectionListener);
+
+ // WHEN registering a future dismissal, nothing happens right away
+ final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK,
+ NotifCollectionTest::defaultStats);
+ verifyNoMoreInteractions(mCollectionListener);
+
+ // WHEN retracting the notification, and then reposting
+ mNoMan.retractNotif(notifEvent.sbn, REASON_CLICK);
+ mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag"));
+ clearInvocations(mCollectionListener);
+
+ // KNOWING that the entry in the collection is different now
+ assertThat(mCollection.getEntry(notifEvent.key)).isNotSameInstanceAs(entry);
+
+ // WHEN finally dismissing
+ onDismiss.run();
+
+ // VERIFY that nothing happens; the notification should not be removed
+ verifyNoMoreInteractions(mCollectionListener);
+ assertThat(mCollection.getEntry(notifEvent.key)).isNotNull();
+ verifyNoMoreInteractions(mStatusBarService);
+ }
+
+ @Test
public void testCannotDismissOngoingNotificationChildren() {
// GIVEN an ongoing notification
final NotificationEntry container = new NotificationEntryBuilder()
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java
index b3c34c1..f36ffe6 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java
@@ -335,7 +335,7 @@
.build();
row.performDismiss(false);
verify(mNotificationTestHelper.mOnUserInteractionCallback)
- .onDismiss(any(), anyInt(), any());
+ .registerFutureDismissal(any(), anyInt());
}
@Test
@@ -347,6 +347,6 @@
.build();
row.performDismiss(false);
verify(mNotificationTestHelper.mOnUserInteractionCallback, never())
- .onDismiss(any(), anyInt(), any());
+ .registerFutureDismissal(any(), anyInt());
}
}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java
index d5ed37a..7a8b329 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java
@@ -23,8 +23,11 @@
import static com.android.systemui.statusbar.NotificationEntryHelper.modifyRanking;
import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import android.annotation.Nullable;
import android.app.ActivityManager;
@@ -121,6 +124,7 @@
private StatusBarStateController mStatusBarStateController;
private final PeopleNotificationIdentifier mPeopleNotificationIdentifier;
public final OnUserInteractionCallback mOnUserInteractionCallback;
+ public final Runnable mFutureDismissalRunnable;
public NotificationTestHelper(
Context context,
@@ -182,6 +186,9 @@
mBindPipelineEntryListener = collectionListenerCaptor.getValue();
mPeopleNotificationIdentifier = mock(PeopleNotificationIdentifier.class);
mOnUserInteractionCallback = mock(OnUserInteractionCallback.class);
+ mFutureDismissalRunnable = mock(Runnable.class);
+ when(mOnUserInteractionCallback.registerFutureDismissal(any(), anyInt()))
+ .thenReturn(mFutureDismissalRunnable);
}
/**
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java
index f1a6eba..273b9a1 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java
@@ -93,6 +93,7 @@
import org.mockito.stubbing.Answer;
import java.util.ArrayList;
+import java.util.List;
import java.util.Optional;
@SmallTest
@@ -141,6 +142,8 @@
@Mock
private OnUserInteractionCallback mOnUserInteractionCallback;
@Mock
+ private Runnable mFutureDismissalRunnable;
+ @Mock
private StatusBarNotificationActivityStarter mNotificationActivityStarter;
@Mock
private ActivityLaunchAnimator mActivityLaunchAnimator;
@@ -187,8 +190,8 @@
when(mEntryManager.getVisibleNotifications()).thenReturn(mActiveNotifications);
when(mStatusBarStateController.getState()).thenReturn(StatusBarState.SHADE);
when(mNotifPipelineFlags.isNewPipelineEnabled()).thenReturn(false);
- when(mOnUserInteractionCallback.getGroupSummaryToDismiss(mNotificationRow.getEntry()))
- .thenReturn(null);
+ when(mOnUserInteractionCallback.registerFutureDismissal(eq(mNotificationRow.getEntry()),
+ anyInt())).thenReturn(mFutureDismissalRunnable);
when(mVisibilityProvider.obtain(anyString(), anyBoolean()))
.thenAnswer(invocation -> NotificationVisibility.obtain(
invocation.getArgument(0), 0, 1, false));
@@ -264,6 +267,10 @@
@Test
public void testOnNotificationClicked_keyGuardShowing()
throws PendingIntent.CanceledException, RemoteException {
+ // To get the order right, collect posted runnables and run them later
+ List<Runnable> runnables = new ArrayList<>();
+ doAnswer(answerVoid(r -> runnables.add((Runnable) r)))
+ .when(mHandler).post(any(Runnable.class));
// Given
NotificationEntry entry = mNotificationRow.getEntry();
Notification notification = entry.getSbn().getNotification();
@@ -275,6 +282,8 @@
// When
mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow);
+ // Run the collected runnables in fifo order, the way post() really does.
+ while (!runnables.isEmpty()) runnables.remove(0).run();
// Then
verify(mShadeController, atLeastOnce()).collapsePanel();
@@ -284,12 +293,14 @@
verify(mAssistManager).hideAssist();
- InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback);
+ InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback,
+ mFutureDismissalRunnable);
+ // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL
+ orderVerifier.verify(mOnUserInteractionCallback)
+ .registerFutureDismissal(eq(entry), eq(REASON_CLICK));
orderVerifier.verify(mClickNotifier).onNotificationClick(
eq(entry.getKey()), any(NotificationVisibility.class));
- // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL
- orderVerifier.verify(mOnUserInteractionCallback).onDismiss(entry,
- REASON_CLICK, null);
+ orderVerifier.verify(mFutureDismissalRunnable).run();
}
@Test
@@ -319,8 +330,9 @@
verifyZeroInteractions(mContentIntent);
// Notification should not be cancelled.
- verify(mOnUserInteractionCallback, never()).onDismiss(eq(mNotificationRow.getEntry()),
- anyInt(), eq(null));
+ verify(mOnUserInteractionCallback, never())
+ .registerFutureDismissal(eq(mNotificationRow.getEntry()), anyInt());
+ verify(mFutureDismissalRunnable, never()).run();
}
@Test