From 4d4b6c47caeed71f8764f77e8c5fb3e1071944da Mon Sep 17 00:00:00 2001 From: Caitlin Cassidy Date: Thu, 13 Jan 2022 19:52:16 +0000 Subject: [PATCH] [Media TTT] Don't use futures for determining whether a transfer has succeeded or not. After discussing with davidsamuelson@, we decided that using futures to communicate across processes was error-prone. Specifically, on SysUI we'll switch to a failure state if we don't get a response from the future after a certain amount of time. However, it's possible that the transfer that D2DI kicks off ends up succeeding after we show the failure chip, so the user would get incorrect information. To avoid this, we're just going to have D2DI notify us when the transfer starts and when it succeeds or fails. Bug: 203800643 Bug: 203800347 Test: verify `adb shell cmd statusbar media-ttt-chip-add-sender Tablet TransferInitiated` stays loading forever and never switches to succeed state. Test: media.taptotransfer tests Change-Id: I38521e9af463a2d8eafdea63f9cf7c32bbbea9c6 --- .../systemui/media/dagger/MediaModule.java | 17 +--- .../MediaTttCommandLineHelper.kt | 11 +-- .../taptotransfer/sender/ChipStateSender.kt | 13 +-- .../sender/MediaTttChipControllerSender.kt | 48 --------- .../MediaTttCommandLineHelperTest.kt | 3 - .../MediaTttChipControllerSenderTest.kt | 97 +------------------ 6 files changed, 12 insertions(+), 177 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/dagger/MediaModule.java b/packages/SystemUI/src/com/android/systemui/media/dagger/MediaModule.java index d1fe7d449bdd8..dd60b30acad9e 100644 --- a/packages/SystemUI/src/com/android/systemui/media/dagger/MediaModule.java +++ b/packages/SystemUI/src/com/android/systemui/media/dagger/MediaModule.java @@ -21,8 +21,6 @@ import android.content.Context; import android.view.WindowManager; import com.android.systemui.dagger.SysUISingleton; -import com.android.systemui.dagger.qualifiers.Background; -import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.media.MediaDataManager; import com.android.systemui.media.MediaHierarchyManager; import com.android.systemui.media.MediaHost; @@ -33,10 +31,8 @@ import com.android.systemui.media.taptotransfer.receiver.MediaTttChipControllerR import com.android.systemui.media.taptotransfer.sender.MediaTttChipControllerSender; import com.android.systemui.media.taptotransfer.sender.MediaTttSenderService; import com.android.systemui.statusbar.commandline.CommandRegistry; -import com.android.systemui.util.concurrency.DelayableExecutor; import java.util.Optional; -import java.util.concurrent.Executor; import javax.inject.Named; @@ -89,14 +85,11 @@ public interface MediaModule { static Optional providesMediaTttChipControllerSender( MediaTttFlags mediaTttFlags, Context context, - WindowManager windowManager, - @Main Executor mainExecutor, - @Background Executor backgroundExecutor) { + WindowManager windowManager) { if (!mediaTttFlags.isMediaTttEnabled()) { return Optional.empty(); } - return Optional.of(new MediaTttChipControllerSender( - context, windowManager, mainExecutor, backgroundExecutor)); + return Optional.of(new MediaTttChipControllerSender(context, windowManager)); } /** */ @@ -120,8 +113,7 @@ public interface MediaModule { CommandRegistry commandRegistry, Context context, MediaTttChipControllerSender mediaTttChipControllerSender, - MediaTttChipControllerReceiver mediaTttChipControllerReceiver, - @Main DelayableExecutor mainExecutor) { + MediaTttChipControllerReceiver mediaTttChipControllerReceiver) { if (!mediaTttFlags.isMediaTttEnabled()) { return Optional.empty(); } @@ -130,8 +122,7 @@ public interface MediaModule { commandRegistry, context, mediaTttChipControllerSender, - mediaTttChipControllerReceiver, - mainExecutor)); + mediaTttChipControllerReceiver)); } /** Inject into MediaTttSenderService. */ diff --git a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelper.kt b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelper.kt index 613dfe03433d9..280f5ed862626 100644 --- a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelper.kt +++ b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelper.kt @@ -28,7 +28,6 @@ import android.util.Log import androidx.annotation.VisibleForTesting import com.android.systemui.R import com.android.systemui.dagger.SysUISingleton -import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.media.taptotransfer.receiver.MediaTttChipControllerReceiver import com.android.systemui.media.taptotransfer.receiver.ChipStateReceiver import com.android.systemui.media.taptotransfer.sender.MediaTttChipControllerSender @@ -42,9 +41,7 @@ import com.android.systemui.shared.mediattt.DeviceInfo import com.android.systemui.shared.mediattt.IDeviceSenderCallback import com.android.systemui.statusbar.commandline.Command import com.android.systemui.statusbar.commandline.CommandRegistry -import com.android.systemui.util.concurrency.DelayableExecutor import java.io.PrintWriter -import java.util.concurrent.FutureTask import javax.inject.Inject /** @@ -57,7 +54,6 @@ class MediaTttCommandLineHelper @Inject constructor( private val context: Context, private val mediaTttChipControllerSender: MediaTttChipControllerSender, private val mediaTttChipControllerReceiver: MediaTttChipControllerReceiver, - @Main private val mainExecutor: DelayableExecutor, ) { private var senderCallback: IDeviceSenderCallback? = null private val senderServiceConnection = SenderServiceConnection() @@ -101,17 +97,13 @@ class MediaTttCommandLineHelper @Inject constructor( // TODO(b/203800643): Migrate other commands to invoke the service instead of the // controller. TRANSFER_INITIATED_COMMAND_NAME -> { - val futureTask = FutureTask { fakeUndoRunnable } mediaTttChipControllerSender.displayChip( TransferInitiated( appIconDrawable, APP_ICON_CONTENT_DESCRIPTION, - otherDeviceName, - futureTask + otherDeviceName ) ) - mainExecutor.executeDelayed({ futureTask.run() }, FUTURE_WAIT_TIME) - } TRANSFER_SUCCEEDED_COMMAND_NAME -> { mediaTttChipControllerSender.displayChip( @@ -248,6 +240,5 @@ val TRANSFER_SUCCEEDED_COMMAND_NAME = TransferSucceeded::class.simpleName!! @VisibleForTesting val TRANSFER_FAILED_COMMAND_NAME = TransferFailed::class.simpleName!! -private const val FUTURE_WAIT_TIME = 2000L private const val APP_ICON_CONTENT_DESCRIPTION = "Fake media app icon" private const val TAG = "MediaTapToTransferCli" diff --git a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/ChipStateSender.kt b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/ChipStateSender.kt index e6f4ca54004fb..37f962ee8ecd4 100644 --- a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/ChipStateSender.kt +++ b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/ChipStateSender.kt @@ -20,7 +20,6 @@ import android.graphics.drawable.Drawable import androidx.annotation.StringRes import com.android.systemui.R import com.android.systemui.media.taptotransfer.common.MediaTttChipState -import java.util.concurrent.Future /** * A class that stores all the information necessary to display the media tap-to-transfer chip on @@ -71,19 +70,11 @@ class MoveCloserToEndCast( otherDeviceName ) -/** - * A state representing that a transfer has been initiated (but not completed). - * - * @property future a future that will be resolved when the transfer has either succeeded or failed. - * If the transfer succeeded, the future can optionally return an undo runnable (see - * [TransferSucceeded.undoRunnable]). [MediaTttChipControllerSender] is responsible for transitioning - * the chip to the [TransferSucceeded] state if the future resolves successfully. - */ +/** A state representing that a transfer has been initiated (but not completed). */ class TransferInitiated( appIconDrawable: Drawable, appIconContentDescription: String, - otherDeviceName: String, - val future: Future + otherDeviceName: String ) : ChipStateSender( appIconDrawable, appIconContentDescription, diff --git a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSender.kt b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSender.kt index 6453b79757ad2..0918f0fbeff1e 100644 --- a/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSender.kt +++ b/packages/SystemUI/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSender.kt @@ -23,11 +23,7 @@ import android.view.WindowManager import android.widget.TextView import com.android.systemui.R import com.android.systemui.dagger.SysUISingleton -import com.android.systemui.dagger.qualifiers.Background -import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.media.taptotransfer.common.MediaTttChipControllerCommon -import java.util.concurrent.Executor -import java.util.concurrent.TimeUnit import javax.inject.Inject /** @@ -38,8 +34,6 @@ import javax.inject.Inject class MediaTttChipControllerSender @Inject constructor( context: Context, windowManager: WindowManager, - @Main private val mainExecutor: Executor, - @Background private val backgroundExecutor: Executor, ) : MediaTttChipControllerCommon( context, windowManager, R.layout.media_ttt_chip ) { @@ -77,47 +71,5 @@ class MediaTttChipControllerSender @Inject constructor( val showFailure = chipState is TransferFailed currentChipView.requireViewById(R.id.failure_icon).visibility = if (showFailure) { View.VISIBLE } else { View.GONE } - - // Future handling - if (chipState is TransferInitiated) { - addFutureCallback(chipState) - } - } - - /** - * Adds the appropriate callbacks to [chipState.future] so that we update the chip correctly - * when the future resolves. - */ - private fun addFutureCallback(chipState: TransferInitiated) { - // Listen to the future on a background thread so we don't occupy the main thread while we - // wait for it to complete. - backgroundExecutor.execute { - try { - val undoRunnable = chipState.future.get(TRANSFER_TIMEOUT_SECONDS, TimeUnit.SECONDS) - // Make UI changes on the main thread - mainExecutor.execute { - displayChip( - TransferSucceeded( - chipState.appIconDrawable, - chipState.appIconContentDescription, - chipState.otherDeviceName, - undoRunnable - ) - ) - } - } catch (ex: Exception) { - mainExecutor.execute { - displayChip( - TransferFailed( - chipState.appIconDrawable, - chipState.appIconContentDescription, - chipState.otherDeviceName, - ) - ) - } - } - } } } - -private const val TRANSFER_TIMEOUT_SECONDS = 10L diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelperTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelperTest.kt index be082be974844..4e34789bdc6ed 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelperTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/MediaTttCommandLineHelperTest.kt @@ -26,11 +26,9 @@ import com.android.systemui.shared.mediattt.DeviceInfo import com.android.systemui.shared.mediattt.IDeviceSenderCallback import com.android.systemui.statusbar.commandline.Command import com.android.systemui.statusbar.commandline.CommandRegistry -import com.android.systemui.util.concurrency.FakeExecutor import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.argumentCaptor import com.android.systemui.util.mockito.capture -import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat import org.junit.Before import org.junit.Test @@ -75,7 +73,6 @@ class MediaTttCommandLineHelperTest : SysuiTestCase() { context, mediaTttChipControllerSender, mediaTttChipControllerReceiver, - FakeExecutor(FakeSystemClock()) ) } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSenderTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSenderTest.kt index 937d221286d55..f18a7ab68fafe 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSenderTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttChipControllerSenderTest.kt @@ -26,26 +26,18 @@ import android.widget.TextView import androidx.test.filters.SmallTest import com.android.systemui.R import com.android.systemui.SysuiTestCase -import com.android.systemui.util.concurrency.FakeExecutor import com.android.systemui.util.mockito.any -import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat -import com.google.common.util.concurrent.SettableFuture import org.junit.Before import org.junit.Test import org.mockito.ArgumentCaptor import org.mockito.Mock import org.mockito.Mockito.verify import org.mockito.MockitoAnnotations -import java.util.concurrent.Future @SmallTest class MediaTttChipControllerSenderTest : SysuiTestCase() { private lateinit var appIconDrawable: Drawable - private lateinit var fakeMainClock: FakeSystemClock - private lateinit var fakeMainExecutor: FakeExecutor - private lateinit var fakeBackgroundClock: FakeSystemClock - private lateinit var fakeBackgroundExecutor: FakeExecutor private lateinit var controllerSender: MediaTttChipControllerSender @@ -56,13 +48,7 @@ class MediaTttChipControllerSenderTest : SysuiTestCase() { fun setUp() { MockitoAnnotations.initMocks(this) appIconDrawable = Icon.createWithResource(context, R.drawable.ic_cake).loadDrawable(context) - fakeMainClock = FakeSystemClock() - fakeMainExecutor = FakeExecutor(fakeMainClock) - fakeBackgroundClock = FakeSystemClock() - fakeBackgroundExecutor = FakeExecutor(fakeBackgroundClock) - controllerSender = MediaTttChipControllerSender( - context, windowManager, fakeMainExecutor, fakeBackgroundExecutor - ) + controllerSender = MediaTttChipControllerSender(context, windowManager) } @Test @@ -92,13 +78,9 @@ class MediaTttChipControllerSenderTest : SysuiTestCase() { } @Test - fun transferInitiated_futureNotResolvedYet_appIcon_loadingIcon_noUndo_noFailureIcon() { - val future: SettableFuture = SettableFuture.create() - controllerSender.displayChip(transferInitiated(future)) + fun transferInitiated_appIcon_loadingIcon_noUndo_noFailureIcon() { + controllerSender.displayChip(transferInitiated()) - // Don't resolve the future in any way and don't run our executors - - // Assert we're still in the loading state val chipView = getChipView() assertThat(chipView.getAppIconView().drawable).isEqualTo(appIconDrawable) assertThat(chipView.getAppIconView().contentDescription).isEqualTo(APP_ICON_CONTENT_DESC) @@ -108,71 +90,6 @@ class MediaTttChipControllerSenderTest : SysuiTestCase() { assertThat(chipView.getFailureIcon().visibility).isEqualTo(View.GONE) } - @Test - fun transferInitiated_futureResolvedSuccessfully_switchesToTransferSucceeded() { - val future: SettableFuture = SettableFuture.create() - val undoRunnable = Runnable { } - - controllerSender.displayChip(transferInitiated(future)) - - future.set(undoRunnable) - fakeBackgroundExecutor.advanceClockToLast() - fakeBackgroundExecutor.runAllReady() - fakeMainExecutor.advanceClockToLast() - val numRun = fakeMainExecutor.runAllReady() - - // Assert we ran the future callback - assertThat(numRun).isEqualTo(1) - // Assert that we've moved to the successful state - val chipView = getChipView() - assertThat(chipView.getChipText()).contains(DEVICE_NAME) - assertThat(chipView.getLoadingIconVisibility()).isEqualTo(View.GONE) - assertThat(chipView.getUndoButton().visibility).isEqualTo(View.VISIBLE) - } - - @Test - fun transferInitiated_futureCancelled_switchesToTransferFailed() { - val future: SettableFuture = SettableFuture.create() - - controllerSender.displayChip(transferInitiated(future)) - - future.cancel(true) - fakeBackgroundExecutor.advanceClockToLast() - fakeBackgroundExecutor.runAllReady() - fakeMainExecutor.advanceClockToLast() - val numRun = fakeMainExecutor.runAllReady() - - // Assert we ran the future callback - assertThat(numRun).isEqualTo(1) - // Assert that we've moved to the failed state - val chipView = getChipView() - assertThat(chipView.getLoadingIconVisibility()).isEqualTo(View.GONE) - assertThat(chipView.getUndoButton().visibility).isEqualTo(View.GONE) - assertThat(chipView.getFailureIcon().visibility).isEqualTo(View.VISIBLE) - } - - @Test - fun transferInitiated_futureNotResolvedAfterTimeout_switchesToTransferFailed() { - val future: SettableFuture = SettableFuture.create() - controllerSender.displayChip(transferInitiated(future)) - - // We won't set anything on the future, but we will still run the executors so that we're - // waiting on the future resolving. If we have a bug in our code, then this test will time - // out because we're waiting on the future indefinitely. - fakeBackgroundExecutor.advanceClockToLast() - fakeBackgroundExecutor.runAllReady() - fakeMainExecutor.advanceClockToLast() - val numRun = fakeMainExecutor.runAllReady() - - // Assert we eventually decide to not wait for the future anymore - assertThat(numRun).isEqualTo(1) - // Assert that we've moved to the failed state - val chipView = getChipView() - assertThat(chipView.getLoadingIconVisibility()).isEqualTo(View.GONE) - assertThat(chipView.getUndoButton().visibility).isEqualTo(View.GONE) - assertThat(chipView.getFailureIcon().visibility).isEqualTo(View.VISIBLE) - } - @Test fun transferSucceeded_appIcon_deviceName_noLoadingIcon_noFailureIcon() { controllerSender.displayChip(transferSucceeded()) @@ -293,9 +210,8 @@ class MediaTttChipControllerSenderTest : SysuiTestCase() { MoveCloserToEndCast(appIconDrawable, APP_ICON_CONTENT_DESC, DEVICE_NAME) /** Helper method providing default parameters to not clutter up the tests. */ - private fun transferInitiated( - future: Future = TEST_FUTURE - ) = TransferInitiated(appIconDrawable, APP_ICON_CONTENT_DESC, DEVICE_NAME, future) + private fun transferInitiated() = + TransferInitiated(appIconDrawable, APP_ICON_CONTENT_DESC, DEVICE_NAME) /** Helper method providing default parameters to not clutter up the tests. */ private fun transferSucceeded( @@ -309,6 +225,3 @@ class MediaTttChipControllerSenderTest : SysuiTestCase() { private const val DEVICE_NAME = "My Tablet" private const val APP_ICON_CONTENT_DESC = "Content description" -// Use a settable future that hasn't yet been set so that we don't immediately switch to the success -// state. -private val TEST_FUTURE: SettableFuture = SettableFuture.create()