diff options
7 files changed, 278 insertions, 30 deletions
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt index 226a9577c820..bd659d294223 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt @@ -23,6 +23,7 @@ import com.android.systemui.statusbar.notification.collection.PipelineDumper import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifSectioner import com.android.systemui.statusbar.notification.collection.provider.SectionStyleProvider +import com.android.systemui.statusbar.notification.shared.NotificationsLiveDataStoreRefactor import javax.inject.Inject /** @@ -61,6 +62,7 @@ class NotifCoordinatorsImpl @Inject constructor( sensitiveContentCoordinator: SensitiveContentCoordinator, dismissibilityCoordinator: DismissibilityCoordinator, dreamCoordinator: DreamCoordinator, + statsLoggerCoordinator: NotificationStatsLoggerCoordinator, ) : NotifCoordinators { private val mCoreCoordinators: MutableList<CoreCoordinator> = ArrayList() @@ -104,6 +106,10 @@ class NotifCoordinatorsImpl @Inject constructor( mCoordinators.add(dreamCoordinator) } + if (NotificationsLiveDataStoreRefactor.isEnabled) { + mCoordinators.add(statsLoggerCoordinator) + } + // Manually add Ordered Sections mOrderedSections.add(headsUpCoordinator.sectioner) // HeadsUp mOrderedSections.add(colorizedFgsCoordinator.sectioner) // ForegroundService diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinator.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinator.kt new file mode 100644 index 000000000000..6e81d9322be5 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinator.kt @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2024 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 com.android.systemui.statusbar.notification.collection.NotifPipeline +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope +import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener +import com.android.systemui.statusbar.notification.shared.NotificationsLiveDataStoreRefactor +import com.android.systemui.statusbar.notification.stack.ui.view.NotificationStatsLogger +import java.util.Optional +import javax.inject.Inject + +@CoordinatorScope +class NotificationStatsLoggerCoordinator +@Inject +constructor(private val loggerOptional: Optional<NotificationStatsLogger>) : Coordinator { + + private val collectionListener = + object : NotifCollectionListener { + override fun onEntryUpdated(entry: NotificationEntry) { + super.onEntryUpdated(entry) + loggerOptional.ifPresent { it.onNotificationUpdated(entry.key) } + } + + override fun onEntryRemoved(entry: NotificationEntry, reason: Int) { + super.onEntryRemoved(entry, reason) + loggerOptional.ifPresent { it.onNotificationRemoved(entry.key) } + } + } + override fun attach(pipeline: NotifPipeline) { + if (NotificationsLiveDataStoreRefactor.isUnexpectedlyInLegacyMode()) { + return + } + pipeline.addCollectionListener(collectionListener) + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLogger.kt index 54186165c54a..365c02f74bfe 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLogger.kt @@ -29,9 +29,7 @@ interface NotificationStatsLogger : NotificationRowStatsLogger { activeNotifications: List<ActiveNotificationModel>, ) fun onLockscreenOrShadeNotInteractive(activeNotifications: List<ActiveNotificationModel>) - fun onNotificationRemoved(key: String) - fun onNotificationUpdated(key: String) - fun onNotificationListUpdated( + fun onNotificationLocationsChanged( locationsProvider: Callable<Map<String, Int>>, notificationRanks: Map<String, Int>, ) @@ -41,4 +39,6 @@ interface NotificationStatsLogger : NotificationRowStatsLogger { location: Int, isUserAction: Boolean ) + fun onNotificationRemoved(key: String) + fun onNotificationUpdated(key: String) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt index 0cb00bc8d01d..4897b4275f75 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerImpl.kt @@ -53,10 +53,11 @@ constructor( private val expansionStates: MutableMap<String, ExpansionState> = ConcurrentHashMap<String, ExpansionState>() - private val lastReportedExpansionValues: MutableMap<String, Boolean> = + @VisibleForTesting + val lastReportedExpansionValues: MutableMap<String, Boolean> = ConcurrentHashMap<String, Boolean>() - override fun onNotificationListUpdated( + override fun onNotificationLocationsChanged( locationsProvider: Callable<Map<String, Int>>, notificationRanks: Map<String, Int>, ) { @@ -152,14 +153,12 @@ constructor( ) } - // TODO(b/308623704) wire this in with NotifPipeline updates override fun onNotificationRemoved(key: String) { // No need to track expansion states for Notifications that are removed. expansionStates.remove(key) lastReportedExpansionValues.remove(key) } - // TODO(b/308623704) wire this in with NotifPipeline updates override fun onNotificationUpdated(key: String) { // When the Notification is updated, we should consider it as not yet logged. lastReportedExpansionValues.remove(key) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/viewbinder/NotificationStatsLoggerBinder.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/viewbinder/NotificationStatsLoggerBinder.kt index a05ad6e45991..a87c85fcaaea 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/viewbinder/NotificationStatsLoggerBinder.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/ui/viewbinder/NotificationStatsLoggerBinder.kt @@ -41,6 +41,8 @@ object NotificationStatsLoggerBinder { logger: NotificationStatsLogger, viewModel: NotificationLoggerViewModel, ) { + // Updates the logger about whether the Notification panel, and the individual Notifications + // are visible to the user. viewModel.isLockscreenOrShadeInteractive .sample( combine(viewModel.isOnLockScreen, viewModel.activeNotifications, ::Pair), @@ -52,14 +54,14 @@ object NotificationStatsLoggerBinder { isOnLockScreen = isOnLockScreen, activeNotifications = notifications, ) - view.onNotificationsUpdated - // Delay the updates with [NOTIFICATION_UPDATES_PERIOD_MS]. If the original + view.onNotificationLocationsUpdated + // Delay the updates with [NOTIFICATION_UPDATE_PERIOD_MS]. If the original // flow emits more than once during this period, only the latest value is // emitted, meaning that we won't log the intermediate Notification states. .throttle(NOTIFICATION_UPDATE_PERIOD_MS) .sample(viewModel.activeNotificationRanks, ::Pair) - .collect { (locationsProvider, notificationRanks) -> - logger.onNotificationListUpdated(locationsProvider, notificationRanks) + .collect { (locationsProvider, ranks) -> + logger.onNotificationLocationsChanged(locationsProvider, ranks) } } else { logger.onLockscreenOrShadeNotInteractive( @@ -70,7 +72,7 @@ object NotificationStatsLoggerBinder { } } -private val NotificationStackScrollLayout.onNotificationsUpdated +private val NotificationStackScrollLayout.onNotificationLocationsUpdated get() = ConflatedCallbackFlow.conflatedCallbackFlow { val callback = diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinatorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinatorTest.kt new file mode 100644 index 000000000000..c29ff416feb9 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/NotificationStatsLoggerCoordinatorTest.kt @@ -0,0 +1,72 @@ +/* + * Copyright (C) 2024 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 android.platform.test.annotations.EnableFlags +import android.service.notification.NotificationListenerService.REASON_CANCEL +import android.testing.AndroidTestingRunner +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.statusbar.notification.collection.NotifPipeline +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionListener +import com.android.systemui.statusbar.notification.shared.NotificationsLiveDataStoreRefactor +import com.android.systemui.statusbar.notification.stack.ui.view.NotificationStatsLogger +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.mockito.whenever +import com.android.systemui.util.mockito.withArgCaptor +import java.util.Optional +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.verify + +@SmallTest +@RunWith(AndroidTestingRunner::class) +@EnableFlags(NotificationsLiveDataStoreRefactor.FLAG_NAME) +class NotificationStatsLoggerCoordinatorTest : SysuiTestCase() { + + private lateinit var collectionListener: NotifCollectionListener + + private val pipeline: NotifPipeline = mock() + private val logger: NotificationStatsLogger = mock() + private val underTest = NotificationStatsLoggerCoordinator(Optional.of(logger)) + + @Before + fun attachPipeline() { + underTest.attach(pipeline) + collectionListener = withArgCaptor { verify(pipeline).addCollectionListener(capture()) } + } + + @Test + fun onEntryAdded_loggerCalled() { + collectionListener.onEntryRemoved(mockEntry("key"), REASON_CANCEL) + + verify(logger).onNotificationRemoved("key") + } + + @Test + fun onEntryRemoved_loggerCalled() { + collectionListener.onEntryUpdated(mockEntry("key")) + + verify(logger).onNotificationUpdated("key") + } + + private fun mockEntry(key: String): NotificationEntry { + return mock { whenever(this.key).thenReturn(key) } + } +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt index d7d1ffcc3339..c61756c42895 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/ui/view/NotificationStatsLoggerTest.kt @@ -67,7 +67,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() { // AND they're visible val (ranks, locations) = fakeNotificationMaps("key0", "key1") val callable = Callable { locations } - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() // THEN visibility changes are reported @@ -103,13 +103,13 @@ class NotificationStatsLoggerTest : SysuiTestCase() { // GIVEN some visible Notifications are reported val (ranks, locations) = fakeNotificationMaps("key0", "key1") val callable = Callable { locations } - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() clearInvocations(mockStatusBarService, mockNotificationListenerService) // WHEN the same Notifications are removed val emptyCallable = Callable { emptyMap<String, Int>() } - underTest.onNotificationListUpdated(emptyCallable, emptyMap()) + underTest.onNotificationLocationsChanged(emptyCallable, emptyMap()) runCurrent() // THEN visibility changes are reported @@ -140,13 +140,13 @@ class NotificationStatsLoggerTest : SysuiTestCase() { // GIVEN some visible Notifications are reported val (ranks, locations) = fakeNotificationMaps("key0", "key1") val callable = Callable { locations } - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() clearInvocations(mockStatusBarService, mockNotificationListenerService) // WHEN the same Notifications are becoming invisible val emptyCallable = Callable { emptyMap<String, Int>() } - underTest.onNotificationListUpdated(emptyCallable, ranks) + underTest.onNotificationLocationsChanged(emptyCallable, ranks) runCurrent() // THEN visibility changes are reported @@ -176,13 +176,13 @@ class NotificationStatsLoggerTest : SysuiTestCase() { testScope.runTest { // GIVEN some visible Notifications are reported val (ranks, locations) = fakeNotificationMaps("key0", "key1") - underTest.onNotificationListUpdated({ locations }, ranks) + underTest.onNotificationLocationsChanged({ locations }, ranks) runCurrent() clearInvocations(mockStatusBarService, mockNotificationListenerService) // WHEN the reported Notifications are changing positions val (newRanks, newLocations) = fakeNotificationMaps("key1", "key0") - underTest.onNotificationListUpdated({ newLocations }, newRanks) + underTest.onNotificationLocationsChanged({ newLocations }, newRanks) runCurrent() // THEN no visibility changes are reported @@ -195,13 +195,13 @@ class NotificationStatsLoggerTest : SysuiTestCase() { // GIVEN some visible Notifications are reported val (ranks, locations) = fakeNotificationMaps("key0", "key1", "key2") val callable = spy(Callable { locations }) - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() clearInvocations(callable) // WHEN a new update comes val otherCallable = spy(Callable { locations }) - underTest.onNotificationListUpdated(otherCallable, ranks) + underTest.onNotificationLocationsChanged(otherCallable, ranks) runCurrent() // THEN we call the new Callable @@ -215,7 +215,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() { // GIVEN some visible Notifications are reported val (ranks, locations) = fakeNotificationMaps("key0", "key1") val callable = Callable { locations } - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() clearInvocations(mockStatusBarService, mockNotificationListenerService) @@ -281,7 +281,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() { } @Test - fun onNotificationExpanded_visibleLocation_expansionLogged() = + fun onNotificationExpansionChanged_whenExpandedInVisibleLocation_logsExpansion() = testScope.runTest { // WHEN a Notification is expanded underTest.onNotificationExpansionChanged( @@ -303,7 +303,33 @@ class NotificationStatsLoggerTest : SysuiTestCase() { } @Test - fun onNotificationExpanded_notVisibleLocation_nothingLogged() = + fun onNotificationExpansionChanged_whenCalledTwiceWithTheSameUpdate_doesNotDuplicateLogs() = + testScope.runTest { + // GIVEN a Notification is expanded + underTest.onNotificationExpansionChanged( + key = "key", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + clearInvocations(mockStatusBarService) + + // WHEN the logger receives the same expansion update + underTest.onNotificationExpansionChanged( + key = "key", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + + // THEN the Expand event is not reported again + verifyZeroInteractions(mockStatusBarService) + } + + @Test + fun onNotificationExpansionChanged_whenCalledForNotVisibleItem_nothingLogged() = testScope.runTest { // WHEN a NOT visible Notification is expanded underTest.onNotificationExpansionChanged( @@ -319,35 +345,73 @@ class NotificationStatsLoggerTest : SysuiTestCase() { } @Test - fun onNotificationExpanded_notVisibleLocation_becomesVisible_expansionLogged() = + fun onNotificationExpansionChanged_whenNotVisibleItemBecomesVisible_logsChanges() = testScope.runTest { // WHEN a NOT visible Notification is expanded underTest.onNotificationExpansionChanged( key = "key", isExpanded = true, location = ExpandableViewState.LOCATION_GONE, - isUserAction = true + isUserAction = false ) runCurrent() // AND it becomes visible val (ranks, locations) = fakeNotificationMaps("key") val callable = Callable { locations } - underTest.onNotificationListUpdated(callable, ranks) + underTest.onNotificationLocationsChanged(callable, ranks) runCurrent() // THEN the Expand event is reported verify(mockStatusBarService) .onNotificationExpansionChanged( /* key = */ "key", - /* userAction = */ true, + /* userAction = */ false, + /* expanded = */ true, + NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal + ) + } + + @Test + fun onNotificationExpansionChanged_whenUpdatedItemBecomesVisible_logsChanges() = + testScope.runTest { + // GIVEN a NOT visible Notification is expanded + underTest.onNotificationExpansionChanged( + key = "key", + isExpanded = true, + location = ExpandableViewState.LOCATION_GONE, + isUserAction = false + ) + runCurrent() + // AND we open the shade, so we log its events + val (ranks, locations) = fakeNotificationMaps("key") + val callable = Callable { locations } + underTest.onNotificationLocationsChanged(callable, ranks) + runCurrent() + // AND we close the shade, so it is NOT visible + val emptyCallable = Callable { emptyMap<String, Int>() } + underTest.onNotificationLocationsChanged(emptyCallable, ranks) + runCurrent() + clearInvocations(mockStatusBarService) // clear the previous expand log + + // WHEN it receives an update + underTest.onNotificationUpdated("key") + // AND it becomes visible again + underTest.onNotificationLocationsChanged(callable, ranks) + runCurrent() + + // THEN we log its expand event again + verify(mockStatusBarService) + .onNotificationExpansionChanged( + /* key = */ "key", + /* userAction = */ false, /* expanded = */ true, NotificationVisibility.NotificationLocation.LOCATION_MAIN_AREA.ordinal ) } @Test - fun onNotificationCollapsed_isFirstInteraction_nothingLogged() = + fun onNotificationExpansionChanged_whenCollapsedForTheFirstTime_nothingLogged() = testScope.runTest { // WHEN a Notification is collapsed, and it is the first interaction underTest.onNotificationExpansionChanged( @@ -364,7 +428,7 @@ class NotificationStatsLoggerTest : SysuiTestCase() { } @Test - fun onNotificationExpandedAndCollapsed_expansionChangesLogged() = + fun onNotificationExpansionChanged_receivesMultipleUpdates_logsChanges() = testScope.runTest { // GIVEN a Notification is expanded underTest.onNotificationExpansionChanged( @@ -403,6 +467,60 @@ class NotificationStatsLoggerTest : SysuiTestCase() { ) } + @Test + fun onNotificationUpdated_clearsTrackedExpansionChanges() = + testScope.runTest { + // GIVEN some notification updates are posted + underTest.onNotificationExpansionChanged( + key = "key1", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + underTest.onNotificationExpansionChanged( + key = "key2", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + clearInvocations(mockStatusBarService) + + // WHEN a Notification is updated + underTest.onNotificationUpdated("key1") + + // THEN the tracked expansion changes are updated + assertThat(underTest.lastReportedExpansionValues.keys).containsExactly("key2") + } + + @Test + fun onNotificationRemoved_clearsTrackedExpansionChanges() = + testScope.runTest { + // GIVEN some notification updates are posted + underTest.onNotificationExpansionChanged( + key = "key1", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + underTest.onNotificationExpansionChanged( + key = "key2", + isExpanded = true, + location = ExpandableViewState.LOCATION_MAIN_AREA, + isUserAction = true + ) + runCurrent() + clearInvocations(mockStatusBarService) + + // WHEN a Notification is removed + underTest.onNotificationRemoved("key1") + + // THEN it is removed from the tracked expansion changes + assertThat(underTest.lastReportedExpansionValues.keys).doesNotContain("key1") + } + private fun fakeNotificationMaps( vararg keys: String ): Pair<Map<String, Int>, Map<String, Int>> { |