diff --git a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java index eab970626bf1b..924d16dd27d7c 100644 --- a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java +++ b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java @@ -89,7 +89,7 @@ public class ForegroundServiceNotificationListener { } @Override - public void onEntryRemoved(NotificationEntry entry, int reason, boolean removedByUser) { + public void onEntryRemoved(NotificationEntry entry, int reason) { removeNotification(entry.getSbn()); } }); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java index eaa9d78c08f42..7fe229c26f3af 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java @@ -16,6 +16,9 @@ package com.android.systemui.statusbar.notification.collection; +import static com.android.systemui.statusbar.notification.collection.NotifCollection.REASON_NOT_CANCELED; +import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; + import java.util.Arrays; import java.util.List; @@ -132,8 +135,20 @@ public class ListDumper { .append(" "); } + if (notifEntry.mCancellationReason != REASON_NOT_CANCELED) { + rksb.append("cancellationReason=") + .append(notifEntry.mCancellationReason) + .append(" "); + } + if (notifEntry.hasInflationError()) { - rksb.append("hasInflationError "); + rksb.append("(!)hasInflationError "); + } + + if (notifEntry.getDismissState() != NOT_DISMISSED) { + rksb.append("dismissState=") + .append(notifEntry.getDismissState()) + .append(" "); } String rkString = rksb.toString(); 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 1b6170326bacc..3b2fe9441c325 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 @@ -35,9 +35,14 @@ import static android.service.notification.NotificationListenerService.REASON_TI import static android.service.notification.NotificationListenerService.REASON_UNAUTOBUNDLED; import static android.service.notification.NotificationListenerService.REASON_USER_STOPPED; +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 java.util.Objects.requireNonNull; + import android.annotation.IntDef; import android.annotation.MainThread; -import android.annotation.NonNull; import android.annotation.Nullable; import android.os.RemoteException; import android.service.notification.NotificationListenerService.Ranking; @@ -45,6 +50,8 @@ import android.service.notification.NotificationListenerService.RankingMap; import android.service.notification.StatusBarNotification; import android.util.ArrayMap; +import androidx.annotation.NonNull; + import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.DumpController; import com.android.systemui.Dumpable; @@ -172,15 +179,65 @@ public class NotifCollection implements Dumpable { /** * Dismiss a notification on behalf of the user. */ - void dismissNotification( - NotificationEntry entry, - @CancellationReason int reason, - @NonNull DismissedByUserStats stats) { + void dismissNotification(NotificationEntry entry, @NonNull DismissedByUserStats stats) { Assert.isMainThread(); - Objects.requireNonNull(stats); + requireNonNull(stats); checkForReentrantCall(); - removeNotification(entry.getKey(), null, reason, stats); + if (entry != mNotificationSet.get(entry.getKey())) { + throw new IllegalStateException("Invalid entry: " + entry.getKey()); + } + + if (entry.getDismissState() == DISMISSED) { + return; + } + + // Optimistically mark the notification as dismissed -- we'll wait for the signal from + // system server before removing it from our notification set. + entry.setDismissState(DISMISSED); + mLogger.logNotifDismissed(entry.getKey()); + + List canceledEntries = new ArrayList<>(); + + if (isCanceled(entry)) { + canceledEntries.add(entry); + } else { + // Ask system server to remove it for us + try { + mStatusBarService.onNotificationClear( + entry.getSbn().getPackageName(), + entry.getSbn().getTag(), + entry.getSbn().getId(), + entry.getSbn().getUser().getIdentifier(), + entry.getSbn().getKey(), + stats.dismissalSurface, + stats.dismissalSentiment, + stats.notificationVisibility); + } catch (RemoteException e) { + // system process is dead if we're here. + } + + // Also mark any children as dismissed as system server will auto-dismiss them as well + if (entry.getSbn().getNotification().isGroupSummary()) { + for (NotificationEntry otherEntry : mNotificationSet.values()) { + if (otherEntry.getSbn().getGroupKey().equals(entry.getSbn().getGroupKey()) + && otherEntry.getDismissState() != DISMISSED) { + otherEntry.setDismissState(PARENT_DISMISSED); + if (isCanceled(otherEntry)) { + canceledEntries.add(otherEntry); + } + } + } + } + } + + // Immediately remove any dismissed notifs that have already been canceled by system server + // (probably due to being lifetime-extended up until this point). + for (NotificationEntry canceledEntry : canceledEntries) { + tryRemoveNotification(canceledEntry); + } + + rebuildList(); } private void onNotificationPosted(StatusBarNotification sbn, RankingMap rankingMap) { @@ -208,7 +265,15 @@ public class NotifCollection implements Dumpable { Assert.isMainThread(); mLogger.logNotifRemoved(sbn.getKey(), reason); - removeNotification(sbn.getKey(), rankingMap, reason, null); + + final NotificationEntry entry = mNotificationSet.get(sbn.getKey()); + if (entry == null) { + throw new IllegalStateException("No notification to remove with key " + sbn.getKey()); + } + entry.mCancellationReason = reason; + applyRanking(rankingMap); + tryRemoveNotification(entry); + rebuildList(); } private void onNotificationRankingUpdate(RankingMap rankingMap) { @@ -242,9 +307,12 @@ public class NotifCollection implements Dumpable { // Update to an existing entry mLogger.logNotifUpdated(sbn.getKey()); - // Notification is updated so it is essentially re-added and thus alive again. Don't + cancelLocalDismissal(entry); + + // Notification is updated so it is essentially re-added and thus alive again. Don't // need to keep its lifetime extended. cancelLifetimeExtension(entry); + entry.mCancellationReason = REASON_NOT_CANCELED; entry.setSbn(sbn); if (rankingMap != null) { @@ -255,59 +323,42 @@ public class NotifCollection implements Dumpable { } } - private void removeNotification( - String key, - @Nullable RankingMap rankingMap, - @CancellationReason int reason, - @Nullable DismissedByUserStats dismissedByUserStats) { - - NotificationEntry entry = mNotificationSet.get(key); - if (entry == null) { - throw new IllegalStateException("No notification to remove with key " + key); + /** + * Tries to remove a notification from the notification set. This removal may be blocked by + * lifetime extenders. Does not trigger a rebuild of the list; caller must do that manually. + * + * @return True if the notification was removed, false otherwise. + */ + private boolean tryRemoveNotification(NotificationEntry entry) { + if (mNotificationSet.get(entry.getKey()) != entry) { + throw new IllegalStateException("No notification to remove with key " + entry.getKey()); } - entry.mLifetimeExtenders.clear(); - mAmDispatchingToOtherCode = true; - for (NotifLifetimeExtender extender : mLifetimeExtenders) { - if (extender.shouldExtendLifetime(entry, reason)) { - entry.mLifetimeExtenders.add(extender); - } + if (!isCanceled(entry)) { + throw new IllegalStateException("Cannot remove notification " + entry.getKey() + + ": has not been marked for removal"); + } + + if (isDismissedByUser(entry)) { + // User-dismissed notifications cannot be lifetime-extended + cancelLifetimeExtension(entry); + } else { + updateLifetimeExtension(entry); } - mAmDispatchingToOtherCode = false; if (!isLifetimeExtended(entry)) { mNotificationSet.remove(entry.getKey()); - - if (dismissedByUserStats != null) { - try { - mStatusBarService.onNotificationClear( - entry.getSbn().getPackageName(), - entry.getSbn().getTag(), - entry.getSbn().getId(), - entry.getSbn().getUser().getIdentifier(), - entry.getSbn().getKey(), - dismissedByUserStats.dismissalSurface, - dismissedByUserStats.dismissalSentiment, - dismissedByUserStats.notificationVisibility); - } catch (RemoteException e) { - // system process is dead if we're here. - } - } - - if (rankingMap != null) { - applyRanking(rankingMap); - } - - dispatchOnEntryRemoved(entry, reason, dismissedByUserStats != null /* removedByUser */); + dispatchOnEntryRemoved(entry, entry.mCancellationReason); dispatchOnEntryCleanUp(entry); + return true; + } else { + return false; } - - rebuildList(); } private void applyRanking(@NonNull RankingMap rankingMap) { for (NotificationEntry entry : mNotificationSet.values()) { - if (!isLifetimeExtended(entry)) { + if (!isCanceled(entry)) { // TODO: (b/148791039) We should crash if we are ever handed a ranking with // incomplete entries. Right now, there's a race condition in NotificationListener @@ -355,9 +406,9 @@ public class NotifCollection implements Dumpable { } if (!isLifetimeExtended(entry)) { - // TODO: This doesn't need to be undefined -- we can set either EXTENDER_EXPIRED or - // save the original reason - removeNotification(entry.getKey(), null, REASON_UNKNOWN, null); + if (tryRemoveNotification(entry)) { + rebuildList(); + } } } @@ -374,6 +425,31 @@ public class NotifCollection implements Dumpable { return entry.mLifetimeExtenders.size() > 0; } + private void updateLifetimeExtension(NotificationEntry entry) { + entry.mLifetimeExtenders.clear(); + mAmDispatchingToOtherCode = true; + for (NotifLifetimeExtender extender : mLifetimeExtenders) { + if (extender.shouldExtendLifetime(entry, entry.mCancellationReason)) { + entry.mLifetimeExtenders.add(extender); + } + } + mAmDispatchingToOtherCode = false; + } + + private void cancelLocalDismissal(NotificationEntry entry) { + if (isDismissedByUser(entry)) { + entry.setDismissState(NOT_DISMISSED); + if (entry.getSbn().getNotification().isGroupSummary()) { + for (NotificationEntry otherEntry : mNotificationSet.values()) { + if (otherEntry.getSbn().getGroupKey().equals(entry.getSbn().getGroupKey()) + && otherEntry.getDismissState() == PARENT_DISMISSED) { + otherEntry.setDismissState(NOT_DISMISSED); + } + } + } + } + } + private void checkForReentrantCall() { if (mAmDispatchingToOtherCode) { throw new IllegalStateException("Reentrant call detected"); @@ -389,6 +465,19 @@ public class NotifCollection implements Dumpable { return ranking; } + /** + * True if the notification has been canceled by system server. Usually, such notifications are + * immediately removed from the collection, but can sometimes stick around due to lifetime + * extenders. + */ + private static boolean isCanceled(NotificationEntry entry) { + return entry.mCancellationReason != REASON_NOT_CANCELED; + } + + private static boolean isDismissedByUser(NotificationEntry entry) { + return entry.getDismissState() != NOT_DISMISSED; + } + private void dispatchOnEntryInit(NotificationEntry entry) { mAmDispatchingToOtherCode = true; for (NotifCollectionListener listener : mNotifCollectionListeners) { @@ -421,13 +510,10 @@ public class NotifCollection implements Dumpable { mAmDispatchingToOtherCode = false; } - private void dispatchOnEntryRemoved( - NotificationEntry entry, - @CancellationReason int reason, - boolean removedByUser) { + private void dispatchOnEntryRemoved(NotificationEntry entry, @CancellationReason int reason) { mAmDispatchingToOtherCode = true; for (NotifCollectionListener listener : mNotifCollectionListeners) { - listener.onEntryRemoved(entry, reason, removedByUser); + listener.onEntryRemoved(entry, reason); } mAmDispatchingToOtherCode = false; } @@ -439,6 +525,20 @@ public class NotifCollection implements Dumpable { } mAmDispatchingToOtherCode = false; } + @Override + public void dump(@NonNull FileDescriptor fd, PrintWriter pw, @NonNull String[] args) { + final List entries = new ArrayList<>(getActiveNotifs()); + + pw.println("\t" + TAG + " unsorted/unfiltered notifications:"); + if (entries.size() == 0) { + pw.println("\t\t None"); + } + pw.println( + ListDumper.dumpList( + entries, + true, + "\t\t")); + } private final BatchableNotificationHandler mNotifHandler = new BatchableNotificationHandler() { @Override @@ -494,20 +594,6 @@ public class NotifCollection implements Dumpable { @Retention(RetentionPolicy.SOURCE) public @interface CancellationReason {} + public static final int REASON_NOT_CANCELED = -1; public static final int REASON_UNKNOWN = 0; - - @Override - public void dump(FileDescriptor fd, PrintWriter pw, String[] args) { - final List entries = new ArrayList<>(getActiveNotifs()); - - pw.println("\t" + TAG + " unsorted/unfiltered notifications:"); - if (entries.size() == 0) { - pw.println("\t\t None"); - } - pw.println( - ListDumper.dumpList( - entries, - true, - "\t\t")); - } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java index e7b772f1c7b23..7a6d4f1c7d7bc 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java @@ -98,13 +98,12 @@ public class NotifInflaterImpl implements NotifInflater { @Override public void run() { int dismissalSurface = NotificationStats.DISMISSAL_SHADE; - /** + /* * TODO: determine dismissal surface (ie: shade / headsup / aod) * see {@link NotificationLogger#logNotificationClear} */ mNotifCollection.dismissNotification( entry, - 0, new DismissedByUserStats( dismissalSurface, DISMISS_SENTIMENT_NEUTRAL, diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java index 1f77ec229041d..a95668b5e48dd 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java @@ -29,9 +29,11 @@ import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_NOTIFICAT import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_PEEK; import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_STATUS_BAR; +import static com.android.systemui.statusbar.notification.collection.NotifCollection.REASON_NOT_CANCELED; import static com.android.systemui.statusbar.notification.stack.NotificationSectionsManager.BUCKET_ALERTING; -import android.annotation.NonNull; +import static java.util.Objects.requireNonNull; + import android.app.Notification; import android.app.Notification.MessagingStyle.Message; import android.app.NotificationChannel; @@ -50,6 +52,7 @@ import android.util.ArraySet; import android.view.View; import android.widget.ImageView; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.android.internal.annotations.VisibleForTesting; @@ -59,6 +62,7 @@ import com.android.internal.util.ContrastColorUtil; import com.android.systemui.statusbar.InflationTask; import com.android.systemui.statusbar.StatusBarIconView; import com.android.systemui.statusbar.notification.InflationException; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifPromoter; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender; @@ -105,9 +109,18 @@ public final class NotificationEntry extends ListEntry { /** If this was a group child that was promoted to the top level, then who did the promoting. */ @Nullable NotifPromoter mNotifPromoter; - /** If this notification had an issue with inflating. Only used with the NewNotifPipeline **/ + /** + * If this notification was cancelled by system server, then the reason that was supplied. + * Uncancelled notifications always have REASON_NOT_CANCELED. Note that lifetime-extended + * notifications will have this set even though they are still in the active notification set. + */ + @CancellationReason int mCancellationReason = REASON_NOT_CANCELED; + + /** @see #hasInflationError() */ private boolean mHasInflationError; + /** @see #getDismissState() */ + @NonNull private DismissState mDismissState = DismissState.NOT_DISMISSED; /* * Old members @@ -169,9 +182,9 @@ public final class NotificationEntry extends ListEntry { public NotificationEntry( @NonNull StatusBarNotification sbn, @NonNull Ranking ranking) { - super(Objects.requireNonNull(Objects.requireNonNull(sbn).getKey())); + super(requireNonNull(requireNonNull(sbn).getKey())); - Objects.requireNonNull(ranking); + requireNonNull(ranking); mKey = sbn.getKey(); setSbn(sbn); @@ -201,8 +214,8 @@ public final class NotificationEntry extends ListEntry { * TODO: Make this package-private */ public void setSbn(@NonNull StatusBarNotification sbn) { - Objects.requireNonNull(sbn); - Objects.requireNonNull(sbn.getKey()); + requireNonNull(sbn); + requireNonNull(sbn.getKey()); if (!sbn.getKey().equals(mKey)) { throw new IllegalArgumentException("New key " + sbn.getKey() @@ -227,8 +240,8 @@ public final class NotificationEntry extends ListEntry { * TODO: Make this package-private */ public void setRanking(@NonNull Ranking ranking) { - Objects.requireNonNull(ranking); - Objects.requireNonNull(ranking.getKey()); + requireNonNull(ranking); + requireNonNull(ranking.getKey()); if (!ranking.getKey().equals(mKey)) { throw new IllegalArgumentException("New key " + ranking.getKey() @@ -238,6 +251,34 @@ public final class NotificationEntry extends ListEntry { mRanking = ranking; } + /* + * Bookkeeping getters and setters + */ + + /** + * Whether this notification had an error when attempting to inflate. This is only used in + * the NewNotifPipeline + */ + public boolean hasInflationError() { + return mHasInflationError; + } + + void setHasInflationError(boolean hasError) { + mHasInflationError = hasError; + } + + /** + * Set if the user has dismissed this notif but we haven't yet heard back from system server to + * confirm the dismissal. + */ + @NonNull public DismissState getDismissState() { + return mDismissState; + } + + void setDismissState(@NonNull DismissState dismissState) { + mDismissState = requireNonNull(dismissState); + } + /* * Convenience getters for SBN and Ranking members */ @@ -275,7 +316,6 @@ public final class NotificationEntry extends ListEntry { return mRanking.canBubble(); } - public @NonNull List getSmartActions() { return mRanking.getSmartActions(); } @@ -578,18 +618,6 @@ public final class NotificationEntry extends ListEntry { remoteInputTextWhenReset = null; } - void setHasInflationError(boolean hasError) { - mHasInflationError = hasError; - } - - /** - * Whether this notification had an error when attempting to inflate. This is only used in - * the NewNotifPipeline - */ - public boolean hasInflationError() { - return mHasInflationError; - } - public void setHasSentReply() { hasSentReply = true; } @@ -974,6 +1002,16 @@ public final class NotificationEntry extends ListEntry { } } + /** @see #getDismissState() */ + public enum DismissState { + /** User has not dismissed this notif or its parent */ + NOT_DISMISSED, + /** User has dismissed this notif specifically */ + DISMISSED, + /** User has dismissed this notif's parent (which implicitly dismisses this one as well) */ + PARENT_DISMISSED, + } + private static final long LAUNCH_COOLDOWN = 2000; private static final long REMOTE_INPUT_COOLDOWN = 500; private static final long INITIALIZATION_DELAY = 400; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HideLocallyDismissedNotifsCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HideLocallyDismissedNotifsCoordinator.java new file mode 100644 index 0000000000000..0059e7baa3c2a --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HideLocallyDismissedNotifsCoordinator.java @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2020 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.coordinator; + +import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; + +import com.android.systemui.statusbar.notification.collection.NotifPipeline; +import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; + +/** + * Filters out notifications that have been dismissed locally (by the user) but that system server + * hasn't yet confirmed the removal of. + */ +public class HideLocallyDismissedNotifsCoordinator implements Coordinator { + @Override + public void attach(NotifPipeline pipeline) { + pipeline.addPreGroupFilter(mFilter); + } + + private final NotifFilter mFilter = new NotifFilter("HideLocallyDismissedNotifsFilter") { + @Override + public boolean shouldFilterOut(NotificationEntry entry, long now) { + return entry.getDismissState() != NOT_DISMISSED; + } + }; +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.java index 8d0dd5b111ba4..0a1e09f4c99db 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.java @@ -56,6 +56,7 @@ public class NotifCoordinators implements Dumpable { PreparationCoordinator preparationCoordinator) { dumpController.registerDumpable(TAG, this); + mCoordinators.add(new HideLocallyDismissedNotifsCoordinator()); mCoordinators.add(keyguardCoordinator); mCoordinators.add(rankingCoordinator); mCoordinators.add(foregroundCoordinator); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java index 20c9cbc8790db..41314b86695ac 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java @@ -72,7 +72,7 @@ public class PreparationCoordinator implements Coordinator { } @Override - public void onEntryRemoved(NotificationEntry entry, int reason, boolean removedByUser) { + public void onEntryRemoved(NotificationEntry entry, int reason) { abortInflation(entry, "entryRemoved reason=" + reason); } }; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionListener.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionListener.java index ff6da12bd0bcc..b2c53dae16cdc 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionListener.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionListener.java @@ -54,10 +54,7 @@ public interface NotifCollectionListener { * immediately after a user dismisses a notification: we wait until we receive confirmation from * system server before considering the notification removed. */ - default void onEntryRemoved( - NotificationEntry entry, - @CancellationReason int reason, - boolean removedByUser) { + default void onEntryRemoved(NotificationEntry entry, @CancellationReason int reason) { } /** 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 0d0a46adb41fa..14e15031056f0 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 @@ -61,6 +61,14 @@ class NotifCollectionLogger @Inject constructor( }) } + fun logNotifDismissed(key: String) { + buffer.log(TAG, INFO, { + str1 = key + }, { + "DISMISSED $str1" + }) + } + fun logRankingMissing(key: String, rankingMap: RankingMap) { buffer.log(TAG, WARNING, { str1 = key }, { "Ranking update is missing ranking for $str1" }) buffer.log(TAG, DEBUG, {}, { "Ranking map contents:" }) 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 b65298097ff15..7c94ed20e95a1 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 @@ -17,9 +17,16 @@ package com.android.systemui.statusbar.notification.collection; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL; +import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CLICK; +import static android.service.notification.NotificationStats.DISMISSAL_SHADE; +import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; +import static com.android.systemui.statusbar.notification.collection.NotifCollection.REASON_NOT_CANCELED; import static com.android.systemui.statusbar.notification.collection.NotifCollection.REASON_UNKNOWN; +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 org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -40,7 +47,6 @@ import static java.util.Objects.requireNonNull; import android.annotation.Nullable; import android.os.RemoteException; import android.service.notification.NotificationListenerService.Ranking; -import android.service.notification.NotificationStats; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; import android.util.ArrayMap; @@ -77,6 +83,7 @@ import org.mockito.Spy; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -236,7 +243,7 @@ public class NotifCollectionTest extends SysuiTestCase { mNoMan.retractNotif(notif.sbn, REASON_APP_CANCEL); // THEN the listener is notified - verify(mCollectionListener).onEntryRemoved(entry, REASON_APP_CANCEL, false); + verify(mCollectionListener).onEntryRemoved(entry, REASON_APP_CANCEL); verify(mCollectionListener).onEntryCleanUp(entry); assertEquals(notif.sbn, entry.getSbn()); assertEquals(notif.ranking, entry.getRanking()); @@ -322,24 +329,15 @@ public class NotifCollectionTest extends SysuiTestCase { } @Test - public void testDismissNotification() throws RemoteException { - // GIVEN a collection with a couple notifications and a lifetime extender - mCollection.addNotificationLifetimeExtender(mExtender1); - + public void testDismissNotificationSentToSystemServer() throws RemoteException { + // GIVEN a collection with a couple notifications NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); NotifEvent notif2 = mNoMan.postNotif(buildNotif(TEST_PACKAGE2, 88, "barTag")); NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); // WHEN a notification is manually dismissed - DismissedByUserStats stats = new DismissedByUserStats( - NotificationStats.DISMISSAL_SHADE, - NotificationStats.DISMISS_SENTIMENT_NEUTRAL, - NotificationVisibility.obtain(entry2.getKey(), 7, 2, true)); - - mCollection.dismissNotification(entry2, REASON_CLICK, stats); - - // THEN we check for lifetime extension - verify(mExtender1).shouldExtendLifetime(entry2, REASON_CLICK); + DismissedByUserStats stats = defaultStats(entry2); + mCollection.dismissNotification(entry2, defaultStats(entry2)); // THEN we send the dismissal to system server verify(mStatusBarService).onNotificationClear( @@ -351,9 +349,96 @@ public class NotifCollectionTest extends SysuiTestCase { stats.dismissalSurface, stats.dismissalSentiment, stats.notificationVisibility); + } - // THEN we fire a remove event - verify(mCollectionListener).onEntryRemoved(entry2, REASON_CLICK, true); + @Test + public void testDismissedNotificationsAreMarkedAsDismissedLocally() { + // GIVEN a collection with a notification + NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN a notification is manually dismissed + mCollection.dismissNotification(entry1, defaultStats(entry1)); + + // THEN the entry is marked as dismissed locally + assertEquals(DISMISSED, entry1.getDismissState()); + } + + @Test + public void testDismissedNotificationsCannotBeLifetimeExtended() { + // GIVEN a collection with a notification and a lifetime extender + mCollection.addNotificationLifetimeExtender(mExtender1); + NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN a notification is manually dismissed + mCollection.dismissNotification(entry1, defaultStats(entry1)); + + // THEN lifetime extenders are never queried + verify(mExtender1, never()).shouldExtendLifetime(eq(entry1), anyInt()); + } + + @Test + public void testDismissedNotificationsDoNotTriggerRemovalEvents() { + // GIVEN a collection with a notification + NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN a notification is manually dismissed + mCollection.dismissNotification(entry1, defaultStats(entry1)); + + // THEN onEntryRemoved is not called + verify(mCollectionListener, never()).onEntryRemoved(eq(entry1), anyInt()); + } + + @Test + public void testDismissedNotificationsStillAppearInNotificationSet() { + // GIVEN a collection with a notification + NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN a notification is manually dismissed + mCollection.dismissNotification(entry1, defaultStats(entry1)); + + // THEN the dismissed entry still appears in the notification set + assertEquals( + new ArraySet<>(Collections.singletonList(entry1)), + new ArraySet<>(mCollection.getActiveNotifs())); + } + + @Test + public void testDismissingLifetimeExtendedSummaryDoesNotDismissChildren() { + // GIVEN A notif group with one summary and two children + mCollection.addNotificationLifetimeExtender(mExtender1); + NotifEvent notif1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 1, "myTag") + .setGroup(mContext, GROUP_1) + .setGroupSummary(mContext, true)); + NotifEvent notif2 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 2, "myTag") + .setGroup(mContext, GROUP_1)); + NotifEvent notif3 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 3, "myTag") + .setGroup(mContext, GROUP_1)); + + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); + NotificationEntry entry3 = mCollectionListener.getEntry(notif3.key); + + // GIVEN that the summary and one child are retracted, but both are lifetime-extended + mExtender1.shouldExtendLifetime = true; + mNoMan.retractNotif(notif1.sbn, REASON_CANCEL); + mNoMan.retractNotif(notif2.sbn, REASON_CANCEL); + assertEquals( + new ArraySet<>(List.of(entry1, entry2, entry3)), + new ArraySet<>(mCollection.getActiveNotifs())); + + // WHEN the summary is dismissed by the user + mCollection.dismissNotification(entry1, defaultStats(entry1)); + + // THEN the summary is removed, but both children stick around + assertEquals( + new ArraySet<>(List.of(entry2, entry3)), + new ArraySet<>(mCollection.getActiveNotifs())); + assertEquals(NOT_DISMISSED, entry2.getDismissState()); + assertEquals(NOT_DISMISSED, entry3.getDismissState()); } @Test(expected = IllegalStateException.class) @@ -366,14 +451,114 @@ public class NotifCollectionTest extends SysuiTestCase { mNoMan.retractNotif(notif2.sbn, REASON_UNKNOWN); // WHEN we try to dismiss a notification that isn't present - mCollection.dismissNotification( - entry2, - REASON_CLICK, - new DismissedByUserStats(0, 0, NotificationVisibility.obtain("foo", 47, 3, true))); + mCollection.dismissNotification(entry2, defaultStats(entry2)); // THEN an exception is thrown } + @Test + public void testGroupChildrenAreDismissedLocallyWhenSummaryIsDismissed() { + // GIVEN a collection with two grouped notifs in it + NotifEvent notif0 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0) + .setGroup(mContext, GROUP_1) + .setGroupSummary(mContext, true)); + NotifEvent notif1 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 1) + .setGroup(mContext, GROUP_1)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN the summary is dismissed + mCollection.dismissNotification(entry0, defaultStats(entry0)); + + // THEN all members of the group are marked as dismissed locally + assertEquals(DISMISSED, entry0.getDismissState()); + assertEquals(PARENT_DISMISSED, entry1.getDismissState()); + } + + @Test + public void testUpdatingDismissedSummaryBringsChildrenBack() { + // GIVEN a collection with two grouped notifs in it + NotifEvent notif0 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0) + .setGroup(mContext, GROUP_1) + .setGroupSummary(mContext, true)); + NotifEvent notif1 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 1) + .setGroup(mContext, GROUP_1)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN the summary is dismissed but then reposted without a group + mCollection.dismissNotification(entry0, defaultStats(entry0)); + NotifEvent notif0a = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0)); + + // THEN it and all of its previous children are no longer dismissed locally + assertEquals(NOT_DISMISSED, entry0.getDismissState()); + assertEquals(NOT_DISMISSED, entry1.getDismissState()); + } + + @Test + public void testDismissedChildrenAreNotResetByParentUpdate() { + // GIVEN a collection with three grouped notifs in it + NotifEvent notif0 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0) + .setGroup(mContext, GROUP_1) + .setGroupSummary(mContext, true)); + NotifEvent notif1 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 1) + .setGroup(mContext, GROUP_1)); + NotifEvent notif2 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 2) + .setGroup(mContext, GROUP_1)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); + + // WHEN a child is dismissed, then the parent is dismissed, then the parent is updated + mCollection.dismissNotification(entry1, defaultStats(entry1)); + mCollection.dismissNotification(entry0, defaultStats(entry0)); + NotifEvent notif0a = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0)); + + // THEN the manually-dismissed child is still marked as dismissed + assertEquals(NOT_DISMISSED, entry0.getDismissState()); + assertEquals(DISMISSED, entry1.getDismissState()); + assertEquals(NOT_DISMISSED, entry2.getDismissState()); + } + + @Test + public void testUpdatingGroupKeyOfDismissedSummaryBringsChildrenBack() { + // GIVEN a collection with two grouped notifs in it + NotifEvent notif0 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0) + .setOverrideGroupKey(GROUP_1) + .setGroupSummary(mContext, true)); + NotifEvent notif1 = mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 1) + .setOverrideGroupKey(GROUP_1)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + + // WHEN the summary is dismissed but then reposted AND in the same update one of the + // children's ranking loses its override group + mCollection.dismissNotification(entry0, defaultStats(entry0)); + mNoMan.setRanking(entry1.getKey(), new RankingBuilder() + .setKey(entry1.getKey()) + .build()); + mNoMan.postNotif( + buildNotif(TEST_PACKAGE, 0) + .setOverrideGroupKey(GROUP_1) + .setGroupSummary(mContext, true)); + + // THEN it and all of its previous children are no longer dismissed locally, including the + // child that is no longer part of the group + assertEquals(NOT_DISMISSED, entry0.getDismissState()); + assertEquals(NOT_DISMISSED, entry1.getDismissState()); + } + @Test public void testLifetimeExtendersAreQueriedWhenNotifRemoved() { // GIVEN a couple notifications and a few lifetime extenders @@ -389,12 +574,12 @@ public class NotifCollectionTest extends SysuiTestCase { NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); // WHEN a notification is removed - mNoMan.retractNotif(notif2.sbn, REASON_UNKNOWN); + mNoMan.retractNotif(notif2.sbn, REASON_CLICK); // THEN each extender is asked whether to extend, even if earlier ones return true - verify(mExtender1).shouldExtendLifetime(entry2, REASON_UNKNOWN); - verify(mExtender2).shouldExtendLifetime(entry2, REASON_UNKNOWN); - verify(mExtender3).shouldExtendLifetime(entry2, REASON_UNKNOWN); + verify(mExtender1).shouldExtendLifetime(entry2, REASON_CLICK); + verify(mExtender2).shouldExtendLifetime(entry2, REASON_CLICK); + verify(mExtender3).shouldExtendLifetime(entry2, REASON_CLICK); // THEN the entry is not removed assertTrue(mCollection.getActiveNotifs().contains(entry2)); @@ -428,9 +613,9 @@ public class NotifCollectionTest extends SysuiTestCase { mExtender2.callback.onEndLifetimeExtension(mExtender2, entry2); // THEN each extender is re-queried - verify(mExtender1).shouldExtendLifetime(entry2, REASON_UNKNOWN); - verify(mExtender2).shouldExtendLifetime(entry2, REASON_UNKNOWN); - verify(mExtender3).shouldExtendLifetime(entry2, REASON_UNKNOWN); + verify(mExtender1).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender2).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender3).shouldExtendLifetime(entry2, REASON_APP_CANCEL); // THEN the entry is not removed assertTrue(mCollection.getActiveNotifs().contains(entry2)); @@ -466,9 +651,9 @@ public class NotifCollectionTest extends SysuiTestCase { assertTrue(mCollection.getActiveNotifs().contains(entry2)); // THEN we don't re-query the extenders - verify(mExtender1, never()).shouldExtendLifetime(eq(entry2), anyInt()); - verify(mExtender2, never()).shouldExtendLifetime(eq(entry2), anyInt()); - verify(mExtender3, never()).shouldExtendLifetime(eq(entry2), anyInt()); + verify(mExtender1, never()).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender2, never()).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender3, never()).shouldExtendLifetime(entry2, REASON_APP_CANCEL); // THEN the entry properly records all extenders that returned true assertEquals(Arrays.asList(mExtender1), entry2.mLifetimeExtenders); @@ -501,7 +686,7 @@ public class NotifCollectionTest extends SysuiTestCase { // THEN the entry removed assertFalse(mCollection.getActiveNotifs().contains(entry2)); - verify(mCollectionListener).onEntryRemoved(entry2, REASON_UNKNOWN, false); + verify(mCollectionListener).onEntryRemoved(entry2, REASON_UNKNOWN); } @Test @@ -591,6 +776,36 @@ public class NotifCollectionTest extends SysuiTestCase { assertEquals(notif2a.ranking, entry2.getRanking()); } + @Test + public void testCancellationReasonIsSetWhenNotifIsCancelled() { + // GIVEN a notification + NotifEvent notif0 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 3)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + + // WHEN the notification is retracted + mNoMan.retractNotif(notif0.sbn, REASON_APP_CANCEL); + + // THEN the retraction reason is stored on the notif + assertEquals(REASON_APP_CANCEL, entry0.mCancellationReason); + } + + @Test + public void testCancellationReasonIsClearedWhenNotifIsUpdated() { + // GIVEN a notification and a lifetime extender that will preserve it + NotifEvent notif0 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 3)); + NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); + mCollection.addNotificationLifetimeExtender(mExtender1); + mExtender1.shouldExtendLifetime = true; + + // WHEN the notification is retracted and subsequently reposted + mNoMan.retractNotif(notif0.sbn, REASON_APP_CANCEL); + assertEquals(REASON_APP_CANCEL, entry0.mCancellationReason); + mNoMan.postNotif(buildNotif(TEST_PACKAGE, 3)); + + // THEN the notification has its cancellation reason cleared + assertEquals(REASON_NOT_CANCELED, entry0.mCancellationReason); + } + private static NotificationEntryBuilder buildNotif(String pkg, int id, String tag) { return new NotificationEntryBuilder() .setPkg(pkg) @@ -604,6 +819,13 @@ public class NotifCollectionTest extends SysuiTestCase { .setId(id); } + private static DismissedByUserStats defaultStats(NotificationEntry entry) { + return new DismissedByUserStats( + DISMISSAL_SHADE, + DISMISS_SENTIMENT_NEUTRAL, + NotificationVisibility.obtain(entry.getKey(), 7, 2, true)); + } + private static class RecordingCollectionListener implements NotifCollectionListener { private final Map mLastSeenEntries = new ArrayMap<>(); @@ -621,7 +843,7 @@ public class NotifCollectionTest extends SysuiTestCase { } @Override - public void onEntryRemoved(NotificationEntry entry, int reason, boolean removedByUser) { + public void onEntryRemoved(NotificationEntry entry, int reason) { } @Override @@ -674,4 +896,6 @@ public class NotifCollectionTest extends SysuiTestCase { private static final String TEST_PACKAGE = "com.android.test.collection"; private static final String TEST_PACKAGE2 = "com.android.test.collection2"; + + private static final String GROUP_1 = "group_1"; }