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 31d9f09d15696..b328ae8cd0bb2 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 @@ -226,8 +226,13 @@ public class ShadeListBuilder implements Dumpable { mNotifSections.clear(); for (NotifSectioner sectioner : sectioners) { - mNotifSections.add(new NotifSection(sectioner, mNotifSections.size())); + final NotifSection section = new NotifSection(sectioner, mNotifSections.size()); + final NotifComparator sectionComparator = section.getComparator(); + mNotifSections.add(section); sectioner.setInvalidationListener(this::onNotifSectionInvalidated); + if (sectionComparator != null) { + sectionComparator.setInvalidationListener(this::onNotifComparatorInvalidated); + } } mNotifSections.add(new NotifSection(DEFAULT_SECTIONER, mNotifSections.size())); @@ -1098,7 +1103,12 @@ public class ShadeListBuilder implements Dumpable { callOnCleanup(mNotifComparators); for (int i = 0; i < mNotifSections.size(); i++) { - mNotifSections.get(i).getSectioner().onCleanup(); + final NotifSection notifSection = mNotifSections.get(i); + notifSection.getSectioner().onCleanup(); + final NotifComparator comparator = notifSection.getComparator(); + if (comparator != null) { + comparator.onCleanup(); + } } callOnCleanup(List.of(getStabilityManager())); @@ -1111,6 +1121,19 @@ public class ShadeListBuilder implements Dumpable { } } + @Nullable + private NotifComparator getSectionComparator( + @NonNull ListEntry o1, @NonNull ListEntry o2) { + final NotifSection section = o1.getSection(); + if (section != o2.getSection()) { + throw new RuntimeException("Entry ordering should only be done within sections"); + } + if (section != null) { + return section.getComparator(); + } + return null; + } + private final Comparator mTopLevelComparator = (o1, o2) -> { int cmp = Integer.compare( o1.getSectionIndex(), @@ -1122,6 +1145,12 @@ public class ShadeListBuilder implements Dumpable { cmp = Integer.compare(index1, index2); if (cmp != 0) return cmp; + NotifComparator sectionComparator = getSectionComparator(o1, o2); + if (sectionComparator != null) { + cmp = sectionComparator.compare(o1, o2); + if (cmp != 0) return cmp; + } + for (int i = 0; i < mNotifComparators.size(); i++) { cmp = mNotifComparators.get(i).compare(o1, o2); if (cmp != 0) return cmp; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinator.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinator.kt index ba88ad7844f1d..a390e9f9b09df 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinator.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinator.kt @@ -51,23 +51,20 @@ class ConversationCoordinator @Inject constructor( val sectioner = object : NotifSectioner("People", BUCKET_PEOPLE) { override fun isInSection(entry: ListEntry): Boolean = isConversation(entry) + + override fun getComparator() = object : NotifComparator("People") { + override fun compare(entry1: ListEntry, entry2: ListEntry): Int { + val type1 = getPeopleType(entry1) + val type2 = getPeopleType(entry2) + return type2.compareTo(type1) + } + } + override fun getHeaderNodeController() = // TODO: remove SHOW_ALL_SECTIONS, this redundant method, and peopleHeaderController if (RankingCoordinator.SHOW_ALL_SECTIONS) peopleHeaderController else null } - val comparator = object : NotifComparator("People") { - override fun compare(entry1: ListEntry, entry2: ListEntry): Int { - assert(entry1.section === entry2.section) - if (entry1.section?.sectioner !== sectioner) { - return 0 - } - val type1 = getPeopleType(entry1) - val type2 = getPeopleType(entry2) - return type2.compareTo(type1) - } - } - override fun attach(pipeline: NotifPipeline) { pipeline.addPromoter(notificationPromoter) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt index 0311324241158..41b070635d4f7 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt @@ -26,6 +26,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.listbuilder.pluggable.NotifComparator import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifPromoter import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener @@ -456,6 +457,13 @@ class HeadsUpCoordinator @Inject constructor( // TODO: This check won't notice if a child of the group is going to HUN... isGoingToShowHunNoRetract(entry) + override fun getComparator(): NotifComparator { + return object : NotifComparator("HeadsUp") { + override fun compare(o1: ListEntry, o2: ListEntry): Int = + mHeadsUpManager.compare(o1.representativeEntry, o2.representativeEntry) + } + } + override fun getHeaderNodeController(): NodeController? = // TODO: remove SHOW_ALL_SECTIONS, this redundant method, and mIncomingHeaderController if (RankingCoordinator.SHOW_ALL_SECTIONS) mIncomingHeaderController else null diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt index 850cb4b881545..757fb5a2fe9a3 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt @@ -20,7 +20,6 @@ import com.android.systemui.dump.DumpManager import com.android.systemui.statusbar.notification.NotifPipelineFlags import com.android.systemui.statusbar.notification.collection.NotifPipeline import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope -import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifComparator import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner import java.io.FileDescriptor import java.io.PrintWriter @@ -64,7 +63,6 @@ class NotifCoordinatorsImpl @Inject constructor( private val mCoordinators: MutableList = ArrayList() private val mOrderedSections: MutableList = ArrayList() - private val mOrderedComparators: MutableList = ArrayList() /** * Creates all the coordinators. @@ -119,9 +117,6 @@ class NotifCoordinatorsImpl @Inject constructor( mOrderedSections.add(rankingCoordinator.alertingSectioner) // Alerting mOrderedSections.add(rankingCoordinator.silentSectioner) // Silent mOrderedSections.add(rankingCoordinator.minimizedSectioner) // Minimized - - // Manually add ordered comparators - mOrderedComparators.add(conversationCoordinator.comparator) } /** @@ -133,7 +128,6 @@ class NotifCoordinatorsImpl @Inject constructor( c.attach(pipeline) } pipeline.setSections(mOrderedSections) - pipeline.setComparators(mOrderedComparators) } override fun dump(fd: FileDescriptor, pw: PrintWriter, args: Array) { 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 8444287cbf60f..263737e20a133 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 @@ -16,6 +16,7 @@ package com.android.systemui.statusbar.notification.collection.listbuilder +import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifComparator import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner import com.android.systemui.statusbar.notification.collection.render.NodeController import com.android.systemui.statusbar.notification.stack.PriorityBucket @@ -29,5 +30,7 @@ data class NotifSection( val headerController: NodeController? = sectioner.headerNodeController + val comparator: NotifComparator? = sectioner.comparator + @PriorityBucket val bucket: Int = sectioner.bucket } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifSectioner.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifSectioner.java index ef9ee11ef116a..8c52c53ea6b27 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifSectioner.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/pluggable/NotifSectioner.java @@ -54,9 +54,23 @@ public abstract class NotifSectioner extends Pluggable { */ public abstract boolean isInSection(ListEntry entry); + /** + * Returns an optional {@link NotifComparator} to sort entries only in this section. + * {@link ListEntry} instances passed to this comparator are guaranteed to have this section, + * and this ordering will take precedence over any global comparators supplied to {@link + * com.android.systemui.statusbar.notification.collection.NotifPipeline#setComparators(List)}. + * + * NOTE: this method is only called once when the Sectioner is attached. + */ + public @Nullable NotifComparator getComparator() { + return null; + } + /** * Returns an optional {@link NodeSpec} for the section header. If {@code null}, no header will * be used for the section. + * + * NOTE: this method is only called once when the Sectioner is attached. */ public @Nullable NodeController getHeaderNodeController() { return null; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/HeadsUpManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/policy/HeadsUpManager.java index 3084a957aeb57..784a54681484f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/HeadsUpManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/HeadsUpManager.java @@ -350,11 +350,14 @@ public abstract class HeadsUpManager extends AlertingNotificationManager { * @return -1 if the first argument should be ranked higher than the second, 1 if the second * one should be ranked higher and 0 if they are equal. */ - public int compare(@NonNull NotificationEntry a, @NonNull NotificationEntry b) { + public int compare(@Nullable NotificationEntry a, @Nullable NotificationEntry b) { + if (a == null || b == null) { + return Boolean.compare(a == null, b == null); + } AlertEntry aEntry = getHeadsUpEntry(a.getKey()); AlertEntry bEntry = getHeadsUpEntry(b.getKey()); if (aEntry == null || bEntry == null) { - return aEntry == null ? 1 : -1; + return Boolean.compare(aEntry == null, bEntry == null); } return aEntry.compareTo(bEntry); } 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 8fb066bb4a391..cb248b05e4bd6 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 @@ -29,6 +29,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.inOrder; @@ -46,6 +47,7 @@ import android.testing.TestableLooper; import android.util.ArrayMap; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import androidx.test.filters.SmallTest; import com.android.systemui.SysuiTestCase; @@ -871,6 +873,73 @@ public class ShadeListBuilderTest extends SysuiTestCase { ); } + @Test + public void testThatSectionComparatorsAreCalled() { + // GIVEN a section with a comparator that elevates some packages over others + NotifComparator comparator = spy(new HypeComparator(PACKAGE_2, PACKAGE_4)); + NotifSectioner sectioner = new PackageSectioner( + List.of(PACKAGE_1, PACKAGE_2, PACKAGE_4, PACKAGE_5), comparator); + mListBuilder.setSectioners(List.of(sectioner)); + + // WHEN the pipeline is kicked off on a bunch of notifications + addNotif(0, PACKAGE_0); + addNotif(1, PACKAGE_1); + addNotif(2, PACKAGE_2); + addNotif(3, PACKAGE_3); + addNotif(4, PACKAGE_4); + addNotif(5, PACKAGE_5); + dispatchBuild(); + + // THEN the notifs are sorted according to both sectioning and the section's comparator + verifyBuiltList( + notif(2), + notif(4), + notif(1), + notif(5), + notif(0), + notif(3) + ); + + // VERIFY that the comparator is invoked at least 3 times + verify(comparator, atLeast(3)).compare(any(), any()); + + // VERIFY that the comparator is never invoked with the entry from package 0 or 3. + final NotificationEntry package0Entry = mEntrySet.get(0); + verify(comparator, never()).compare(eq(package0Entry), any()); + verify(comparator, never()).compare(any(), eq(package0Entry)); + final NotificationEntry package3Entry = mEntrySet.get(3); + verify(comparator, never()).compare(eq(package3Entry), any()); + verify(comparator, never()).compare(any(), eq(package3Entry)); + } + + @Test + public void testThatSectionComparatorsAreNotCalledForSectionWithSingleEntry() { + // GIVEN a section with a comparator that will have only 1 element + NotifComparator comparator = spy(new HypeComparator(PACKAGE_3)); + NotifSectioner sectioner = new PackageSectioner(List.of(PACKAGE_3), comparator); + mListBuilder.setSectioners(List.of(sectioner)); + + // WHEN the pipeline is kicked off on a bunch of notifications + addNotif(0, PACKAGE_1); + addNotif(1, PACKAGE_2); + addNotif(2, PACKAGE_3); + addNotif(3, PACKAGE_4); + addNotif(4, PACKAGE_5); + dispatchBuild(); + + // THEN the notifs are sorted according to the sectioning + verifyBuiltList( + notif(2), + notif(0), + notif(1), + notif(3), + notif(4) + ); + + // VERIFY that the comparator is never invoked + verify(comparator, never()).compare(any(), any()); + } + @Test public void testListenersAndPluggablesAreFiredInOrder() { // GIVEN a bunch of registered listeners and pluggables @@ -934,7 +1003,8 @@ public class ShadeListBuilderTest extends SysuiTestCase { // GIVEN a variety of pluggables NotifFilter packageFilter = new PackageFilter(PACKAGE_1); NotifPromoter idPromoter = new IdPromoter(4); - NotifSectioner section = new PackageSectioner(PACKAGE_1); + NotifComparator sectionComparator = new HypeComparator(PACKAGE_1); + NotifSectioner section = new PackageSectioner(List.of(PACKAGE_1), sectionComparator); NotifComparator hypeComparator = new HypeComparator(PACKAGE_2); Invalidator preRenderInvalidator = new Invalidator("PreRenderInvalidator") {}; @@ -968,6 +1038,10 @@ public class ShadeListBuilderTest extends SysuiTestCase { hypeComparator.invalidateList(); verify(mOnRenderListListener).onRenderList(anyList()); + clearInvocations(mOnRenderListListener); + sectionComparator.invalidateList(); + verify(mOnRenderListListener).onRenderList(anyList()); + clearInvocations(mOnRenderListListener); preRenderInvalidator.invalidateList(); verify(mOnRenderListListener).onRenderList(anyList()); @@ -2037,16 +2111,30 @@ public class ShadeListBuilderTest extends SysuiTestCase { /** Represents a section for the passed pkg */ private static class PackageSectioner extends NotifSectioner { - private final String mPackage; + private final List mPackages; + private final NotifComparator mComparator; + + PackageSectioner(List pkgs, NotifComparator comparator) { + super("PackageSection_" + pkgs, 0); + mPackages = pkgs; + mComparator = comparator; + } PackageSectioner(String pkg) { super("PackageSection_" + pkg, 0); - mPackage = pkg; + mPackages = List.of(pkg); + mComparator = null; + } + + @Nullable + @Override + public NotifComparator getComparator() { + return mComparator; } @Override public boolean isInSection(ListEntry entry) { - return entry.getRepresentativeEntry().getSbn().getPackageName().equals(mPackage); + return mPackages.contains(entry.getRepresentativeEntry().getSbn().getPackageName()); } } @@ -2157,6 +2245,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { } } + private static final String PACKAGE_0 = "com.test0"; private static final String PACKAGE_1 = "com.test1"; private static final String PACKAGE_2 = "com.test2"; private static final String PACKAGE_3 = "org.test3"; diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinatorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinatorTest.kt index 8deac94214bdd..7692a05eb5fcb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinatorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ConversationCoordinatorTest.kt @@ -32,7 +32,6 @@ import com.android.systemui.statusbar.notification.collection.render.NodeControl import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier.Companion.TYPE_IMPORTANT_PERSON import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier.Companion.TYPE_PERSON -import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.withArgCaptor import com.google.common.truth.Truth.assertThat import org.junit.Assert.assertFalse @@ -41,7 +40,6 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock -import org.mockito.Mockito.never import org.mockito.Mockito.verify import org.mockito.MockitoAnnotations import org.mockito.Mockito.`when` as whenever @@ -79,7 +77,7 @@ class ConversationCoordinatorTest : SysuiTestCase() { } peopleSectioner = coordinator.sectioner - peopleComparator = coordinator.comparator + peopleComparator = peopleSectioner.comparator!! entry = NotificationEntryBuilder().setChannel(channel).build() @@ -107,16 +105,6 @@ class ConversationCoordinatorTest : SysuiTestCase() { assertFalse(peopleSectioner.isInSection(NotificationEntryBuilder().build())) } - @Test - fun testComparatorIgnoresFromOtherSection() { - val e1 = NotificationEntryBuilder().setId(1).setChannel(channel).build() - val e2 = NotificationEntryBuilder().setId(2).setChannel(channel).build() - - // wrong section -- never classify - assertThat(peopleComparator.compare(e1, e2)).isEqualTo(0) - verify(peopleNotificationIdentifier, never()).getPeopleNotificationType(any()) - } - @Test fun testComparatorPutsImportantPeopleFirst() { whenever(peopleNotificationIdentifier.getPeopleNotificationType(entryA)) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HeadsUpManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HeadsUpManagerTest.java index d3258408b33a5..424a400589977 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HeadsUpManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HeadsUpManagerTest.java @@ -127,6 +127,28 @@ public class HeadsUpManagerTest extends AlertingNotificationManagerTest { assertFalse(mHeadsUpManager.isAlerting(mEntry.getKey())); } + @Test + public void testCompareTo_withNullEntries() { + NotificationEntry alertEntry = new NotificationEntryBuilder().setTag("alert").build(); + mHeadsUpManager.showNotification(alertEntry); + + assertThat(mHeadsUpManager.compare(alertEntry, null)).isLessThan(0); + assertThat(mHeadsUpManager.compare(null, alertEntry)).isGreaterThan(0); + assertThat(mHeadsUpManager.compare(null, null)).isEqualTo(0); + } + + @Test + public void testCompareTo_withNonAlertEntries() { + NotificationEntry nonAlertEntry1 = new NotificationEntryBuilder().setTag("nae1").build(); + NotificationEntry nonAlertEntry2 = new NotificationEntryBuilder().setTag("nae2").build(); + NotificationEntry alertEntry = new NotificationEntryBuilder().setTag("alert").build(); + mHeadsUpManager.showNotification(alertEntry); + + assertThat(mHeadsUpManager.compare(alertEntry, nonAlertEntry1)).isLessThan(0); + assertThat(mHeadsUpManager.compare(nonAlertEntry1, alertEntry)).isGreaterThan(0); + assertThat(mHeadsUpManager.compare(nonAlertEntry1, nonAlertEntry2)).isEqualTo(0); + } + @Test public void testAlertEntryCompareTo_ongoingCallLessThanActiveRemoteInput() { HeadsUpManager.HeadsUpEntry ongoingCall = mHeadsUpManager.new HeadsUpEntry();