Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6c579ef0 authored by Jeff DeCew's avatar Jeff DeCew
Browse files

Clean up index assignments and comparators 

* The main "fix" here is to set the stable index on the summary of a notification, which ensures that if we prune the group, we won't lose the ordering.
* Additionally, I made a functional change here to ignore the stable index when a notification moves between sections (which is super rare anyway) because that value would enforce an incorrect order.

Test: atest ShadeListBuilderTest
Bug: 237216329
Change-Id: Ia36f652478a9efdad71ba651640a12da5fa1345b
parent 64f07c00

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListAttachState.kt

+3 −0
Original line number Diff line number Diff line
@@ -68,6 +68,9 @@ data class ListAttachState private constructor( 
     */

    var stableIndex: Int = -1



    /** Access the index of the [section] or -1 if the entry does not have one */

    val sectionIndex: Int get() = section?.index ?: -1



    /** Copies the state of another instance. */

    fun clone(other: ListAttachState) {

        parent = other.parent

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java

+44 −27
Original line number Diff line number Diff line
@@ -1039,22 +1039,25 @@ public class ShadeListBuilder implements Dumpable { 
        NotifSection currentSection = requireNonNull(notifList.get(0).getSection());

        int sectionMemberIndex = 0;

        for (int i = 0; i < notifList.size(); i++) {

            ListEntry entry = notifList.get(i);

            final ListEntry entry = notifList.get(i);

            NotifSection section = requireNonNull(entry.getSection());

            if (section.getIndex() != currentSection.getIndex()) {

                sectionMemberIndex = 0;

                currentSection = section;

            }

            entry.getAttachState().setStableIndex(sectionMemberIndex);

            entry.getAttachState().setStableIndex(sectionMemberIndex++);

            if (entry instanceof GroupEntry) {

                GroupEntry parent = (GroupEntry) entry;

                for (int j = 0; j < parent.getChildren().size(); j++) {

                    entry = parent.getChildren().get(j);

                    entry.getAttachState().setStableIndex(sectionMemberIndex);

                    sectionMemberIndex++;

                final GroupEntry parent = (GroupEntry) entry;

                final NotificationEntry summary = parent.getSummary();

                if (summary != null) {

                    summary.getAttachState().setStableIndex(sectionMemberIndex++);

                }

                final List<NotificationEntry> children = parent.getChildren();

                for (int j = 0; j < children.size(); j++) {

                    final NotificationEntry child = children.get(j);

                    child.getAttachState().setStableIndex(sectionMemberIndex++);

                }

            }

            sectionMemberIndex++;

        }

    }
@@ -1194,9 +1197,9 @@ public class ShadeListBuilder implements Dumpable { 
                o2.getSectionIndex());

        if (cmp != 0) return cmp;



        int index1 = canReorder(o1) ? -1 : o1.getPreviousAttachState().getStableIndex();

        int index2 = canReorder(o2) ? -1 : o2.getPreviousAttachState().getStableIndex();

        cmp = Integer.compare(index1, index2);

        cmp = Integer.compare(

                getStableOrderIndex(o1),

                getStableOrderIndex(o2));

        if (cmp != 0) return cmp;



        NotifComparator sectionComparator = getSectionComparator(o1, o2);
@@ -1210,31 +1213,32 @@ public class ShadeListBuilder implements Dumpable { 
            if (cmp != 0) return cmp;

        }



        final NotificationEntry rep1 = o1.getRepresentativeEntry();

        final NotificationEntry rep2 = o2.getRepresentativeEntry();

            cmp = rep1.getRanking().getRank() - rep2.getRanking().getRank();

        cmp = Integer.compare(

                o1.getRepresentativeEntry().getRanking().getRank(),

                o2.getRepresentativeEntry().getRanking().getRank());

        if (cmp != 0) return cmp;



        cmp = Long.compare(

                rep2.getSbn().getNotification().when,

                rep1.getSbn().getNotification().when);

        cmp = -1 * Long.compare(

                o1.getRepresentativeEntry().getSbn().getNotification().when,

                o2.getRepresentativeEntry().getSbn().getNotification().when);

        return cmp;

    };





    private final Comparator<NotificationEntry> mGroupChildrenComparator = (o1, o2) -> {

        int index1 = canReorder(o1) ? -1 : o1.getPreviousAttachState().getStableIndex();

        int index2 = canReorder(o2) ? -1 : o2.getPreviousAttachState().getStableIndex();

        int cmp = Integer.compare(index1, index2);

        int cmp = Integer.compare(

                getStableOrderIndex(o1),

                getStableOrderIndex(o2));

        if (cmp != 0) return cmp;



        cmp = o1.getRepresentativeEntry().getRanking().getRank()

                - o2.getRepresentativeEntry().getRanking().getRank();

        cmp = Integer.compare(

                o1.getRepresentativeEntry().getRanking().getRank(),

                o2.getRepresentativeEntry().getRanking().getRank());

        if (cmp != 0) return cmp;



        cmp = Long.compare(

                o2.getRepresentativeEntry().getSbn().getNotification().when,

                o1.getRepresentativeEntry().getSbn().getNotification().when);

        cmp = -1 * Long.compare(

                o1.getRepresentativeEntry().getSbn().getNotification().when,

                o2.getRepresentativeEntry().getSbn().getNotification().when);

        return cmp;

    };
@@ -1244,8 +1248,21 @@ public class ShadeListBuilder implements Dumpable { 
     */

    private boolean mForceReorderable = false;



    private boolean canReorder(ListEntry entry) {

        return mForceReorderable || getStabilityManager().isEntryReorderingAllowed(entry);

    private int getStableOrderIndex(ListEntry entry) {

        if (mForceReorderable) {

            // this is used to determine if the list is correctly sorted

            return -1;

        }

        if (getStabilityManager().isEntryReorderingAllowed(entry)) {

            // let the stability manager constrain or allow reordering

            return -1;

        }

        if (entry.getAttachState().getSectionIndex()

                != entry.getPreviousAttachState().getSectionIndex()) {

            // stable index is only valid within the same section; otherwise we allow reordering

            return -1;

        }

        return entry.getPreviousAttachState().getStableIndex();

    }



    private boolean applyFilters(NotificationEntry entry, long now, List<NotifFilter> filters) {

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java

+101 −0
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ import static org.mockito.Mockito.atLeast; 
import static org.mockito.Mockito.atLeastOnce;

import static org.mockito.Mockito.clearInvocations;

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;
@@ -1844,6 +1845,103 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        );

    }



    @Test

    public void stableOrderingDisregardedWithSectionChange() {

        // GIVEN the first sectioner's packages can be changed from run-to-run

        List<String> mutableSectionerPackages = new ArrayList<>();

        mutableSectionerPackages.add(PACKAGE_1);

        mListBuilder.setSectioners(asList(

                new PackageSectioner(mutableSectionerPackages, null),

                new PackageSectioner(List.of(PACKAGE_1, PACKAGE_2, PACKAGE_3), null)));

        mStabilityManager.setAllowEntryReordering(false);



        // WHEN the list is originally built with reordering disabled (and section changes allowed)

        addNotif(0, PACKAGE_1).setRank(1);

        addNotif(1, PACKAGE_1).setRank(5);

        addNotif(2, PACKAGE_2).setRank(2);

        addNotif(3, PACKAGE_2).setRank(3);

        addNotif(4, PACKAGE_3).setRank(4);

        dispatchBuild();



        // VERIFY the order and that entry reordering has not been suppressed

        verifyBuiltList(

                notif(0),

                notif(1),

                notif(2),

                notif(3),

                notif(4)

        );

        verify(mStabilityManager, never()).onEntryReorderSuppressed();



        // WHEN the first section now claims PACKAGE_3 notifications

        mutableSectionerPackages.add(PACKAGE_3);

        dispatchBuild();



        // VERIFY the re-sectioned notification is inserted at the top of the first section, because

        // it's effectively "new" and "new" things are inserted at the top of their section.

        verifyBuiltList(

                notif(4),

                notif(0),

                notif(1),

                notif(2),

                notif(3)

        );

        verify(mStabilityManager).onEntryReorderSuppressed();

        clearInvocations(mStabilityManager);



        // WHEN reordering is now allowed again

        mStabilityManager.setAllowEntryReordering(true);

        dispatchBuild();



        // VERIFY that list order changes to put the re-sectioned notification in the middle where

        // it is ranked.

        verifyBuiltList(

                notif(0),

                notif(4),

                notif(1),

                notif(2),

                notif(3)

        );

        verify(mStabilityManager, never()).onEntryReorderSuppressed();

    }



    @Test

    public void groupRevertingToSummaryRetainsStablePosition() {

        // GIVEN a notification group is on screen

        mStabilityManager.setAllowEntryReordering(false);



        // WHEN the list is originally built with reordering disabled (and section changes allowed)

        addNotif(0, PACKAGE_1).setRank(2);

        addNotif(1, PACKAGE_1).setRank(3);

        addGroupSummary(2, PACKAGE_1, "group").setRank(4);

        addGroupChild(3, PACKAGE_1, "group").setRank(5);

        addGroupChild(4, PACKAGE_1, "group").setRank(6);

        dispatchBuild();



        verifyBuiltList(

                notif(0),

                notif(1),

                group(

                        summary(2),

                        child(3),

                        child(4)

                )

        );



        // WHEN the notification summary rank increases and children removed

        setNewRank(notif(2).entry, 1);

        mEntrySet.remove(4);

        mEntrySet.remove(3);

        dispatchBuild();



        // VERIFY the summary stays in the same location on rebuild

        verifyBuiltList(

                notif(0),

                notif(1),

                notif(2)

        );

    }



    @Test

    public void testStableChildOrdering() {

        // WHEN the list is originally built with reordering disabled
@@ -2040,6 +2138,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 


    private void assertOrder(String visible, String active, String expected) {

        StringBuilder differenceSb = new StringBuilder();

        NotifSection section = new NotifSection(mock(NotifSectioner.class), 0);

        for (char c : active.toCharArray()) {

            if (visible.indexOf(c) < 0) differenceSb.append(c);

        }
@@ -2048,6 +2147,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        for (int i = 0; i < visible.length(); i++) {

            addNotif(i, String.valueOf(visible.charAt(i)))

                    .setRank(active.indexOf(visible.charAt(i)))

                    .setSection(section)

                    .setStableIndex(i);



        }
@@ -2055,6 +2155,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        for (int i = 0; i < difference.length(); i++) {

            addNotif(i + visible.length(), String.valueOf(difference.charAt(i)))

                    .setRank(active.indexOf(difference.charAt(i)))

                    .setSection(section)

                    .setStableIndex(-1);

        }