From b9df9d9653c4a131d2bf9174bc37664e62e0de3d Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 5 Nov 2021 20:08:56 -0400 Subject: [PATCH] New Pipeline: Separate sectioning and sorting steps. * Fixes a bug where the last NotifSectioner with entries would not have onEntriesUpdated called. * Changes the contract of onEntriesUpdated so that its called for empty sections. * Update RankingCoordinator - remove resetClearAllFlags() now that onEntriesUpdated() is called when the section is empty. Bug: 205348428 Test: atest ShadeListBuilderTest Change-Id: Idafbc6643aed11f4ee9db23c59a6d5d5da45b09f --- .../collection/ShadeListBuilder.java | 53 ++++++++------ .../coordinator/RankingCoordinator.java | 10 +-- .../collection/ShadeListBuilderTest.java | 48 ++++++++++--- .../coordinator/RankingCoordinatorTest.java | 70 ++++++++++--------- 4 files changed, 108 insertions(+), 73 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 15872da144162..b4147e1535537 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 @@ -28,6 +28,8 @@ import static com.android.systemui.statusbar.notification.collection.listbuilder import static com.android.systemui.statusbar.notification.collection.listbuilder.PipelineState.STATE_SORTING; import static com.android.systemui.statusbar.notification.collection.listbuilder.PipelineState.STATE_TRANSFORMING; +import static java.util.Objects.requireNonNull; + import android.annotation.MainThread; import android.annotation.Nullable; import android.os.Trace; @@ -372,12 +374,14 @@ public class ShadeListBuilder implements Dumpable { applyNewNotifList(); pruneIncompleteGroups(mNotifList); - // Step 6: Sort - // Assign each top-level entry a section, then sort the list by section and then within - // section by our list of custom comparators + // Step 6: Section & Sort + // Assign each top-level entry a section, and copy to all of its children dispatchOnBeforeSort(mReadOnlyNotifList); mPipelineState.incrementTo(STATE_SORTING); - sortListAndNotifySections(); + assignSections(); + notifySectionEntriesUpdated(); + // Sort the list by section and then within section by our list of custom comparators + sortListAndGroups(); // Step 7: Lock in our group structure and log anything that's changed since the last run mPipelineState.incrementTo(STATE_FINALIZING); @@ -408,18 +412,15 @@ public class ShadeListBuilder implements Dumpable { private void notifySectionEntriesUpdated() { Trace.beginSection("ShadeListBuilder.notifySectionEntriesUpdated"); - NotifSection currentSection = null; mTempSectionMembers.clear(); - for (int i = 0; i < mNotifList.size(); i++) { - ListEntry currentEntry = mNotifList.get(i); - if (currentSection != currentEntry.getSection()) { - if (currentSection != null) { - currentSection.getSectioner().onEntriesUpdated(mTempSectionMembers); - mTempSectionMembers.clear(); + for (NotifSection section : mNotifSections) { + for (ListEntry entry : mNotifList) { + if (section == entry.getSection()) { + mTempSectionMembers.add(entry); } - currentSection = currentEntry.getSection(); } - mTempSectionMembers.add(currentEntry); + section.getSectioner().onEntriesUpdated(mTempSectionMembers); + mTempSectionMembers.clear(); } Trace.endSection(); } @@ -765,9 +766,9 @@ public class ShadeListBuilder implements Dumpable { } } - private void sortListAndNotifySections() { - Trace.beginSection("ShadeListBuilder.sortListAndNotifySections"); - // Assign sections to top-level elements and sort their children + private void assignSections() { + Trace.beginSection("ShadeListBuilder.assignSections"); + // Assign sections to top-level elements and their children for (ListEntry entry : mNotifList) { NotifSection section = applySections(entry); if (entry instanceof GroupEntry) { @@ -775,27 +776,35 @@ public class ShadeListBuilder implements Dumpable { for (NotificationEntry child : parent.getChildren()) { setEntrySection(child, section); } + } + } + Trace.endSection(); + } + + private void sortListAndGroups() { + Trace.beginSection("ShadeListBuilder.sortListAndGroups"); + // Assign sections to top-level elements and sort their children + for (ListEntry entry : mNotifList) { + if (entry instanceof GroupEntry) { + GroupEntry parent = (GroupEntry) entry; parent.sortChildren(mGroupChildrenComparator); } } mNotifList.sort(mTopLevelComparator); assignIndexes(mNotifList); - - notifySectionEntriesUpdated(); Trace.endSection(); } /** * Assign the index of each notification relative to the total order - * @param notifList */ - private void assignIndexes(List notifList) { + private static void assignIndexes(List notifList) { if (notifList.size() == 0) return; - NotifSection currentSection = notifList.get(0).getSection(); + NotifSection currentSection = requireNonNull(notifList.get(0).getSection()); int sectionMemberIndex = 0; for (int i = 0; i < notifList.size(); i++) { ListEntry entry = notifList.get(i); - NotifSection section = entry.getSection(); + NotifSection section = requireNonNull(entry.getSection()); if (section.getIndex() != currentSection.getIndex()) { sectionMemberIndex = 0; currentSection = section; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinator.java index 79eb0898bc0b8..71979735a030f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinator.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinator.java @@ -19,7 +19,6 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import android.annotation.NonNull; import android.annotation.Nullable; -import com.android.internal.annotations.VisibleForTesting; import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.collection.ListEntry; import com.android.systemui.statusbar.notification.collection.NotifPipeline; @@ -76,7 +75,6 @@ public class RankingCoordinator implements Coordinator { pipeline.addPreGroupFilter(mSuspendedFilter); pipeline.addPreGroupFilter(mDndVisualEffectsFilter); - pipeline.addOnBeforeSortListener(entries -> resetClearAllFlags()); } public NotifSectioner getAlertingSectioner() { @@ -126,6 +124,7 @@ public class RankingCoordinator implements Coordinator { @Nullable @Override public void onEntriesUpdated(@NonNull List entries) { + mHasSilentEntries = false; for (int i = 0; i < entries.size(); i++) { if (entries.get(i).getRepresentativeEntry().getSbn().isClearable()) { mHasSilentEntries = true; @@ -154,6 +153,7 @@ public class RankingCoordinator implements Coordinator { @Nullable @Override public void onEntriesUpdated(@NonNull List entries) { + mHasMinimizedEntries = false; for (int i = 0; i < entries.size(); i++) { if (entries.get(i).getRepresentativeEntry().getSbn().isClearable()) { mHasMinimizedEntries = true; @@ -189,12 +189,6 @@ public class RankingCoordinator implements Coordinator { } }; - @VisibleForTesting - protected void resetClearAllFlags() { - mHasSilentEntries = false; - mHasMinimizedEntries = false; - } - private final StatusBarStateController.StateListener mStatusBarStateCallback = new StatusBarStateController.StateListener() { @Override 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 913ffd44437a1..602886e65d461 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 @@ -18,6 +18,8 @@ package com.android.systemui.statusbar.notification.collection; import static com.android.systemui.statusbar.notification.collection.ListDumper.dumpTree; +import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -33,7 +35,6 @@ import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; 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 java.util.Arrays.asList; @@ -44,7 +45,6 @@ import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; import android.util.ArrayMap; -import androidx.annotation.Nullable; import androidx.test.filters.SmallTest; import com.android.systemui.SysuiTestCase; @@ -82,7 +82,6 @@ import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; @SmallTest @@ -618,26 +617,53 @@ public class ShadeListBuilderTest extends SysuiTestCase { @Test public void testNotifSectionsChildrenUpdated() { - AtomicBoolean validChildren = new AtomicBoolean(false); + ArrayList pkg1Entries = new ArrayList<>(); + ArrayList pkg2Entries = new ArrayList<>(); + ArrayList pkg3Entries = new ArrayList<>(); final NotifSectioner pkg1Sectioner = spy(new PackageSectioner(PACKAGE_1) { - @Nullable @Override public void onEntriesUpdated(List entries) { super.onEntriesUpdated(entries); - validChildren.set(entries.size() == 2); + pkg1Entries.addAll(entries); } }); - mListBuilder.setSectioners(asList(pkg1Sectioner)); + final NotifSectioner pkg2Sectioner = spy(new PackageSectioner(PACKAGE_2) { + @Override + public void onEntriesUpdated(List entries) { + super.onEntriesUpdated(entries); + pkg2Entries.addAll(entries); + } + }); + final NotifSectioner pkg3Sectioner = spy(new PackageSectioner(PACKAGE_3) { + @Override + public void onEntriesUpdated(List entries) { + super.onEntriesUpdated(entries); + pkg3Entries.addAll(entries); + } + }); + mListBuilder.setSectioners(asList(pkg1Sectioner, pkg2Sectioner, pkg3Sectioner)); - addNotif(0, PACKAGE_4); + addNotif(0, PACKAGE_1); addNotif(1, PACKAGE_1); - addNotif(2, PACKAGE_1); + addNotif(2, PACKAGE_3); addNotif(3, PACKAGE_3); + addNotif(4, PACKAGE_3); dispatchBuild(); - verify(pkg1Sectioner, times(1)).onEntriesUpdated(any()); - assertTrue(validChildren.get()); + verify(pkg1Sectioner).onEntriesUpdated(any()); + verify(pkg2Sectioner).onEntriesUpdated(any()); + verify(pkg3Sectioner).onEntriesUpdated(any()); + assertThat(pkg1Entries).containsExactly( + mEntrySet.get(0), + mEntrySet.get(1) + ).inOrder(); + assertThat(pkg2Entries).isEmpty(); + assertThat(pkg3Entries).containsExactly( + mEntrySet.get(2), + mEntrySet.get(3), + mEntrySet.get(4) + ).inOrder(); } @Test diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinatorTest.java index 2091cf8cb0283..1d45aff00a035 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/RankingCoordinatorTest.java @@ -19,6 +19,7 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_AMBIENT; import static android.app.NotificationManager.Policy.SUPPRESSED_EFFECT_NOTIFICATION_LIST; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.eq; @@ -28,8 +29,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.app.Notification; -import android.service.notification.NotificationListenerService; -import android.service.notification.StatusBarNotification; +import android.app.NotificationManager; import android.testing.AndroidTestingRunner; import androidx.annotation.Nullable; @@ -38,6 +38,7 @@ import androidx.test.filters.SmallTest; import com.android.systemui.SysuiTestCase; import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.RankingBuilder; +import com.android.systemui.statusbar.SbnBuilder; import com.android.systemui.statusbar.notification.collection.ListEntry; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -54,7 +55,6 @@ import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import java.util.ArrayList; @@ -70,8 +70,6 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Mock private NodeController mAlertingHeaderController; @Mock private NodeController mSilentNodeController; @Mock private SectionHeaderController mSilentHeaderController; - @Mock private NotificationListenerService.Ranking mRanking; - @Mock private StatusBarNotification mSbn; @Captor private ArgumentCaptor mNotifFilterCaptor; @@ -92,9 +90,7 @@ public class RankingCoordinatorTest extends SysuiTestCase { mStatusBarStateController, mHighPriorityProvider, mAlertingHeaderController, mSilentHeaderController, mSilentNodeController); mEntry = spy(new NotificationEntryBuilder().build()); - mRanking = spy(getRankingForUnfilteredNotif().build()); - mEntry.setRanking(mRanking); - when(mEntry.getSbn()).thenReturn(mSbn); + mEntry.setRanking(getRankingForUnfilteredNotif().build()); mRankingCoordinator.attach(mNotifPipeline); verify(mNotifPipeline, times(2)).addPreGroupFilter(mNotifFilterCaptor.capture()); @@ -109,23 +105,19 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Test public void testSilentHeaderClearableChildrenUpdate() { - StatusBarNotification sbn = Mockito.mock(StatusBarNotification.class); - Mockito.doReturn("key").when(sbn).getKey(); - Mockito.doReturn(Mockito.mock(Notification.class)).when(sbn).getNotification(); - NotificationEntry entry = new NotificationEntryBuilder().setSbn(sbn).build(); - ListEntry listEntry = new ListEntry("key", 0L) { + ListEntry listEntry = new ListEntry(mEntry.getKey(), 0L) { @Nullable @Override public NotificationEntry getRepresentativeEntry() { - return entry; + return mEntry; } }; - Mockito.doReturn(true).when(sbn).isClearable(); + setRankingAmbient(false); + setSbnClearable(true); mSilentSectioner.onEntriesUpdated(Arrays.asList(listEntry)); - when(mRanking.isAmbient()).thenReturn(false); verify(mSilentHeaderController).setClearSectionEnabled(eq(true)); - mRankingCoordinator.resetClearAllFlags(); - Mockito.doReturn(false).when(sbn).isClearable(); + + setSbnClearable(false); mSilentSectioner.onEntriesUpdated(Arrays.asList(listEntry)); verify(mSilentHeaderController).setClearSectionEnabled(eq(false)); } @@ -204,7 +196,7 @@ public class RankingCoordinatorTest extends SysuiTestCase { public void testIncludeInSectionSilent() { // GIVEN the entry isn't high priority when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mRanking.isAmbient()).thenReturn(false); + setRankingAmbient(false); // THEN entry is in the silent section assertFalse(mAlertingSectioner.isInSection(mEntry)); @@ -213,24 +205,23 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Test public void testMinSection() { - when(mEntry.getRanking()).thenReturn(mRanking); when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mRanking.isAmbient()).thenReturn(true); + setRankingAmbient(true); assertInSection(mEntry, mMinimizedSectioner); } @Test public void testSilentSection() { when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mRanking.isAmbient()).thenReturn(false); + setRankingAmbient(false); assertInSection(mEntry, mSilentSectioner); } @Test public void testClearableSilentSection() { when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mSbn.isClearable()).thenReturn(true); - when(mRanking.isAmbient()).thenReturn(false); + setSbnClearable(true); + setRankingAmbient(false); mSilentSectioner.onEntriesUpdated(Arrays.asList(mEntry)); verify(mSilentHeaderController).setClearSectionEnabled(eq(true)); } @@ -238,17 +229,17 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Test public void testClearableMinimizedSection() { when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mSbn.isClearable()).thenReturn(true); - when(mRanking.isAmbient()).thenReturn(true); + setSbnClearable(true); + setRankingAmbient(true); mMinimizedSectioner.onEntriesUpdated(Arrays.asList(mEntry)); verify(mSilentHeaderController).setClearSectionEnabled(eq(true)); } @Test public void testNotClearableSilentSection() { - when(mSbn.isClearable()).thenReturn(false); + setSbnClearable(false); when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mRanking.isAmbient()).thenReturn(false); + setRankingAmbient(false); mSilentSectioner.onEntriesUpdated(Arrays.asList(mEntry)); mMinimizedSectioner.onEntriesUpdated(Arrays.asList(mEntry)); mAlertingSectioner.onEntriesUpdated(Arrays.asList(mEntry)); @@ -257,9 +248,9 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Test public void testNotClearableMinimizedSection() { - when(mSbn.isClearable()).thenReturn(false); + setSbnClearable(false); when(mHighPriorityProvider.isHighPriority(mEntry)).thenReturn(false); - when(mRanking.isAmbient()).thenReturn(true); + setRankingAmbient(true); mSilentSectioner.onEntriesUpdated(Arrays.asList(mEntry)); mMinimizedSectioner.onEntriesUpdated(Arrays.asList(mEntry)); mAlertingSectioner.onEntriesUpdated(Arrays.asList(mEntry)); @@ -277,9 +268,24 @@ public class RankingCoordinatorTest extends SysuiTestCase { } private RankingBuilder getRankingForUnfilteredNotif() { - return new RankingBuilder() - .setKey(mEntry.getKey()) + return new RankingBuilder(mEntry.getRanking()) .setSuppressedVisualEffects(0) .setSuspended(false); } + + private void setSbnClearable(boolean clearable) { + mEntry.setSbn(new SbnBuilder(mEntry.getSbn()) + .setFlag(mContext, Notification.FLAG_NO_CLEAR, !clearable) + .build()); + assertEquals(clearable, mEntry.getSbn().isClearable()); + } + + private void setRankingAmbient(boolean ambient) { + mEntry.setRanking(new RankingBuilder(mEntry.getRanking()) + .setImportance(ambient + ? NotificationManager.IMPORTANCE_MIN + : NotificationManager.IMPORTANCE_DEFAULT) + .build()); + assertEquals(ambient, mEntry.getRanking().isAmbient()); + } }