From b9df9d9653c4a131d2bf9174bc37664e62e0de3d Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 5 Nov 2021 20:08:56 -0400 Subject: [PATCH 1/4] 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()); + } } From 0e5706709bb42d2a1bf3aac6bc307827bb580911 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Sat, 6 Nov 2021 16:39:18 -0400 Subject: [PATCH 2/4] New Pipeline: Restore the ShadeListBuilder order to sort before the finalize filter. * Fixes the bug where cleaning up incomplete groups would break section continuity by ensuring that children removed from a group are inserted into the list at the rank where the group was. * This enables future work which allows using sort order and/or section to determine view inflation specs. Fixes: 203233468 Fixes: 196408434 Test: atest ShadeListBuilderTest Change-Id: If75d0d4f594b462ca49d2c19c98edfcf93892cad --- .../notification/collection/NotifPipeline.kt | 13 +- .../collection/ShadeListBuilder.java | 33 ++-- .../collection/listbuilder/PipelineState.java | 6 +- .../collection/ShadeListBuilderTest.java | 143 ++++++++++++++---- 4 files changed, 137 insertions(+), 58 deletions(-) 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 From e4a15a4d72f8f59ddae86a7a9796fc4e55532272 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Sat, 6 Nov 2021 08:53:09 -0400 Subject: [PATCH 3/4] New Pipeline: Fix minimized notifications not appearing minimized. * The bug was that the LowPriorityInflationHelper checks the parent after a delay to inflate an empty ExpandableNotificationRow, by which time the ListEntry's parent was cleared by the FinalizeFilter. In order to solve this, we need to pass state into the inflation pipeline for this particular parameter. I created the NotifInflater.Params class to hold this, and then shoehorned that class into the not-yet-modernized NotificationRowBinder interface/impl. * Another bug I discovered when looking for params like this was that a ranking update (such as the addition of SmartReplies) would not result in reinflating the notification, because the system had no way to detect that a ranking update would affect the view itself. This is why I added the NotifUiAdjustment class (copied, kotlinized, and tweaked to include the 'minimized' state from the NotificationUiAdjustment). Fixes: 205360544 Test: atest SystemUITests Change-Id: Ic1aa2dded98b14efe0219684aa754549d9d53d81 --- .../NotificationViewHierarchyManager.java | 2 +- .../dagger/StatusBarDependenciesModule.java | 4 +- .../NotificationEntryManager.java | 4 +- .../collection/NotifInflaterImpl.java | 24 +-- .../collection/ShadeListBuilder.java | 7 +- .../coordinator/PreparationCoordinator.java | 82 +++++++-- .../coordinator/RankingCoordinator.java | 6 + .../{NotifInflater.java => NotifInflater.kt} | 25 +-- .../collection/inflation/NotifUiAdjustment.kt | 95 ++++++++++ .../inflation/NotifUiAdjustmentProvider.kt | 63 +++++++ .../inflation/NotificationRowBinder.java | 4 +- .../inflation/NotificationRowBinderImpl.java | 36 +++- .../LowPriorityInflationHelper.java | 14 +- .../collection/listbuilder/NotifSection.kt | 3 +- .../NotificationViewHierarchyManagerTest.java | 2 +- .../collection/NotificationEntryBuilder.java | 16 ++ .../PreparationCoordinatorTest.java | 162 +++++++++++++++--- .../coordinator/RankingCoordinatorTest.java | 10 +- ...NotificationEntryManagerInflationTest.java | 3 +- 19 files changed, 465 insertions(+), 97 deletions(-) rename packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/{NotifInflater.java => NotifInflater.kt} (73%) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustment.kt create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustmentProvider.kt rename packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/{inflation => legacy}/LowPriorityInflationHelper.java (84%) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java index 464b2b69c58e8..ff3e97af72a7f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java @@ -35,7 +35,7 @@ import com.android.systemui.statusbar.notification.DynamicChildBindController; import com.android.systemui.statusbar.notification.DynamicPrivacyController; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; -import com.android.systemui.statusbar.notification.collection.inflation.LowPriorityInflationHelper; +import com.android.systemui.statusbar.notification.collection.legacy.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy; import com.android.systemui.statusbar.notification.collection.legacy.VisualStabilityManager; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java index aa86daaae1258..d5cba72f99d3c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java @@ -55,7 +55,7 @@ import com.android.systemui.statusbar.notification.DynamicPrivacyController; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotifCollection; import com.android.systemui.statusbar.notification.collection.NotifPipeline; -import com.android.systemui.statusbar.notification.collection.inflation.LowPriorityInflationHelper; +import com.android.systemui.statusbar.notification.collection.legacy.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy; import com.android.systemui.statusbar.notification.collection.legacy.VisualStabilityManager; import com.android.systemui.statusbar.notification.collection.notifcollection.CommonNotifCollection; @@ -73,8 +73,6 @@ import com.android.systemui.statusbar.phone.ongoingcall.OngoingCallController; import com.android.systemui.statusbar.phone.ongoingcall.OngoingCallLogger; import com.android.systemui.statusbar.policy.RemoteInputUriController; import com.android.systemui.statusbar.window.StatusBarWindowController; -import com.android.systemui.statusbar.window.StatusBarWindowModule; -import com.android.systemui.statusbar.window.StatusBarWindowView; import com.android.systemui.tracing.ProtoTracer; import com.android.systemui.util.concurrency.DelayableExecutor; import com.android.systemui.util.time.SystemClock; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java index 82f35a814d226..2437415d0c7e2 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java @@ -638,7 +638,7 @@ public class NotificationEntryManager implements // Construct the expanded view. if (!mFeatureFlags.isNewNotifPipelineRenderingEnabled()) { - mNotificationRowBinderLazy.get().inflateViews(entry, mInflationCallback); + mNotificationRowBinderLazy.get().inflateViews(entry, null, mInflationCallback); } mPendingNotifications.put(key, entry); @@ -695,7 +695,7 @@ public class NotificationEntryManager implements } if (!mFeatureFlags.isNewNotifPipelineRenderingEnabled()) { - mNotificationRowBinderLazy.get().inflateViews(entry, mInflationCallback); + mNotificationRowBinderLazy.get().inflateViews(entry, null, mInflationCallback); } updateNotifications("updateNotificationInternal"); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java index 8562a2e55a4fc..4f3c287d5f1e2 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java @@ -16,7 +16,8 @@ package com.android.systemui.statusbar.notification.collection; -import com.android.internal.statusbar.IStatusBarService; +import androidx.annotation.NonNull; + import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.statusbar.notification.InflationException; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; @@ -34,23 +35,13 @@ import javax.inject.Inject; @SysUISingleton public class NotifInflaterImpl implements NotifInflater { - private final IStatusBarService mStatusBarService; - private final NotifCollection mNotifCollection; private final NotifInflationErrorManager mNotifErrorManager; - private final NotifPipeline mNotifPipeline; private NotificationRowBinderImpl mNotificationRowBinder; @Inject - public NotifInflaterImpl( - IStatusBarService statusBarService, - NotifCollection notifCollection, - NotifInflationErrorManager errorManager, - NotifPipeline notifPipeline) { - mStatusBarService = statusBarService; - mNotifCollection = notifCollection; + public NotifInflaterImpl(NotifInflationErrorManager errorManager) { mNotifErrorManager = errorManager; - mNotifPipeline = notifPipeline; } /** @@ -61,8 +52,9 @@ public class NotifInflaterImpl implements NotifInflater { } @Override - public void rebindViews(NotificationEntry entry, InflationCallback callback) { - inflateViews(entry, callback); + public void rebindViews(@NonNull NotificationEntry entry, @NonNull Params params, + @NonNull InflationCallback callback) { + inflateViews(entry, params, callback); } /** @@ -70,10 +62,12 @@ public class NotifInflaterImpl implements NotifInflater { * views are bound. */ @Override - public void inflateViews(NotificationEntry entry, InflationCallback callback) { + public void inflateViews(@NonNull NotificationEntry entry, @NonNull Params params, + @NonNull InflationCallback callback) { try { requireBinder().inflateViews( entry, + params, wrapInflationCallback(callback)); } catch (InflationException e) { mNotifErrorManager.setInflationError(entry, e); 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 464250090e1a1..4cdf8927c68b3 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 @@ -1041,8 +1041,11 @@ public class ShadeListBuilder implements Dumpable { private void setEntrySection(ListEntry entry, NotifSection finalSection) { entry.getAttachState().setSection(finalSection); NotificationEntry representativeEntry = entry.getRepresentativeEntry(); - if (representativeEntry != null && finalSection != null) { - representativeEntry.setBucket(finalSection.getBucket()); + if (representativeEntry != null) { + representativeEntry.getAttachState().setSection(finalSection); + if (finalSection != null) { + representativeEntry.setBucket(finalSection.getBucket()); + } } } 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 afdfb3bdeef60..644f248fca008 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 @@ -26,6 +26,9 @@ import android.service.notification.StatusBarNotification; import android.util.ArrayMap; import android.util.ArraySet; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; + import com.android.internal.annotations.VisibleForTesting; import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.dagger.SysUISingleton; @@ -35,6 +38,8 @@ import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.ShadeListBuilder; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; +import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustment; +import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; import com.android.systemui.statusbar.notification.collection.listbuilder.OnBeforeFinalizeFilterListener; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener; @@ -46,7 +51,6 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.util.List; import java.util.Map; -import java.util.Set; import javax.inject.Inject; @@ -66,14 +70,24 @@ public class PreparationCoordinator implements Coordinator { private final NotifInflater mNotifInflater; private final NotifInflationErrorManager mNotifErrorManager; private final NotifViewBarn mViewBarn; - private final Map mInflationStates = new ArrayMap<>(); + private final NotifUiAdjustmentProvider mAdjustmentProvider; + private final ArrayMap mInflationStates = new ArrayMap<>(); + + /** + * The map of notifications to the NotifUiAdjustment (i.e. parameters) that were calculated + * when the inflation started. If an update of any kind results in the adjustment changing, + * then the row must be reinflated. If the row is being inflated, then the inflation must be + * aborted and restarted. + */ + private final ArrayMap mInflationAdjustments = + new ArrayMap<>(); /** * The set of notifications that are currently inflating something. Note that this is * separate from inflation state as a view could either be uninflated or inflated and still be * inflating something. */ - private final Set mInflatingNotifs = new ArraySet<>(); + private final ArraySet mInflatingNotifs = new ArraySet<>(); private final IStatusBarService mStatusBarService; @@ -92,12 +106,14 @@ public class PreparationCoordinator implements Coordinator { NotifInflater notifInflater, NotifInflationErrorManager errorManager, NotifViewBarn viewBarn, + NotifUiAdjustmentProvider adjustmentProvider, IStatusBarService service) { this( logger, notifInflater, errorManager, viewBarn, + adjustmentProvider, service, CHILD_BIND_CUTOFF, MAX_GROUP_INFLATION_DELAY); @@ -109,6 +125,7 @@ public class PreparationCoordinator implements Coordinator { NotifInflater notifInflater, NotifInflationErrorManager errorManager, NotifViewBarn viewBarn, + NotifUiAdjustmentProvider adjustmentProvider, IStatusBarService service, int childBindCutoff, long maxGroupInflationDelay) { @@ -116,6 +133,7 @@ public class PreparationCoordinator implements Coordinator { mNotifInflater = notifInflater; mNotifErrorManager = errorManager; mViewBarn = viewBarn; + mAdjustmentProvider = adjustmentProvider; mStatusBarService = service; mChildBindCutoff = childBindCutoff; mMaxGroupInflationDelay = maxGroupInflationDelay; @@ -160,6 +178,7 @@ public class PreparationCoordinator implements Coordinator { public void onEntryCleanUp(NotificationEntry entry) { mInflationStates.remove(entry); mViewBarn.removeViewForEntry(entry); + mInflationAdjustments.remove(entry); } }; @@ -269,39 +288,78 @@ public class PreparationCoordinator implements Coordinator { } private void inflateRequiredNotifViews(NotificationEntry entry) { + NotifUiAdjustment newAdjustment = mAdjustmentProvider.calculateAdjustment(entry); if (mInflatingNotifs.contains(entry)) { // Already inflating this entry + String errorIfNoOldAdjustment = "Inflating notification has no adjustments"; + if (needToReinflate(entry, newAdjustment, errorIfNoOldAdjustment)) { + inflateEntry(entry, newAdjustment, "adjustment changed while inflating"); + } return; } @InflationState int state = mInflationStates.get(entry); switch (state) { case STATE_UNINFLATED: - inflateEntry(entry, "entryAdded"); + inflateEntry(entry, newAdjustment, "entryAdded"); break; case STATE_INFLATED_INVALID: - rebind(entry, "entryUpdated"); + rebind(entry, newAdjustment, "entryUpdated"); break; case STATE_INFLATED: + String errorIfNoOldAdjustment = "Fully inflated notification has no adjustments"; + if (needToReinflate(entry, newAdjustment, errorIfNoOldAdjustment)) { + rebind(entry, newAdjustment, "adjustment changed after inflated"); + } + break; case STATE_ERROR: + if (needToReinflate(entry, newAdjustment, null)) { + inflateEntry(entry, newAdjustment, "adjustment changed after error"); + } + break; default: // Nothing to do. } } - private void inflateEntry(NotificationEntry entry, String reason) { - abortInflation(entry, reason); - mInflatingNotifs.add(entry); - mNotifInflater.inflateViews(entry, this::onInflationFinished); + private boolean needToReinflate(@NonNull NotificationEntry entry, + @NonNull NotifUiAdjustment newAdjustment, @Nullable String oldAdjustmentMissingError) { + NotifUiAdjustment oldAdjustment = mInflationAdjustments.get(entry); + if (oldAdjustment == null) { + if (oldAdjustmentMissingError == null) { + return true; + } else { + throw new IllegalStateException(oldAdjustmentMissingError); + } + } + return NotifUiAdjustment.needReinflate(oldAdjustment, newAdjustment); } - private void rebind(NotificationEntry entry, String reason) { + private void inflateEntry(NotificationEntry entry, + NotifUiAdjustment newAdjustment, + String reason) { + abortInflation(entry, reason); + mInflationAdjustments.put(entry, newAdjustment); mInflatingNotifs.add(entry); - mNotifInflater.rebindViews(entry, this::onInflationFinished); + NotifInflater.Params params = getInflaterParams(newAdjustment, reason); + mNotifInflater.inflateViews(entry, params, this::onInflationFinished); + } + + private void rebind(NotificationEntry entry, + NotifUiAdjustment newAdjustment, + String reason) { + mInflationAdjustments.put(entry, newAdjustment); + mInflatingNotifs.add(entry); + NotifInflater.Params params = getInflaterParams(newAdjustment, reason); + mNotifInflater.rebindViews(entry, params, this::onInflationFinished); + } + + NotifInflater.Params getInflaterParams(NotifUiAdjustment adjustment, String reason) { + return new NotifInflater.Params(adjustment.isMinimized(), reason); } private void abortInflation(NotificationEntry entry, String reason) { mLogger.logInflationAborted(entry.getKey(), reason); - entry.abortTask(); + mNotifInflater.abortInflation(entry); mInflatingNotifs.remove(entry); } 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 71979735a030f..c60ebcdc5fd12 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 @@ -24,6 +24,7 @@ import com.android.systemui.statusbar.notification.collection.ListEntry; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope; +import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner; import com.android.systemui.statusbar.notification.collection.provider.HighPriorityProvider; @@ -33,6 +34,7 @@ import com.android.systemui.statusbar.notification.dagger.AlertingHeader; import com.android.systemui.statusbar.notification.dagger.SilentHeader; import com.android.systemui.statusbar.notification.stack.NotificationPriorityBucketKt; +import java.util.Collections; import java.util.List; import javax.inject.Inject; @@ -49,6 +51,7 @@ public class RankingCoordinator implements Coordinator { public static final boolean SHOW_ALL_SECTIONS = false; private final StatusBarStateController mStatusBarStateController; private final HighPriorityProvider mHighPriorityProvider; + private final NotifUiAdjustmentProvider mAdjustmentProvider; private final NodeController mSilentNodeController; private final SectionHeaderController mSilentHeaderController; private final NodeController mAlertingHeaderController; @@ -59,11 +62,13 @@ public class RankingCoordinator implements Coordinator { public RankingCoordinator( StatusBarStateController statusBarStateController, HighPriorityProvider highPriorityProvider, + NotifUiAdjustmentProvider adjustmentProvider, @AlertingHeader NodeController alertingHeaderController, @SilentHeader SectionHeaderController silentHeaderController, @SilentHeader NodeController silentNodeController) { mStatusBarStateController = statusBarStateController; mHighPriorityProvider = highPriorityProvider; + mAdjustmentProvider = adjustmentProvider; mAlertingHeaderController = alertingHeaderController; mSilentNodeController = silentNodeController; mSilentHeaderController = silentHeaderController; @@ -72,6 +77,7 @@ public class RankingCoordinator implements Coordinator { @Override public void attach(NotifPipeline pipeline) { mStatusBarStateController.addCallback(mStatusBarStateCallback); + mAdjustmentProvider.setLowPrioritySections(Collections.singleton(mMinimizedNotifSectioner)); pipeline.addPreGroupFilter(mSuspendedFilter); pipeline.addPreGroupFilter(mDndVisualEffectsFilter); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.kt similarity index 73% rename from packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.java rename to packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.kt index e3d76113d5378..c59f18436b74a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifInflater.kt @@ -14,22 +14,22 @@ * limitations under the License. */ -package com.android.systemui.statusbar.notification.collection.inflation; -import com.android.systemui.statusbar.notification.collection.NotificationEntry; -import com.android.systemui.statusbar.notification.collection.coordinator.PreparationCoordinator; +package com.android.systemui.statusbar.notification.collection.inflation + +import com.android.systemui.statusbar.notification.collection.NotificationEntry /** - * Used by the {@link PreparationCoordinator}. When notifications are added or updated, the + * Used by the [PreparationCoordinator]. When notifications are added or updated, the * NotifInflater is asked to (re)inflated and prepare their views. This inflation occurs off the * main thread. When the inflation is finished, NotifInflater will trigger its InflationCallback. */ -public interface NotifInflater { +interface NotifInflater { /** * Called to rebind the entry's views. * * @param callback callback called after inflation finishes */ - void rebindViews(NotificationEntry entry, InflationCallback callback); + fun rebindViews(entry: NotificationEntry, params: Params, callback: InflationCallback) /** * Called to inflate the views of an entry. Views are not considered inflated until all of its @@ -37,18 +37,23 @@ public interface NotifInflater { * * @param callback callback called after inflation finishes */ - void inflateViews(NotificationEntry entry, InflationCallback callback); + fun inflateViews(entry: NotificationEntry, params: Params, callback: InflationCallback) /** * Request to stop the inflation of an entry. For example, called when a notification is * removed and no longer needs to be inflated. */ - void abortInflation(NotificationEntry entry); + fun abortInflation(entry: NotificationEntry) /** * Callback once all the views are inflated and bound for a given NotificationEntry. */ interface InflationCallback { - void onInflationFinished(NotificationEntry entry); + fun onInflationFinished(entry: NotificationEntry) } -} + + /** + * A class holding parameters used when inflating the notification row + */ + class Params(val isLowPriority: Boolean, val reason: String) +} \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustment.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustment.kt new file mode 100644 index 0000000000000..9d86b7831f184 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustment.kt @@ -0,0 +1,95 @@ +/* + * Copyright 2021 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.inflation + +import android.app.Notification +import android.app.RemoteInput +import android.graphics.drawable.Icon +import android.text.TextUtils + +/** + * An immutable object which contains minimal state extracted from an entry that represents state + * which can change without a direct app update (e.g. with a ranking update). + * Diffing two entries determines if view re-inflation is needed. + */ +class NotifUiAdjustment internal constructor( + val key: String, + val smartActions: List, + val smartReplies: List, + val isConversation: Boolean, + val isMinimized: Boolean +) { + companion object { + @JvmStatic + fun needReinflate( + oldAdjustment: NotifUiAdjustment, + newAdjustment: NotifUiAdjustment + ): Boolean = when { + oldAdjustment === newAdjustment -> false + oldAdjustment.isConversation != newAdjustment.isConversation -> true + oldAdjustment.isMinimized != newAdjustment.isMinimized -> true + areDifferent(oldAdjustment.smartActions, newAdjustment.smartActions) -> true + newAdjustment.smartReplies != oldAdjustment.smartReplies -> true + else -> false + } + + private fun areDifferent( + first: List, + second: List + ): Boolean = when { + first === second -> false + first.size != second.size -> true + else -> first.asSequence().zip(second.asSequence()).any { + (!TextUtils.equals(it.first.title, it.second.title)) || + (areDifferent(it.first.getIcon(), it.second.getIcon())) || + (it.first.actionIntent != it.second.actionIntent) || + (areDifferent(it.first.remoteInputs, it.second.remoteInputs)) + } + } + + private fun areDifferent(first: Icon?, second: Icon?): Boolean = when { + first === second -> false + first == null || second == null -> true + else -> !first.sameAs(second) + } + + private fun areDifferent( + first: Array?, + second: Array? + ): Boolean = when { + first === second -> false + first == null || second == null -> true + first.size != second.size -> true + else -> first.asSequence().zip(second.asSequence()).any { + (!TextUtils.equals(it.first.label, it.second.label)) || + (areDifferent(it.first.choices, it.second.choices)) + } + } + + private fun areDifferent( + first: Array?, + second: Array? + ): Boolean = when { + first === second -> false + first == null || second == null -> true + first.size != second.size -> true + else -> first.asSequence().zip(second.asSequence()).any { + !TextUtils.equals(it.first, it.second) + } + } + } +} \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustmentProvider.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustmentProvider.kt new file mode 100644 index 0000000000000..3290cdffdceb5 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotifUiAdjustmentProvider.kt @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2021 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.inflation + +import com.android.systemui.dagger.SysUISingleton +import com.android.systemui.statusbar.notification.collection.GroupEntry +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner +import javax.inject.Inject + +/** + * A class which provides an adjustment object to the preparation coordinator which is uses + * to ensure that notifications are reinflated when ranking-derived information changes. + */ +@SysUISingleton +open class NotifUiAdjustmentProvider @Inject constructor() { + + private lateinit var lowPrioritySections: Set + + /** + * Feed the provider the information it needs about which sections should have minimized top + * level views, so that it can calculate the correct minimized value in the adjustment. + */ + fun setLowPrioritySections(sections: Collection) { + lowPrioritySections = sections.toSet() + } + + private fun isEntryMinimized(entry: NotificationEntry): Boolean { + val section = entry.section ?: error("Entry must have a section to determine if minimized") + val parent = entry.parent ?: error("Entry must have a parent to determine if minimized") + val isLowPrioritySection = lowPrioritySections.contains(section.sectioner) + val isTopLevelEntry = parent == GroupEntry.ROOT_ENTRY + val isGroupSummary = parent.summary == entry + return isLowPrioritySection && (isTopLevelEntry || isGroupSummary) + } + + /** + * Returns a adjustment object for the given entry. This can be compared to a previous instance + * from the same notification using [NotifUiAdjustment.needReinflate] to determine if it + * should be reinflated. + */ + fun calculateAdjustment(entry: NotificationEntry) = NotifUiAdjustment( + key = entry.key, + smartActions = entry.ranking.smartActions, + smartReplies = entry.ranking.smartReplies, + isConversation = entry.ranking.isConversation, + isMinimized = isEntryMinimized(entry) + ) +} \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinder.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinder.java index 1215ade2abb1b..3a4701c9ac765 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinder.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinder.java @@ -32,12 +32,10 @@ public interface NotificationRowBinder { /** * Called when a notification has been added or updated. The binder must asynchronously inflate * and bind the views associated with the notification. - * - * TODO: The caller is notified when the inflation completes, but this is currently a very - * roundabout business. Add an explicit completion/failure callback to this method. */ void inflateViews( NotificationEntry entry, + NotifInflater.Params params, NotificationRowContentBinder.InflationCallback callback) throws InflationException; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinderImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinderImpl.java index e5425cfc8c93c..5c8e8b244abb7 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinderImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/NotificationRowBinderImpl.java @@ -25,6 +25,7 @@ import android.view.ViewGroup; import com.android.internal.util.NotificationMessagingUtil; import com.android.systemui.dagger.SysUISingleton; +import com.android.systemui.flags.FeatureFlags; import com.android.systemui.statusbar.NotificationLockscreenUserManager; import com.android.systemui.statusbar.NotificationPresenter; import com.android.systemui.statusbar.NotificationRemoteInputManager; @@ -32,6 +33,7 @@ import com.android.systemui.statusbar.NotificationUiAdjustment; import com.android.systemui.statusbar.notification.InflationException; import com.android.systemui.statusbar.notification.NotificationClicker; import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.legacy.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.icon.IconManager; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRowController; @@ -53,6 +55,7 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { private static final String TAG = "NotificationViewManager"; private final Context mContext; + private final FeatureFlags mFeatureFlags; private final NotificationMessagingUtil mMessagingUtil; private final NotificationRemoteInputManager mNotificationRemoteInputManager; private final NotificationLockscreenUserManager mNotificationLockscreenUserManager; @@ -72,6 +75,7 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { @Inject public NotificationRowBinderImpl( Context context, + FeatureFlags featureFlags, NotificationMessagingUtil notificationMessagingUtil, NotificationRemoteInputManager notificationRemoteInputManager, NotificationLockscreenUserManager notificationLockscreenUserManager, @@ -82,6 +86,7 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { IconManager iconManager, LowPriorityInflationHelper lowPriorityInflationHelper) { mContext = context; + mFeatureFlags = featureFlags; mNotifBindPipeline = notifBindPipeline; mRowContentBindStage = rowContentBindStage; mMessagingUtil = notificationMessagingUtil; @@ -116,8 +121,13 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { @Override public void inflateViews( NotificationEntry entry, + NotifInflater.Params params, NotificationRowContentBinder.InflationCallback callback) throws InflationException { + if (params == null) { + // weak assert that the params should always be passed in the new pipeline + mFeatureFlags.checkLegacyPipelineEnabled(); + } ViewGroup parent = mListContainer.getViewParentForNotification(entry); if (entry.rowExists()) { @@ -125,7 +135,7 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { ExpandableNotificationRow row = entry.getRow(); row.reset(); updateRow(entry, row); - inflateContentViews(entry, row, callback); + inflateContentViews(entry, params, row, callback); } else { mIconManager.createIcons(entry); mRowInflaterTaskProvider.get().inflate(mContext, parent, entry, @@ -144,7 +154,7 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { entry.setRowController(rowController); bindRow(entry, row); updateRow(entry, row); - inflateContentViews(entry, row, callback); + inflateContentViews(entry, params, row, callback); }); } } @@ -177,12 +187,13 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { NotificationUiAdjustment oldAdjustment, NotificationUiAdjustment newAdjustment, NotificationRowContentBinder.InflationCallback callback) { + mFeatureFlags.checkLegacyPipelineEnabled(); if (NotificationUiAdjustment.needReinflate(oldAdjustment, newAdjustment)) { if (entry.rowExists()) { ExpandableNotificationRow row = entry.getRow(); row.reset(); updateRow(entry, row); - inflateContentViews(entry, row, callback); + inflateContentViews(entry, null, row, callback); } else { // Once the RowInflaterTask is done, it will pick up the updated entry, so // no-op here. @@ -216,15 +227,24 @@ public class NotificationRowBinderImpl implements NotificationRowBinder { */ private void inflateContentViews( NotificationEntry entry, + NotifInflater.Params inflaterParams, ExpandableNotificationRow row, @Nullable NotificationRowContentBinder.InflationCallback inflationCallback) { final boolean useIncreasedCollapsedHeight = mMessagingUtil.isImportantMessaging(entry.getSbn(), entry.getImportance()); - // If this is our first time inflating, we don't actually know the groupings for real - // yet, so we might actually inflate a low priority content view incorrectly here and have - // to correct it later in the pipeline. On subsequent inflations (i.e. updates), this - // should inflate the correct view. - final boolean isLowPriority = mLowPriorityInflationHelper.shouldUseLowPriorityView(entry); + final boolean isLowPriority; + if (inflaterParams != null) { + // NEW pipeline + isLowPriority = inflaterParams.isLowPriority(); + } else { + // LEGACY pipeline + mFeatureFlags.checkLegacyPipelineEnabled(); + // If this is our first time inflating, we don't actually know the groupings for real + // yet, so we might actually inflate a low priority content view incorrectly here and + // have to correct it later in the pipeline. On subsequent inflations (i.e. updates), + // this should inflate the correct view. + isLowPriority = mLowPriorityInflationHelper.shouldUseLowPriorityView(entry); + } RowContentBindParams params = mRowContentBindStage.getStageParams(entry); params.setUseIncreasedCollapsedHeight(useIncreasedCollapsedHeight); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/LowPriorityInflationHelper.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/LowPriorityInflationHelper.java similarity index 84% rename from packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/LowPriorityInflationHelper.java rename to packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/LowPriorityInflationHelper.java index 518c3f1d19483..dd1f9485a4f3f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/LowPriorityInflationHelper.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/LowPriorityInflationHelper.java @@ -14,13 +14,11 @@ * limitations under the License. */ -package com.android.systemui.statusbar.notification.collection.inflation; +package com.android.systemui.statusbar.notification.collection.legacy; import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.flags.FeatureFlags; -import com.android.systemui.statusbar.notification.collection.GroupEntry; import com.android.systemui.statusbar.notification.collection.NotificationEntry; -import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; import com.android.systemui.statusbar.notification.row.RowContentBindParams; import com.android.systemui.statusbar.notification.row.RowContentBindStage; @@ -61,6 +59,7 @@ public class LowPriorityInflationHelper { public void recheckLowPriorityViewAndInflate( NotificationEntry entry, ExpandableNotificationRow row) { + mFeatureFlags.checkLegacyPipelineEnabled(); RowContentBindParams params = mRowContentBindStage.getStageParams(entry); final boolean shouldBeLowPriority = shouldUseLowPriorityView(entry); if (!row.isRemoved() && row.isLowPriority() != shouldBeLowPriority) { @@ -74,12 +73,7 @@ public class LowPriorityInflationHelper { * Whether the notification should inflate a low priority version of its content views. */ public boolean shouldUseLowPriorityView(NotificationEntry entry) { - boolean isGroupChild; - if (mFeatureFlags.isNewNotifPipelineRenderingEnabled()) { - isGroupChild = (entry.getParent() != GroupEntry.ROOT_ENTRY); - } else { - isGroupChild = mGroupManager.isChildInGroup(entry); - } - return entry.isAmbient() && !isGroupChild; + mFeatureFlags.checkLegacyPipelineEnabled(); + return entry.isAmbient() && !mGroupManager.isChildInGroup(entry); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/NotifSection.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/NotifSection.kt index 6424e37ad3282..8444287cbf60f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/NotifSection.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/NotifSection.kt @@ -27,8 +27,7 @@ data class NotifSection( val label: String get() = "Section($index, $bucket, \"${sectioner.name}\")" - val headerController: NodeController? - get() = sectioner.headerNodeController + val headerController: NodeController? = sectioner.headerNodeController @PriorityBucket val bucket: Int = sectioner.bucket } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationViewHierarchyManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationViewHierarchyManagerTest.java index cf58c63e3d263..acd62b2fabe7a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationViewHierarchyManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationViewHierarchyManagerTest.java @@ -44,7 +44,7 @@ import com.android.systemui.statusbar.notification.DynamicPrivacyController; import com.android.systemui.statusbar.notification.NotificationActivityStarter; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; -import com.android.systemui.statusbar.notification.collection.inflation.LowPriorityInflationHelper; +import com.android.systemui.statusbar.notification.collection.legacy.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy; import com.android.systemui.statusbar.notification.collection.legacy.VisualStabilityManager; import com.android.systemui.statusbar.notification.logging.NotificationLogger; diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationEntryBuilder.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationEntryBuilder.java index fc94262465b13..b91f7e6b6169e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationEntryBuilder.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationEntryBuilder.java @@ -30,6 +30,7 @@ import android.service.notification.StatusBarNotification; import com.android.internal.logging.InstanceId; import com.android.systemui.statusbar.RankingBuilder; import com.android.systemui.statusbar.SbnBuilder; +import com.android.systemui.statusbar.notification.collection.listbuilder.NotifSection; import com.android.systemui.util.time.FakeSystemClock; import java.util.ArrayList; @@ -53,6 +54,7 @@ public class NotificationEntryBuilder { /* ListEntry properties */ private GroupEntry mParent; + private NotifSection mNotifSection; /* If set, use this creation time instead of mClock.uptimeMillis */ private long mCreationTime = -1; @@ -71,6 +73,11 @@ public class NotificationEntryBuilder { mCreationTime = source.getCreationTime(); } + /** Update an the parent on an existing entry */ + public static void setNewParent(NotificationEntry entry, GroupEntry parent) { + entry.setParent(parent); + } + /** Build a new instance of NotificationEntry */ public NotificationEntry build() { return buildOrApply(null); @@ -103,6 +110,7 @@ public class NotificationEntryBuilder { /* ListEntry properties */ entry.setParent(mParent); + entry.getAttachState().setSection(mNotifSection); entry.getAttachState().setStableIndex(mStableIndex); return entry; } @@ -115,6 +123,14 @@ public class NotificationEntryBuilder { return this; } + /** + * Sets the parent. + */ + public NotificationEntryBuilder setSection(@Nullable NotifSection section) { + mNotifSection = section; + return this; + } + /** * Sets the SBN directly. If set, causes all calls to delegated SbnBuilder methods to be * ignored. diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java index bec5174aceba6..c3e10aa3178ff 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java @@ -26,6 +26,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import static java.util.Objects.requireNonNull; @@ -33,10 +34,12 @@ import android.os.RemoteException; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; +import androidx.annotation.NonNull; import androidx.test.filters.SmallTest; import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.SysuiTestCase; +import com.android.systemui.statusbar.RankingBuilder; import com.android.systemui.statusbar.notification.collection.GroupEntry; import com.android.systemui.statusbar.notification.collection.GroupEntryBuilder; import com.android.systemui.statusbar.notification.collection.ListEntry; @@ -44,8 +47,11 @@ import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; +import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; +import com.android.systemui.statusbar.notification.collection.listbuilder.NotifSection; import com.android.systemui.statusbar.notification.collection.listbuilder.OnBeforeFinalizeFilterListener; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener; import com.android.systemui.statusbar.notification.collection.render.NotifViewBarn; import com.android.systemui.statusbar.notification.row.NotifInflationErrorManager; @@ -60,6 +66,7 @@ import org.mockito.MockitoAnnotations; import org.mockito.Spy; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -79,24 +86,36 @@ public class PreparationCoordinatorTest extends SysuiTestCase { @Captor private ArgumentCaptor mCollectionListenerCaptor; @Captor private ArgumentCaptor mBeforeFilterListenerCaptor; @Captor private ArgumentCaptor mCallbackCaptor; + @Captor private ArgumentCaptor mParamsCaptor; + @Mock private NotifSectioner mNotifSectioner; + @Mock private NotifSection mNotifSection; @Mock private NotifPipeline mNotifPipeline; @Mock private IStatusBarService mService; @Spy private FakeNotifInflater mNotifInflater = new FakeNotifInflater(); + private final TestableAdjustmentProvider mAdjustmentProvider = new TestableAdjustmentProvider(); + + @NonNull + private NotificationEntryBuilder getNotificationEntryBuilder() { + return new NotificationEntryBuilder().setSection(mNotifSection); + } @Before public void setUp() { MockitoAnnotations.initMocks(this); - mEntry = new NotificationEntryBuilder().setParent(ROOT_ENTRY).build(); + mEntry = getNotificationEntryBuilder().setParent(ROOT_ENTRY).build(); mInflationError = new Exception(TEST_MESSAGE); mErrorManager = new NotifInflationErrorManager(); + when(mNotifSection.getSectioner()).thenReturn(mNotifSectioner); + mAdjustmentProvider.setSectionIsLowPriority(false); PreparationCoordinator coordinator = new PreparationCoordinator( mock(PreparationCoordinatorLogger.class), mNotifInflater, mErrorManager, mock(NotifViewBarn.class), + mAdjustmentProvider, mService, TEST_CHILD_BIND_CUTOFF, TEST_MAX_GROUP_DELAY); @@ -150,7 +169,7 @@ public class PreparationCoordinatorTest extends SysuiTestCase { mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); // THEN we inflate it - verify(mNotifInflater).inflateViews(eq(mEntry), any()); + verify(mNotifInflater).inflateViews(eq(mEntry), any(), any()); // THEN we filter it out until it's done inflating. assertTrue(mUninflatedFilter.shouldFilterOut(mEntry, 0)); @@ -161,7 +180,7 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN an inflated notification mCollectionListener.onEntryAdded(mEntry); mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); - verify(mNotifInflater).inflateViews(eq(mEntry), mCallbackCaptor.capture()); + verify(mNotifInflater).inflateViews(eq(mEntry), any(), mCallbackCaptor.capture()); mCallbackCaptor.getValue().onInflationFinished(mEntry); // WHEN notification is updated @@ -169,7 +188,90 @@ public class PreparationCoordinatorTest extends SysuiTestCase { mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); // THEN we rebind it - verify(mNotifInflater).rebindViews(eq(mEntry), any()); + verify(mNotifInflater).rebindViews(eq(mEntry), any(), any()); + + // THEN we do not filter it because it's not the first inflation. + assertFalse(mUninflatedFilter.shouldFilterOut(mEntry, 0)); + } + + @Test + public void testEntrySmartReplyAdditionWillRebindViews() { + // GIVEN an inflated notification + mCollectionListener.onEntryAdded(mEntry); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + verify(mNotifInflater).inflateViews(eq(mEntry), any(), mCallbackCaptor.capture()); + mCallbackCaptor.getValue().onInflationFinished(mEntry); + + // WHEN notification ranking now has smart replies + mEntry.setRanking(new RankingBuilder(mEntry.getRanking()).setSmartReplies("yes").build()); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + + // THEN we rebind it + verify(mNotifInflater).rebindViews(eq(mEntry), any(), any()); + + // THEN we do not filter it because it's not the first inflation. + assertFalse(mUninflatedFilter.shouldFilterOut(mEntry, 0)); + } + + @Test + public void testEntryChangedToMinimizedSectionWillRebindViews() { + // GIVEN an inflated notification + mCollectionListener.onEntryAdded(mEntry); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + verify(mNotifInflater).inflateViews(eq(mEntry), + mParamsCaptor.capture(), mCallbackCaptor.capture()); + assertFalse(mParamsCaptor.getValue().isLowPriority()); + mCallbackCaptor.getValue().onInflationFinished(mEntry); + + // WHEN notification moves to a min priority section + mAdjustmentProvider.setSectionIsLowPriority(true); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + + // THEN we rebind it + verify(mNotifInflater).rebindViews(eq(mEntry), mParamsCaptor.capture(), any()); + assertTrue(mParamsCaptor.getValue().isLowPriority()); + + // THEN we do not filter it because it's not the first inflation. + assertFalse(mUninflatedFilter.shouldFilterOut(mEntry, 0)); + } + + @Test + public void testMinimizedEntryMovedIntoGroupWillRebindViews() { + // GIVEN an inflated, minimized notification + mAdjustmentProvider.setSectionIsLowPriority(true); + mCollectionListener.onEntryAdded(mEntry); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + verify(mNotifInflater).inflateViews(eq(mEntry), + mParamsCaptor.capture(), mCallbackCaptor.capture()); + assertTrue(mParamsCaptor.getValue().isLowPriority()); + mCallbackCaptor.getValue().onInflationFinished(mEntry); + + // WHEN notification is moved under a parent + NotificationEntryBuilder.setNewParent(mEntry, mock(GroupEntry.class)); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + + // THEN we rebind it as not-minimized + verify(mNotifInflater).rebindViews(eq(mEntry), mParamsCaptor.capture(), any()); + assertFalse(mParamsCaptor.getValue().isLowPriority()); + + // THEN we do not filter it because it's not the first inflation. + assertFalse(mUninflatedFilter.shouldFilterOut(mEntry, 0)); + } + + @Test + public void testEntryRankChangeWillNotRebindViews() { + // GIVEN an inflated notification + mCollectionListener.onEntryAdded(mEntry); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + verify(mNotifInflater).inflateViews(eq(mEntry), any(), mCallbackCaptor.capture()); + mCallbackCaptor.getValue().onInflationFinished(mEntry); + + // WHEN notification ranking changes rank, which does not affect views + mEntry.setRanking(new RankingBuilder(mEntry.getRanking()).setRank(100).build()); + mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); + + // THEN we do not rebind it + verify(mNotifInflater, never()).rebindViews(eq(mEntry), any(), any()); // THEN we do not filter it because it's not the first inflation. assertFalse(mUninflatedFilter.shouldFilterOut(mEntry, 0)); @@ -180,7 +282,7 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN an inflated notification mCollectionListener.onEntryAdded(mEntry); mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); - verify(mNotifInflater).inflateViews(eq(mEntry), mCallbackCaptor.capture()); + verify(mNotifInflater).inflateViews(eq(mEntry), any(), mCallbackCaptor.capture()); mCallbackCaptor.getValue().onInflationFinished(mEntry); // THEN it isn't filtered from shade list @@ -191,13 +293,13 @@ public class PreparationCoordinatorTest extends SysuiTestCase { public void testCutoffGroupChildrenNotInflated() { // WHEN there is a new notification group is posted int id = 0; - NotificationEntry summary = new NotificationEntryBuilder() + NotificationEntry summary = getNotificationEntryBuilder() .setOverrideGroupKey(TEST_GROUP_KEY) .setId(id++) .build(); List children = new ArrayList<>(); for (int i = 0; i < TEST_CHILD_BIND_CUTOFF + 1; i++) { - NotificationEntry child = new NotificationEntryBuilder() + NotificationEntry child = getNotificationEntryBuilder() .setOverrideGroupKey(TEST_GROUP_KEY) .setId(id++) .build(); @@ -224,9 +326,9 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // THEN we inflate up to the cut-off only for (int i = 0; i < children.size(); i++) { if (i < TEST_CHILD_BIND_CUTOFF) { - verify(mNotifInflater).inflateViews(eq(children.get(i)), any()); + verify(mNotifInflater).inflateViews(eq(children.get(i)), any(), any()); } else { - verify(mNotifInflater, never()).inflateViews(eq(children.get(i)), any()); + verify(mNotifInflater, never()).inflateViews(eq(children.get(i)), any(), any()); } } } @@ -236,9 +338,9 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN a newly-posted group with a summary and two children final GroupEntry group = new GroupEntryBuilder() .setCreationTime(400) - .setSummary(new NotificationEntryBuilder().setId(1).build()) - .addChild(new NotificationEntryBuilder().setId(2).build()) - .addChild(new NotificationEntryBuilder().setId(3).build()) + .setSummary(getNotificationEntryBuilder().setId(1).build()) + .addChild(getNotificationEntryBuilder().setId(2).build()) + .addChild(getNotificationEntryBuilder().setId(3).build()) .build(); fireAddEvents(List.of(group)); final NotificationEntry child0 = group.getChildren().get(0); @@ -256,9 +358,9 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN a newly-posted group with a summary and two children final GroupEntry group = new GroupEntryBuilder() .setCreationTime(400) - .setSummary(new NotificationEntryBuilder().setId(1).build()) - .addChild(new NotificationEntryBuilder().setId(2).build()) - .addChild(new NotificationEntryBuilder().setId(3).build()) + .setSummary(getNotificationEntryBuilder().setId(1).build()) + .addChild(getNotificationEntryBuilder().setId(2).build()) + .addChild(getNotificationEntryBuilder().setId(3).build()) .build(); fireAddEvents(List.of(group)); final NotificationEntry summary = group.getSummary(); @@ -281,9 +383,9 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN a newly-posted group with a summary and two children final GroupEntry group = new GroupEntryBuilder() .setCreationTime(400) - .setSummary(new NotificationEntryBuilder().setId(1).build()) - .addChild(new NotificationEntryBuilder().setId(2).build()) - .addChild(new NotificationEntryBuilder().setId(3).build()) + .setSummary(getNotificationEntryBuilder().setId(1).build()) + .addChild(getNotificationEntryBuilder().setId(2).build()) + .addChild(getNotificationEntryBuilder().setId(3).build()) .build(); fireAddEvents(List.of(group)); final NotificationEntry summary = group.getSummary(); @@ -307,9 +409,9 @@ public class PreparationCoordinatorTest extends SysuiTestCase { // GIVEN a newly-posted group with a summary and two children final GroupEntry group = new GroupEntryBuilder() .setCreationTime(400) - .setSummary(new NotificationEntryBuilder().setId(1).build()) - .addChild(new NotificationEntryBuilder().setId(2).build()) - .addChild(new NotificationEntryBuilder().setId(3).build()) + .setSummary(getNotificationEntryBuilder().setId(1).build()) + .addChild(getNotificationEntryBuilder().setId(2).build()) + .addChild(getNotificationEntryBuilder().setId(3).build()) .build(); fireAddEvents(List.of(group)); final NotificationEntry child0 = group.getChildren().get(0); @@ -324,19 +426,21 @@ public class PreparationCoordinatorTest extends SysuiTestCase { } private static class FakeNotifInflater implements NotifInflater { - private Map mInflateCallbacks = new HashMap<>(); + private final Map mInflateCallbacks = new HashMap<>(); @Override - public void inflateViews(NotificationEntry entry, InflationCallback callback) { + public void inflateViews(@NonNull NotificationEntry entry, @NonNull Params params, + @NonNull InflationCallback callback) { mInflateCallbacks.put(entry, callback); } @Override - public void rebindViews(NotificationEntry entry, InflationCallback callback) { + public void rebindViews(@NonNull NotificationEntry entry, @NonNull Params params, + @NonNull InflationCallback callback) { } @Override - public void abortInflation(NotificationEntry entry) { + public void abortInflation(@NonNull NotificationEntry entry) { } public InflationCallback getInflateCallback(NotificationEntry entry) { @@ -365,4 +469,12 @@ public class PreparationCoordinatorTest extends SysuiTestCase { private static final String TEST_GROUP_KEY = "TEST_GROUP_KEY"; private static final int TEST_CHILD_BIND_CUTOFF = 9; private static final int TEST_MAX_GROUP_DELAY = 100; + + private class TestableAdjustmentProvider extends NotifUiAdjustmentProvider { + private void setSectionIsLowPriority(boolean lowPriority) { + setLowPrioritySections(lowPriority + ? Collections.singleton(mNotifSection.getSectioner()) + : Collections.emptyList()); + } + } } 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 1d45aff00a035..abe33aae7fc68 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 @@ -43,6 +43,7 @@ import com.android.systemui.statusbar.notification.collection.ListEntry; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder; +import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner; import com.android.systemui.statusbar.notification.collection.provider.HighPriorityProvider; @@ -66,6 +67,7 @@ public class RankingCoordinatorTest extends SysuiTestCase { @Mock private StatusBarStateController mStatusBarStateController; @Mock private HighPriorityProvider mHighPriorityProvider; + @Mock private NotifUiAdjustmentProvider mAdjustmentProvider; @Mock private NotifPipeline mNotifPipeline; @Mock private NodeController mAlertingHeaderController; @Mock private NodeController mSilentNodeController; @@ -87,8 +89,12 @@ public class RankingCoordinatorTest extends SysuiTestCase { public void setup() { MockitoAnnotations.initMocks(this); mRankingCoordinator = new RankingCoordinator( - mStatusBarStateController, mHighPriorityProvider, mAlertingHeaderController, - mSilentHeaderController, mSilentNodeController); + mStatusBarStateController, + mHighPriorityProvider, + mAdjustmentProvider, + mAlertingHeaderController, + mSilentHeaderController, + mSilentNodeController); mEntry = spy(new NotificationEntryBuilder().build()); mEntry.setRanking(getRankingForUnfilteredNotif().build()); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java index ed42ac3efe806..0d996025392f7 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java @@ -70,8 +70,8 @@ import com.android.systemui.statusbar.notification.NotificationFilter; import com.android.systemui.statusbar.notification.NotificationSectionsFeatureManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.NotificationRankingManager; -import com.android.systemui.statusbar.notification.collection.inflation.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.collection.inflation.NotificationRowBinderImpl; +import com.android.systemui.statusbar.notification.collection.legacy.LowPriorityInflationHelper; import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy; import com.android.systemui.statusbar.notification.collection.provider.HighPriorityProvider; import com.android.systemui.statusbar.notification.icon.IconBuilder; @@ -283,6 +283,7 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { mRowBinder = new NotificationRowBinderImpl( mContext, + mFeatureFlags, new NotificationMessagingUtil(mContext), mRemoteInputManager, mLockscreenUserManager, From 35d1990c45f0aca37fa24550ee488b9a1565cfdd Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Sun, 7 Nov 2021 21:28:48 +0000 Subject: [PATCH 4/4] New Pipeline: Fix stability cases where stability did not invalidate * We were using the incorrect condition when section reordering was suppressed. * Entries being ordered wrong was not feeding back into the stability coordinator. Fixes: 205441010 Test: atest VisualStabilityCoordinatorTest Change-Id: Iba4cfadd639979d802894b69e03b8ae696814acd --- .../collection/ShadeListBuilder.java | 39 +++++++++- .../VisualStabilityCoordinator.java | 49 ++++++++++-- .../pluggable/NotifStabilityManager.java | 15 ++++ .../collection/ShadeListBuilderTest.java | 36 ++++++++- .../VisualStabilityCoordinatorTest.java | 78 +++++++++++++------ 5 files changed, 186 insertions(+), 31 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 4cdf8927c68b3..72cd95128779b 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 @@ -36,6 +36,7 @@ import android.os.Trace; import android.util.ArrayMap; import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; import com.android.systemui.Dumpable; import com.android.systemui.dagger.SysUISingleton; @@ -66,6 +67,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -793,9 +795,38 @@ public class ShadeListBuilder implements Dumpable { } mNotifList.sort(mTopLevelComparator); assignIndexes(mNotifList); + + // Check for suppressed order changes + if (!mNotifStabilityManager.isEveryChangeAllowed()) { + mForceReorderable = true; + boolean isSorted = isSorted(mNotifList, mTopLevelComparator); + mForceReorderable = false; + if (!isSorted) { + mNotifStabilityManager.onEntryReorderSuppressed(); + } + } Trace.endSection(); } + /** Determine whether the items in the list are sorted according to the comparator */ + @VisibleForTesting + public static boolean isSorted(List items, Comparator comparator) { + if (items.size() <= 1) { + return true; + } + Iterator iterator = items.iterator(); + T previous = iterator.next(); + T current; + while (iterator.hasNext()) { + current = iterator.next(); + if (comparator.compare(previous, current) > 0) { + return false; + } + previous = current; + } + return true; + } + /** * Assign the index of each notification relative to the total order */ @@ -970,8 +1001,14 @@ public class ShadeListBuilder implements Dumpable { return cmp; }; + /** + * A flag that is set to true when we want to run the comparators as if all reordering is + * allowed. This is used to check if the list is "out of order" after the sort is complete. + */ + private boolean mForceReorderable = false; + private boolean canReorder(ListEntry entry) { - return mNotifStabilityManager.isEntryReorderingAllowed(entry); + return mForceReorderable || mNotifStabilityManager.isEntryReorderingAllowed(entry); } private boolean applyFilters(NotificationEntry entry, long now, List filters) { 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 32b1cf6bfdcaa..75489b1faadb8 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 @@ -19,11 +19,12 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import static com.android.systemui.keyguard.WakefulnessLifecycle.WAKEFULNESS_AWAKE; import static com.android.systemui.keyguard.WakefulnessLifecycle.WAKEFULNESS_WAKING; -import android.annotation.NonNull; - +import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; +import com.android.systemui.Dumpable; 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.NotificationViewHierarchyManager; @@ -34,6 +35,8 @@ import com.android.systemui.statusbar.notification.collection.listbuilder.plugga import com.android.systemui.statusbar.policy.HeadsUpManager; import com.android.systemui.util.concurrency.DelayableExecutor; +import java.io.FileDescriptor; +import java.io.PrintWriter; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -53,7 +56,7 @@ import javax.inject.Inject; */ // TODO(b/204468557): Move to @CoordinatorScope @SysUISingleton -public class VisualStabilityCoordinator implements Coordinator { +public class VisualStabilityCoordinator implements Coordinator, Dumpable { private final DelayableExecutor mDelayableExecutor; private final WakefulnessLifecycle mWakefulnessLifecycle; private final StatusBarStateController mStatusBarStateController; @@ -66,6 +69,7 @@ public class VisualStabilityCoordinator implements Coordinator { private boolean mReorderingAllowed; private boolean mIsSuppressingGroupChange = false; private final Set mEntriesWithSuppressedSectionChange = new HashSet<>(); + private boolean mIsSuppressingEntryReorder = false; // key: notification key that can temporarily change its section // value: runnable that when run removes its associated RemoveOverrideSuppressionRunnable @@ -77,6 +81,7 @@ public class VisualStabilityCoordinator implements Coordinator { @Inject public VisualStabilityCoordinator( + DumpManager dumpManager, HeadsUpManager headsUpManager, WakefulnessLifecycle wakefulnessLifecycle, StatusBarStateController statusBarStateController, @@ -86,6 +91,8 @@ public class VisualStabilityCoordinator implements Coordinator { mWakefulnessLifecycle = wakefulnessLifecycle; mStatusBarStateController = statusBarStateController; mDelayableExecutor = delayableExecutor; + + dumpManager.registerDumpable(this); } @Override @@ -99,7 +106,6 @@ public class VisualStabilityCoordinator implements Coordinator { pipeline.setVisualStabilityManager(mNotifStabilityManager); } - // TODO(b/203828145): Ensure stability manager handles minimized state changes // TODO(b/203826051): Ensure stability manager can allow reordering off-screen // HUNs to the top of the shade private final NotifStabilityManager mNotifStabilityManager = @@ -108,6 +114,7 @@ public class VisualStabilityCoordinator implements Coordinator { public void onBeginRun() { mIsSuppressingGroupChange = false; mEntriesWithSuppressedSectionChange.clear(); + mIsSuppressingEntryReorder = false; } @Override @@ -124,7 +131,7 @@ public class VisualStabilityCoordinator implements Coordinator { mReorderingAllowed || mHeadsUpManager.isAlerting(entry.getKey()) || mEntriesThatCanChangeSection.containsKey(entry.getKey()); - if (isSectionChangeAllowedForEntry) { + if (!isSectionChangeAllowedForEntry) { mEntriesWithSuppressedSectionChange.add(entry.getKey()); } return isSectionChangeAllowedForEntry; @@ -134,11 +141,22 @@ public class VisualStabilityCoordinator implements Coordinator { public boolean isEntryReorderingAllowed(ListEntry section) { return mReorderingAllowed; } + + @Override + public boolean isEveryChangeAllowed() { + return mReorderingAllowed; + } + + @Override + public void onEntryReorderSuppressed() { + mIsSuppressingEntryReorder = true; + } }; private void updateAllowedStates() { mReorderingAllowed = isReorderingAllowed(); - if (mReorderingAllowed && (mIsSuppressingGroupChange || isSuppressingSectionChange())) { + if (mReorderingAllowed && (mIsSuppressingGroupChange || isSuppressingSectionChange() + || mIsSuppressingEntryReorder)) { mNotifStabilityManager.invalidateList(); } } @@ -211,4 +229,23 @@ public class VisualStabilityCoordinator implements Coordinator { updateAllowedStates(); } }; + + @Override + public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { + pw.println("reorderingAllowed: " + mReorderingAllowed); + pw.println(" screenOn: " + mScreenOn); + pw.println(" panelExpanded: " + mPanelExpanded); + pw.println(" pulsing: " + mPulsing); + pw.println("isSuppressingGroupChange: " + mIsSuppressingGroupChange); + pw.println("isSuppressingEntryReorder: " + mIsSuppressingEntryReorder); + pw.println("entriesWithSuppressedSectionChange: " + + mEntriesWithSuppressedSectionChange.size()); + for (String key : mEntriesWithSuppressedSectionChange) { + pw.println(" " + key); + } + pw.println("entriesThatCanChangeSection: " + mEntriesThatCanChangeSection.size()); + for (String key : mEntriesThatCanChangeSection.keySet()) { + pw.println(" " + key); + } + } } 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 index 520791c918033..cb2d3cb97468a 100644 --- 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 @@ -60,4 +60,19 @@ public abstract class NotifStabilityManager extends Pluggable intCmp = Integer::compare; + assertTrue(ShadeListBuilder.isSorted(Collections.emptyList(), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Collections.singletonList(1), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 2), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 3), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 3, 4), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 3, 4, 5), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 1, 1, 1, 1), intCmp)); + assertTrue(ShadeListBuilder.isSorted(Arrays.asList(1, 1, 2, 2, 3, 3), intCmp)); + + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(2, 1), intCmp)); + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(2, 1, 2), intCmp)); + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 1), intCmp)); + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 3, 2, 5), intCmp)); + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(5, 2, 3, 4, 5), intCmp)); + assertFalse(ShadeListBuilder.isSorted(Arrays.asList(1, 2, 3, 4, 1), intCmp)); + } + /** * Adds a notif to the collection that will be passed to the list builder when * {@link #dispatchBuild()}s is called. @@ -1918,6 +1943,15 @@ public class ShadeListBuilderTest extends SysuiTestCase { public boolean isEntryReorderingAllowed(ListEntry entry) { return mAllowEntryReodering; } + + @Override + public boolean isEveryChangeAllowed() { + return mAllowEntryReodering && mAllowGroupChanges && mAllowSectionChanges; + } + + @Override + public void onEntryReorderSuppressed() { + } } private static final String PACKAGE_1 = "com.test1"; 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 4edca7dd43d94..5df1d28073fc9 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 @@ -19,8 +19,10 @@ package com.android.systemui.statusbar.notification.collection.coordinator; import static junit.framework.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -30,6 +32,7 @@ import android.testing.TestableLooper; import androidx.test.filters.SmallTest; 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.NotifPipeline; @@ -37,7 +40,6 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifStabilityManager; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.Pluggable; -import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener; import com.android.systemui.statusbar.policy.HeadsUpManager; import com.android.systemui.util.concurrency.FakeExecutor; import com.android.systemui.util.time.FakeSystemClock; @@ -57,9 +59,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { private VisualStabilityCoordinator mCoordinator; - // captured listeners and pluggables: - private NotifCollectionListener mCollectionListener; - + @Mock private DumpManager mDumpManager; @Mock private NotifPipeline mNotifPipeline; @Mock private WakefulnessLifecycle mWakefulnessLifecycle; @Mock private StatusBarStateController mStatusBarStateController; @@ -69,7 +69,6 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { @Captor private ArgumentCaptor mWakefulnessObserverCaptor; @Captor private ArgumentCaptor mSBStateListenerCaptor; @Captor private ArgumentCaptor mNotifStabilityManagerCaptor; - @Captor private ArgumentCaptor mNotifCollectionListenerCaptor; private FakeSystemClock mFakeSystemClock = new FakeSystemClock(); private FakeExecutor mFakeExecutor = new FakeExecutor(mFakeSystemClock); @@ -84,6 +83,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { MockitoAnnotations.initMocks(this); mCoordinator = new VisualStabilityCoordinator( + mDumpManager, mHeadsUpManager, mWakefulnessLifecycle, mStatusBarStateController, @@ -107,6 +107,12 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { .build(); when(mHeadsUpManager.isAlerting(mEntry.getKey())).thenReturn(false); + + // Whenever we invalidate, the pipeline runs again, so we invalidate the state + doAnswer(i -> { + mNotifStabilityManager.onBeginRun(); + return null; + }).when(mInvalidateListener).onPluggableInvalidated(eq(mNotifStabilityManager)); } @Test @@ -211,7 +217,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { mCoordinator.temporarilyAllowSectionChanges(mEntry, mFakeSystemClock.uptimeMillis()); // THEN the notification list is invalidated - verifyInvalidateCalled(true); + verify(mInvalidateListener, times(1)).onPluggableInvalidated(mNotifStabilityManager); } @Test @@ -225,7 +231,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { mCoordinator.temporarilyAllowSectionChanges(mEntry, mFakeSystemClock.currentTimeMillis()); // THEN invalidate is not called because this entry was never suppressed from reordering - verifyInvalidateCalled(false); + verify(mInvalidateListener, never()).onPluggableInvalidated(mNotifStabilityManager); } @Test @@ -241,7 +247,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN invalidate is not called because this entry was never suppressed from reordering; // THEN section changes are allowed for this notification - verifyInvalidateCalled(false); + verify(mInvalidateListener, never()).onPluggableInvalidated(mNotifStabilityManager); assertTrue(mNotifStabilityManager.isSectionChangeAllowed(mEntry)); // WHEN we're pulsing (now disallowing reordering) @@ -268,13 +274,14 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // WHEN we temporarily allow section changes for this notification entry mCoordinator.temporarilyAllowSectionChanges(mEntry, mFakeSystemClock.currentTimeMillis()); - verifyInvalidateCalled(true); // can now reorder, so invalidates + // can now reorder, so invalidates + verify(mInvalidateListener, times(1)).onPluggableInvalidated(mNotifStabilityManager); // WHEN reordering is now allowed because device isn't pulsing anymore setPulsing(false); - // THEN invalidate isn't called since reordering was already allowed - verifyInvalidateCalled(false); + // THEN invalidate isn't called a second time since reordering was already allowed + verify(mInvalidateListener, times(1)).onPluggableInvalidated(mNotifStabilityManager); } @Test @@ -292,7 +299,7 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { // THEN we never see any calls to invalidate since there weren't any notifications that // were being suppressed from grouping or section changes - verifyInvalidateCalled(false); + verify(mInvalidateListener, never()).onPluggableInvalidated(mNotifStabilityManager); } @Test @@ -308,7 +315,41 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { setPanelExpanded(false); // invalidate is called because we were previously suppressing a group change - verifyInvalidateCalled(true); + verify(mInvalidateListener, times(1)).onPluggableInvalidated(mNotifStabilityManager); + } + + @Test + public void testNotSuppressingEntryReorderingAnymoreWillInvalidate() { + // GIVEN visual stability is being maintained b/c panel is expanded + setPulsing(false); + setScreenOn(true); + setPanelExpanded(true); + + assertFalse(mNotifStabilityManager.isEntryReorderingAllowed(mEntry)); + // The pipeline still has to report back that entry reordering was suppressed + mNotifStabilityManager.onEntryReorderSuppressed(); + + // WHEN the panel isn't expanded anymore + setPanelExpanded(false); + + // invalidate is called because we were previously suppressing an entry reorder + verify(mInvalidateListener, times(1)).onPluggableInvalidated(mNotifStabilityManager); + } + + @Test + public void testQueryingEntryReorderingButNotReportingReorderSuppressedDoesNotInvalidate() { + // GIVEN visual stability is being maintained b/c panel is expanded + setPulsing(false); + setScreenOn(true); + setPanelExpanded(true); + + assertFalse(mNotifStabilityManager.isEntryReorderingAllowed(mEntry)); + + // WHEN the panel isn't expanded anymore + setPanelExpanded(false); + + // invalidate is not called because we were not told that an entry reorder was suppressed + verify(mInvalidateListener, never()).onPluggableInvalidated(mNotifStabilityManager); } @Test @@ -345,13 +386,4 @@ public class VisualStabilityCoordinatorTest extends SysuiTestCase { mStatusBarStateListener.onExpandedChanged(expanded); } - private void verifyInvalidateCalled(boolean invalidateCalled) { - if (invalidateCalled) { - verify(mInvalidateListener).onPluggableInvalidated(mNotifStabilityManager); - } else { - verify(mInvalidateListener, never()).onPluggableInvalidated(mNotifStabilityManager); - } - - reset(mInvalidateListener); - } }