diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java index 3fa1954a7fccf..b5c81b2adfa0f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ListDumper.java @@ -151,10 +151,6 @@ public class ListDumper { .append(" "); } - if (notifEntry.hasInflationError()) { - rksb.append("(!)hasInflationError "); - } - if (notifEntry.getDismissState() != NOT_DISMISSED) { rksb.append("dismissState=") .append(notifEntry.getDismissState()) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java index 9272e51b6aa05..83f56cc1e83d4 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifInflaterImpl.java @@ -18,9 +18,7 @@ package com.android.systemui.statusbar.notification.collection; import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; -import android.os.RemoteException; import android.service.notification.NotificationStats; -import android.service.notification.StatusBarNotification; import com.android.internal.statusbar.IStatusBarService; import com.android.internal.statusbar.NotificationVisibility; @@ -29,6 +27,7 @@ import com.android.systemui.statusbar.notification.collection.inflation.NotifInf import com.android.systemui.statusbar.notification.collection.inflation.NotificationRowBinderImpl; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; import com.android.systemui.statusbar.notification.logging.NotificationLogger; +import com.android.systemui.statusbar.notification.row.NotifInflationErrorManager; import com.android.systemui.statusbar.notification.row.NotificationContentInflater; import javax.inject.Inject; @@ -44,6 +43,7 @@ public class NotifInflaterImpl implements NotifInflater { private final IStatusBarService mStatusBarService; private final NotifCollection mNotifCollection; + private final NotifInflationErrorManager mNotifErrorManager; private NotificationRowBinderImpl mNotificationRowBinder; private InflationCallback mExternalInflationCallback; @@ -51,9 +51,11 @@ public class NotifInflaterImpl implements NotifInflater { @Inject public NotifInflaterImpl( IStatusBarService statusBarService, - NotifCollection notifCollection) { + NotifCollection notifCollection, + NotifInflationErrorManager errorManager) { mStatusBarService = statusBarService; mNotifCollection = notifCollection; + mNotifErrorManager = errorManager; } /** @@ -81,7 +83,6 @@ public class NotifInflaterImpl implements NotifInflater { @Override public void inflateViews(NotificationEntry entry) { try { - entry.setHasInflationError(false); requireBinder().inflateViews(entry, getDismissCallback(entry)); } catch (InflationException e) { // logged in mInflationCallback.handleInflationException @@ -131,25 +132,12 @@ public class NotifInflaterImpl implements NotifInflater { public void handleInflationException( NotificationEntry entry, Exception e) { - entry.setHasInflationError(true); - try { - final StatusBarNotification sbn = entry.getSbn(); - // report notification inflation errors back up - // to notification delegates - mStatusBarService.onNotificationError( - sbn.getPackageName(), - sbn.getTag(), - sbn.getId(), - sbn.getUid(), - sbn.getInitialPid(), - e.getMessage(), - sbn.getUserId()); - } catch (RemoteException ex) { - } + mNotifErrorManager.setInflationError(entry, e); } @Override public void onAsyncInflationFinished(NotificationEntry entry) { + mNotifErrorManager.clearInflationError(entry); if (mExternalInflationCallback != null) { mExternalInflationCallback.onInflationFinished(entry); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java index 006d40ddbac57..f482d37deb552 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java @@ -122,9 +122,6 @@ public final class NotificationEntry extends ListEntry { */ @CancellationReason int mCancellationReason = REASON_NOT_CANCELED; - /** @see #hasInflationError() */ - private boolean mHasInflationError; - /** @see #getDismissState() */ @NonNull private DismissState mDismissState = DismissState.NOT_DISMISSED; @@ -273,23 +270,6 @@ public final class NotificationEntry extends ListEntry { * Bookkeeping getters and setters */ - /** - * Whether this notification had an error when attempting to inflate. This is only used in - * the NewNotifPipeline - */ - public boolean hasInflationError() { - return mHasInflationError; - } - - /** - * Set whether the notification has an error while inflating. - * - * TODO: Move this into an inflation error manager class. - */ - public void setHasInflationError(boolean hasError) { - mHasInflationError = hasError; - } - /** * Set if the user has dismissed this notif but we haven't yet heard back from system server to * confirm the dismissal. 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 1e5946a85cfae..1c8fdac4c51bb 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 @@ -16,12 +16,18 @@ package com.android.systemui.statusbar.notification.collection.coordinator; +import android.os.RemoteException; +import android.service.notification.StatusBarNotification; + +import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl; import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.ShadeListBuilder; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener; +import com.android.systemui.statusbar.notification.row.NotifInflationErrorManager; import java.util.ArrayList; import java.util.List; @@ -34,7 +40,7 @@ import javax.inject.Singleton; * Aborts inflation when a notification is removed. * * If a notification is not done inflating, this coordinator will filter the notification out - * from the NotifListBuilder. + * from the {@link ShadeListBuilder}. */ @Singleton public class PreparationCoordinator implements Coordinator { @@ -42,15 +48,22 @@ public class PreparationCoordinator implements Coordinator { private final PreparationCoordinatorLogger mLogger; private final NotifInflater mNotifInflater; + private final NotifInflationErrorManager mNotifErrorManager; private final List mPendingNotifications = new ArrayList<>(); + private final IStatusBarService mStatusBarService; @Inject public PreparationCoordinator( PreparationCoordinatorLogger logger, - NotifInflaterImpl notifInflater) { + NotifInflaterImpl notifInflater, + NotifInflationErrorManager errorManager, + IStatusBarService service) { mLogger = logger; mNotifInflater = notifInflater; mNotifInflater.setInflationCallback(mInflationCallback); + mNotifErrorManager = errorManager; + mNotifErrorManager.addInflationErrorListener(mInflationErrorListener); + mStatusBarService = service; } @Override @@ -84,8 +97,7 @@ public class PreparationCoordinator implements Coordinator { */ @Override public boolean shouldFilterOut(NotificationEntry entry, long now) { - if (entry.hasInflationError()) { - mPendingNotifications.remove(entry); + if (mNotifErrorManager.hasInflationError(entry)) { return true; } return false; @@ -112,6 +124,34 @@ public class PreparationCoordinator implements Coordinator { } }; + private final NotifInflationErrorManager.NotifInflationErrorListener mInflationErrorListener = + new NotifInflationErrorManager.NotifInflationErrorListener() { + @Override + public void onNotifInflationError(NotificationEntry entry, Exception e) { + mPendingNotifications.remove(entry); + try { + final StatusBarNotification sbn = entry.getSbn(); + // report notification inflation errors back up + // to notification delegates + mStatusBarService.onNotificationError( + sbn.getPackageName(), + sbn.getTag(), + sbn.getId(), + sbn.getUid(), + sbn.getInitialPid(), + e.getMessage(), + sbn.getUserId()); + } catch (RemoteException ex) { + } + mNotifInflationErrorFilter.invalidateList(); + } + + @Override + public void onNotifInflationErrorCleared(NotificationEntry entry) { + mNotifInflationErrorFilter.invalidateList(); + } + }; + private void inflateEntry(NotificationEntry entry, String reason) { abortInflation(entry, reason); mPendingNotifications.add(entry); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotifInflationErrorManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotifInflationErrorManager.java new file mode 100644 index 0000000000000..a3ca084327459 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotifInflationErrorManager.java @@ -0,0 +1,100 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.notification.row; + +import androidx.annotation.NonNull; +import androidx.collection.ArraySet; + +import com.android.systemui.statusbar.notification.collection.NotificationEntry; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +import javax.inject.Inject; +import javax.inject.Singleton; + +/** + * A manager handling the error state of a notification when it encounters an exception while + * inflating. We don't want to show these notifications to the user but may want to keep them + * around for logging purposes. + */ +@Singleton +public class NotifInflationErrorManager { + + Set mErroredNotifs = new ArraySet<>(); + List mListeners = new ArrayList<>(); + + @Inject + public NotifInflationErrorManager() { } + + /** + * Mark the notification as errored out due to encountering an exception while inflating. + * + * @param e the exception encountered while inflating + */ + public void setInflationError(NotificationEntry entry, Exception e) { + mErroredNotifs.add(entry); + for (int i = 0; i < mListeners.size(); i++) { + mListeners.get(i).onNotifInflationError(entry, e); + } + } + + /** + * Notification inflated successfully and is no longer errored out. + */ + public void clearInflationError(NotificationEntry entry) { + if (mErroredNotifs.contains(entry)) { + mErroredNotifs.remove(entry); + for (int i = 0; i < mListeners.size(); i++) { + mListeners.get(i).onNotifInflationErrorCleared(entry); + } + } + } + + /** + * Whether or not the notification encountered an exception while inflating. + */ + public boolean hasInflationError(@NonNull NotificationEntry entry) { + return mErroredNotifs.contains(entry); + } + + /** + * Add listener for changes in inflation error state. + */ + public void addInflationErrorListener(NotifInflationErrorListener listener) { + mListeners.add(listener); + } + + /** + * Listener for changes in notification inflation error state. + */ + public interface NotifInflationErrorListener { + + /** + * Called when notification encounters an inflation exception. + * + * @param e the exception encountered while inflating + */ + void onNotifInflationError(NotificationEntry entry, Exception e); + + /** + * Called when notification inflation error is cleared. + */ + default void onNotifInflationErrorCleared(NotificationEntry entry) {} + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/RowContentBindStage.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/RowContentBindStage.java index f1241799d3d17..f78324596fb40 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/RowContentBindStage.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/RowContentBindStage.java @@ -18,12 +18,8 @@ package com.android.systemui.statusbar.notification.row; import static com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.FLAG_CONTENT_VIEW_ALL; -import android.os.RemoteException; -import android.service.notification.StatusBarNotification; - import androidx.annotation.NonNull; -import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.BindParams; import com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.InflationCallback; @@ -41,14 +37,14 @@ import javax.inject.Singleton; @Singleton public class RowContentBindStage extends BindStage { private final NotificationRowContentBinder mBinder; - private final IStatusBarService mStatusBarService; + private final NotifInflationErrorManager mNotifInflationErrorManager; @Inject RowContentBindStage( NotificationRowContentBinder binder, - IStatusBarService statusBarService) { + NotifInflationErrorManager errorManager) { mBinder = binder; - mStatusBarService = statusBarService; + mNotifInflationErrorManager = errorManager; } @Override @@ -78,24 +74,12 @@ public class RowContentBindStage extends BindStage { InflationCallback inflationCallback = new InflationCallback() { @Override public void handleInflationException(NotificationEntry entry, Exception e) { - entry.setHasInflationError(true); - try { - final StatusBarNotification sbn = entry.getSbn(); - mStatusBarService.onNotificationError( - sbn.getPackageName(), - sbn.getTag(), - sbn.getId(), - sbn.getUid(), - sbn.getInitialPid(), - e.getMessage(), - sbn.getUserId()); - } catch (RemoteException ex) { - } + mNotifInflationErrorManager.setInflationError(entry, e); } @Override public void onAsyncInflationFinished(NotificationEntry entry) { - entry.setHasInflationError(false); + mNotifInflationErrorManager.clearInflationError(entry); getStageParams(entry).clearDirtyContentViews(); callback.onStageFinished(entry); } 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 new file mode 100644 index 0000000000000..61a4fbe3526cf --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/PreparationCoordinatorTest.java @@ -0,0 +1,111 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.notification.collection.coordinator; + +import static junit.framework.Assert.assertTrue; + +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import android.os.RemoteException; +import android.testing.AndroidTestingRunner; +import android.testing.TestableLooper; + +import androidx.test.filters.SmallTest; + +import com.android.internal.statusbar.IStatusBarService; +import com.android.systemui.SysuiTestCase; +import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl; +import com.android.systemui.statusbar.notification.collection.NotifPipeline; +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.NotifFilter; +import com.android.systemui.statusbar.notification.row.NotifInflationErrorManager; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import java.util.List; + +@SmallTest +@RunWith(AndroidTestingRunner.class) +@TestableLooper.RunWithLooper +public class PreparationCoordinatorTest extends SysuiTestCase { + private static final String TEST_MESSAGE = "TEST_MESSAGE"; + + private PreparationCoordinator mCoordinator; + private NotifFilter mInflationErrorFilter; + private NotifInflationErrorManager mErrorManager; + private NotificationEntry mEntry; + private Exception mInflationError; + + @Mock + private NotifPipeline mNotifPipeline; + @Mock private IStatusBarService mService; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + + mEntry = new NotificationEntryBuilder().build(); + mInflationError = new Exception(TEST_MESSAGE); + mErrorManager = new NotifInflationErrorManager(); + + mCoordinator = new PreparationCoordinator( + mock(PreparationCoordinatorLogger.class), + mock(NotifInflaterImpl.class), + mErrorManager, + mService); + + ArgumentCaptor filterCaptor = ArgumentCaptor.forClass(NotifFilter.class); + mCoordinator.attach(mNotifPipeline); + verify(mNotifPipeline, times(2)).addPreRenderFilter(filterCaptor.capture()); + List filters = filterCaptor.getAllValues(); + mInflationErrorFilter = filters.get(0); + } + + @Test + public void testErrorLogsToService() throws RemoteException { + // WHEN an entry has an inflation error. + mErrorManager.setInflationError(mEntry, mInflationError); + + // THEN we log to status bar service. + verify(mService).onNotificationError( + eq(mEntry.getSbn().getPackageName()), + eq(mEntry.getSbn().getTag()), + eq(mEntry.getSbn().getId()), + eq(mEntry.getSbn().getUid()), + eq(mEntry.getSbn().getInitialPid()), + eq(mInflationError.getMessage()), + eq(mEntry.getSbn().getUserId())); + } + + @Test + public void testFiltersOutErroredNotifications() { + // WHEN an entry has an inflation error. + mErrorManager.setInflationError(mEntry, mInflationError); + + // THEN we filter it from the notification list. + assertTrue(mInflationErrorFilter.shouldFilterOut(mEntry, 0)); + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java index 35b55087873bb..2319758e0b5c1 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java @@ -41,7 +41,6 @@ import android.text.TextUtils; import android.view.LayoutInflater; import android.widget.RemoteViews; -import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.TestableDependency; import com.android.systemui.bubbles.BubbleController; import com.android.systemui.bubbles.BubblesTestActivity; @@ -112,7 +111,7 @@ public class NotificationTestHelper { mock(NotifRemoteViewCache.class), mock(NotificationRemoteInputManager.class)); contentBinder.setInflateSynchronously(true); - mBindStage = new RowContentBindStage(contentBinder, mock(IStatusBarService.class)); + mBindStage = new RowContentBindStage(contentBinder, mock(NotifInflationErrorManager.class)); NotificationEntryManager entryManager = mock(NotificationEntryManager.class); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java index 775f722b13f9f..d9fe6551ba1c2 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java @@ -34,7 +34,6 @@ import android.testing.TestableLooper; import androidx.test.filters.SmallTest; -import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.SysuiTestCase; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.BindParams; @@ -62,7 +61,7 @@ public class RowContentBindStageTest extends SysuiTestCase { MockitoAnnotations.initMocks(this); mRowContentBindStage = new RowContentBindStage(mBinder, - mock(IStatusBarService.class)); + mock(NotifInflationErrorManager.class)); mRowContentBindStage.createStageParams(mEntry); }