From 8519a9cb82d830560522b39a7df499b2bd8e0d3c Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 21 Jan 2022 22:12:25 +0000 Subject: [PATCH] Add a BindEventManager to fix ConversationNotifications.kt Fixes: 207960274 Test: atest PreparationCoordinatorTest Change-Id: I75686ba113fbcdafecd11daff1ab04adaaadfe3d --- .../notification/ConversationNotifications.kt | 82 ++++++------------- .../coordinator/PreparationCoordinator.java | 16 ++-- .../collection/inflation/BindEventManager.kt | 45 ++++++++++ .../inflation/BindEventManagerImpl.kt | 42 ++++++++++ .../dagger/NotificationsModule.java | 6 ++ .../init/NotificationsControllerImpl.kt | 3 + .../PreparationCoordinatorTest.java | 10 ++- 7 files changed, 135 insertions(+), 69 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManager.kt create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManagerImpl.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/ConversationNotifications.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/ConversationNotifications.kt index 3449bd8e26863..5aeab84b677c1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/ConversationNotifications.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/ConversationNotifications.kt @@ -23,7 +23,6 @@ import android.graphics.drawable.AnimatedImageDrawable import android.os.Handler import android.service.notification.NotificationListenerService.Ranking import android.service.notification.NotificationListenerService.RankingMap -import com.android.internal.statusbar.NotificationVisibility import com.android.internal.widget.ConversationLayout import com.android.internal.widget.MessagingImageMessage import com.android.internal.widget.MessagingLayout @@ -31,6 +30,7 @@ import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.plugins.statusbar.StatusBarStateController import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManager import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy import com.android.systemui.statusbar.notification.collection.notifcollection.CommonNotifCollection import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener @@ -72,7 +72,8 @@ class ConversationNotificationProcessor @Inject constructor( */ @SysUISingleton class AnimatedImageNotificationManager @Inject constructor( - private val notificationEntryManager: NotificationEntryManager, + private val notifCollection: CommonNotifCollection, + private val bindEventManager: BindEventManager, private val headsUpManager: HeadsUpManager, private val statusBarStateController: StatusBarStateController ) { @@ -83,33 +84,23 @@ class AnimatedImageNotificationManager @Inject constructor( fun bind() { headsUpManager.addListener(object : OnHeadsUpChangedListener { override fun onHeadsUpStateChanged(entry: NotificationEntry, isHeadsUp: Boolean) { - entry.row?.let { row -> - updateAnimatedImageDrawables(row, animating = isHeadsUp || isStatusBarExpanded) - } + updateAnimatedImageDrawables(entry) } }) statusBarStateController.addCallback(object : StatusBarStateController.StateListener { override fun onExpandedChanged(isExpanded: Boolean) { isStatusBarExpanded = isExpanded - notificationEntryManager.activeNotificationsForCurrentUser.forEach { entry -> - entry.row?.let { row -> - updateAnimatedImageDrawables(row, animating = isExpanded || row.isHeadsUp) - } - } + notifCollection.allNotifs.forEach(::updateAnimatedImageDrawables) } }) - notificationEntryManager.addNotificationEntryListener(object : NotificationEntryListener { - override fun onEntryInflated(entry: NotificationEntry) { - entry.row?.let { row -> - updateAnimatedImageDrawables( - row, - animating = isStatusBarExpanded || row.isHeadsUp) - } - } - override fun onEntryReinflated(entry: NotificationEntry) = onEntryInflated(entry) - }) + bindEventManager.addListener(::updateAnimatedImageDrawables) } + private fun updateAnimatedImageDrawables(entry: NotificationEntry) = + entry.row?.let { row -> + updateAnimatedImageDrawables(row, animating = row.isHeadsUp || isStatusBarExpanded) + } + private fun updateAnimatedImageDrawables(row: ExpandableNotificationRow, animating: Boolean) = (row.layouts?.asSequence() ?: emptySequence()) .flatMap { layout -> layout.allViews.asSequence() } @@ -138,7 +129,7 @@ class AnimatedImageNotificationManager @Inject constructor( */ @SysUISingleton class ConversationNotificationManager @Inject constructor( - private val notificationEntryManager: NotificationEntryManager, + private val bindEventManager: BindEventManager, private val notificationGroupManager: NotificationGroupManagerLegacy, private val context: Context, private val notifCollection: CommonNotifCollection, @@ -151,35 +142,12 @@ class ConversationNotificationManager @Inject constructor( private var notifPanelCollapsed = true - private val entryManagerListener = object : NotificationEntryListener { - override fun onNotificationRankingUpdated(rankingMap: RankingMap) = - updateNotificationRanking(rankingMap) - override fun onEntryInflated(entry: NotificationEntry) = - onEntryViewBound(entry) - override fun onEntryReinflated(entry: NotificationEntry) = onEntryInflated(entry) - override fun onEntryRemoved( - entry: NotificationEntry, - visibility: NotificationVisibility?, - removedByUser: Boolean, - reason: Int - ) = removeTrackedEntry(entry) - } - - private val notifCollectionListener = object : NotifCollectionListener { - override fun onRankingUpdate(ranking: RankingMap) = - updateNotificationRanking(ranking) - - override fun onEntryRemoved(entry: NotificationEntry, reason: Int) { - removeTrackedEntry(entry) - } - } - private fun updateNotificationRanking(rankingMap: RankingMap) { fun getLayouts(view: NotificationContentView) = sequenceOf(view.contractedChild, view.expandedChild, view.headsUpChild) val ranking = Ranking() val activeConversationEntries = states.keys.asSequence() - .mapNotNull { notificationEntryManager.getActiveNotificationUnfiltered(it) } + .mapNotNull { notifCollection.getEntry(it) } for (entry in activeConversationEntries) { if (rankingMap.getRanking(entry.sbn.key, ranking) && ranking.isConversation) { val important = ranking.channel.isImportantConversation @@ -204,7 +172,7 @@ class ConversationNotificationManager @Inject constructor( layout.setIsImportantConversation(important, false) } } - if (changed) { + if (changed && !featureFlags.isNewPipelineEnabled()) { notificationGroupManager.updateIsolation(entry) } } @@ -233,11 +201,14 @@ class ConversationNotificationManager @Inject constructor( } init { - if (featureFlags.isNewPipelineEnabled()) { - notifCollection.addCollectionListener(notifCollectionListener) - } else { - notificationEntryManager.addNotificationEntryListener(entryManagerListener) - } + notifCollection.addCollectionListener(object : NotifCollectionListener { + override fun onRankingUpdate(ranking: RankingMap) = + updateNotificationRanking(ranking) + + override fun onEntryRemoved(entry: NotificationEntry, reason: Int) = + removeTrackedEntry(entry) + }) + bindEventManager.addListener(::onEntryViewBound) } private fun ConversationState.shouldIncrementUnread(newBuilder: Notification.Builder) = @@ -265,11 +236,10 @@ class ConversationNotificationManager @Inject constructor( val expanded = states .asSequence() .mapNotNull { (key, _) -> - notificationEntryManager.getActiveNotificationUnfiltered(key) - ?.let { entry -> - if (entry.row?.isExpanded == true) key to entry - else null - } + notifCollection.getEntry(key)?.let { entry -> + if (entry.row?.isExpanded == true) key to entry + else null + } } .toMap() states.replaceAll { key, state -> 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 195f3672dc562..35fe0ee7cdb1f 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 @@ -31,13 +31,13 @@ import androidx.annotation.Nullable; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.statusbar.IStatusBarService; -import com.android.systemui.statusbar.notification.ConversationNotificationManager; import com.android.systemui.statusbar.notification.collection.GroupEntry; import com.android.systemui.statusbar.notification.collection.ListEntry; 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.coordinator.dagger.CoordinatorScope; +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManagerImpl; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustment; import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; @@ -99,7 +99,7 @@ public class PreparationCoordinator implements Coordinator { /** How long we can delay a group while waiting for all children to inflate */ private final long mMaxGroupInflationDelay; - private final ConversationNotificationManager mConversationManager; + private final BindEventManagerImpl mBindEventManager; @Inject public PreparationCoordinator( @@ -109,7 +109,7 @@ public class PreparationCoordinator implements Coordinator { NotifViewBarn viewBarn, NotifUiAdjustmentProvider adjustmentProvider, IStatusBarService service, - ConversationNotificationManager conversationManager) { + BindEventManagerImpl bindEventManager) { this( logger, notifInflater, @@ -117,7 +117,7 @@ public class PreparationCoordinator implements Coordinator { viewBarn, adjustmentProvider, service, - conversationManager, + bindEventManager, CHILD_BIND_CUTOFF, MAX_GROUP_INFLATION_DELAY); } @@ -130,7 +130,7 @@ public class PreparationCoordinator implements Coordinator { NotifViewBarn viewBarn, NotifUiAdjustmentProvider adjustmentProvider, IStatusBarService service, - ConversationNotificationManager conversationManager, + BindEventManagerImpl bindEventManager, int childBindCutoff, long maxGroupInflationDelay) { mLogger = logger; @@ -141,7 +141,7 @@ public class PreparationCoordinator implements Coordinator { mStatusBarService = service; mChildBindCutoff = childBindCutoff; mMaxGroupInflationDelay = maxGroupInflationDelay; - mConversationManager = conversationManager; + mBindEventManager = bindEventManager; } @Override @@ -369,9 +369,7 @@ public class PreparationCoordinator implements Coordinator { mInflatingNotifs.remove(entry); mViewBarn.registerViewForEntry(entry, controller); mInflationStates.put(entry, STATE_INFLATED); - // NOTE: under the new pipeline there's no way to register for an inflation callback, - // so this one method is called by the PreparationCoordinator directly. - mConversationManager.onEntryViewBound(entry); + mBindEventManager.notifyViewBound(entry); mNotifInflatingFilter.invalidateList(); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManager.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManager.kt new file mode 100644 index 0000000000000..51bdd00d09be8 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManager.kt @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2022 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.inflation + +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.util.ListenerSet + +/** + * Helper class that allows distributing bind events regardless of the pipeline. + * + * NOTE: This class isn't ideal; this exposes the concept of view inflation as something that can be + * globally registered for. This is built as it is to provide compatibility with patterns developed + * for the legacy pipeline. Ideally we'd have functionality that needs to know this information be + * handled by events that go through the ViewController itself. + */ +open class BindEventManager { + protected val listeners = ListenerSet() + + /** Register a listener */ + fun addListener(listener: Listener) = + listeners.addIfAbsent(listener) + + /** Deregister a listener */ + fun removeListener(listener: Listener) = + listeners.remove(listener) + + /** Listener interface for view bind events */ + fun interface Listener { + fun onViewBound(entry: NotificationEntry) + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManagerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManagerImpl.kt new file mode 100644 index 0000000000000..9d5b859ef29c8 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/BindEventManagerImpl.kt @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2022 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.inflation + +import com.android.systemui.dagger.SysUISingleton +import com.android.systemui.statusbar.notification.NotificationEntryListener +import com.android.systemui.statusbar.notification.NotificationEntryManager +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManager.Listener +import javax.inject.Inject + +/** + * Helper class that allows distributing bind events regardless of the pipeline. + */ +@SysUISingleton +class BindEventManagerImpl @Inject constructor() : BindEventManager() { + /** Emit the [Listener.onViewBound] event to all registered listeners. */ + fun notifyViewBound(entry: NotificationEntry) = + listeners.forEach { listener -> listener.onViewBound(entry) } + + /** Initialize this for the legacy pipeline. */ + fun attachToLegacyPipeline(notificationEntryManager: NotificationEntryManager) { + notificationEntryManager.addNotificationEntryListener(object : NotificationEntryListener { + override fun onEntryInflated(entry: NotificationEntry) = notifyViewBound(entry) + override fun onEntryReinflated(entry: NotificationEntry) = notifyViewBound(entry) + }) + } +} 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 f1cba34158d1c..05c40b204c862 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 @@ -50,6 +50,8 @@ import com.android.systemui.statusbar.notification.collection.NotifPipeline; import com.android.systemui.statusbar.notification.collection.coordinator.ShadeEventCoordinator; import com.android.systemui.statusbar.notification.collection.coordinator.VisualStabilityCoordinator; import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorsModule; +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManager; +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManagerImpl; 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.inflation.OnUserInteractionCallbackImpl; @@ -356,6 +358,10 @@ public interface NotificationsModule { @Binds NotifInflater bindNotifInflater(NotifInflaterImpl notifInflaterImpl); + /** */ + @Binds + BindEventManager bindBindEventManagerImpl(BindEventManagerImpl bindEventManagerImpl); + /** */ @Binds NotifLiveDataStore bindNotifLiveDataStore(NotifLiveDataStoreImpl notifLiveDataStoreImpl); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/init/NotificationsControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/init/NotificationsControllerImpl.kt index 38f3c39b5b1ab..48f2dafedcbbe 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/init/NotificationsControllerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/init/NotificationsControllerImpl.kt @@ -32,6 +32,7 @@ import com.android.systemui.statusbar.notification.collection.NotifLiveDataStore import com.android.systemui.statusbar.notification.collection.NotifPipeline import com.android.systemui.statusbar.notification.collection.NotificationRankingManager import com.android.systemui.statusbar.notification.collection.TargetSdkResolver +import com.android.systemui.statusbar.notification.collection.inflation.BindEventManagerImpl import com.android.systemui.statusbar.notification.collection.inflation.NotificationRowBinderImpl import com.android.systemui.statusbar.notification.collection.init.NotifPipelineInitializer import com.android.systemui.statusbar.notification.collection.legacy.NotificationGroupManagerLegacy @@ -76,6 +77,7 @@ class NotificationsControllerImpl @Inject constructor( private val notifBindPipelineInitializer: NotifBindPipelineInitializer, private val deviceProvisionedController: DeviceProvisionedController, private val notificationRowBinder: NotificationRowBinderImpl, + private val bindEventManagerImpl: BindEventManagerImpl, private val remoteInputUriController: RemoteInputUriController, private val groupManagerLegacy: Lazy, private val groupAlertTransferHelper: NotificationGroupAlertTransferHelper, @@ -131,6 +133,7 @@ class NotificationsControllerImpl @Inject constructor( targetSdkResolver.initialize(entryManager) remoteInputUriController.attach(entryManager) groupAlertTransferHelper.bind(entryManager, groupManagerLegacy.get()) + bindEventManagerImpl.attachToLegacyPipeline(entryManager) headsUpManager.addListener(groupManagerLegacy.get()) headsUpManager.addListener(groupAlertTransferHelper) headsUpController.attach(entryManager, headsUpManager) 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 bde6734bbf926..3b034f7af9a69 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 @@ -26,6 +26,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import static java.util.Objects.requireNonNull; @@ -40,7 +41,6 @@ import androidx.test.filters.SmallTest; import com.android.internal.statusbar.IStatusBarService; import com.android.systemui.SysuiTestCase; import com.android.systemui.statusbar.RankingBuilder; -import com.android.systemui.statusbar.notification.ConversationNotificationManager; import com.android.systemui.statusbar.notification.SectionClassifier; import com.android.systemui.statusbar.notification.collection.GroupEntry; import com.android.systemui.statusbar.notification.collection.GroupEntryBuilder; @@ -48,6 +48,7 @@ import com.android.systemui.statusbar.notification.collection.ListEntry; 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.inflation.BindEventManagerImpl; import com.android.systemui.statusbar.notification.collection.inflation.NotifInflater; import com.android.systemui.statusbar.notification.collection.inflation.NotifUiAdjustmentProvider; import com.android.systemui.statusbar.notification.collection.listbuilder.NotifSection; @@ -93,7 +94,7 @@ public class PreparationCoordinatorTest extends SysuiTestCase { @Mock private NotifSection mNotifSection; @Mock private NotifPipeline mNotifPipeline; @Mock private IStatusBarService mService; - @Mock private ConversationNotificationManager mConvoManager; + @Mock private BindEventManagerImpl mBindEventManagerImpl; @Spy private FakeNotifInflater mNotifInflater = new FakeNotifInflater(); private final SectionClassifier mSectionClassifier = new SectionClassifier(); private final NotifUiAdjustmentProvider mAdjustmentProvider = @@ -121,7 +122,7 @@ public class PreparationCoordinatorTest extends SysuiTestCase { mock(NotifViewBarn.class), mAdjustmentProvider, mService, - mConvoManager, + mBindEventManagerImpl, TEST_CHILD_BIND_CUTOFF, TEST_MAX_GROUP_DELAY); @@ -411,7 +412,8 @@ public class PreparationCoordinatorTest extends SysuiTestCase { public void testCallConversationManagerBindWhenInflated() { mBeforeFilterListener.onBeforeFinalizeFilter(List.of(mEntry)); mNotifInflater.getInflateCallback(mEntry).onInflationFinished(mEntry, null); - verify(mConvoManager, times(1)).onEntryViewBound(eq(mEntry)); + verify(mBindEventManagerImpl, times(1)).notifyViewBound(eq(mEntry)); + verifyNoMoreInteractions(mBindEventManagerImpl); } @Test