From ee42e8c1434cc5fe57a37a34a80783ade10a2262 Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Thu, 30 Apr 2020 01:09:29 -0400 Subject: [PATCH] Clean up mediabrowser callbacks The test connection was not getting cleaned up properly, which is why VLC's foreground notification stayed around. It also should have been replaced with the actual resumption controls once those were ready. At the same time Spotify was not restarting on the first tap because the browser was being disconnected prematurely. So, if we have successfully connected to restart, wait until the new session info has come in to disconnect the browser. Fixes: 154825715 Fixes: 155098687 Test: manual Change-Id: I207ae4790e5ae2b073def1ceb654808907d5a5c4 --- .../systemui/media/MediaControlPanel.java | 34 ++++++- .../android/systemui/qs/QSMediaBrowser.java | 91 +++++++++++++------ .../src/com/android/systemui/qs/QSPanel.java | 84 ++++++++++------- 3 files changed, 142 insertions(+), 67 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java index f87605382913c..557132bdf08e7 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -98,6 +98,7 @@ public class MediaControlPanel { private SharedPreferences mSharedPrefs; private boolean mCheckedForResumption = false; private boolean mIsRemotePlayback; + private QSMediaBrowser mQSMediaBrowser; // Button IDs used in notifications protected static final int[] NOTIF_ACTION_IDS = { @@ -232,6 +233,11 @@ public class MediaControlPanel { String key) { // Ensure that component names are updated if token has changed if (mToken == null || !mToken.equals(token)) { + if (mQSMediaBrowser != null) { + Log.d(TAG, "Disconnecting old media browser"); + mQSMediaBrowser.disconnect(); + mQSMediaBrowser = null; + } mToken = token; mServiceComponent = null; mCheckedForResumption = false; @@ -618,8 +624,22 @@ public class MediaControlPanel { ImageButton btn = mMediaNotifView.findViewById(mActionIds[0]); btn.setOnClickListener(v -> { Log.d(TAG, "Attempting to restart session"); - QSMediaBrowser browser = new QSMediaBrowser(mContext, null, mServiceComponent); - browser.restart(); + if (mQSMediaBrowser != null) { + mQSMediaBrowser.disconnect(); + } + mQSMediaBrowser = new QSMediaBrowser(mContext, new QSMediaBrowser.Callback(){ + @Override + public void onConnected() { + Log.d(TAG, "Successfully restarted"); + } + @Override + public void onError() { + Log.e(TAG, "Error restarting"); + mQSMediaBrowser.disconnect(); + mQSMediaBrowser = null; + } + }, mServiceComponent); + mQSMediaBrowser.restart(); }); btn.setImageDrawable(mContext.getResources().getDrawable(R.drawable.lb_ic_play)); btn.setImageTintList(ColorStateList.valueOf(mForegroundColor)); @@ -654,13 +674,18 @@ public class MediaControlPanel { */ private void tryUpdateResumptionList(ComponentName componentName) { Log.d(TAG, "Testing if we can connect to " + componentName); - QSMediaBrowser.testConnection(mContext, + if (mQSMediaBrowser != null) { + mQSMediaBrowser.disconnect(); + } + mQSMediaBrowser = new QSMediaBrowser(mContext, new QSMediaBrowser.Callback() { @Override public void onConnected() { Log.d(TAG, "yes we can resume with " + componentName); mServiceComponent = componentName; updateResumptionList(componentName); + mQSMediaBrowser.disconnect(); + mQSMediaBrowser = null; } @Override @@ -671,9 +696,12 @@ public class MediaControlPanel { // If it's not active and we can't resume, remove removePlayer(); } + mQSMediaBrowser.disconnect(); + mQSMediaBrowser = null; } }, componentName); + mQSMediaBrowser.testConnection(); } /** diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSMediaBrowser.java b/packages/SystemUI/src/com/android/systemui/qs/QSMediaBrowser.java index 9e532868427f1..a5b73dcbd289a 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSMediaBrowser.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSMediaBrowser.java @@ -58,21 +58,25 @@ public class QSMediaBrowser { mContext = context; mCallback = callback; mComponentName = componentName; + } + /** + * Connects to the MediaBrowserService and looks for valid media. If a media item is returned + * by the service, QSMediaBrowser.Callback#addTrack will be called with its MediaDescription. + * QSMediaBrowser.Callback#onConnected and QSMediaBrowser.Callback#onError will also be called + * when the initial connection is successful, or an error occurs. Note that it is possible for + * the service to connect but for no playable tracks to be found later. + * QSMediaBrowser#disconnect will be called automatically with this function. + */ + public void findRecentMedia() { + Log.d(TAG, "Connecting to " + mComponentName); + disconnect(); Bundle rootHints = new Bundle(); rootHints.putBoolean(MediaBrowserService.BrowserRoot.EXTRA_RECENT, true); mMediaBrowser = new MediaBrowser(mContext, mComponentName, mConnectionCallback, rootHints); - } - - /** - * Connects to the MediaBrowserService and looks for valid media. If a media item is returned - * by the service, QSMediaBrowser.Callback#addTrack will be called with its MediaDescription - */ - public void findRecentMedia() { - Log.d(TAG, "Connecting to " + mComponentName); mMediaBrowser.connect(); } @@ -94,20 +98,22 @@ public class QSMediaBrowser { } else { Log.e(TAG, "Child found but not playable for " + mComponentName); } - mMediaBrowser.disconnect(); + disconnect(); } @Override public void onError(String parentId) { Log.e(TAG, "Subscribe error for " + mComponentName + ": " + parentId); - mMediaBrowser.disconnect(); + mCallback.onError(); + disconnect(); } @Override public void onError(String parentId, Bundle options) { Log.e(TAG, "Subscribe error for " + mComponentName + ": " + parentId + ", options: " + options); - mMediaBrowser.disconnect(); + mCallback.onError(); + disconnect(); } }; @@ -134,6 +140,8 @@ public class QSMediaBrowser { @Override public void onConnectionSuspended() { Log.d(TAG, "Connection suspended for " + mComponentName); + mCallback.onError(); + disconnect(); } /** @@ -143,16 +151,28 @@ public class QSMediaBrowser { public void onConnectionFailed() { Log.e(TAG, "Connection failed for " + mComponentName); mCallback.onError(); + disconnect(); } }; /** - * Connects to the MediaBrowserService and starts playback + * Disconnect the media browser. This should be called after restart or testConnection have + * completed to close the connection. */ - public void restart() { - if (mMediaBrowser.isConnected()) { + public void disconnect() { + if (mMediaBrowser != null) { mMediaBrowser.disconnect(); } + mMediaBrowser = null; + } + + /** + * Connects to the MediaBrowserService and starts playback. QSMediaBrowser.Callback#onError or + * QSMediaBrowser.Callback#onConnected will be called depending on whether it was successful. + * QSMediaBrowser#disconnect should be called after this to ensure the connection is closed. + */ + public void restart() { + disconnect(); Bundle rootHints = new Bundle(); rootHints.putBoolean(MediaBrowserService.BrowserRoot.EXTRA_RECENT, true); mMediaBrowser = new MediaBrowser(mContext, mComponentName, @@ -165,6 +185,17 @@ public class QSMediaBrowser { controller.getTransportControls(); controller.getTransportControls().prepare(); controller.getTransportControls().play(); + mCallback.onConnected(); + } + + @Override + public void onConnectionFailed() { + mCallback.onError(); + } + + @Override + public void onConnectionSuspended() { + mCallback.onError(); } }, rootHints); mMediaBrowser.connect(); @@ -192,42 +223,44 @@ public class QSMediaBrowser { } /** - * Used to test if SystemUI is allowed to connect to the given component as a MediaBrowser - * @param mContext the context - * @param callback methods onConnected or onError will be called to indicate whether the - * connection was successful or not - * @param mComponentName Component name of the MediaBrowserService this browser will connect to + * Used to test if SystemUI is allowed to connect to the given component as a MediaBrowser. + * QSMediaBrowser.Callback#onError or QSMediaBrowser.Callback#onConnected will be called + * depending on whether it was successful. + * QSMediaBrowser#disconnect should be called after this to ensure the connection is closed. */ - public static MediaBrowser testConnection(Context mContext, Callback callback, - ComponentName mComponentName) { - final MediaBrowser.ConnectionCallback mConnectionCallback = + public void testConnection() { + disconnect(); + final MediaBrowser.ConnectionCallback connectionCallback = new MediaBrowser.ConnectionCallback() { @Override public void onConnected() { Log.d(TAG, "connected"); - callback.onConnected(); + if (mMediaBrowser.getRoot() == null) { + mCallback.onError(); + } else { + mCallback.onConnected(); + } } @Override public void onConnectionSuspended() { Log.d(TAG, "suspended"); - callback.onError(); + mCallback.onError(); } @Override public void onConnectionFailed() { Log.d(TAG, "failed"); - callback.onError(); + mCallback.onError(); } }; Bundle rootHints = new Bundle(); rootHints.putBoolean(MediaBrowserService.BrowserRoot.EXTRA_RECENT, true); - MediaBrowser browser = new MediaBrowser(mContext, + mMediaBrowser = new MediaBrowser(mContext, mComponentName, - mConnectionCallback, + connectionCallback, rootHints); - browser.connect(); - return browser; + mMediaBrowser.connect(); } /** diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java index a3004bdc004d6..e8f6c9668e9b3 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java @@ -270,27 +270,8 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne return; } - QSMediaPlayer player = null; String packageName = notif.getPackageName(); - for (QSMediaPlayer p : mMediaPlayers) { - if (p.getKey() == null) { - // No notification key = loaded via mediabrowser, so just match on package - if (packageName.equals(p.getMediaPlayerPackage())) { - Log.d(TAG, "Found matching resume player by package: " + packageName); - player = p; - break; - } - } else if (p.getMediaSessionToken().equals(token)) { - Log.d(TAG, "Found matching player by token " + packageName); - player = p; - break; - } else if (packageName.equals(p.getMediaPlayerPackage()) && key.equals(p.getKey())) { - // Also match if it's the same package and notification key - Log.d(TAG, "Found matching player by package " + packageName + ", " + key); - player = p; - break; - } - } + QSMediaPlayer player = findMediaPlayer(packageName, token, key); int playerWidth = (int) getResources().getDimension(R.dimen.qs_media_width); int padding = (int) getResources().getDimension(R.dimen.qs_media_padding); @@ -299,7 +280,7 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne lp.setMarginEnd(padding); if (player == null) { - Log.d(TAG, "creating new player"); + Log.d(TAG, "creating new player for " + packageName); // Set up listener for device changes // TODO: integrate with MediaTransferManager? InfoMediaManager imm = new InfoMediaManager(mContext, notif.getPackageName(), @@ -331,6 +312,35 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne } } + /** + * Check for an existing media player using the given information + * @param packageName + * @param token + * @param key + * @return a player, or null if no match found + */ + private QSMediaPlayer findMediaPlayer(String packageName, MediaSession.Token token, + String key) { + for (QSMediaPlayer player : mMediaPlayers) { + if (player.getKey() == null || key == null) { + // No notification key = loaded via mediabrowser, so just match on package + if (packageName.equals(player.getMediaPlayerPackage())) { + Log.d(TAG, "Found matching resume player by package: " + packageName); + return player; + } + } else if (player.getMediaSessionToken().equals(token)) { + Log.d(TAG, "Found matching player by token " + packageName); + return player; + } else if (packageName.equals(player.getMediaPlayerPackage()) + && key.equals(player.getKey())) { + // Also match if it's the same package and notification key + Log.d(TAG, "Found matching player by package " + packageName + ", " + key); + return player; + } + } + return null; + } + protected View getMediaPanel() { return mMediaCarousel; } @@ -369,26 +379,30 @@ public class QSPanel extends LinearLayout implements Tunable, Callback, Brightne } Log.d(TAG, "adding track from browser: " + desc + ", " + component); - QSMediaPlayer player = new QSMediaPlayer(mContext, QSPanel.this, - null, mForegroundExecutor, mBackgroundExecutor, mActivityStarter); + // Check if there's an old player for this app String pkgName = component.getPackageName(); + MediaSession.Token token = browser.getToken(); + QSMediaPlayer player = findMediaPlayer(pkgName, token, null); - // Add controls to carousel - int playerWidth = (int) getResources().getDimension(R.dimen.qs_media_width); - int padding = (int) getResources().getDimension(R.dimen.qs_media_padding); - LinearLayout.LayoutParams lp = new LinearLayout.LayoutParams(playerWidth, - LayoutParams.MATCH_PARENT); - lp.setMarginStart(padding); - lp.setMarginEnd(padding); - mMediaCarousel.addView(player.getView(), lp); - ((View) mMediaCarousel.getParent()).setVisibility(View.VISIBLE); - mMediaPlayers.add(player); + if (player == null) { + player = new QSMediaPlayer(mContext, QSPanel.this, + null, mForegroundExecutor, mBackgroundExecutor, mActivityStarter); + + // Add to carousel + int playerWidth = (int) getResources().getDimension(R.dimen.qs_media_width); + int padding = (int) getResources().getDimension(R.dimen.qs_media_padding); + LinearLayout.LayoutParams lp = new LinearLayout.LayoutParams(playerWidth, + LayoutParams.MATCH_PARENT); + lp.setMarginStart(padding); + lp.setMarginEnd(padding); + mMediaCarousel.addView(player.getView(), lp); + ((View) mMediaCarousel.getParent()).setVisibility(View.VISIBLE); + mMediaPlayers.add(player); + } int iconColor = Color.DKGRAY; int bgColor = Color.LTGRAY; - - MediaSession.Token token = browser.getToken(); player.setMediaSession(token, desc, iconColor, bgColor, browser.getAppIntent(), pkgName); }