diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipeline.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipeline.kt index 50d7324df2b44..9ae9fe5089441 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipeline.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipeline.kt @@ -58,12 +58,13 @@ import javax.inject.Inject * appropriately). * 3. OnBeforeTransformGroupListeners are fired ([.addOnBeforeTransformGroupsListener]) * 4. NotifPromoters are called on each notification with a parent ([.addPromoter]) - * 5. Finalize filters are fired on each notification ([.addFinalizeFilter]) - * 6. OnBeforeSortListeners are fired ([.addOnBeforeSortListener]) - * 7. Top-level entries are assigned sections by NotifSections ([.setSections]) - * 8. Top-level entries within the same section are sorted by NotifComparators ([.setComparators]) - * 9. OnBeforeRenderListListeners are fired ([.addOnBeforeRenderListListener]) - * 10. The list is handed off to the view layer to be rendered + * 5. OnBeforeSortListeners are fired ([.addOnBeforeSortListener]) + * 6. Top-level entries are assigned sections by NotifSections ([.setSections]) + * 7. Top-level entries within the same section are sorted by NotifComparators ([.setComparators]) + * 8. OnBeforeFinalizeFilterListeners are fired ([.addOnBeforeFinalizeFilterListener]) + * 9. Finalize filters are fired on each notification ([.addFinalizeFilter]) + * 10. OnBeforeRenderListListeners are fired ([.addOnBeforeRenderListListener]) + * 11. The list is handed off to the view layer to be rendered */ @SysUISingleton class NotifPipeline @Inject constructor( 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 b4147e1535537..464250090e1a1 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 @@ -365,16 +365,7 @@ public class ShadeListBuilder implements Dumpable { mPipelineState.incrementTo(STATE_GROUP_STABILIZING); stabilizeGroupingNotifs(mNotifList); - - // Step 5: Filter out entries after pre-group filtering, grouping and promoting - // Now filters can see grouping information to determine whether to filter or not. - dispatchOnBeforeFinalizeFilter(mReadOnlyNotifList); - mPipelineState.incrementTo(STATE_FINALIZE_FILTERING); - filterNotifs(mNotifList, mNewNotifList, mNotifFinalizeFilters); - applyNewNotifList(); - pruneIncompleteGroups(mNotifList); - - // Step 6: Section & Sort + // Step 5: Section & Sort // Assign each top-level entry a section, and copy to all of its children dispatchOnBeforeSort(mReadOnlyNotifList); mPipelineState.incrementTo(STATE_SORTING); @@ -383,6 +374,15 @@ public class ShadeListBuilder implements Dumpable { // Sort the list by section and then within section by our list of custom comparators sortListAndGroups(); + // Step 6: Filter out entries after pre-group filtering, grouping, promoting, and sorting + // Now filters can see grouping, sectioning, and order information to determine whether + // to filter or not. + dispatchOnBeforeFinalizeFilter(mReadOnlyNotifList); + mPipelineState.incrementTo(STATE_FINALIZE_FILTERING); + filterNotifs(mNotifList, mNewNotifList, mNotifFinalizeFilters); + applyNewNotifList(); + pruneIncompleteGroups(mNotifList); + // Step 7: Lock in our group structure and log anything that's changed since the last run mPipelineState.incrementTo(STATE_FINALIZING); logChanges(); @@ -654,16 +654,15 @@ public class ShadeListBuilder implements Dumpable { final List children = group.getRawChildren(); if (group.getSummary() != null && children.size() == 0) { - shadeList.remove(i); - i--; - NotificationEntry summary = group.getSummary(); summary.setParent(ROOT_ENTRY); - shadeList.add(summary); + // The list may be sorted; replace the group with the summary, in its place + shadeList.set(i, summary); group.setSummary(null); annulAddition(group, shadeList); + 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) { @@ -681,7 +680,6 @@ public class ShadeListBuilder implements Dumpable { // its children (if any) directly to top-level. shadeList.remove(i); - i--; if (group.getSummary() != null) { final NotificationEntry summary = group.getSummary(); @@ -692,11 +690,14 @@ public class ShadeListBuilder implements Dumpable { for (int j = 0; j < children.size(); j++) { final NotificationEntry child = children.get(j); child.setParent(ROOT_ENTRY); - shadeList.add(child); + // 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); + + i--; // The node we visited is gone, so be sure to visit this index again. } } } 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 027ac0f66b350..798bfe7f39d08 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 @@ -82,8 +82,8 @@ public class PipelineState { public static final int STATE_GROUPING = 4; public static final int STATE_TRANSFORMING = 5; public static final int STATE_GROUP_STABILIZING = 6; - public static final int STATE_FINALIZE_FILTERING = 7; - public static final int STATE_SORTING = 8; + public static final int STATE_SORTING = 7; + public static final int STATE_FINALIZE_FILTERING = 8; public static final int STATE_FINALIZING = 9; @IntDef(prefix = { "STATE_" }, value = { @@ -94,8 +94,8 @@ public class PipelineState { STATE_GROUPING, STATE_TRANSFORMING, STATE_GROUP_STABILIZING, - STATE_FINALIZE_FILTERING, STATE_SORTING, + STATE_FINALIZE_FILTERING, STATE_FINALIZING, }) @Retention(RetentionPolicy.SOURCE) 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 602886e65d461..3a4096187a25a 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 @@ -45,6 +45,7 @@ import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; import android.util.ArrayMap; +import androidx.annotation.NonNull; import androidx.test.filters.SmallTest; import com.android.systemui.SysuiTestCase; @@ -861,13 +862,13 @@ public class ShadeListBuilderTest extends SysuiTestCase { .onBeforeTransformGroups(anyList()); inOrder.verify(promoter, atLeastOnce()) .shouldPromoteToTopLevel(any(NotificationEntry.class)); - inOrder.verify(mOnBeforeFinalizeFilterListener).onBeforeFinalizeFilter(anyList()); - inOrder.verify(preRenderFilter, atLeastOnce()) - .shouldFilterOut(any(NotificationEntry.class), anyLong()); inOrder.verify(mOnBeforeSortListener).onBeforeSort(anyList()); inOrder.verify(section, atLeastOnce()).isInSection(any(ListEntry.class)); inOrder.verify(comparator, atLeastOnce()) .compare(any(ListEntry.class), any(ListEntry.class)); + inOrder.verify(mOnBeforeFinalizeFilterListener).onBeforeFinalizeFilter(anyList()); + inOrder.verify(preRenderFilter, atLeastOnce()) + .shouldFilterOut(any(NotificationEntry.class), anyLong()); inOrder.verify(mOnBeforeRenderListListener).onBeforeRenderList(anyList()); inOrder.verify(mOnRenderListListener).onRenderList(anyList()); } @@ -973,13 +974,11 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); // THEN all the new notifs, including the new GroupEntry, are passed to the listener - assertEquals( - asList( - mEntrySet.get(0), - mBuiltList.get(1), - mEntrySet.get(4)), - listener.mEntriesReceived - ); + assertThat(listener.mEntriesReceived).containsExactly( + mEntrySet.get(0), + mBuiltList.get(1), + mEntrySet.get(4) + ).inOrder(); // Order is a bonus because this listener is before sort } @Test @@ -1019,14 +1018,12 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); // THEN all the new notifs, including the new GroupEntry, are passed to the listener - assertEquals( - asList( - mEntrySet.get(0), - mBuiltList.get(2), - mEntrySet.get(7), - mEntrySet.get(1)), - listener.mEntriesReceived - ); + assertThat(listener.mEntriesReceived).containsExactly( + mEntrySet.get(0), + mEntrySet.get(1), + mBuiltList.get(2), + mEntrySet.get(7) + ).inOrder(); // Order is a bonus because this listener is before sort } @Test @@ -1116,10 +1113,94 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); } + @Test + public void testFinalizeFilteringGroupSummaryDoesNotBreakSort() { + // GIVEN children from 3 packages, with one in the middle of the sort order being a group + addNotif(0, PACKAGE_1); + addNotif(1, PACKAGE_2); + addNotif(2, PACKAGE_3); + addNotif(3, PACKAGE_1); + addNotif(4, PACKAGE_2); + addNotif(5, PACKAGE_3); + addGroupSummary(6, PACKAGE_2, GROUP_1); + addGroupChild(7, PACKAGE_2, GROUP_1); + addGroupChild(8, PACKAGE_2, GROUP_1); + + // GIVEN that they should be sorted by package + mListBuilder.setComparators(asList( + new HypeComparator(PACKAGE_1), + new HypeComparator(PACKAGE_2), + new HypeComparator(PACKAGE_3) + )); + + // WHEN a finalize filter removes the summary + mListBuilder.addFinalizeFilter(new NotifFilter("Test") { + @Override + public boolean shouldFilterOut(@NonNull NotificationEntry entry, long now) { + return entry == notif(6).entry; + } + }); + + dispatchBuild(); + + // THEN the notifications remain ordered by package, even though the children were promoted + verifyBuiltList( + notif(0), + notif(3), + notif(1), + notif(4), + notif(7), // promoted child + notif(8), // promoted child + notif(2), + notif(5) + ); + } + + @Test + public void testFinalizeFilteringGroupChildDoesNotBreakSort() { + // GIVEN children from 3 packages, with one in the middle of the sort order being a group + addNotif(0, PACKAGE_1); + addNotif(1, PACKAGE_2); + addNotif(2, PACKAGE_3); + addNotif(3, PACKAGE_1); + addNotif(4, PACKAGE_2); + addNotif(5, PACKAGE_3); + addGroupSummary(6, PACKAGE_2, GROUP_1); + addGroupChild(7, PACKAGE_2, GROUP_1); + addGroupChild(8, PACKAGE_2, GROUP_1); + + // GIVEN that they should be sorted by package + mListBuilder.setComparators(asList( + new HypeComparator(PACKAGE_1), + new HypeComparator(PACKAGE_2), + new HypeComparator(PACKAGE_3) + )); + + // WHEN a finalize filter one of the 2 children from a group + mListBuilder.addFinalizeFilter(new NotifFilter("Test") { + @Override + public boolean shouldFilterOut(@NonNull NotificationEntry entry, long now) { + return entry == notif(7).entry; + } + }); + + dispatchBuild(); + + // THEN the notifications remain ordered by package, even though the children were promoted + verifyBuiltList( + notif(0), + notif(3), + notif(1), + notif(4), + notif(8), // promoted child + notif(2), + notif(5) + ); + } + @Test public void testBrokenGroupNotificationOrdering() { // GIVEN two group children with different sections & without a summary yet - addGroupChild(0, PACKAGE_2, GROUP_1); addNotif(1, PACKAGE_1); addGroupChild(2, PACKAGE_2, GROUP_1); @@ -1250,13 +1331,11 @@ public class ShadeListBuilderTest extends SysuiTestCase { dispatchBuild(); // THEN all the new notifs are passed to the listener out of order - assertEquals( - asList( - mEntrySet.get(0), - mEntrySet.get(1), - mEntrySet.get(2)), - listener.mEntriesReceived - ); + assertThat(listener.mEntriesReceived).containsExactly( + mEntrySet.get(0), + mEntrySet.get(1), + mEntrySet.get(2) + ).inOrder(); // Checking out-of-order input to validate sorted output // THEN the final list is in order verifyBuiltList( @@ -1282,13 +1361,11 @@ public class ShadeListBuilderTest extends SysuiTestCase { dispatchBuild(); // THEN all the new notifs are passed to the listener - assertEquals( - asList( - mEntrySet.get(0), - mEntrySet.get(1), - mEntrySet.get(2)), - listener.mEntriesReceived - ); + assertThat(listener.mEntriesReceived).containsExactly( + mEntrySet.get(0), + mEntrySet.get(1), + mEntrySet.get(2) + ).inOrder(); } @Test