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

Commit 35d1990c authored by Jeff DeCew's avatar Jeff DeCew
Browse files

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

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

+38 −1
Original line number Diff line number Diff line
@@ -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 <T> boolean isSorted(List<T> items, Comparator<T> comparator) {

        if (items.size() <= 1) {

            return true;

        }

        Iterator<T> 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<NotifFilter> filters) {

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

+43 −6
Original line number Diff line number Diff line
@@ -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<String> 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);

        }

    }

}

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

+15 −0
Original line number Diff line number Diff line
@@ -60,4 +60,19 @@ public abstract class NotifStabilityManager extends Pluggable<NotifStabilityMana 
     * @return if can re-order

     */

    public abstract boolean isEntryReorderingAllowed(ListEntry entry);



    /**

     * Called by the pipeline to determine if every call to the other stability methods would

     * return true, regardless of parameters.  This allows the pipeline to skip any pieces of

     * work related to stability.

     *

     * @return true if all other methods will return true for any parameters.

     */

    public abstract boolean isEveryChangeAllowed();



    /**

     * Called by the pipeline to inform the stability manager that an entry reordering was indeed

     * suppressed as the result of a previous call to {@link #isEntryReorderingAllowed(ListEntry)}.

     */

    public abstract void onEntryReorderSuppressed();

}

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

+35 −1
Original line number Diff line number Diff line
@@ -35,6 +35,7 @@ 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;
@@ -80,6 +81,8 @@ import org.mockito.Spy; 


import java.util.ArrayList;

import java.util.Arrays;

import java.util.Collections;

import java.util.Comparator;

import java.util.List;

import java.util.Map;

import java.util.Objects;
@@ -124,7 +127,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 


        mListBuilder.attach(mNotifCollection);



        mStabilityManager = new TestableStabilityManager();

        mStabilityManager = spy(new TestableStabilityManager());

        mListBuilder.setNotifStabilityManager(mStabilityManager);



        Mockito.verify(mNotifCollection).setBuildListener(mBuildListenerCaptor.capture());
@@ -1468,6 +1471,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        assertOrder("ABCDEFG", "ACDEFBG", "ABCDEFG"); // no change

        assertOrder("ABCDEFG", "ACDEFBXZG", "XZABCDEFG"); // Z and X

        assertOrder("ABCDEFG", "AXCDEZFBG", "XZABCDEFG"); // Z and X + gap

        verify(mStabilityManager, times(4)).onEntryReorderSuppressed();

    }



    @Test
@@ -1476,6 +1480,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        assertOrder("ABCDEFG", "ACDEFBG", "ACDEFBG"); // no change

        assertOrder("ABCDEFG", "ACDEFBXZG", "ACDEFBXZG"); // Z and X

        assertOrder("ABCDEFG", "AXCDEZFBG", "AXCDEZFBG"); // Z and X + gap

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

    }



    @Test
@@ -1513,6 +1518,26 @@ public class ShadeListBuilderTest extends SysuiTestCase { 
        // THEN no exception thrown

    }



    @Test

    public void testIsSorted() {

        Comparator<Integer> 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";

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

+55 −23
Original line number Diff line number Diff line
@@ -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<WakefulnessLifecycle.Observer> mWakefulnessObserverCaptor;

    @Captor private ArgumentCaptor<StatusBarStateController.StateListener> mSBStateListenerCaptor;

    @Captor private ArgumentCaptor<NotifStabilityManager> mNotifStabilityManagerCaptor;

    @Captor private ArgumentCaptor<NotifCollectionListener> 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);

    }

}