From f5adb86e645f3253af956025cb0c957c97aa58e9 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 28 Apr 2022 14:35:27 +0000 Subject: [PATCH] New pipeline: Separate stability rules for group pruning from group changing Group changes for alerting children are allowed under the stability manager rules, but we were using the same method to determine if the group as a whole could be pruned, but that shouldn't actually be allowed for alerting summaries - it results in the weird unstable animation (and is a regression from S). Though I made this change more cleanly, the effective delta of this change is just a boolean logic change from: isGroupChangeAllowed = isReorderingAllowed || isEntryAlerting to: isGroupChangeAllowed = isReorderingAllowed || (isEntryAlerting && !isPruning) Fixes: 230734197 Test: atest VisualStabilityCoordinatorTest ShadeListBuilderTest Change-Id: Ib1a927de2b4cc9b614be29deaf9754c2dc79390f --- .../collection/ShadeListBuilder.java | 2 +- .../coordinator/VisualStabilityCoordinator.java | 8 ++++++++ .../pluggable/NotifStabilityManager.kt | 10 ++++++++++ .../collection/ShadeListBuilderTest.java | 15 ++++++++++++++- .../VisualStabilityCoordinatorTest.java | 17 +++++++++++++++++ 5 files changed, 50 insertions(+), 2 deletions(-) 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 51af9559eda29..b98acbc1489ba 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 @@ -749,7 +749,7 @@ public class ShadeListBuilder implements Dumpable { continue; } if (group.wasAttachedInPreviousPass() - && !getStabilityManager().isGroupChangeAllowed(group.getSummary())) { + && !getStabilityManager().isGroupPruneAllowed(group)) { 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. 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 085ce859de877..d7bd95c0949e5 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 @@ -27,6 +27,7 @@ import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.dump.DumpManager; import com.android.systemui.keyguard.WakefulnessLifecycle; import com.android.systemui.plugins.statusbar.StatusBarStateController; +import com.android.systemui.statusbar.notification.collection.GroupEntry; import com.android.systemui.statusbar.notification.collection.ListEntry; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -138,6 +139,13 @@ public class VisualStabilityCoordinator implements Coordinator, Dumpable, return isGroupChangeAllowedForEntry; } + @Override + public boolean isGroupPruneAllowed(@NonNull GroupEntry entry) { + final boolean isGroupPruneAllowedForEntry = mReorderingAllowed; + mIsSuppressingGroupChange |= !isGroupPruneAllowedForEntry; + return isGroupPruneAllowedForEntry; + } + @Override public boolean isSectionChangeAllowed(@NonNull NotificationEntry entry) { final boolean isSectionChangeAllowedForEntry = 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 index 4464498646060..58bbca822164d 100644 --- 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 @@ -15,6 +15,7 @@ */ package com.android.systemui.statusbar.notification.collection.listbuilder.pluggable +import com.android.systemui.statusbar.notification.collection.GroupEntry import com.android.systemui.statusbar.notification.collection.ListEntry import com.android.systemui.statusbar.notification.collection.NotificationEntry @@ -51,6 +52,14 @@ abstract class NotifStabilityManager protected constructor(name: String) : */ abstract fun isGroupChangeAllowed(entry: NotificationEntry): Boolean + /** + * Returns whether this notification group can be pruned for not having enough children. + * 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 prune for + * this entry is being suppressed. + */ + abstract fun isGroupPruneAllowed(entry: GroupEntry): Boolean + /** * Returns whether this notification entry can currently change sections. * Per iteration of the notification pipeline, locally stores this information until the next @@ -89,6 +98,7 @@ object DefaultNotifStabilityManager : NotifStabilityManager("DefaultNotifStabili override fun isPipelineRunAllowed(): Boolean = true override fun onBeginRun() {} override fun isGroupChangeAllowed(entry: NotificationEntry): Boolean = true + override fun isGroupPruneAllowed(entry: GroupEntry): Boolean = true override fun isSectionChangeAllowed(entry: NotificationEntry): Boolean = true override fun isEntryReorderingAllowed(entry: ListEntry): Boolean = true override fun isEveryChangeAllowed(): Boolean = true 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 7638452eaf905..4304a6769ef62 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 @@ -1439,14 +1439,16 @@ public class ShadeListBuilderTest extends SysuiTestCase { // WHEN a new child is added and the old one gets filtered while group changes are disabled. mStabilityManager.setAllowGroupChanges(false); + mStabilityManager.setAllowGroupPruning(false); mFinalizeFilter.mIndicesToFilter.add(1); addGroupChild(2, PACKAGE_1, GROUP_1); dispatchBuild(); // THEN the new child should be shown without a group + // (Note that this is the same as the expected result if there were no stability rules.) verifyBuiltList( - notif(2) // previously promoted child + notif(2) // new child ); } @@ -2269,6 +2271,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { private static class TestableStabilityManager extends NotifStabilityManager { boolean mAllowPipelineRun = true; boolean mAllowGroupChanges = true; + boolean mAllowGroupPruning = true; boolean mAllowSectionChanges = true; boolean mAllowEntryReodering = true; @@ -2281,6 +2284,11 @@ public class ShadeListBuilderTest extends SysuiTestCase { return this; } + TestableStabilityManager setAllowGroupPruning(boolean allowGroupPruning) { + mAllowGroupPruning = allowGroupPruning; + return this; + } + TestableStabilityManager setAllowSectionChanges(boolean allowSectionChanges) { mAllowSectionChanges = allowSectionChanges; return this; @@ -2310,6 +2318,11 @@ public class ShadeListBuilderTest extends SysuiTestCase { return mAllowGroupChanges; } + @Override + public boolean isGroupPruneAllowed(@NonNull GroupEntry entry) { + return mAllowGroupPruning; + } + @Override public boolean isSectionChangeAllowed(@NonNull NotificationEntry entry) { return mAllowSectionChanges; diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinatorTest.java index 6f8e5d8e514e8..f3aa20ba45271 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/VisualStabilityCoordinatorTest.java @@ -35,6 +35,8 @@ import com.android.systemui.SysuiTestCase; import com.android.systemui.dump.DumpManager; import com.android.systemui.keyguard.WakefulnessLifecycle; import com.android.systemui.plugins.statusbar.StatusBarStateController; +import com.android.systemui.statusbar.notification.collection.GroupEntry; +import com.android.systemui.statusbar.notification.collection.GroupEntryBuilder; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder; @@ -83,6 +85,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { private NotifPanelEvents.Listener mNotifPanelEventsCallback; private NotifStabilityManager mNotifStabilityManager; private NotificationEntry mEntry; + private GroupEntry mGroupEntry; @Before public void setUp() { @@ -117,6 +120,10 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { .setPkg("testPkg1") .build(); + mGroupEntry = new GroupEntryBuilder() + .setSummary(mEntry) + .build(); + when(mHeadsUpManager.isAlerting(mEntry.getKey())).thenReturn(false); // Whenever we invalidate, the pipeline runs again, so we invalidate the state @@ -135,6 +142,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes are allowed assertTrue(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertTrue(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are allowed assertTrue(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -149,6 +157,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes are allowed assertTrue(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertTrue(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are allowed assertTrue(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -163,6 +172,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes are NOT allowed assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are NOT allowed assertFalse(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -176,6 +186,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes are NOT allowed assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are NOT allowed assertFalse(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -190,6 +201,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes are NOT allowed assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are NOT allowed assertFalse(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -208,6 +220,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN group changes aren't allowed assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // THEN section changes are allowed for this notification but not other notifications assertTrue(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); @@ -321,6 +334,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { setPanelExpanded(true); assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); // WHEN the panel isn't expanded anymore setPanelExpanded(false); @@ -422,6 +436,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { setPanelExpanded(true); setPulsing(true); assertFalse(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); assertFalse(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); // GIVEN mEntry is a HUN @@ -431,6 +446,8 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { assertTrue(mNotifStabilityManager.isGroupChangeAllowed(mEntry)); assertTrue(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); + // BUT pruning the group for which this is the summary would still NOT be allowed. + assertFalse(mNotifStabilityManager.isGroupPruneAllowed(mGroupEntry)); } private void setActivityLaunching(boolean activityLaunching) {