From 8a86dc702873803a4cd29b218fab0e21fbfc4442 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Mon, 23 Jan 2023 18:41:54 +0000 Subject: [PATCH] [Media TTT] Remove `displayedState` and always use the map instead. Two bugs around `displayedState`: 1) We weren't correctly re-setting it on timeout. 2) We weren't taking IDs into account when seeing if we could ignore a FAR_FROM_RECEIVER. This CL just removes `displayedState` entirely and uses `stateMap` as the one source of truth. Note: All the test cases added actually pass without any changes to `MediaTttSenderCoordinator` because we had safeguards in other parts of the code. When I wrote the tests and included assertions about `displayedState`, then they failed. But, now that `displayedState` is gone, we can't have those sorts of assertions anymore. Fixes: 266218672 Test: TRIGGERED -> SUCCEEDED -> {wait for timeout} -> FAR ==> nothing in the logs about "removal was ignored because transferStatus=SUCCEEDED" Test: ALMOST for id=1 -> ALMOST for id=2 -> FAR for id=1 ==> chip stays, state map just has id=2 Test: atest MediaTttSenderCoordinatorTest Change-Id: I58955fd8c4a60842476102a6c504ca987d93739d --- .../sender/MediaTttSenderCoordinator.kt | 19 +- .../sender/MediaTttSenderCoordinatorTest.kt | 162 ++++++++++++++++++ 2 files changed, 170 insertions(+), 11 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinator.kt b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinator.kt index e5b9a03718bd0..89ca5d33645c2 100644 --- a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinator.kt +++ b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinator.kt @@ -56,7 +56,6 @@ constructor( private val uiEventLogger: MediaTttSenderUiEventLogger, ) : CoreStartable, Dumpable { - private var displayedState: ChipStateSender? = null // A map to store current chip state per id. private var stateMap: MutableMap = mutableMapOf() @@ -96,11 +95,11 @@ constructor( return } - val currentState = stateMap[routeInfo.id] - if (!ChipStateSender.isValidStateTransition(currentState, chipState)) { + val currentStateForId: ChipStateSender? = stateMap[routeInfo.id] + if (!ChipStateSender.isValidStateTransition(currentStateForId, chipState)) { // ChipStateSender.FAR_FROM_RECEIVER is the default state when there is no state. logger.logInvalidStateTransitionError( - currentState = currentState?.name ?: ChipStateSender.FAR_FROM_RECEIVER.name, + currentState = currentStateForId?.name ?: ChipStateSender.FAR_FROM_RECEIVER.name, chipState.name ) return @@ -108,31 +107,29 @@ constructor( uiEventLogger.logSenderStateChange(chipState) if (chipState == ChipStateSender.FAR_FROM_RECEIVER) { - // Return early if we're not displaying a chip anyway - val currentDisplayedState = displayedState ?: return + // Return early if we're not displaying a chip for this ID anyway + if (currentStateForId == null) return val removalReason = ChipStateSender.FAR_FROM_RECEIVER.name if ( - currentDisplayedState.transferStatus == TransferStatus.IN_PROGRESS || - currentDisplayedState.transferStatus == TransferStatus.SUCCEEDED + currentStateForId.transferStatus == TransferStatus.IN_PROGRESS || + currentStateForId.transferStatus == TransferStatus.SUCCEEDED ) { // Don't remove the chip if we're in progress or succeeded, since the user should // still be able to see the status of the transfer. logger.logRemovalBypass( removalReason, - bypassReason = "transferStatus=${currentDisplayedState.transferStatus.name}" + bypassReason = "transferStatus=${currentStateForId.transferStatus.name}" ) return } // No need to store the state since it is the default state removeIdFromStore(routeInfo.id, reason = removalReason) - displayedState = null chipbarCoordinator.removeView(routeInfo.id, removalReason) } else { stateMap[routeInfo.id] = chipState logger.logStateMap(stateMap) - displayedState = chipState chipbarCoordinator.registerListener(displayListener) chipbarCoordinator.displayView( createChipbarInfo( diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt index 68eccd7abcbc1..400a504e4d023 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt @@ -1193,6 +1193,168 @@ class MediaTttSenderCoordinatorTest : SysuiTestCase() { verify(logger).logStateMapRemoval("route1", "reason") } + /** Regression test for b/266218672. */ + @Test + fun twoIdsDisplayed_oldIdIsFar_viewStillDisplayed() { + // WHEN there are two different media transfers with different IDs + val route1 = + MediaRoute2Info.Builder("route1", OTHER_DEVICE_NAME) + .addFeature("feature") + .setClientPackageName(PACKAGE_NAME) + .build() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_ALMOST_CLOSE_TO_END_CAST, + route1, + null, + ) + verify(windowManager).addView(any(), any()) + reset(windowManager) + + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_ALMOST_CLOSE_TO_START_CAST, + MediaRoute2Info.Builder("route2", "Route 2 name") + .addFeature("feature") + .setClientPackageName(PACKAGE_NAME) + .build(), + null, + ) + val newView = getChipbarView() + + // WHEN there's a FAR event for the earlier one + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_FAR_FROM_RECEIVER, + route1, + null, + ) + + // THEN it's ignored and the more recent one is still displayed + assertThat(newView.getChipText()) + .isEqualTo( + ChipStateSender.ALMOST_CLOSE_TO_START_CAST.getExpectedStateText("Route 2 name") + ) + } + + /** Regression test for b/266218672. */ + @Test + fun receiverSucceededThenTimedOut_internalStateResetAndCanDisplayAlmostCloseToEnd() { + displayReceiverTriggered() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_RECEIVER_SUCCEEDED, + routeInfo, + null, + ) + + fakeClock.advanceTime(TIMEOUT + 1L) + verify(windowManager).removeView(any()) + + reset(windowManager) + + // WHEN we try to show ALMOST_CLOSE_TO_END + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_ALMOST_CLOSE_TO_END_CAST, + routeInfo, + null, + ) + + // THEN it succeeds + val chipbarView = getChipbarView() + assertThat(chipbarView.getChipText()) + .isEqualTo(ChipStateSender.ALMOST_CLOSE_TO_END_CAST.getExpectedStateText()) + } + + /** Regression test for b/266218672. */ + @Test + fun receiverSucceededThenTimedOut_internalStateResetAndCanDisplayReceiverTriggered() { + displayReceiverTriggered() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_RECEIVER_SUCCEEDED, + routeInfo, + null, + ) + + fakeClock.advanceTime(TIMEOUT + 1L) + verify(windowManager).removeView(any()) + + reset(windowManager) + + // WHEN we try to show RECEIVER_TRIGGERED + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_RECEIVER_TRIGGERED, + routeInfo, + null, + ) + + // THEN it succeeds + val chipbarView = getChipbarView() + assertThat(chipbarView.getChipText()) + .isEqualTo(ChipStateSender.TRANSFER_TO_RECEIVER_TRIGGERED.getExpectedStateText()) + assertThat(chipbarView.getLoadingIcon().visibility).isEqualTo(View.VISIBLE) + } + + /** Regression test for b/266218672. */ + @Test + fun toThisDeviceSucceededThenTimedOut_internalStateResetAndCanDisplayAlmostCloseToStart() { + displayThisDeviceTriggered() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_THIS_DEVICE_SUCCEEDED, + routeInfo, + null, + ) + + fakeClock.advanceTime(TIMEOUT + 1L) + verify(windowManager).removeView(any()) + + reset(windowManager) + + // WHEN we try to show ALMOST_CLOSE_TO_START + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_ALMOST_CLOSE_TO_START_CAST, + routeInfo, + null, + ) + + // THEN it succeeds + val chipbarView = getChipbarView() + assertThat(chipbarView.getChipText()) + .isEqualTo(ChipStateSender.ALMOST_CLOSE_TO_START_CAST.getExpectedStateText()) + } + + /** Regression test for b/266218672. */ + @Test + fun toThisDeviceSucceededThenTimedOut_internalStateResetAndCanDisplayThisDeviceTriggered() { + displayThisDeviceTriggered() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_THIS_DEVICE_SUCCEEDED, + routeInfo, + null, + ) + + fakeClock.advanceTime(TIMEOUT + 1L) + verify(windowManager).removeView(any()) + + reset(windowManager) + + // WHEN we try to show THIS_DEVICE_TRIGGERED + val newRouteInfo = + MediaRoute2Info.Builder(DEFAULT_ID, "New Name") + .addFeature("feature") + .setClientPackageName(PACKAGE_NAME) + .build() + commandQueueCallback.updateMediaTapToTransferSenderDisplay( + StatusBarManager.MEDIA_TRANSFER_SENDER_STATE_TRANSFER_TO_THIS_DEVICE_TRIGGERED, + newRouteInfo, + null, + ) + + // THEN it succeeds + val chipbarView = getChipbarView() + assertThat(chipbarView.getChipText()) + .isEqualTo( + ChipStateSender.TRANSFER_TO_THIS_DEVICE_TRIGGERED.getExpectedStateText("New Name") + ) + assertThat(chipbarView.getLoadingIcon().visibility).isEqualTo(View.VISIBLE) + } + private fun getChipbarView(): ViewGroup { val viewCaptor = ArgumentCaptor.forClass(View::class.java) verify(windowManager).addView(viewCaptor.capture(), any())