From b0c2be9dca8b692f418c524c7105a6546ad300bb Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Mon, 1 Aug 2022 19:12:23 +0000 Subject: [PATCH] Summaries receive a stability section index This fixes an issue where the summary with no children wouldn't have its sort position stabilized, resulting in unintended move animations while the shade is open when child notifications were cancelled. Bug: 241229236 Test: atest ShadeListBuilderTest Change-Id: I742d68ce05e080f3b7a928247e46d8406f21eebb --- .../src/com/android/systemui/flags/Flags.java | 4 +- .../notification/NotifPipelineFlags.kt | 4 + .../collection/ShadeListBuilder.java | 36 ++++++--- .../collection/ShadeListBuilderTest.java | 80 +++++++++++++++++++ 4 files changed, 113 insertions(+), 11 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/flags/Flags.java b/packages/SystemUI/src/com/android/systemui/flags/Flags.java index 3868b6f063e81..0363a207f0556 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/Flags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/Flags.java @@ -73,7 +73,9 @@ public class Flags { public static final UnreleasedFlag NOTIFICATION_DISMISSAL_FADE = new UnreleasedFlag(113, true); - // next id: 114 + public static final UnreleasedFlag STABILITY_INDEX_FIX = new UnreleasedFlag(114, true); + + // next id: 115 /***************************************/ // 200 - keyguard/lockscreen diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt index 7fbdd35796c13..2f0a1aa9b0d58 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt @@ -53,4 +53,8 @@ class NotifPipelineFlags @Inject constructor( fun fullScreenIntentRequiresKeyguard(): Boolean = featureFlags.isEnabled(Flags.FSI_REQUIRES_KEYGUARD) + + val isStabilityIndexFixEnabled: Boolean by lazy { + featureFlags.isEnabled(Flags.STABILITY_INDEX_FIX) + } } 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 e129ee45817a4..225236f951795 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 @@ -96,6 +96,7 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable { // used exclusivly by ShadeListBuilder#notifySectionEntriesUpdated // TODO replace temp with collection pool for readability private final ArrayList mTempSectionMembers = new ArrayList<>(); + private final NotifPipelineFlags mFlags; private final boolean mAlwaysLogList; private List mNotifList = new ArrayList<>(); @@ -141,6 +142,7 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable { ) { mSystemClock = systemClock; mLogger = logger; + mFlags = flags; mAlwaysLogList = flags.isDevLoggingEnabled(); mInteractionTracker = interactionTracker; mChoreographer = pipelineChoreographer; @@ -1036,27 +1038,41 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable { /** * Assign the index of each notification relative to the total order */ - private static void assignIndexes(List notifList) { + private void assignIndexes(List notifList) { if (notifList.size() == 0) return; NotifSection currentSection = requireNonNull(notifList.get(0).getSection()); int sectionMemberIndex = 0; for (int i = 0; i < notifList.size(); i++) { - ListEntry entry = notifList.get(i); + final ListEntry entry = notifList.get(i); NotifSection section = requireNonNull(entry.getSection()); if (section.getIndex() != currentSection.getIndex()) { sectionMemberIndex = 0; currentSection = section; } - entry.getAttachState().setStableIndex(sectionMemberIndex); - if (entry instanceof GroupEntry) { - GroupEntry parent = (GroupEntry) entry; - for (int j = 0; j < parent.getChildren().size(); j++) { - entry = parent.getChildren().get(j); - entry.getAttachState().setStableIndex(sectionMemberIndex); - sectionMemberIndex++; + if (mFlags.isStabilityIndexFixEnabled()) { + entry.getAttachState().setStableIndex(sectionMemberIndex++); + if (entry instanceof GroupEntry) { + final GroupEntry parent = (GroupEntry) entry; + final NotificationEntry summary = parent.getSummary(); + if (summary != null) { + summary.getAttachState().setStableIndex(sectionMemberIndex++); + } + for (NotificationEntry child : parent.getChildren()) { + child.getAttachState().setStableIndex(sectionMemberIndex++); + } } + } else { + // This old implementation uses the same index number for the group as the first + // child, and fails to assign an index to the summary. Remove once tested. + entry.getAttachState().setStableIndex(sectionMemberIndex); + if (entry instanceof GroupEntry) { + final GroupEntry parent = (GroupEntry) entry; + for (NotificationEntry child : parent.getChildren()) { + child.getAttachState().setStableIndex(sectionMemberIndex++); + } + } + sectionMemberIndex++; } - sectionMemberIndex++; } } 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 82e32b2fdc64e..896ce2b3994bb 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 @@ -38,6 +38,7 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; @@ -2112,6 +2113,85 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); } + @Test + public void groupRevertingToSummaryDoesNotRetainStablePositionWithLegacyIndexLogic() { + when(mNotifPipelineFlags.isStabilityIndexFixEnabled()).thenReturn(false); + + // GIVEN a notification group is on screen + mStabilityManager.setAllowEntryReordering(false); + + // WHEN the list is originally built with reordering disabled (and section changes allowed) + addNotif(0, PACKAGE_1).setRank(2); + addNotif(1, PACKAGE_1).setRank(3); + addGroupSummary(2, PACKAGE_1, "group").setRank(4); + addGroupChild(3, PACKAGE_1, "group").setRank(5); + addGroupChild(4, PACKAGE_1, "group").setRank(6); + dispatchBuild(); + + verifyBuiltList( + notif(0), + notif(1), + group( + summary(2), + child(3), + child(4) + ) + ); + + // WHEN the notification summary rank increases and children removed + setNewRank(notif(2).entry, 1); + mEntrySet.remove(4); + mEntrySet.remove(3); + dispatchBuild(); + + // VERIFY the summary (incorrectly) moves to the top of the section where it is ranked, + // despite visual stability being active + verifyBuiltList( + notif(2), + notif(0), + notif(1) + ); + } + + @Test + public void groupRevertingToSummaryRetainsStablePosition() { + when(mNotifPipelineFlags.isStabilityIndexFixEnabled()).thenReturn(true); + + // GIVEN a notification group is on screen + mStabilityManager.setAllowEntryReordering(false); + + // WHEN the list is originally built with reordering disabled (and section changes allowed) + addNotif(0, PACKAGE_1).setRank(2); + addNotif(1, PACKAGE_1).setRank(3); + addGroupSummary(2, PACKAGE_1, "group").setRank(4); + addGroupChild(3, PACKAGE_1, "group").setRank(5); + addGroupChild(4, PACKAGE_1, "group").setRank(6); + dispatchBuild(); + + verifyBuiltList( + notif(0), + notif(1), + group( + summary(2), + child(3), + child(4) + ) + ); + + // WHEN the notification summary rank increases and children removed + setNewRank(notif(2).entry, 1); + mEntrySet.remove(4); + mEntrySet.remove(3); + dispatchBuild(); + + // VERIFY the summary stays in the same location on rebuild + verifyBuiltList( + notif(0), + notif(1), + notif(2) + ); + } + private static void setNewRank(NotificationEntry entry, int rank) { entry.setRanking(new RankingBuilder(entry.getRanking()).setRank(rank).build()); }