From 445d441b441f8d0960d8d8c9aa56a7fa6c400ff6 Mon Sep 17 00:00:00 2001 From: Robert Snoeberger Date: Wed, 15 Apr 2020 00:03:13 -0400 Subject: [PATCH] NotificationMediaManager is bad signal for resumption Entering resumption on STATE_NONE from NotificationMediaManager causes flicker because it sends STATE_NONE to often. Instead, we can listen for PlaybackState changes from the MediaController directly. For cast, we need to enter the resumption state on STATE_CONNECTING, because that is the resulting state after ending a cast session. Fixes: 153937833 Test: manual - play music and toggle play / pause. Look for flicker caused by action buttons rapidly switching in and out of the resumption state. Test: manual - cast from YT to TV. End cast session. Verify that player is in the resumption state. Change-Id: Ie7f577d1b06719a1ce2ae5927b2ee20745076303 --- .../systemui/media/MediaControlPanel.java | 22 +++++++++---------- .../android/systemui/qs/QSMediaPlayer.java | 10 ++++----- .../src/com/android/systemui/qs/QSPanel.java | 8 ++----- .../systemui/qs/QuickQSMediaPlayer.java | 8 +++---- .../com/android/systemui/qs/QuickQSPanel.java | 6 ++--- .../com/android/systemui/qs/QSPanelTest.java | 5 +---- 6 files changed, 22 insertions(+), 37 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java index 62efd8ce4cee9..e5c0bf7d44533 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -55,8 +55,6 @@ import com.android.settingslib.widget.AdaptiveIcon; import com.android.systemui.Dependency; import com.android.systemui.R; import com.android.systemui.plugins.ActivityStarter; -import com.android.systemui.statusbar.NotificationMediaManager; -import com.android.systemui.statusbar.NotificationMediaManager.MediaListener; import com.android.systemui.util.Assert; import java.util.List; @@ -67,7 +65,6 @@ import java.util.concurrent.Executor; */ public class MediaControlPanel { private static final String TAG = "MediaControlPanel"; - private final NotificationMediaManager mMediaManager; @Nullable private final LocalMediaManager mLocalMediaManager; private final Executor mForegroundExecutor; private final Executor mBackgroundExecutor; @@ -102,12 +99,15 @@ public class MediaControlPanel { clearControls(); makeInactive(); } - }; - - private final MediaListener mMediaListener = new MediaListener() { @Override - public void onMetadataOrStateChanged(MediaMetadata metadata, int state) { - if (state == PlaybackState.STATE_NONE) { + public void onPlaybackStateChanged(PlaybackState state) { + final int s = state != null ? state.getState() : PlaybackState.STATE_NONE; + // When the playback state is NONE or CONNECTING, transition the player to the + // resumption state. State CONNECTING needs to be considered for Cast sessions. Ending + // a cast session in YT results in the CONNECTING state, which makes sense if you + // thinking of the session as waiting to connect to another cast device. + if (s == PlaybackState.STATE_NONE || s == PlaybackState.STATE_CONNECTING) { + Log.d(TAG, "playback state change will trigger resumption, state=" + state); clearControls(); makeInactive(); } @@ -160,7 +160,7 @@ public class MediaControlPanel { * @param foregroundExecutor foreground executor * @param backgroundExecutor background executor, used for processing artwork */ - public MediaControlPanel(Context context, ViewGroup parent, NotificationMediaManager manager, + public MediaControlPanel(Context context, ViewGroup parent, @Nullable LocalMediaManager routeManager, @LayoutRes int layoutId, int[] actionIds, Executor foregroundExecutor, Executor backgroundExecutor) { mContext = context; @@ -172,7 +172,6 @@ public class MediaControlPanel { // attach/detach of views instead of inflating them in the constructor, which would allow // mStateListener to be unregistered in detach. mMediaNotifView.addOnAttachStateChangeListener(mStateListener); - mMediaManager = manager; mLocalMediaManager = routeManager; mActionIds = actionIds; mForegroundExecutor = foregroundExecutor; @@ -443,6 +442,7 @@ public class MediaControlPanel { * Put controls into a resumption state */ public void clearControls() { + Log.d(TAG, "clearControls to resumption state package=" + getMediaPlayerPackage()); // Hide all the old buttons for (int i = 0; i < mActionIds.length; i++) { ImageButton thisBtn = mMediaNotifView.findViewById(mActionIds[i]); @@ -485,7 +485,6 @@ public class MediaControlPanel { private void makeActive() { Assert.isMainThread(); if (!mIsRegistered) { - mMediaManager.addCallback(mMediaListener); if (mLocalMediaManager != null) { mLocalMediaManager.registerCallback(mDeviceCallback); mLocalMediaManager.startScan(); @@ -501,7 +500,6 @@ public class MediaControlPanel { mLocalMediaManager.stopScan(); mLocalMediaManager.unregisterCallback(mDeviceCallback); } - mMediaManager.removeCallback(mMediaListener); mIsRegistered = false; } } diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSMediaPlayer.java b/packages/SystemUI/src/com/android/systemui/qs/QSMediaPlayer.java index e636707a97221..6da24d23b42ee 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSMediaPlayer.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSMediaPlayer.java @@ -39,7 +39,6 @@ import com.android.systemui.R; import com.android.systemui.media.MediaControlPanel; import com.android.systemui.media.SeekBarObserver; import com.android.systemui.media.SeekBarViewModel; -import com.android.systemui.statusbar.NotificationMediaManager; import com.android.systemui.util.concurrency.DelayableExecutor; import java.util.concurrent.Executor; @@ -69,14 +68,13 @@ public class QSMediaPlayer extends MediaControlPanel { * Initialize quick shade version of player * @param context * @param parent - * @param manager + * @param routeManager Provides information about device * @param foregroundExecutor * @param backgroundExecutor */ - public QSMediaPlayer(Context context, ViewGroup parent, NotificationMediaManager manager, - LocalMediaManager routeManager, Executor foregroundExecutor, - DelayableExecutor backgroundExecutor) { - super(context, parent, manager, routeManager, R.layout.qs_media_panel, QS_ACTION_IDS, + public QSMediaPlayer(Context context, ViewGroup parent, LocalMediaManager routeManager, + Executor foregroundExecutor, DelayableExecutor backgroundExecutor) { + super(context, parent, routeManager, R.layout.qs_media_panel, QS_ACTION_IDS, foregroundExecutor, backgroundExecutor); mParent = (QSPanel) parent; mBackgroundExecutor = backgroundExecutor; diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java index 0566b2e621db2..80c6ccd19e6fa 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java @@ -63,7 +63,6 @@ import com.android.systemui.qs.external.CustomTile; import com.android.systemui.qs.logging.QSLogger; import com.android.systemui.settings.BrightnessController; import com.android.systemui.settings.ToggleSliderView; -import com.android.systemui.statusbar.NotificationMediaManager; import com.android.systemui.statusbar.policy.BrightnessMirrorController; import com.android.systemui.statusbar.policy.BrightnessMirrorController.BrightnessMirrorListener; import com.android.systemui.tuner.TunerService; @@ -99,7 +98,6 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne private final LinearLayout mMediaCarousel; private final ArrayList mMediaPlayers = new ArrayList<>(); - private final NotificationMediaManager mNotificationMediaManager; private final LocalBluetoothManager mLocalBluetoothManager; private final Executor mForegroundExecutor; private final DelayableExecutor mBackgroundExecutor; @@ -133,7 +131,6 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne DumpManager dumpManager, BroadcastDispatcher broadcastDispatcher, QSLogger qsLogger, - NotificationMediaManager notificationMediaManager, @Main Executor foregroundExecutor, @Background DelayableExecutor backgroundExecutor, @Nullable LocalBluetoothManager localBluetoothManager @@ -142,7 +139,6 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne mContext = context; mQSLogger = qsLogger; mDumpManager = dumpManager; - mNotificationMediaManager = notificationMediaManager; mForegroundExecutor = foregroundExecutor; mBackgroundExecutor = backgroundExecutor; mLocalBluetoothManager = localBluetoothManager; @@ -252,8 +248,8 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne LocalMediaManager routeManager = new LocalMediaManager(mContext, mLocalBluetoothManager, imm, notif.getPackageName()); - player = new QSMediaPlayer(mContext, this, mNotificationMediaManager, routeManager, - mForegroundExecutor, mBackgroundExecutor); + player = new QSMediaPlayer(mContext, this, routeManager, mForegroundExecutor, + mBackgroundExecutor); player.setListening(mListening); if (player.isPlaying()) { mMediaCarousel.addView(player.getView(), 0, lp); // add in front diff --git a/packages/SystemUI/src/com/android/systemui/qs/QuickQSMediaPlayer.java b/packages/SystemUI/src/com/android/systemui/qs/QuickQSMediaPlayer.java index 0ba4cb159024b..ccfddea01ce27 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QuickQSMediaPlayer.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QuickQSMediaPlayer.java @@ -29,7 +29,6 @@ import android.widget.LinearLayout; import com.android.systemui.R; import com.android.systemui.media.MediaControlPanel; -import com.android.systemui.statusbar.NotificationMediaManager; import java.util.concurrent.Executor; @@ -47,13 +46,12 @@ public class QuickQSMediaPlayer extends MediaControlPanel { * Initialize mini media player for QQS * @param context * @param parent - * @param manager * @param foregroundExecutor * @param backgroundExecutor */ - public QuickQSMediaPlayer(Context context, ViewGroup parent, NotificationMediaManager manager, - Executor foregroundExecutor, Executor backgroundExecutor) { - super(context, parent, manager, null, R.layout.qqs_media_panel, QQS_ACTION_IDS, + public QuickQSMediaPlayer(Context context, ViewGroup parent, Executor foregroundExecutor, + Executor backgroundExecutor) { + super(context, parent, null, R.layout.qqs_media_panel, QQS_ACTION_IDS, foregroundExecutor, backgroundExecutor); } diff --git a/packages/SystemUI/src/com/android/systemui/qs/QuickQSPanel.java b/packages/SystemUI/src/com/android/systemui/qs/QuickQSPanel.java index e6876bd98d218..2169677b83fb4 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QuickQSPanel.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QuickQSPanel.java @@ -39,7 +39,6 @@ import com.android.systemui.plugins.qs.QSTile.SignalState; import com.android.systemui.plugins.qs.QSTile.State; import com.android.systemui.qs.customize.QSCustomizer; import com.android.systemui.qs.logging.QSLogger; -import com.android.systemui.statusbar.NotificationMediaManager; import com.android.systemui.tuner.TunerService; import com.android.systemui.tuner.TunerService.Tunable; import com.android.systemui.util.Utils; @@ -83,12 +82,11 @@ public class QuickQSPanel extends QSPanel { DumpManager dumpManager, BroadcastDispatcher broadcastDispatcher, QSLogger qsLogger, - NotificationMediaManager notificationMediaManager, @Main Executor foregroundExecutor, @Background DelayableExecutor backgroundExecutor, @Nullable LocalBluetoothManager localBluetoothManager ) { - super(context, attrs, dumpManager, broadcastDispatcher, qsLogger, notificationMediaManager, + super(context, attrs, dumpManager, broadcastDispatcher, qsLogger, foregroundExecutor, backgroundExecutor, localBluetoothManager); if (mFooter != null) { removeView(mFooter.getView()); @@ -109,7 +107,7 @@ public class QuickQSPanel extends QSPanel { int marginSize = (int) mContext.getResources().getDimension(R.dimen.qqs_media_spacing); mMediaPlayer = new QuickQSMediaPlayer(mContext, mHorizontalLinearLayout, - notificationMediaManager, foregroundExecutor, backgroundExecutor); + foregroundExecutor, backgroundExecutor); LayoutParams lp2 = new LayoutParams(0, LayoutParams.MATCH_PARENT, 1); lp2.setMarginEnd(marginSize); lp2.setMarginStart(0); diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.java index 862ebe13bd93f..9112b654c1cc8 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.java @@ -43,7 +43,6 @@ import com.android.systemui.plugins.qs.QSTileView; import com.android.systemui.qs.customize.QSCustomizer; import com.android.systemui.qs.logging.QSLogger; import com.android.systemui.qs.tileimpl.QSTileImpl; -import com.android.systemui.statusbar.NotificationMediaManager; import com.android.systemui.util.concurrency.DelayableExecutor; import org.junit.Before; @@ -84,8 +83,6 @@ public class QSPanelTest extends SysuiTestCase { @Mock private QSTileView mQSTileView; @Mock - private NotificationMediaManager mNotificationMediaManager; - @Mock private Executor mForegroundExecutor; @Mock private DelayableExecutor mBackgroundExecutor; @@ -100,7 +97,7 @@ public class QSPanelTest extends SysuiTestCase { mTestableLooper.runWithLooper(() -> { mMetricsLogger = mDependency.injectMockDependency(MetricsLogger.class); mQsPanel = new QSPanel(mContext, null, mDumpManager, mBroadcastDispatcher, - mQSLogger, mNotificationMediaManager, mForegroundExecutor, mBackgroundExecutor, + mQSLogger, mForegroundExecutor, mBackgroundExecutor, mLocalBluetoothManager); // Provides a parent with non-zero size for QSPanel mParentView = new FrameLayout(mContext);