[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
2022-01-13 19:52:16 +00:00
parent a0cf54232c
commit 4d4b6c47ca
6 changed files with 12 additions and 177 deletions
--- 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<MediaTttChipControllerSender> 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. */
--- 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"
--- 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<Runnable?>
+    otherDeviceName: String
 ) : ChipStateSender(
    appIconDrawable,
    appIconContentDescription,
--- 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<ChipStateSender>(
    context, windowManager, R.layout.media_ttt_chip
 ) {
@@ -77,47 +71,5 @@ class MediaTttChipControllerSender @Inject constructor(
        val showFailure = chipState is TransferFailed
        currentChipView.requireViewById<View>(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
--- 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())
            )
    }

--- 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<Runnable?> = 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<Runnable?> = 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<Runnable?> = 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<Runnable?> = 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<Runnable?> = 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<Runnable?> = SettableFuture.create()