From 3850e10f2f19e1a95402fa137ee972a25472ba7b Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 24 Nov 2021 21:38:54 +0000 Subject: [PATCH] New Pipeline: Fix groups not grouping when shade is open. * Add logging when the reason an entry's group is pruned changes. * Remove the incorrectly implemented ListEntry.hasBeenAttachedBefore(), and replace with ListEntry.wasAttachedInPreviousPass() which is sufficient. * Fix ShadeListBuilder "change" logs containing many rows that didn't really change just because stableIndex changes frequently. * Fix ShadeListBuilderLogger errors with the buildId not appearing. Bug: 205099899 Test: atest PreparationCoordinatorTest ShadeListBuilderTest Change-Id: Icf6bbcdce557ec6bb5f18da734caca89649f177a --- .../collection/ListAttachState.kt | 37 ++- .../notification/collection/ListEntry.java | 10 - .../collection/ShadeListBuilder.java | 242 +++++++++++++----- .../coordinator/PreparationCoordinator.java | 5 +- .../PreparationCoordinatorLogger.kt | 10 +- .../VisualStabilityCoordinator.java | 6 +- .../collection/listbuilder/PipelineState.java | 38 +++ .../listbuilder/ShadeListBuilderLogger.kt | 50 ++-- ...yManager.java => NotifStabilityManager.kt} | 51 ++-- .../collection/ShadeListBuilderTest.java | 201 ++++++++++++--- 10 files changed, 475 insertions(+), 175 deletions(-) rename packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/{NotifStabilityManager.java => NotifStabilityManager.kt} (53%) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListAttachState.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListAttachState.kt index 8b0252bf8ea4b..9e5dab1152ece 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListAttachState.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListAttachState.kt @@ -30,11 +30,6 @@ data class ListAttachState private constructor( */ var parent: GroupEntry?, - /** - * Identifies the notification order in the entire notification list - */ - var stableIndex: Int = -1, - /** * The section that this ListEntry was sorted into. If the child of the group, this will be the * parent's section. Null if not attached to the list. @@ -52,6 +47,11 @@ data class ListAttachState private constructor( */ var promoter: NotifPromoter?, + /** + * If an entry's group was pruned from the list by NotifPipeline logic, the reason is here. + */ + var groupPruneReason: String?, + /** * If the [VisualStabilityManager] is suppressing group or section changes for this entry, * suppressedChanges will contain the new parent or section that we would have assigned to @@ -60,14 +60,23 @@ data class ListAttachState private constructor( var suppressedChanges: SuppressedAttachState ) { + /** + * Identifies the notification order in the entire notification list. + * NOTE: this property is intentionally excluded from equals calculation (by not making it a + * constructor arg) because its value changes based on the presence of other members in the + * list, rather than anything having to do with this entry's attachment. + */ + var stableIndex: Int = -1 + /** Copies the state of another instance. */ fun clone(other: ListAttachState) { parent = other.parent - stableIndex = other.stableIndex section = other.section excludingFilter = other.excludingFilter promoter = other.promoter + groupPruneReason = other.groupPruneReason suppressedChanges.clone(other.suppressedChanges) + stableIndex = other.stableIndex } /** Resets back to a "clean" state (the same as created by the factory method) */ @@ -76,20 +85,22 @@ data class ListAttachState private constructor( section = null excludingFilter = null promoter = null - stableIndex = -1 + groupPruneReason = null suppressedChanges.reset() + stableIndex = -1 } companion object { @JvmStatic fun create(): ListAttachState { return ListAttachState( - null, - -1, - null, - null, - null, - SuppressedAttachState.create()) + null, + null, + null, + null, + null, + SuppressedAttachState.create() + ) } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListEntry.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListEntry.java index 37eacada19fc2..915057fe735ba 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListEntry.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListEntry.java @@ -31,8 +31,6 @@ public abstract class ListEntry { private final String mKey; private final long mCreationTime; - int mFirstAddedIteration = -1; - private final ListAttachState mPreviousAttachState = ListAttachState.create(); private final ListAttachState mAttachState = ListAttachState.create(); @@ -94,14 +92,6 @@ public abstract class ListEntry { return mPreviousAttachState; } - /** - * True if this entry has been attached to the shade at least once in its lifetime (it may not - * currently be attached). - */ - public boolean hasBeenAttachedBefore() { - return mFirstAddedIteration != -1; - } - /** * Stores the current attach state into {@link #getPreviousAttachState()}} and then starts a * fresh attach state (all entries will be null/default-initialized). diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java index 748c69d24c361..120c7228e372b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java @@ -16,6 +16,8 @@ package com.android.systemui.statusbar.notification.collection; +import static com.android.internal.util.Preconditions.checkArgument; +import static com.android.internal.util.Preconditions.checkState; import static com.android.systemui.statusbar.notification.collection.GroupEntry.ROOT_ENTRY; import static com.android.systemui.statusbar.notification.collection.listbuilder.PipelineState.STATE_BUILD_STARTED; import static com.android.systemui.statusbar.notification.collection.listbuilder.PipelineState.STATE_FINALIZE_FILTERING; @@ -34,6 +36,7 @@ import android.annotation.MainThread; import android.annotation.Nullable; import android.os.Trace; import android.util.ArrayMap; +import android.util.ArraySet; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; @@ -50,6 +53,7 @@ import com.android.systemui.statusbar.notification.collection.listbuilder.OnBefo import com.android.systemui.statusbar.notification.collection.listbuilder.OnBeforeTransformGroupsListener; import com.android.systemui.statusbar.notification.collection.listbuilder.PipelineState; import com.android.systemui.statusbar.notification.collection.listbuilder.ShadeListBuilderLogger; +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.DefaultNotifStabilityManager; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.Invalidator; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifComparator; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; @@ -72,6 +76,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import javax.inject.Inject; @@ -103,7 +108,7 @@ public class ShadeListBuilder implements Dumpable { private final List mNotifFinalizeFilters = new ArrayList<>(); private final List mNotifComparators = new ArrayList<>(); private final List mNotifSections = new ArrayList<>(); - @Nullable private NotifStabilityManager mNotifStabilityManager; + private NotifStabilityManager mNotifStabilityManager; private final List mOnBeforeTransformGroupsListeners = new ArrayList<>(); @@ -228,7 +233,7 @@ public class ShadeListBuilder implements Dumpable { mNotifSections.add(new NotifSection(DEFAULT_SECTIONER, mNotifSections.size())); } - void setNotifStabilityManager(NotifStabilityManager notifStabilityManager) { + void setNotifStabilityManager(@NonNull NotifStabilityManager notifStabilityManager) { Assert.isMainThread(); mPipelineState.requireState(STATE_IDLE); @@ -244,6 +249,14 @@ public class ShadeListBuilder implements Dumpable { mNotifStabilityManager.setInvalidationListener(this::onReorderingAllowedInvalidated); } + @NonNull + private NotifStabilityManager getStabilityManager() { + if (mNotifStabilityManager == null) { + return DefaultNotifStabilityManager.INSTANCE; + } + return mNotifStabilityManager; + } + void setComparators(List comparators) { Assert.isMainThread(); mPipelineState.requireState(STATE_IDLE); @@ -460,10 +473,6 @@ public class ShadeListBuilder implements Dumpable { for (NotificationEntry entry : mAllEntries) { entry.beginNewAttachState(); - - if (entry.mFirstAddedIteration == -1) { - entry.mFirstAddedIteration = mIterationCount; - } } mNotifList.clear(); @@ -519,7 +528,6 @@ public class ShadeListBuilder implements Dumpable { GroupEntry group = mGroups.get(topLevelKey); if (group == null) { group = new GroupEntry(topLevelKey, mSystemClock.uptimeMillis()); - group.mFirstAddedIteration = mIterationCount; mGroups.put(topLevelKey, group); } if (group.getParent() == null) { @@ -569,7 +577,7 @@ public class ShadeListBuilder implements Dumpable { } private void stabilizeGroupingNotifs(List topLevelList) { - if (mNotifStabilityManager == null) { + if (getStabilityManager().isEveryChangeAllowed()) { return; } Trace.beginSection("ShadeListBuilder.stabilizeGroupingNotifs"); @@ -612,7 +620,7 @@ public class ShadeListBuilder implements Dumpable { final GroupEntry prevParent = entry.getPreviousAttachState().getParent(); final GroupEntry assignedParent = entry.getParent(); if (prevParent != assignedParent - && !mNotifStabilityManager.isGroupChangeAllowed(entry.getRepresentativeEntry())) { + && !getStabilityManager().isGroupChangeAllowed(entry.getRepresentativeEntry())) { entry.getAttachState().getSuppressedChanges().setParent(assignedParent); entry.setParent(prevParent); if (prevParent == ROOT_ENTRY) { @@ -655,56 +663,60 @@ public class ShadeListBuilder implements Dumpable { private void pruneIncompleteGroups(List shadeList) { Trace.beginSection("ShadeListBuilder.pruneIncompleteGroups"); + // Any group which lost a child on this run to stability is exempt from being pruned or + // having its summary promoted, regardless of how many children it has + Set groupsWithChildrenLostToStability = + getGroupsWithChildrenLostToStability(shadeList); for (int i = 0; i < shadeList.size(); i++) { final ListEntry tle = shadeList.get(i); if (tle instanceof GroupEntry) { final GroupEntry group = (GroupEntry) tle; final List children = group.getRawChildren(); + final boolean hasSummary = group.getSummary() != null; - if (group.getSummary() != null && children.size() == 0) { - NotificationEntry summary = group.getSummary(); - summary.setParent(ROOT_ENTRY); - // The list may be sorted; replace the group with the summary, in its place - shadeList.set(i, summary); - - group.setSummary(null); - annulAddition(group, shadeList); + if (hasSummary && children.size() == 0) { + if (groupsWithChildrenLostToStability.contains(group.getKey())) { + // This group lost a child on this run to stability, so it is exempt from + // having its summary promoted to the top level, so prune it. + // It has no children, so it will just vanish. + pruneGroupAtIndexAndPromoteAnyChildren(shadeList, group, i); + } else { + // For any other summary with no children, promote the summary. + pruneGroupAtIndexAndPromoteSummary(shadeList, group, i); + } i--; // The node we visited is gone, so be sure to visit this index again. - } else if (group.getSummary() == null - || children.size() < MIN_CHILDREN_FOR_GROUP) { + } else if (!hasSummary) { + // If the group doesn't provide a summary, ignore it and add + // any children it may have directly to top-level. + pruneGroupAtIndexAndPromoteAnyChildren(shadeList, group, i); - if (group.getSummary() != null - && group.wasAttachedInPreviousPass() - && mNotifStabilityManager != null - && !mNotifStabilityManager.isGroupChangeAllowed(group.getSummary())) { - // if this group was previously attached and group changes aren't - // allowed, keep it around until group changes are allowed again + i--; // The node we visited is gone, so be sure to visit this index again. + } else if (children.size() < MIN_CHILDREN_FOR_GROUP) { + // This group has a summary and insufficient, but nonzero children. + checkState(hasSummary, "group must have summary at this point"); + checkState(!children.isEmpty(), "empty group should have been promoted"); + + if (groupsWithChildrenLostToStability.contains(group.getKey())) { + // This group lost a child on this run to stability, so it is exempt from + // the "min children" requirement; keep it around in case more children are + // added before changes are allowed again. + group.getAttachState().getSuppressedChanges().setWasPruneSuppressed(true); + continue; + } + if (group.wasAttachedInPreviousPass() + && !getStabilityManager().isGroupChangeAllowed(group.getSummary())) { + checkState(!children.isEmpty(), "empty group should have been pruned"); + // This group was previously attached and group changes aren't + // allowed; keep it around until group changes are allowed again. group.getAttachState().getSuppressedChanges().setWasPruneSuppressed(true); continue; } - // If the group doesn't provide a summary or is too small, ignore it and add + // The group is too small, ignore it and add // its children (if any) directly to top-level. - - shadeList.remove(i); - - if (group.getSummary() != null) { - final NotificationEntry summary = group.getSummary(); - group.setSummary(null); - annulAddition(summary, shadeList); - } - - for (int j = 0; j < children.size(); j++) { - final NotificationEntry child = children.get(j); - child.setParent(ROOT_ENTRY); - // The list may be sorted, so add the children in order where the group was. - shadeList.add(i + j, child); - } - children.clear(); - - annulAddition(group, shadeList); + pruneGroupAtIndexAndPromoteAnyChildren(shadeList, group, i); i--; // The node we visited is gone, so be sure to visit this index again. } @@ -713,6 +725,99 @@ public class ShadeListBuilder implements Dumpable { Trace.endSection(); } + private void pruneGroupAtIndexAndPromoteSummary(List shadeList, + GroupEntry group, int index) { + // Validate that the group has no children + checkArgument(group.getChildren().isEmpty(), "group should have no children"); + + NotificationEntry summary = group.getSummary(); + summary.setParent(ROOT_ENTRY); + // The list may be sorted; replace the group with the summary, in its place + ListEntry oldEntry = shadeList.set(index, summary); + + // Validate that the replaced entry was the group entry + checkState(oldEntry == group); + + group.setSummary(null); + annulAddition(group, shadeList); + summary.getAttachState().setGroupPruneReason( + "SUMMARY with no children @ " + mPipelineState.getStateName()); + } + + private void pruneGroupAtIndexAndPromoteAnyChildren(List shadeList, + GroupEntry group, int index) { + // REMOVE the GroupEntry at this index + ListEntry oldEntry = shadeList.remove(index); + + // Validate that the replaced entry was the group entry + checkState(oldEntry == group); + + List children = group.getRawChildren(); + boolean hasSummary = group.getSummary() != null; + + // Remove the group summary, if present, and leave detached. + if (hasSummary) { + final NotificationEntry summary = group.getSummary(); + group.setSummary(null); + annulAddition(summary, shadeList); + summary.getAttachState().setGroupPruneReason( + "SUMMARY with too few children @ " + mPipelineState.getStateName()); + } + + // Promote any children + if (!children.isEmpty()) { + // create the reason we will report on the child for why its group was pruned. + String childReason = hasSummary + ? ("CHILD with " + (children.size() - 1) + " siblings @ " + + mPipelineState.getStateName()) + : ("CHILD with no summary @ " + mPipelineState.getStateName()); + + // Remove children from the group and add them to the shadeList. + for (int j = 0; j < children.size(); j++) { + final NotificationEntry child = children.get(j); + child.setParent(ROOT_ENTRY); + child.getAttachState().setGroupPruneReason(requireNonNull(childReason)); + } + // The list may be sorted, so add the children in order where the group was. + shadeList.addAll(index, children); + children.clear(); + } + + annulAddition(group, shadeList); + } + + /** + * Collect the keys of any groups which have already lost a child to stability this run. + * + * If stability is being enforced, then {@link #stabilizeGroupingNotifs(List)} might have + * detached some children from their groups and left them at the top level because the child was + * previously attached at the top level. Doing so would set the + * {@link SuppressedAttachState#getParent() suppressed parent} for the current attach state. + * + * If we've already removed a child from this group, we don't want to remove any more children + * from the group (even if that would leave only a single notification in the group) because + * that could cascade over multiple runs and allow a large group of notifications all show up as + * top level (ungrouped) notifications. + */ + @NonNull + private Set getGroupsWithChildrenLostToStability(List shadeList) { + if (getStabilityManager().isEveryChangeAllowed()) { + return Collections.emptySet(); + } + ArraySet groupsWithChildrenLostToStability = new ArraySet<>(); + for (int i = 0; i < shadeList.size(); i++) { + final ListEntry tle = shadeList.get(i); + final GroupEntry suppressedParent = + tle.getAttachState().getSuppressedChanges().getParent(); + if (suppressedParent != null) { + // This top-level-entry was supposed to be attached to this group, + // so mark the group as having lost a child to stability. + groupsWithChildrenLostToStability.add(suppressedParent.getKey()); + } + } + return groupsWithChildrenLostToStability; + } + /** * If a ListEntry was added to the shade list and then later removed (e.g. because it was a * group that was broken up), this method will erase any bookkeeping traces of that addition @@ -727,10 +832,9 @@ public class ShadeListBuilder implements Dumpable { // lot of them), it will put the system into an inconsistent state. So we check all of them // here. - if (entry.getParent() == null || entry.mFirstAddedIteration == -1) { + if (entry.getParent() == null) { throw new IllegalStateException( - "Cannot nullify addition of " + entry.getKey() + ": no such addition. (" - + entry.getParent() + " " + entry.mFirstAddedIteration + ")"); + "Cannot nullify addition of " + entry.getKey() + ": no parent."); } if (entry.getParent() == ROOT_ENTRY) { @@ -771,9 +875,6 @@ public class ShadeListBuilder implements Dumpable { entry.setParent(null); entry.getAttachState().setSection(null); entry.getAttachState().setPromoter(null); - if (entry.mFirstAddedIteration == mIterationCount) { - entry.mFirstAddedIteration = -1; - } } private void assignSections() { @@ -804,12 +905,12 @@ public class ShadeListBuilder implements Dumpable { assignIndexes(mNotifList); // Check for suppressed order changes - if (!mNotifStabilityManager.isEveryChangeAllowed()) { + if (!getStabilityManager().isEveryChangeAllowed()) { mForceReorderable = true; boolean isSorted = isSorted(mNotifList, mTopLevelComparator); mForceReorderable = false; if (!isSorted) { - mNotifStabilityManager.onEntryReorderSuppressed(); + getStabilityManager().onEntryReorderSuppressed(); } } Trace.endSection(); @@ -897,12 +998,26 @@ public class ShadeListBuilder implements Dumpable { curr.getParent()); } + if (curr.getSuppressedChanges().getSection() != null) { + mLogger.logSectionChangeSuppressed( + mIterationCount, + curr.getSuppressedChanges().getSection(), + curr.getSection()); + } + if (curr.getSuppressedChanges().getWasPruneSuppressed()) { mLogger.logGroupPruningSuppressed( mIterationCount, curr.getParent()); } + if (!Objects.equals(curr.getGroupPruneReason(), prev.getGroupPruneReason())) { + mLogger.logPrunedReasonChanged( + mIterationCount, + prev.getGroupPruneReason(), + curr.getGroupPruneReason()); + } + if (curr.getExcludingFilter() != prev.getExcludingFilter()) { mLogger.logFilterChanged( mIterationCount, @@ -927,20 +1042,11 @@ public class ShadeListBuilder implements Dumpable { prev.getSection(), curr.getSection()); } - - if (curr.getSuppressedChanges().getSection() != null) { - mLogger.logSectionChangeSuppressed( - mIterationCount, - curr.getSuppressedChanges().getSection(), - curr.getSection()); - } } } private void onBeginRun() { - if (mNotifStabilityManager != null) { - mNotifStabilityManager.onBeginRun(); - } + getStabilityManager().onBeginRun(); } private void cleanupPluggables() { @@ -953,9 +1059,7 @@ public class ShadeListBuilder implements Dumpable { mNotifSections.get(i).getSectioner().onCleanup(); } - if (mNotifStabilityManager != null) { - callOnCleanup(List.of(mNotifStabilityManager)); - } + callOnCleanup(List.of(getStabilityManager())); } private void callOnCleanup(List> pluggables) { @@ -1015,7 +1119,7 @@ public class ShadeListBuilder implements Dumpable { private boolean mForceReorderable = false; private boolean canReorder(ListEntry entry) { - return mForceReorderable || mNotifStabilityManager.isEntryReorderingAllowed(entry); + return mForceReorderable || getStabilityManager().isEntryReorderingAllowed(entry); } private boolean applyFilters(NotificationEntry entry, long now, List filters) { @@ -1064,12 +1168,10 @@ public class ShadeListBuilder implements Dumpable { NotifSection finalSection = newSection; // have we seen this entry before and are we changing its section? - if (mNotifStabilityManager != null - && entry.wasAttachedInPreviousPass() - && newSection != prevAttachState.getSection()) { + if (entry.wasAttachedInPreviousPass() && newSection != prevAttachState.getSection()) { // are section changes allowed? - if (!mNotifStabilityManager.isSectionChangeAllowed(entry.getRepresentativeEntry())) { + if (!getStabilityManager().isSectionChangeAllowed(entry.getRepresentativeEntry())) { // record the section that we wanted to change to entry.getAttachState().getSuppressedChanges().setSection(newSection); 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 bbb97d145b9f9..ec4e0391c171b 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 @@ -385,7 +385,7 @@ public class PreparationCoordinator implements Coordinator { } private boolean shouldWaitForGroupToInflate(GroupEntry group, long now) { - if (group == GroupEntry.ROOT_ENTRY || group.hasBeenAttachedBefore()) { + if (group == GroupEntry.ROOT_ENTRY || group.wasAttachedInPreviousPass()) { return false; } if (isBeyondGroupInitializationWindow(group, now)) { @@ -397,11 +397,12 @@ public class PreparationCoordinator implements Coordinator { return true; } for (NotificationEntry child : group.getChildren()) { - if (mInflatingNotifs.contains(child) && !child.hasBeenAttachedBefore()) { + if (mInflatingNotifs.contains(child) && !child.wasAttachedInPreviousPass()) { mLogger.logDelayingGroupRelease(group.getKey(), child.getKey()); return true; } } + mLogger.logDoneWaitingForGroupInflation(group.getKey()); return false; } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt index dd4794f0a4520..f8352500923e1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt @@ -41,11 +41,19 @@ class PreparationCoordinatorLogger @Inject constructor( }) } + fun logDoneWaitingForGroupInflation(groupKey: String) { + buffer.log(TAG, LogLevel.DEBUG, { + str1 = groupKey + }, { + "Finished inflating all members of group $str1, releasing group" + }) + } + fun logGroupInflationTookTooLong(groupKey: String) { buffer.log(TAG, LogLevel.WARNING, { str1 = groupKey }, { - "Group inflation took too long far $str1, releasing children early" + "Group inflation took too long for $str1, releasing children early" }) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinator.java index 75489b1faadb8..327876c95b7f0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinator.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinator.java @@ -118,7 +118,7 @@ public class VisualStabilityCoordinator implements Coordinator, Dumpable { } @Override - public boolean isGroupChangeAllowed(NotificationEntry entry) { + public boolean isGroupChangeAllowed(@NonNull NotificationEntry entry) { final boolean isGroupChangeAllowedForEntry = mReorderingAllowed || mHeadsUpManager.isAlerting(entry.getKey()); mIsSuppressingGroupChange |= !isGroupChangeAllowedForEntry; @@ -126,7 +126,7 @@ public class VisualStabilityCoordinator implements Coordinator, Dumpable { } @Override - public boolean isSectionChangeAllowed(NotificationEntry entry) { + public boolean isSectionChangeAllowed(@NonNull NotificationEntry entry) { final boolean isSectionChangeAllowedForEntry = mReorderingAllowed || mHeadsUpManager.isAlerting(entry.getKey()) @@ -138,7 +138,7 @@ public class VisualStabilityCoordinator implements Coordinator, Dumpable { } @Override - public boolean isEntryReorderingAllowed(ListEntry section) { + public boolean isEntryReorderingAllowed(@NonNull ListEntry section) { return mReorderingAllowed; } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/PipelineState.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/PipelineState.java index 798bfe7f39d08..feb48daf78d83 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/PipelineState.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/PipelineState.java @@ -18,6 +18,8 @@ package com.android.systemui.statusbar.notification.collection.listbuilder; import android.annotation.IntDef; +import androidx.annotation.NonNull; + import com.android.systemui.statusbar.notification.collection.ShadeListBuilder; import java.lang.annotation.Retention; @@ -35,6 +37,7 @@ public class PipelineState { return state == mState; } + /** Get the current state's integer representation */ public @StateName int getState() { return mState; } @@ -75,6 +78,41 @@ public class PipelineState { } } + /** Get the current state's string representation */ + @NonNull + public String getStateName() { + return getStateName(mState); + } + + /** Get the given state's string representation */ + @NonNull + public static String getStateName(@StateName int state) { + switch (state) { + case STATE_IDLE: + return "STATE_IDLE"; + case STATE_BUILD_STARTED: + return "STATE_BUILD_STARTED"; + case STATE_RESETTING: + return "STATE_RESETTING"; + case STATE_PRE_GROUP_FILTERING: + return "STATE_PRE_GROUP_FILTERING"; + case STATE_GROUPING: + return "STATE_GROUPING"; + case STATE_TRANSFORMING: + return "STATE_TRANSFORMING"; + case STATE_GROUP_STABILIZING: + return "STATE_GROUP_STABILIZING"; + case STATE_SORTING: + return "STATE_SORTING"; + case STATE_FINALIZE_FILTERING: + return "STATE_FINALIZE_FILTERING"; + case STATE_FINALIZING: + return "STATE_FINALIZING"; + default: + return "STATE:" + state; + } + } + public static final int STATE_IDLE = 0; public static final int STATE_BUILD_STARTED = 1; public static final int STATE_RESETTING = 2; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/ShadeListBuilderLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/ShadeListBuilderLogger.kt index 8fff905047981..ba3e8554c16e2 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/ShadeListBuilderLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/ShadeListBuilderLogger.kt @@ -37,9 +37,9 @@ class ShadeListBuilderLogger @Inject constructor( }) } - fun logEndBuildList(iterationCount: Int, topLevelEntries: Int, numChildren: Int) { + fun logEndBuildList(buildId: Int, topLevelEntries: Int, numChildren: Int) { buffer.log(TAG, INFO, { - long1 = iterationCount.toLong() + long1 = buildId.toLong() int1 = topLevelEntries int2 = numChildren }, { @@ -112,21 +112,21 @@ class ShadeListBuilderLogger @Inject constructor( fun logDuplicateSummary(buildId: Int, groupKey: String, existingKey: String, newKey: String) { buffer.log(TAG, WARNING, { - int1 = buildId + long1 = buildId.toLong() str1 = groupKey str2 = existingKey str3 = newKey }, { - """(Build $int1) Duplicate summary for group "$str1": "$str2" vs. "$str3"""" + """(Build $long1) Duplicate summary for group "$str1": "$str2" vs. "$str3"""" }) } fun logDuplicateTopLevelKey(buildId: Int, topLevelKey: String) { buffer.log(TAG, WARNING, { - int1 = buildId + long1 = buildId.toLong() str1 = topLevelKey }, { - "(Build $int1) Duplicate top-level key: $str1" + "(Build $long1) Duplicate top-level key: $str1" }) } @@ -137,7 +137,7 @@ class ShadeListBuilderLogger @Inject constructor( newParent: GroupEntry? ) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = key str2 = prevParent?.key str3 = newParent?.key @@ -153,22 +153,22 @@ class ShadeListBuilderLogger @Inject constructor( "MODIFIED (ATTACHED)" } - "(Build $int1) $action {$str1}" + "(Build $long1) $action {$str1}" }) } fun logParentChanged(buildId: Int, prevParent: GroupEntry?, newParent: GroupEntry?) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = prevParent?.key str2 = newParent?.key }, { if (str1 == null && str2 != null) { - "(Build $int1) Parent is {$str2}" + "(Build $long1) Parent is {$str2}" } else if (str1 != null && str2 == null) { - "(Build $int1) Parent was {$str1}" + "(Build $long1) Parent was {$str1}" } else { - "(Build $int1) Reparent: {$str1} -> {$str2}" + "(Build $long1) Reparent: {$str1} -> {$str2}" } }) } @@ -179,7 +179,7 @@ class ShadeListBuilderLogger @Inject constructor( keepingParent: GroupEntry? ) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = suppressedParent?.key str2 = keepingParent?.key }, { @@ -192,24 +192,38 @@ class ShadeListBuilderLogger @Inject constructor( keepingParent: GroupEntry? ) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = keepingParent?.key }, { "(Build $long1) Group pruning suppressed; keeping parent '$str1'" }) } + fun logPrunedReasonChanged( + buildId: Int, + prevReason: String?, + newReason: String? + ) { + buffer.log(TAG, INFO, { + long1 = buildId.toLong() + str1 = prevReason + str2 = newReason + }, { + "(Build $long1) Pruned reason changed: $str1 -> $str2" + }) + } + fun logFilterChanged( buildId: Int, prevFilter: NotifFilter?, newFilter: NotifFilter? ) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = prevFilter?.name str2 = newFilter?.name }, { - "(Build $int1) Filter changed: $str1 -> $str2" + "(Build $long1) Filter changed: $str1 -> $str2" }) } @@ -219,11 +233,11 @@ class ShadeListBuilderLogger @Inject constructor( newPromoter: NotifPromoter? ) { buffer.log(TAG, INFO, { - int1 = buildId + long1 = buildId.toLong() str1 = prevPromoter?.name str2 = newPromoter?.name }, { - "(Build $int1) Promoter changed: $str1 -> $str2" + "(Build $long1) Promoter changed: $str1 -> $str2" }) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt similarity index 53% rename from packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.java rename to packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt index cb2d3cb97468a..60f557c9bf7ef 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt @@ -13,11 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +package com.android.systemui.statusbar.notification.collection.listbuilder.pluggable -package com.android.systemui.statusbar.notification.collection.listbuilder.pluggable; - -import com.android.systemui.statusbar.notification.collection.ListEntry; -import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.ListEntry +import com.android.systemui.statusbar.notification.collection.NotificationEntry /** * Pluggable for participating in notif stabilization. In particular, suppressing group and @@ -26,40 +25,38 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry; * The stability manager should be invalidated when previously suppressed a group or * section change is now allowed. */ -public abstract class NotifStabilityManager extends Pluggable { - - protected NotifStabilityManager(String name) { - super(name); - } - +abstract class NotifStabilityManager protected constructor(name: String) : + Pluggable(name) { /** * Called at the beginning of every pipeline run to perform any necessary cleanup from the * previous run. */ - public abstract void onBeginRun(); + abstract fun onBeginRun() /** * Returns whether this notification can currently change groups/parents. * Per iteration of the notification pipeline, locally stores this information until the next - * run of the pipeline. When this method returns true, it's expected that a group change for + * run of the pipeline. When this method returns false, it's expected that a group change for * this entry is being suppressed. */ - public abstract boolean isGroupChangeAllowed(NotificationEntry entry); + abstract fun isGroupChangeAllowed(entry: NotificationEntry): Boolean /** * Returns whether this notification entry can currently change sections. * Per iteration of the notification pipeline, locally stores this information until the next - * run of the pipeline. When this method returns true, it's expected that a section change is + * run of the pipeline. When this method returns false, it's expected that a section change is * being suppressed. */ - public abstract boolean isSectionChangeAllowed(NotificationEntry entry); + abstract fun isSectionChangeAllowed(entry: NotificationEntry): Boolean /** - * Is a notification entry is allowed be reordered - * @param entry - * @return if can re-order + * Returns whether this list entry is allowed to be reordered within its section. + * Unlike [isGroupChangeAllowed] or [isSectionChangeAllowed], this method is called on every + * entry, so an implementation may not assume that returning false means an order change is + * being suppressed. However, if an order change is suppressed, that will be reported to ths + * implementation by calling [onEntryReorderSuppressed] after ordering is complete. */ - public abstract boolean isEntryReorderingAllowed(ListEntry entry); + abstract fun isEntryReorderingAllowed(entry: ListEntry): Boolean /** * Called by the pipeline to determine if every call to the other stability methods would @@ -68,11 +65,21 @@ public abstract class NotifStabilityManager extends Pluggable mEntrySet = new ArrayList<>(); private List mBuiltList; private TestableStabilityManager mStabilityManager; + private TestableNotifFilter mFinalizeFilter; private Map mNextIdMap = new ArrayMap<>(); private int mNextRank = 0; @@ -136,6 +137,8 @@ public class ShadeListBuilderTest extends SysuiTestCase { mStabilityManager = spy(new TestableStabilityManager()); mListBuilder.setNotifStabilityManager(mStabilityManager); + mFinalizeFilter = spy(new TestableNotifFilter()); + mListBuilder.addFinalizeFilter(mFinalizeFilter); Mockito.verify(mNotifCollection).setBuildListener(mBuildListenerCaptor.capture()); mReadyForBuildListener = Objects.requireNonNull(mBuildListenerCaptor.getValue()); @@ -408,7 +411,6 @@ public class ShadeListBuilderTest extends SysuiTestCase { // THEN the summary has a null parent and an unset firstAddedIteration assertNull(mEntrySet.get(1).getParent()); - assertEquals(-1, mEntrySet.get(1).mFirstAddedIteration); } @Test @@ -1037,7 +1039,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { } @Test - public void testStabilizeGroupsDoesNotAllowGrouping() { + public void testStabilizeGroupsDoesNotAllowGroupingExistingNotifications() { // GIVEN one group child without a summary yet addGroupChild(0, PACKAGE_1, GROUP_1); @@ -1056,7 +1058,10 @@ public class ShadeListBuilderTest extends SysuiTestCase { // because group changes aren't allowed by the stability manager verifyBuiltList( notif(0), - notif(2) + group( + summary(1), + child(2) + ) ); } @@ -1110,11 +1115,13 @@ public class ShadeListBuilderTest extends SysuiTestCase { dispatchBuild(); - // THEN all notifications are top-level and the summary doesn't show yet - // because group changes aren't allowed by the stability manager + // THEN first notification stays top-level but the other notifications are grouped. verifyBuiltList( notif(0), - notif(2), + group( + summary(1), + child(2) + ), group( summary(3), child(4), @@ -1208,6 +1215,140 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); } + @Test + public void testStabilityIsolationAllowsGroupToHaveSingleChild() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + + dispatchBuild(); + // NOTICE that the group is pruned and the child is moved to the top level + verifyBuiltList( + notif(1) // group with only one child is promoted + ); + + // WHEN another child is added while group changes are disabled. + mStabilityManager.setAllowGroupChanges(false); + addGroupChild(2, PACKAGE_1, GROUP_1); + + dispatchBuild(); + + // THEN the new child should be added to the group + verifyBuiltList( + group( + summary(0), + child(2) + ), + notif(1) + ); + } + + @Test + public void testStabilityIsolationExemptsGroupWithFinalizeFilteredChildFromShowingSummary() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + + dispatchBuild(); + // NOTICE that the group is pruned and the child is moved to the top level + verifyBuiltList( + notif(1) // group with only one child is promoted + ); + + // WHEN another child is added but still filtered while group changes are disabled. + mStabilityManager.setAllowGroupChanges(false); + mFinalizeFilter.mIndicesToFilter.add(2); + addGroupChild(2, PACKAGE_1, GROUP_1); + + dispatchBuild(); + + // THEN the new child should be shown without the summary + verifyBuiltList( + notif(1) // previously promoted child + ); + } + + @Test + public void testStabilityIsolationOfRemovedChildDoesNotExemptGroupFromPrune() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + + dispatchBuild(); + // NOTICE that the group is pruned and the child is moved to the top level + verifyBuiltList( + notif(1) // group with only one child is promoted + ); + + // WHEN a new child is added and the old one gets filtered while group changes are disabled. + mStabilityManager.setAllowGroupChanges(false); + mFinalizeFilter.mIndicesToFilter.add(1); + addGroupChild(2, PACKAGE_1, GROUP_1); + + dispatchBuild(); + + // THEN the new child should be shown without a group + verifyBuiltList( + notif(2) // previously promoted child + ); + } + + @Test + public void testFinalizeFilteredSummaryPromotesChildren() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + addGroupChild(2, PACKAGE_1, GROUP_1); + + // WHEN the parent is filtered out at the finalize step + mFinalizeFilter.mIndicesToFilter.add(0); + + dispatchBuild(); + + // THEN the children should be promoted to the top level + verifyBuiltList( + notif(1), + notif(2) + ); + } + + @Test + public void testFinalizeFilteredChildrenPromotesSummary() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + addGroupChild(2, PACKAGE_1, GROUP_1); + + // WHEN the parent is filtered out at the finalize step + mFinalizeFilter.mIndicesToFilter.add(1); + mFinalizeFilter.mIndicesToFilter.add(2); + + dispatchBuild(); + + // THEN the children should be promoted to the top level + verifyBuiltList( + notif(0) + ); + } + + @Test + public void testFinalizeFilteredChildPromotesSibling() { + // GIVEN a group with only one child was already drawn + addGroupSummary(0, PACKAGE_1, GROUP_1); + addGroupChild(1, PACKAGE_1, GROUP_1); + addGroupChild(2, PACKAGE_1, GROUP_1); + + // WHEN the parent is filtered out at the finalize step + mFinalizeFilter.mIndicesToFilter.add(1); + + dispatchBuild(); + + // THEN the children should be promoted to the top level + verifyBuiltList( + notif(2) + ); + } + @Test public void testBrokenGroupNotificationOrdering() { // GIVEN two group children with different sections & without a summary yet @@ -1227,30 +1368,6 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); } - @Test - public void testStabilizeGroupsHidesGroupSummary() { - // GIVEN one group child with a summary - addGroupChild(0, PACKAGE_1, GROUP_1); - addGroupSummary(1, PACKAGE_1, GROUP_1); - - dispatchBuild(); // group summary is hidden because it needs at least 2 children to group - - // GIVEN visual stability manager doesn't allow any group changes - mStabilityManager.setAllowGroupChanges(false); - - // WHEN we run the pipeline with the addition of a child - addGroupChild(2, PACKAGE_1, GROUP_1); - - dispatchBuild(); - - // THEN the children notifications are top-level and the summary still doesn't show yet - // because group changes aren't allowed by the stability manager - verifyBuiltList( - notif(0), - notif(2) - ); - } - @Test public void testStabilizeGroupsDelayedSummaryRendersAllNotifsTopLevel() { // GIVEN group children posted without a summary @@ -1269,13 +1386,12 @@ public class ShadeListBuilderTest extends SysuiTestCase { dispatchBuild(); - // THEN all entries are top-level since group changes aren't allowed + // THEN all entries are top-level, but summary is suppressed verifyBuiltList( notif(0), notif(1), notif(2), - notif(3), - notif(4) + notif(3) ); // WHEN visual stability manager allows group changes again @@ -1907,6 +2023,19 @@ public class ShadeListBuilderTest extends SysuiTestCase { } } + private class TestableNotifFilter extends NotifFilter { + ArrayList mIndicesToFilter = new ArrayList<>(); + + protected TestableNotifFilter() { + super("TestFilter"); + } + + @Override + public boolean shouldFilterOut(@NonNull NotificationEntry entry, long now) { + return mIndicesToFilter.stream().anyMatch(i -> notif(i).entry == entry); + } + } + private static class TestableStabilityManager extends NotifStabilityManager { boolean mAllowGroupChanges = true; boolean mAllowSectionChanges = true; @@ -1937,17 +2066,17 @@ public class ShadeListBuilderTest extends SysuiTestCase { } @Override - public boolean isGroupChangeAllowed(NotificationEntry entry) { + public boolean isGroupChangeAllowed(@NonNull NotificationEntry entry) { return mAllowGroupChanges; } @Override - public boolean isSectionChangeAllowed(NotificationEntry entry) { + public boolean isSectionChangeAllowed(@NonNull NotificationEntry entry) { return mAllowSectionChanges; } @Override - public boolean isEntryReorderingAllowed(ListEntry entry) { + public boolean isEntryReorderingAllowed(@NonNull ListEntry entry) { return mAllowEntryReodering; }