Merge "Clean up STABILITY_INDEX_FIX and SEMI_STABLE_SORT flags which have already been released." into tm-qpr-dev

packages/SystemUI/src/com/android/systemui/flags/Flags.kt

@@ -77,12 +77,6 @@ object Flags {
     // TODO(b/254512731): Tracking Bug
     @JvmField val NOTIFICATION_DISMISSAL_FADE = releasedFlag(113, "notification_dismissal_fade")
 
-    // TODO(b/259558771): Tracking Bug
-    val STABILITY_INDEX_FIX = releasedFlag(114, "stability_index_fix")
-
-    // TODO(b/259559750): Tracking Bug
-    val SEMI_STABLE_SORT = releasedFlag(115, "semi_stable_sort")
-
     @JvmField val USE_ROUNDNESS_SOURCETYPES = releasedFlag(116, "use_roundness_sourcetype")
 
     // TODO(b/259217907)

packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt

@@ -35,14 +35,6 @@ class NotifPipelineFlags @Inject constructor(
 
     fun fsiOnDNDUpdate(): Boolean = featureFlags.isEnabled(Flags.FSI_ON_DND_UPDATE)
 
-    val isStabilityIndexFixEnabled: Boolean by lazy {
-        featureFlags.isEnabled(Flags.STABILITY_INDEX_FIX)
-    }
-
-    val isSemiStableSortEnabled: Boolean by lazy {
-        featureFlags.isEnabled(Flags.SEMI_STABLE_SORT)
-    }
-
     val shouldFilterUnseenNotifsOnKeyguard: Boolean by lazy {
         featureFlags.isEnabled(Flags.FILTER_UNSEEN_NOTIFS_ON_KEYGUARD)
     }

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

@@ -98,14 +98,12 @@ data class ListAttachState private constructor(
      * This can happen if the entry is removed from a group that was broken up or if the entry was
      * filtered out during any of the filtering steps.
      */
-    fun detach(includingStableIndex: Boolean) {
+    fun detach() {
         parent = null
         section = null
         promoter = null
-        if (includingStableIndex) {
         stableIndex = -1
     }
-    }
 
     companion object {
         @JvmStatic

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

@@ -965,8 +965,7 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
      * filtered out during any of the filtering steps.
      */
     private void annulAddition(ListEntry entry) {
-        // NOTE(b/241229236): Don't clear stableIndex until we fix stability fragility
+        entry.getAttachState().detach();
-        entry.getAttachState().detach(/* includingStableIndex= */ mFlags.isSemiStableSortEnabled());
     }
 
     private void assignSections() {
@@ -986,50 +985,10 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
 
     private void sortListAndGroups() {
         Trace.beginSection("ShadeListBuilder.sortListAndGroups");
-        if (mFlags.isSemiStableSortEnabled()) {
         sortWithSemiStableSort();
-        } else {
-            sortWithLegacyStability();
-        }
         Trace.endSection();
     }
 
-    private void sortWithLegacyStability() {
-        // Sort all groups and the top level list
-        for (ListEntry entry : mNotifList) {
-            if (entry instanceof GroupEntry) {
-                GroupEntry parent = (GroupEntry) entry;
-                parent.sortChildren(mGroupChildrenComparator);
-            }
-        }
-        mNotifList.sort(mTopLevelComparator);
-        assignIndexes(mNotifList);
-
-        // Check for suppressed order changes
-        if (!getStabilityManager().isEveryChangeAllowed()) {
-            mForceReorderable = true;
-            boolean isSorted = isShadeSortedLegacy();
-            mForceReorderable = false;
-            if (!isSorted) {
-                getStabilityManager().onEntryReorderSuppressed();
-            }
-        }
-    }
-
-    private boolean isShadeSortedLegacy() {
-        if (!isSorted(mNotifList, mTopLevelComparator)) {
-            return false;
-        }
-        for (ListEntry entry : mNotifList) {
-            if (entry instanceof GroupEntry) {
-                if (!isSorted(((GroupEntry) entry).getChildren(), mGroupChildrenComparator)) {
-                    return false;
-                }
-            }
-        }
-        return true;
-    }
-
     private void sortWithSemiStableSort() {
         // Sort each group's children
         boolean allSorted = true;
@@ -1100,7 +1059,6 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
                 sectionMemberIndex = 0;
                 currentSection = section;
             }
-            if (mFlags.isStabilityIndexFixEnabled()) {
             entry.getAttachState().setStableIndex(sectionMemberIndex++);
             if (entry instanceof GroupEntry) {
                 final GroupEntry parent = (GroupEntry) entry;
@@ -1112,18 +1070,6 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
                     child.getAttachState().setStableIndex(sectionMemberIndex++);
                 }
             }
-            } else {
-                // This old implementation uses the same index number for the group as the first
-                // child, and fails to assign an index to the summary.  Remove once tested.
-                entry.getAttachState().setStableIndex(sectionMemberIndex);
-                if (entry instanceof GroupEntry) {
-                    final GroupEntry parent = (GroupEntry) entry;
-                    for (NotificationEntry child : parent.getChildren()) {
-                        child.getAttachState().setStableIndex(sectionMemberIndex++);
-                    }
-                }
-                sectionMemberIndex++;
-            }
         }
     }
 
@@ -1272,11 +1218,6 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
                 o2.getSectionIndex());
         if (cmp != 0) return cmp;
 
-        cmp = mFlags.isSemiStableSortEnabled() ? 0 : Integer.compare(
-                getStableOrderIndex(o1),
-                getStableOrderIndex(o2));
-        if (cmp != 0) return cmp;
-
         NotifComparator sectionComparator = getSectionComparator(o1, o2);
         if (sectionComparator != null) {
             cmp = sectionComparator.compare(o1, o2);
@@ -1301,12 +1242,7 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
 
 
     private final Comparator<NotificationEntry> mGroupChildrenComparator = (o1, o2) -> {
-        int cmp = mFlags.isSemiStableSortEnabled() ? 0 : Integer.compare(
+        int cmp = Integer.compare(
-                getStableOrderIndex(o1),
-                getStableOrderIndex(o2));
-        if (cmp != 0) return cmp;
-
-        cmp = Integer.compare(
                 o1.getRepresentativeEntry().getRanking().getRank(),
                 o2.getRepresentativeEntry().getRanking().getRank());
         if (cmp != 0) return cmp;
@@ -1317,25 +1253,6 @@ public class ShadeListBuilder implements Dumpable, PipelineDumpable {
         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 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;
-        }
-        // NOTE(b/241229236): Can't use cleared section index until we fix stability fragility
-        return entry.getPreviousAttachState().getStableIndex();
-    }
-
     @Nullable
     private Integer getStableOrderRank(ListEntry entry) {
         if (getStabilityManager().isEntryReorderingAllowed(entry)) {

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

@@ -39,7 +39,6 @@ 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 org.mockito.Mockito.when;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
@@ -137,7 +136,6 @@ public class ShadeListBuilderTest extends SysuiTestCase {
     public void setUp() {
         MockitoAnnotations.initMocks(this);
         allowTestableLooperAsMainThread();
-        when(mNotifPipelineFlags.isStabilityIndexFixEnabled()).thenReturn(true);
 
         mListBuilder = new ShadeListBuilder(
                 mDumpManager,
@@ -1997,30 +1995,8 @@ public class ShadeListBuilderTest extends SysuiTestCase {
         return wtf;
     }
 
-    @Test
-    public void testActiveOrdering_withLegacyStability() {
-        when(mNotifPipelineFlags.isSemiStableSortEnabled()).thenReturn(false);
-        assertOrder("ABCDEFG", "ABCDEFG", "ABCDEFG", true); // no change
-        assertOrder("ABCDEFG", "ACDEFXBG", "ACDEFXBG", true); // X
-        assertOrder("ABCDEFG", "ACDEFBG", "ACDEFBG", true); // no change
-        assertOrder("ABCDEFG", "ACDEFBXZG", "ACDEFBXZG", true); // Z and X
-        assertOrder("ABCDEFG", "AXCDEZFBG", "AXCDEZFBG", true); // Z and X + gap
-    }
-
-    @Test
-    public void testStableOrdering_withLegacyStability() {
-        when(mNotifPipelineFlags.isSemiStableSortEnabled()).thenReturn(false);
-        mStabilityManager.setAllowEntryReordering(false);
-        assertOrder("ABCDEFG", "ABCDEFG", "ABCDEFG", true); // no change
-        assertOrder("ABCDEFG", "ACDEFXBG", "XABCDEFG", false); // X
-        assertOrder("ABCDEFG", "ACDEFBG", "ABCDEFG", false); // no change
-        assertOrder("ABCDEFG", "ACDEFBXZG", "XZABCDEFG", false); // Z and X
-        assertOrder("ABCDEFG", "AXCDEZFBG", "XZABCDEFG", false); // Z and X + gap
-    }
-
     @Test
     public void testStableOrdering() {
-        when(mNotifPipelineFlags.isSemiStableSortEnabled()).thenReturn(true);
         mStabilityManager.setAllowEntryReordering(false);
         // No input or output
         assertOrder("", "", "", true);
@@ -2076,7 +2052,6 @@ public class ShadeListBuilderTest extends SysuiTestCase {
 
     @Test
     public void testActiveOrdering() {
-        when(mNotifPipelineFlags.isSemiStableSortEnabled()).thenReturn(true);
         assertOrder("ABCDEFG", "ACDEFXBG", "ACDEFXBG", true); // X
         assertOrder("ABCDEFG", "ACDEFBG", "ACDEFBG", true); // no change
         assertOrder("ABCDEFG", "ACDEFBXZG", "ACDEFBXZG", true); // Z and X
@@ -2133,7 +2108,6 @@ public class ShadeListBuilderTest extends SysuiTestCase {
 
     @Test
     public void stableOrderingDisregardedWithSectionChange() {
-        when(mNotifPipelineFlags.isSemiStableSortEnabled()).thenReturn(true);
         // GIVEN the first sectioner's packages can be changed from run-to-run
         List<String> mutableSectionerPackages = new ArrayList<>();
         mutableSectionerPackages.add(PACKAGE_1);
@@ -2228,50 +2202,8 @@ public class ShadeListBuilderTest extends SysuiTestCase {
         );
     }
 
-    @Test
-    public void groupRevertingToSummaryDoesNotRetainStablePositionWithLegacyIndexLogic() {
-        when(mNotifPipelineFlags.isStabilityIndexFixEnabled()).thenReturn(false);
-
-        // 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 (incorrectly) moves to the top of the section where it is ranked,
-        // despite visual stability being active
-        verifyBuiltList(
-                notif(2),
-                notif(0),
-                notif(1)
-        );
-    }
-
     @Test
     public void groupRevertingToSummaryRetainsStablePosition() {
-        when(mNotifPipelineFlags.isStabilityIndexFixEnabled()).thenReturn(true);
-
         // GIVEN a notification group is on screen
         mStabilityManager.setAllowEntryReordering(false);