Don't show new groups until their children inflate
At the moment, the pipeline has an awkward behavior when inflating
incoming notification groups:
1. Group is posted, we start inflating the summary and the children
2. Summary finishes inflating. Because it has no children, we just show
it as a singleton notification in the shade.
3. First child finishes inflating. Now we have a group with one child,
so we remove the summary and just show the child as a singleton.
4. Second child finishes inflating. Now we have a group with two
children, so we remove the singleton child, reattach the summary, and
add the children inside it.
This causes a lot of confusion for the NSSL. Instead, we should block
the ENTIRE group from being in the shade until its children are
completely inflated (with some timeout if it takes too long).
Test: manual
Test: atest SystemUITests
Change-Id: If9b31b3b07cae3abdb7b415ab6a090458bd29369
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java
index fe99cf8..9baaddd 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinator.java
@@ -30,7 +30,6 @@
import com.android.internal.statusbar.IStatusBarService;
import com.android.systemui.statusbar.notification.collection.GroupEntry;
import com.android.systemui.statusbar.notification.collection.ListEntry;
-import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl;
import com.android.systemui.statusbar.notification.collection.NotifPipeline;
import com.android.systemui.statusbar.notification.collection.NotifViewBarn;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
@@ -83,30 +82,42 @@
*/
private final int mChildBindCutoff;
+ /** How long we can delay a group while waiting for all children to inflate */
+ private final long mMaxGroupInflationDelay;
+
@Inject
public PreparationCoordinator(
PreparationCoordinatorLogger logger,
- NotifInflaterImpl notifInflater,
+ NotifInflater notifInflater,
NotifInflationErrorManager errorManager,
NotifViewBarn viewBarn,
IStatusBarService service) {
- this(logger, notifInflater, errorManager, viewBarn, service, CHILD_BIND_CUTOFF);
+ this(
+ logger,
+ notifInflater,
+ errorManager,
+ viewBarn,
+ service,
+ CHILD_BIND_CUTOFF,
+ MAX_GROUP_INFLATION_DELAY);
}
@VisibleForTesting
PreparationCoordinator(
PreparationCoordinatorLogger logger,
- NotifInflaterImpl notifInflater,
+ NotifInflater notifInflater,
NotifInflationErrorManager errorManager,
NotifViewBarn viewBarn,
IStatusBarService service,
- int childBindCutoff) {
+ int childBindCutoff,
+ long maxGroupInflationDelay) {
mLogger = logger;
mNotifInflater = notifInflater;
mNotifErrorManager = errorManager;
mViewBarn = viewBarn;
mStatusBarService = service;
mChildBindCutoff = childBindCutoff;
+ mMaxGroupInflationDelay = maxGroupInflationDelay;
}
@Override
@@ -166,12 +177,27 @@
};
private final NotifFilter mNotifInflatingFilter = new NotifFilter(TAG + "Inflating") {
+ private final Map<GroupEntry, Boolean> mIsDelayedGroupCache = new ArrayMap<>();
+
/**
- * Filters out notifications that aren't inflated
+ * Filters out notifications that either (a) aren't inflated or (b) are part of a group
+ * that isn't completely inflated yet
*/
@Override
public boolean shouldFilterOut(NotificationEntry entry, long now) {
- return !isInflated(entry);
+ final GroupEntry parent = requireNonNull(entry.getParent());
+ Boolean isMemberOfDelayedGroup = mIsDelayedGroupCache.get(parent);
+ if (isMemberOfDelayedGroup == null) {
+ isMemberOfDelayedGroup = shouldWaitForGroupToInflate(parent, now);
+ mIsDelayedGroupCache.put(parent, isMemberOfDelayedGroup);
+ }
+
+ return !isInflated(entry) || isMemberOfDelayedGroup;
+ }
+
+ @Override
+ public void onCleanup() {
+ mIsDelayedGroupCache.clear();
}
};
@@ -304,6 +330,31 @@
return stateObj;
}
+ private boolean shouldWaitForGroupToInflate(GroupEntry group, long now) {
+ if (group == GroupEntry.ROOT_ENTRY || group.hasBeenAttachedBefore()) {
+ return false;
+ }
+ if (isBeyondGroupInitializationWindow(group, now)) {
+ mLogger.logGroupInflationTookTooLong(group.getKey());
+ return false;
+ }
+ if (mInflatingNotifs.contains(group.getSummary())) {
+ mLogger.logDelayingGroupRelease(group.getKey(), group.getSummary().getKey());
+ return true;
+ }
+ for (NotificationEntry child : group.getChildren()) {
+ if (mInflatingNotifs.contains(child) && !child.hasBeenAttachedBefore()) {
+ mLogger.logDelayingGroupRelease(group.getKey(), child.getKey());
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean isBeyondGroupInitializationWindow(GroupEntry entry, long now) {
+ return now - entry.getCreationTime() > mMaxGroupInflationDelay;
+ }
+
@Retention(RetentionPolicy.SOURCE)
@IntDef(prefix = {"STATE_"},
value = {STATE_UNINFLATED, STATE_INFLATED_INVALID, STATE_INFLATED, STATE_ERROR})
@@ -330,6 +381,8 @@
*/
private static final int EXTRA_VIEW_BUFFER_COUNT = 1;
+ private static final long MAX_GROUP_INFLATION_DELAY = 500;
+
private static final int CHILD_BIND_CUTOFF =
NUMBER_OF_CHILDREN_WHEN_CHILDREN_EXPANDED + EXTRA_VIEW_BUFFER_COUNT;
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt
index 75e7bc9..dd4794f 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorLogger.kt
@@ -40,6 +40,23 @@
"NOTIF INFLATION ABORTED $str1 reason=$str2"
})
}
+
+ fun logGroupInflationTookTooLong(groupKey: String) {
+ buffer.log(TAG, LogLevel.WARNING, {
+ str1 = groupKey
+ }, {
+ "Group inflation took too long far $str1, releasing children early"
+ })
+ }
+
+ fun logDelayingGroupRelease(groupKey: String, childKey: String) {
+ buffer.log(TAG, LogLevel.DEBUG, {
+ str1 = groupKey
+ str2 = childKey
+ }, {
+ "Delaying release of group $str1 because child $str2 is still inflating"
+ })
+ }
}
private const val TAG = "PreparationCoordinator"
\ No newline at end of file
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
index d661b5e..ee471c3 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java
@@ -40,8 +40,10 @@
import com.android.systemui.statusbar.notification.NotificationEntryManager;
import com.android.systemui.statusbar.notification.NotificationEntryManagerLogger;
import com.android.systemui.statusbar.notification.VisualStabilityManager;
+import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl;
import com.android.systemui.statusbar.notification.collection.NotifPipeline;
import com.android.systemui.statusbar.notification.collection.NotificationRankingManager;
+import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater;
import com.android.systemui.statusbar.notification.collection.inflation.NotificationRowBinder;
import com.android.systemui.statusbar.notification.collection.notifcollection.CommonNotifCollection;
import com.android.systemui.statusbar.notification.collection.provider.HighPriorityProvider;
@@ -221,4 +223,8 @@
@Binds
NotificationInterruptStateProvider bindNotificationInterruptStateProvider(
NotificationInterruptStateProviderImpl notificationInterruptStateProviderImpl);
+
+ /** */
+ @Binds
+ NotifInflater bindNotifInflater(NotifInflaterImpl notifInflaterImpl);
}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java
index c1780fd..37561c4 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java
@@ -16,6 +16,8 @@
package com.android.systemui.statusbar.notification.collection.coordinator;
+import static com.android.systemui.statusbar.notification.collection.GroupEntry.ROOT_ENTRY;
+
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
@@ -25,6 +27,8 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import static java.util.Objects.requireNonNull;
+
import android.os.RemoteException;
import android.testing.AndroidTestingRunner;
import android.testing.TestableLooper;
@@ -35,11 +39,12 @@
import com.android.systemui.SysuiTestCase;
import com.android.systemui.statusbar.notification.collection.GroupEntry;
import com.android.systemui.statusbar.notification.collection.GroupEntryBuilder;
-import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl;
+import com.android.systemui.statusbar.notification.collection.ListEntry;
import com.android.systemui.statusbar.notification.collection.NotifPipeline;
import com.android.systemui.statusbar.notification.collection.NotifViewBarn;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.collection.NotificationEntryBuilder;
+import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater;
import com.android.systemui.statusbar.notification.collection.listbuilder.OnBeforeFinalizeFilterListener;
import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter;
import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener;
@@ -52,19 +57,17 @@
import org.mockito.Captor;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
@SmallTest
@RunWith(AndroidTestingRunner.class)
@TestableLooper.RunWithLooper
public class PreparationCoordinatorTest extends SysuiTestCase {
- private static final String TEST_MESSAGE = "TEST_MESSAGE";
- private static final String TEST_GROUP_KEY = "TEST_GROUP_KEY";
- private static final int TEST_CHILD_BIND_CUTOFF = 9;
-
- private PreparationCoordinator mCoordinator;
private NotifCollectionListener mCollectionListener;
private OnBeforeFinalizeFilterListener mBeforeFilterListener;
private NotifFilter mUninflatedFilter;
@@ -75,30 +78,31 @@
@Captor private ArgumentCaptor<NotifCollectionListener> mCollectionListenerCaptor;
@Captor private ArgumentCaptor<OnBeforeFinalizeFilterListener> mBeforeFilterListenerCaptor;
- @Captor private ArgumentCaptor<NotifInflaterImpl.InflationCallback> mCallbackCaptor;
+ @Captor private ArgumentCaptor<NotifInflater.InflationCallback> mCallbackCaptor;
@Mock private NotifPipeline mNotifPipeline;
@Mock private IStatusBarService mService;
- @Mock private NotifInflaterImpl mNotifInflater;
+ @Spy private FakeNotifInflater mNotifInflater = new FakeNotifInflater();
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
- mEntry = new NotificationEntryBuilder().build();
+ mEntry = new NotificationEntryBuilder().setParent(ROOT_ENTRY).build();
mInflationError = new Exception(TEST_MESSAGE);
mErrorManager = new NotifInflationErrorManager();
- mCoordinator = new PreparationCoordinator(
+ PreparationCoordinator coordinator = new PreparationCoordinator(
mock(PreparationCoordinatorLogger.class),
mNotifInflater,
mErrorManager,
mock(NotifViewBarn.class),
mService,
- TEST_CHILD_BIND_CUTOFF);
+ TEST_CHILD_BIND_CUTOFF,
+ TEST_MAX_GROUP_DELAY);
ArgumentCaptor<NotifFilter> filterCaptor = ArgumentCaptor.forClass(NotifFilter.class);
- mCoordinator.attach(mNotifPipeline);
+ coordinator.attach(mNotifPipeline);
verify(mNotifPipeline, times(2)).addFinalizeFilter(filterCaptor.capture());
List<NotifFilter> filters = filterCaptor.getAllValues();
mInflationErrorFilter = filters.get(0);
@@ -127,7 +131,7 @@
eq(mEntry.getSbn().getUid()),
eq(mEntry.getSbn().getInitialPid()),
eq(mInflationError.getMessage()),
- eq(mEntry.getSbn().getUserId()));
+ eq(mEntry.getSbn().getUser().getIdentifier()));
}
@Test
@@ -226,4 +230,139 @@
}
}
}
+
+ @Test
+ public void testPartiallyInflatedGroupsAreFilteredOut() {
+ // GIVEN a newly-posted group with a summary and two children
+ final GroupEntry group = new GroupEntryBuilder()
+ .setCreationTime(400)
+ .setSummary(new NotificationEntryBuilder().setId(1).build())
+ .addChild(new NotificationEntryBuilder().setId(2).build())
+ .addChild(new NotificationEntryBuilder().setId(3).build())
+ .build();
+ fireAddEvents(List.of(group));
+ final NotificationEntry child0 = group.getChildren().get(0);
+ mBeforeFilterListener.onBeforeFinalizeFilter(List.of(group));
+
+ // WHEN one of this children finishes inflating
+ mNotifInflater.getInflateCallback(child0).onInflationFinished(child0);
+
+ // THEN the inflated child is still filtered out
+ assertTrue(mUninflatedFilter.shouldFilterOut(child0, 401));
+ }
+
+ @Test
+ public void testPartiallyInflatedGroupsAreFilteredOutSummaryVersion() {
+ // GIVEN a newly-posted group with a summary and two children
+ final GroupEntry group = new GroupEntryBuilder()
+ .setCreationTime(400)
+ .setSummary(new NotificationEntryBuilder().setId(1).build())
+ .addChild(new NotificationEntryBuilder().setId(2).build())
+ .addChild(new NotificationEntryBuilder().setId(3).build())
+ .build();
+ fireAddEvents(List.of(group));
+ final NotificationEntry summary = group.getSummary();
+ final NotificationEntry child0 = group.getChildren().get(0);
+ final NotificationEntry child1 = group.getChildren().get(1);
+ mBeforeFilterListener.onBeforeFinalizeFilter(List.of(group));
+
+ // WHEN all of the children (but not the summary) finish inflating
+ mNotifInflater.getInflateCallback(child0).onInflationFinished(child0);
+ mNotifInflater.getInflateCallback(child1).onInflationFinished(child1);
+
+ // THEN the entire group is still filtered out
+ assertTrue(mUninflatedFilter.shouldFilterOut(summary, 401));
+ assertTrue(mUninflatedFilter.shouldFilterOut(child0, 401));
+ assertTrue(mUninflatedFilter.shouldFilterOut(child1, 401));
+ }
+
+ @Test
+ public void testCompletedInflatedGroupsAreReleased() {
+ // GIVEN a newly-posted group with a summary and two children
+ final GroupEntry group = new GroupEntryBuilder()
+ .setCreationTime(400)
+ .setSummary(new NotificationEntryBuilder().setId(1).build())
+ .addChild(new NotificationEntryBuilder().setId(2).build())
+ .addChild(new NotificationEntryBuilder().setId(3).build())
+ .build();
+ fireAddEvents(List.of(group));
+ final NotificationEntry summary = group.getSummary();
+ final NotificationEntry child0 = group.getChildren().get(0);
+ final NotificationEntry child1 = group.getChildren().get(1);
+ mBeforeFilterListener.onBeforeFinalizeFilter(List.of(group));
+
+ // WHEN all of the children (and the summary) finish inflating
+ mNotifInflater.getInflateCallback(child0).onInflationFinished(child0);
+ mNotifInflater.getInflateCallback(child1).onInflationFinished(child1);
+ mNotifInflater.getInflateCallback(summary).onInflationFinished(summary);
+
+ // THEN the entire group is still filtered out
+ assertFalse(mUninflatedFilter.shouldFilterOut(summary, 401));
+ assertFalse(mUninflatedFilter.shouldFilterOut(child0, 401));
+ assertFalse(mUninflatedFilter.shouldFilterOut(child1, 401));
+ }
+
+ @Test
+ public void testPartiallyInflatedGroupsAreReleasedAfterTimeout() {
+ // GIVEN a newly-posted group with a summary and two children
+ final GroupEntry group = new GroupEntryBuilder()
+ .setCreationTime(400)
+ .setSummary(new NotificationEntryBuilder().setId(1).build())
+ .addChild(new NotificationEntryBuilder().setId(2).build())
+ .addChild(new NotificationEntryBuilder().setId(3).build())
+ .build();
+ fireAddEvents(List.of(group));
+ final NotificationEntry child0 = group.getChildren().get(0);
+ mBeforeFilterListener.onBeforeFinalizeFilter(List.of(group));
+
+ // WHEN one of this children finishes inflating and enough time passes
+ mNotifInflater.getInflateCallback(child0).onInflationFinished(child0);
+
+ // THEN the inflated child is not filtered out even though the rest of the group hasn't
+ // finished inflating yet
+ assertTrue(mUninflatedFilter.shouldFilterOut(child0, TEST_MAX_GROUP_DELAY + 1));
+ }
+
+ private static class FakeNotifInflater implements NotifInflater {
+ private Map<NotificationEntry, InflationCallback> mInflateCallbacks = new HashMap<>();
+
+ @Override
+ public void inflateViews(NotificationEntry entry, InflationCallback callback) {
+ mInflateCallbacks.put(entry, callback);
+ }
+
+ @Override
+ public void rebindViews(NotificationEntry entry, InflationCallback callback) {
+ }
+
+ @Override
+ public void abortInflation(NotificationEntry entry) {
+ }
+
+ public InflationCallback getInflateCallback(NotificationEntry entry) {
+ return requireNonNull(mInflateCallbacks.get(entry));
+ }
+ }
+
+ private void fireAddEvents(List<? extends ListEntry> entries) {
+ for (ListEntry entry : entries) {
+ if (entry instanceof GroupEntry) {
+ GroupEntry ge = (GroupEntry) entry;
+ fireAddEvents(ge.getSummary());
+ fireAddEvents(ge.getChildren());
+ } else {
+ fireAddEvents((NotificationEntry) entry);
+ }
+ }
+ }
+
+ private void fireAddEvents(NotificationEntry entry) {
+ mCollectionListener.onEntryInit(entry);
+ mCollectionListener.onEntryAdded(entry);
+ }
+
+ private static final String TEST_MESSAGE = "TEST_MESSAGE";
+ private static final String TEST_GROUP_KEY = "TEST_GROUP_KEY";
+ private static final int TEST_CHILD_BIND_CUTOFF = 9;
+ private static final int TEST_MAX_GROUP_DELAY = 100;
}