From b12fdd807343d33da227b80829c062fd52b7f729 Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Wed, 30 Jun 2021 15:44:17 -0400 Subject: [PATCH] Remove controls from carousel when not in manager If the user is in a bad state where controls exist in the carousel but their keys have been removed from the manager, allow the dismiss button to remove the player from the carousel directly. The device isn't expected to get into this state normally but this will allow users to remove the bad controls without needing to reboot. Also added dump to MediaCarouselController to help debug future reports. Bug: 190799184 Test: atest MediaDataManagerTest MediaControlPanelTest Test: take bugreport, verify MediaCarouselController data is in dumpsys Test: manual - force bad controls state, verify can still remove Change-Id: I7af130c90c7b661cc7cc8bbeb3ca1fd5d81c8913 --- .../systemui/media/MediaCarouselController.kt | 21 ++++++++++++++++--- .../systemui/media/MediaControlPanel.java | 8 +++++-- .../systemui/media/MediaDataManager.kt | 7 ++++++- .../systemui/media/MediaControlPanelTest.kt | 18 ++++++++++++++++ .../systemui/media/MediaDataManagerTest.kt | 9 +++++++- 5 files changed, 56 insertions(+), 7 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index cc9414cbdf50e..2facf3dedd184 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt @@ -12,10 +12,12 @@ import android.view.View import android.view.ViewGroup import android.widget.LinearLayout import androidx.annotation.VisibleForTesting +import com.android.systemui.Dumpable import com.android.systemui.R import com.android.systemui.classifier.FalsingCollector import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Main +import com.android.systemui.dump.DumpManager import com.android.systemui.plugins.ActivityStarter import com.android.systemui.plugins.FalsingManager import com.android.systemui.qs.PageIndicator @@ -27,6 +29,8 @@ import com.android.systemui.util.animation.UniqueObjectHostView import com.android.systemui.util.animation.requiresRemeasuring import com.android.systemui.util.concurrency.DelayableExecutor import com.android.systemui.util.time.SystemClock +import java.io.FileDescriptor +import java.io.PrintWriter import java.util.TreeMap import javax.inject.Inject import javax.inject.Provider @@ -51,8 +55,9 @@ class MediaCarouselController @Inject constructor( private val mediaManager: MediaDataManager, configurationController: ConfigurationController, falsingCollector: FalsingCollector, - falsingManager: FalsingManager -) { + falsingManager: FalsingManager, + dumpManager: DumpManager +) : Dumpable { /** * The current width of the carousel */ @@ -166,6 +171,7 @@ class MediaCarouselController @Inject constructor( lateinit var updateUserVisibility: () -> Unit init { + dumpManager.registerDumpable(TAG, this) mediaFrame = inflateMediaCarousel() mediaCarousel = mediaFrame.requireViewById(R.id.media_carousel_scroller) pageIndicator = mediaFrame.requireViewById(R.id.media_page_indicator) @@ -421,7 +427,7 @@ class MediaCarouselController @Inject constructor( } } - private fun removePlayer( + fun removePlayer( key: String, dismissMediaData: Boolean = true, dismissRecommendation: Boolean = true @@ -748,6 +754,15 @@ class MediaCarouselController @Inject constructor( } mediaManager.onSwipeToDismiss() } + + override fun dump(fd: FileDescriptor, pw: PrintWriter, args: Array) { + pw.apply { + println("keysNeedRemoval: $keysNeedRemoval") + println("playerKeys: ${MediaPlayerData.playerKeys()}") + println("smartspaceMediaData: ${MediaPlayerData.smartspaceMediaData}") + println("shouldPrioritizeSs: ${MediaPlayerData.shouldPrioritizeSs}") + } + } } @VisibleForTesting diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java index a3d7a811cab11..902e8c28a85d4 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -455,8 +455,12 @@ public class MediaControlPanel { if (mKey != null) { closeGuts(); - mMediaDataManagerLazy.get().dismissMediaData(mKey, - MediaViewController.GUTS_ANIMATION_DURATION + 100); + if (!mMediaDataManagerLazy.get().dismissMediaData(mKey, + MediaViewController.GUTS_ANIMATION_DURATION + 100)) { + Log.w(TAG, "Manager failed to dismiss media " + mKey); + // Remove directly from carousel to let user recover - TODO(b/190799184) + mMediaCarouselController.removePlayer(key, false, false); + } } else { Log.w(TAG, "Dismiss media with null notification. Token uid=" + data.getToken().getUid()); diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index df1b07f3e0b2c..0a28b47923dac 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -430,7 +430,11 @@ class MediaDataManager( notifyMediaDataRemoved(key) } - fun dismissMediaData(key: String, delay: Long) { + /** + * Dismiss a media entry. Returns false if the key was not found. + */ + fun dismissMediaData(key: String, delay: Long): Boolean { + val existed = mediaEntries[key] != null backgroundExecutor.execute { mediaEntries[key]?.let { mediaData -> if (mediaData.isLocalSession) { @@ -442,6 +446,7 @@ class MediaDataManager( } } foregroundExecutor.executeDelayed({ removeEntry(key) }, delay) + return existed } /** diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt index c9d41900b9965..b129fdd0d8441 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt @@ -345,4 +345,22 @@ public class MediaControlPanelTest : SysuiTestCase() { assertThat(dismiss.isEnabled).isEqualTo(false) } + + @Test + fun dismissButtonClick_notInManager() { + val mediaKey = "key for dismissal" + whenever(mediaDataManager.dismissMediaData(eq(mediaKey), anyLong())).thenReturn(false) + + player.attachPlayer(holder) + val state = MediaData(USER_ID, true, BG_COLOR, APP, null, ARTIST, TITLE, null, emptyList(), + emptyList(), PACKAGE, session.getSessionToken(), null, null, true, null, + notificationKey = KEY) + player.bindPlayer(state, mediaKey) + + assertThat(dismiss.isEnabled).isEqualTo(true) + dismiss.callOnClick() + + verify(mediaDataManager).dismissMediaData(eq(mediaKey), anyLong()) + verify(mediaCarouselController).removePlayer(eq(mediaKey), eq(false), eq(false)) + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt index 5b4e12463370d..ba6dfd3c453a7 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt @@ -317,7 +317,8 @@ class MediaDataManagerTest : SysuiTestCase() { fun testDismissMedia_listenerCalled() { mediaDataManager.onNotificationAdded(KEY, mediaNotification) mediaDataManager.onMediaDataLoaded(KEY, oldKey = null, data = mock(MediaData::class.java)) - mediaDataManager.dismissMediaData(KEY, 0L) + val removed = mediaDataManager.dismissMediaData(KEY, 0L) + assertThat(removed).isTrue() foregroundExecutor.advanceClockToLast() foregroundExecutor.runAllReady() @@ -325,6 +326,12 @@ class MediaDataManagerTest : SysuiTestCase() { verify(listener).onMediaDataRemoved(eq(KEY)) } + @Test + fun testDismissMedia_keyDoesNotExist_returnsFalse() { + val removed = mediaDataManager.dismissMediaData(KEY, 0L) + assertThat(removed).isFalse() + } + @Test fun testBadArtwork_doesNotUse() { // WHEN notification has a too-small artwork