Merge "New Pipeline: Fix groups not grouping when shade is open."
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 8b0252b..9e5dab1 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
@@ -31,11 +31,6 @@
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.
*/
@@ -53,6 +48,11 @@
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
* the entry had it not been suppressed by the VisualStabilityManager.
@@ -60,14 +60,23 @@
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 @@
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 37eacad..915057f 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 @@
private final String mKey;
private final long mCreationTime;
- int mFirstAddedIteration = -1;
-
private final ListAttachState mPreviousAttachState = ListAttachState.create();
private final ListAttachState mAttachState = ListAttachState.create();
@@ -95,14 +93,6 @@
}
/**
- * 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 748c69d..120c722 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.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.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.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import javax.inject.Inject;
@@ -103,7 +108,7 @@
private final List<NotifFilter> mNotifFinalizeFilters = new ArrayList<>();
private final List<NotifComparator> mNotifComparators = new ArrayList<>();
private final List<NotifSection> mNotifSections = new ArrayList<>();
- @Nullable private NotifStabilityManager mNotifStabilityManager;
+ private NotifStabilityManager mNotifStabilityManager;
private final List<OnBeforeTransformGroupsListener> mOnBeforeTransformGroupsListeners =
new ArrayList<>();
@@ -228,7 +233,7 @@
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 @@
mNotifStabilityManager.setInvalidationListener(this::onReorderingAllowedInvalidated);
}
+ @NonNull
+ private NotifStabilityManager getStabilityManager() {
+ if (mNotifStabilityManager == null) {
+ return DefaultNotifStabilityManager.INSTANCE;
+ }
+ return mNotifStabilityManager;
+ }
+
void setComparators(List<NotifComparator> comparators) {
Assert.isMainThread();
mPipelineState.requireState(STATE_IDLE);
@@ -460,10 +473,6 @@
for (NotificationEntry entry : mAllEntries) {
entry.beginNewAttachState();
-
- if (entry.mFirstAddedIteration == -1) {
- entry.mFirstAddedIteration = mIterationCount;
- }
}
mNotifList.clear();
@@ -519,7 +528,6 @@
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 @@
}
private void stabilizeGroupingNotifs(List<ListEntry> topLevelList) {
- if (mNotifStabilityManager == null) {
+ if (getStabilityManager().isEveryChangeAllowed()) {
return;
}
Trace.beginSection("ShadeListBuilder.stabilizeGroupingNotifs");
@@ -612,7 +620,7 @@
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 @@
private void pruneIncompleteGroups(List<ListEntry> 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<String> 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<NotificationEntry> 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 @@
Trace.endSection();
}
+ private void pruneGroupAtIndexAndPromoteSummary(List<ListEntry> 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<ListEntry> 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<NotificationEntry> 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<String> getGroupsWithChildrenLostToStability(List<ListEntry> shadeList) {
+ if (getStabilityManager().isEveryChangeAllowed()) {
+ return Collections.emptySet();
+ }
+ ArraySet<String> 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 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
mNotifSections.get(i).getSectioner().onCleanup();
}
- if (mNotifStabilityManager != null) {
- callOnCleanup(List.of(mNotifStabilityManager));
- }
+ callOnCleanup(List.of(getStabilityManager()));
}
private void callOnCleanup(List<? extends Pluggable<?>> pluggables) {
@@ -1015,7 +1119,7 @@
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<NotifFilter> filters) {
@@ -1064,12 +1168,10 @@
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 bbb97d1..ec4e039 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 @@
}
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 @@
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 dd4794f..f835250 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 @@
})
}
+ 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 75489b1..327876c 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 @@
}
@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 @@
}
@Override
- public boolean isSectionChangeAllowed(NotificationEntry entry) {
+ public boolean isSectionChangeAllowed(@NonNull NotificationEntry entry) {
final boolean isSectionChangeAllowedForEntry =
mReorderingAllowed
|| mHeadsUpManager.isAlerting(entry.getKey())
@@ -138,7 +138,7 @@
}
@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 798bfe7..feb48da 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 @@
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 @@
return state == mState;
}
+ /** Get the current state's integer representation */
public @StateName int getState() {
return mState;
}
@@ -75,6 +78,41 @@
}
}
+ /** 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 8fff905..ba3e855 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 @@
})
}
- 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 @@
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 @@
newParent: GroupEntry?
) {
buffer.log(TAG, INFO, {
- int1 = buildId
+ long1 = buildId.toLong()
str1 = key
str2 = prevParent?.key
str3 = newParent?.key
@@ -153,22 +153,22 @@
"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 @@
keepingParent: GroupEntry?
) {
buffer.log(TAG, INFO, {
- int1 = buildId
+ long1 = buildId.toLong()
str1 = suppressedParent?.key
str2 = keepingParent?.key
}, {
@@ -192,24 +192,38 @@
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 @@
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.java
deleted file mode 100644
index cb2d3cb..0000000
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.java
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- * 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.listbuilder.pluggable;
-
-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
- * section changes.
- *
- * The stability manager should be invalidated when previously suppressed a group or
- * section change is now allowed.
- */
-public abstract class NotifStabilityManager extends Pluggable<NotifStabilityManager> {
-
- protected NotifStabilityManager(String name) {
- super(name);
- }
-
- /**
- * Called at the beginning of every pipeline run to perform any necessary cleanup from the
- * previous run.
- */
- public abstract void 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
- * this entry is being suppressed.
- */
- public abstract boolean isGroupChangeAllowed(NotificationEntry entry);
-
- /**
- * 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
- * being suppressed.
- */
- public abstract boolean isSectionChangeAllowed(NotificationEntry entry);
-
- /**
- * Is a notification entry is allowed be reordered
- * @param entry
- * @return if can re-order
- */
- public abstract boolean isEntryReorderingAllowed(ListEntry entry);
-
- /**
- * Called by the pipeline to determine if every call to the other stability methods would
- * return true, regardless of parameters. This allows the pipeline to skip any pieces of
- * work related to stability.
- *
- * @return true if all other methods will return true for any parameters.
- */
- public abstract boolean isEveryChangeAllowed();
-
- /**
- * Called by the pipeline to inform the stability manager that an entry reordering was indeed
- * suppressed as the result of a previous call to {@link #isEntryReorderingAllowed(ListEntry)}.
- */
- public abstract void onEntryReorderSuppressed();
-}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt
new file mode 100644
index 0000000..60f557c
--- /dev/null
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifStabilityManager.kt
@@ -0,0 +1,85 @@
+/*
+ * 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.listbuilder.pluggable
+
+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
+ * section changes.
+ *
+ * The stability manager should be invalidated when previously suppressed a group or
+ * section change is now allowed.
+ */
+abstract class NotifStabilityManager protected constructor(name: String) :
+ Pluggable<NotifStabilityManager>(name) {
+ /**
+ * Called at the beginning of every pipeline run to perform any necessary cleanup from the
+ * previous run.
+ */
+ 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 false, it's expected that a group change for
+ * this entry is being suppressed.
+ */
+ 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 false, it's expected that a section change is
+ * being suppressed.
+ */
+ abstract fun isSectionChangeAllowed(entry: NotificationEntry): Boolean
+
+ /**
+ * 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.
+ */
+ abstract fun isEntryReorderingAllowed(entry: ListEntry): Boolean
+
+ /**
+ * Called by the pipeline to determine if every call to the other stability methods would
+ * return true, regardless of parameters. This allows the pipeline to skip any pieces of
+ * work related to stability.
+ *
+ * @return true if all other methods will return true for any parameters.
+ */
+ abstract fun isEveryChangeAllowed(): Boolean
+
+ /**
+ * Called by the pipeline to inform the stability manager that an entry reordering was indeed
+ * suppressed as the result of a previous call to [.isEntryReorderingAllowed].
+ */
+ abstract fun onEntryReorderSuppressed()
+}
+
+/** The default, no-op instance of the stability manager which always allows all changes */
+object DefaultNotifStabilityManager : NotifStabilityManager("DefaultNotifStabilityManager") {
+ override fun onBeginRun() {}
+ override fun isGroupChangeAllowed(entry: NotificationEntry): Boolean = true
+ override fun isSectionChangeAllowed(entry: NotificationEntry): Boolean = true
+ override fun isEntryReorderingAllowed(entry: ListEntry): Boolean = true
+ override fun isEveryChangeAllowed(): Boolean = true
+ override fun onEntryReorderSuppressed() {}
+}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java
index 82cd9fa..b832577 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java
@@ -114,6 +114,7 @@
private List<NotificationEntry> mEntrySet = new ArrayList<>();
private List<ListEntry> mBuiltList;
private TestableStabilityManager mStabilityManager;
+ private TestableNotifFilter mFinalizeFilter;
private Map<String, Integer> mNextIdMap = new ArrayMap<>();
private int mNextRank = 0;
@@ -136,6 +137,8 @@
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 @@
// 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 @@
}
@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 @@
// because group changes aren't allowed by the stability manager
verifyBuiltList(
notif(0),
- notif(2)
+ group(
+ summary(1),
+ child(2)
+ )
);
}
@@ -1110,11 +1115,13 @@
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),
@@ -1209,6 +1216,140 @@
}
@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
addGroupChild(0, PACKAGE_2, GROUP_1);
@@ -1228,30 +1369,6 @@
}
@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
addGroupChild(0, PACKAGE_1, GROUP_1);
@@ -1269,13 +1386,12 @@
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 @@
}
}
+ private class TestableNotifFilter extends NotifFilter {
+ ArrayList<Integer> 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 @@
}
@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;
}