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

Commit 0e570670 authored by Jeff DeCew's avatar Jeff DeCew
Browse files

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
parent b9df9d96

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

+7 −6
Original line number Diff line number Diff line
@@ -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(

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

+17 −16
Original line number Diff line number Diff line
@@ -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<NotificationEntry> 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.

                }

            }

        }

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

+3 −3
Original line number Diff line number Diff line
@@ -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)

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

+110 −33
Original line number Diff line number Diff line
@@ -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(

        assertThat(listener.mEntriesReceived).containsExactly(

                mEntrySet.get(0),

                mBuiltList.get(1),

                        mEntrySet.get(4)),

                listener.mEntriesReceived

        );

                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(

        assertThat(listener.mEntriesReceived).containsExactly(

                mEntrySet.get(0),

                mEntrySet.get(1),

                mBuiltList.get(2),

                        mEntrySet.get(7),

                        mEntrySet.get(1)),

                listener.mEntriesReceived

        );

                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(

        assertThat(listener.mEntriesReceived).containsExactly(

                mEntrySet.get(0),

                mEntrySet.get(1),

                        mEntrySet.get(2)),

                listener.mEntriesReceived

        );

                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(

        assertThat(listener.mEntriesReceived).containsExactly(

                mEntrySet.get(0),

                mEntrySet.get(1),

                        mEntrySet.get(2)),

                listener.mEntriesReceived

        );

                mEntrySet.get(2)

        ).inOrder();

    }



    @Test