From 17ee5277d5daf3285e10f5a9bffd1ca027ae51aa Mon Sep 17 00:00:00 2001 From: bachinger Date: Wed, 26 Jul 2023 16:14:39 +0100 Subject: [PATCH] Only use result.sendError where supported by legacy media library `MediaLibraryServiceLegacyStub` handles various edge cases by calling `result.sendError(null)` with the intention to send back an error to the legacy browser [1]. `MediaBrowserServiceCompat` of the legacy media1 Compat library has an inner base class `Result` that has a default implementation of `onErrorSent` that throws an `UnsupportedOperationException` [2]. However, most anonymous inner classes for `Result` created in `MediaBrowserServiceCompat` do not override `onErrorSent` [3]. Hence Media3 must not call `sendError` in these cases. Instead we call `sendResult(null)` according to what the default implementation of the callbacks in `MediaBrowserServiceCompat` do ([4] as an example). Issue: androidx/media#78 Issue: androidx/media#334 [1] https://github.com/androidx/media/blob/release/libraries/session/src/main/java/androidx/media3/session/MediaLibraryServiceLegacyStub.java#L200 [2] https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:media/media/src/main/java/androidx/media/MediaBrowserServiceCompat.java;l=872 [3] https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:media/media/src/main/java/androidx/media/MediaBrowserServiceCompat.java;l=578-604?q=MediaBrowserServiceCompat&ss=androidx [4] https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:media/media/src/main/java/androidx/media/MediaBrowserServiceCompat.java;l=1395 PiperOrigin-RevId: 551210137 --- RELEASENOTES.md | 4 + .../MediaLibraryServiceLegacyStub.java | 22 ++--- ...wserCompatWithMediaLibraryServiceTest.java | 86 ++++++++++++++++++- .../session/MockMediaLibraryService.java | 18 ++-- 4 files changed, 108 insertions(+), 22 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index b093eaa66c..c94249bb9e 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -111,6 +111,10 @@ do this in `MediaSession.Callback.onConnect` by using an `AcceptedResultBuilder` to make sure the custom layout is available to the controller when connection completes. + * Fix cases where `MediaLibraryServiceLegacyStub` sent an error to a + `Result` that didn't support this which produced an + `UnsuportedOperationException` + ([#78](https://github.com/androidx/media/issues/78)). * UI: * Add a `Player.Listener` implementation for Wear OS devices that handles playback suppression due to diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaLibraryServiceLegacyStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaLibraryServiceLegacyStub.java index 6d1fb5e7a1..5bb8e0124a 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaLibraryServiceLegacyStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaLibraryServiceLegacyStub.java @@ -197,12 +197,12 @@ import java.util.concurrent.atomic.AtomicReference; @Nullable Bundle options) { @Nullable ControllerInfo controller = getCurrentController(); if (controller == null) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } if (TextUtils.isEmpty(parentId)) { Log.w(TAG, "onLoadChildren(): Ignoring empty parentId from " + controller); - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } result.detach(); @@ -212,7 +212,7 @@ import java.util.concurrent.atomic.AtomicReference; if (!getConnectedControllersManager() .isSessionCommandAvailable( controller, SessionCommand.COMMAND_CODE_LIBRARY_GET_CHILDREN)) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } if (options != null) { @@ -260,12 +260,12 @@ import java.util.concurrent.atomic.AtomicReference; public void onLoadItem(String itemId, Result result) { @Nullable ControllerInfo controller = getCurrentController(); if (controller == null) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } if (TextUtils.isEmpty(itemId)) { Log.w(TAG, "Ignoring empty itemId from " + controller); - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } result.detach(); @@ -275,7 +275,7 @@ import java.util.concurrent.atomic.AtomicReference; if (!getConnectedControllersManager() .isSessionCommandAvailable( controller, SessionCommand.COMMAND_CODE_LIBRARY_GET_ITEM)) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } ListenableFuture> future = @@ -291,12 +291,12 @@ import java.util.concurrent.atomic.AtomicReference; String query, @Nullable Bundle extras, Result> result) { @Nullable ControllerInfo controller = getCurrentController(); if (controller == null) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } if (TextUtils.isEmpty(query)) { Log.w(TAG, "Ignoring empty query from " + controller); - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } if (!(controller.getControllerCb() instanceof BrowserLegacyCb)) { @@ -308,7 +308,7 @@ import java.util.concurrent.atomic.AtomicReference; () -> { if (!getConnectedControllersManager() .isSessionCommandAvailable(controller, SessionCommand.COMMAND_CODE_LIBRARY_SEARCH)) { - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); return; } BrowserLegacyCb cb = (BrowserLegacyCb) checkStateNotNull(controller.getControllerCb()); @@ -389,7 +389,7 @@ import java.util.concurrent.atomic.AtomicReference; result.sendResult(mediaItem); } catch (CancellationException | ExecutionException | InterruptedException e) { Log.w(TAG, "Library operation failed", e); - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); } }, MoreExecutors.directExecutor()); @@ -408,7 +408,7 @@ import java.util.concurrent.atomic.AtomicReference; : MediaUtils.truncateListBySize(mediaItems, TRANSACTION_SIZE_LIMIT_IN_BYTES)); } catch (CancellationException | ExecutionException | InterruptedException e) { Log.w(TAG, "Library operation failed", e); - result.sendError(/* extras= */ null); + result.sendResult(/* result= */ null); } }, MoreExecutors.directExecutor()); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserCompatWithMediaLibraryServiceTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserCompatWithMediaLibraryServiceTest.java index 4e3ae96507..ba03a8b9e5 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserCompatWithMediaLibraryServiceTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserCompatWithMediaLibraryServiceTest.java @@ -54,6 +54,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.SEARCH_R import static androidx.media3.test.session.common.MediaBrowserConstants.SEARCH_RESULT_COUNT; import static androidx.media3.test.session.common.TestUtils.LONG_TIMEOUT_MS; import static androidx.media3.test.session.common.TestUtils.NO_RESPONSE_TIMEOUT_MS; +import static androidx.media3.test.session.common.TestUtils.SERVICE_CONNECTION_TIMEOUT_MS; import static androidx.media3.test.session.common.TestUtils.TIMEOUT_MS; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; @@ -210,6 +211,29 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); } + @Test + public void getItem_commandGetItemNotAvailable_reportsNull() throws Exception { + Bundle rootHints = new Bundle(); + rootHints.putInt( + MockMediaLibraryService.CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE, + SessionCommand.COMMAND_CODE_LIBRARY_GET_ITEM); + connectAndWait(rootHints); + CountDownLatch latch = new CountDownLatch(1); + List capturedMediaItems = new ArrayList<>(); + browserCompat.getItem( + MEDIA_ID_GET_BROWSABLE_ITEM, + new ItemCallback() { + @Override + public void onItemLoaded(MediaItem item) { + capturedMediaItems.add(item); + latch.countDown(); + } + }); + + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(capturedMediaItems).containsExactly((Object) null); + } + @Test public void getChildren() throws Exception { String testParentId = PARENT_ID; @@ -415,6 +439,34 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); } + @Test + public void getChildren_commandGetChildrenNotAvailable_reportsError() throws Exception { + Bundle rootHints = new Bundle(); + rootHints.putInt( + MockMediaLibraryService.CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE, + SessionCommand.COMMAND_CODE_LIBRARY_GET_CHILDREN); + handler.postAndSync( + () -> { + browserCompat = + new MediaBrowserCompat(context, getServiceComponent(), connectionCallback, rootHints); + }); + browserCompat.connect(); + assertThat(connectionCallback.connectedLatch.await(SERVICE_CONNECTION_TIMEOUT_MS, MILLISECONDS)) + .isTrue(); + CountDownLatch errorLatch = new CountDownLatch(1); + + browserCompat.subscribe( + PARENT_ID, + new MediaBrowserCompat.SubscriptionCallback() { + @Override + public void onError(String parentId) { + errorLatch.countDown(); + } + }); + + assertThat(errorLatch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + } + @Test public void search() throws Exception { String testQuery = SEARCH_QUERY; @@ -511,6 +563,35 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); } + @Test + public void search_commandSearchNotAvailable_reportsError() throws Exception { + String testQuery = SEARCH_QUERY; + int page = 4; + int pageSize = 10; + Bundle testExtras = new Bundle(); + testExtras.putString(testQuery, testQuery); + testExtras.putInt(MediaBrowserCompat.EXTRA_PAGE, page); + testExtras.putInt(MediaBrowserCompat.EXTRA_PAGE_SIZE, pageSize); + Bundle rootHints = new Bundle(); + rootHints.putInt( + MockMediaLibraryService.CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE, + SessionCommand.COMMAND_CODE_LIBRARY_SEARCH); + connectAndWait(rootHints); + CountDownLatch latch = new CountDownLatch(1); + + browserCompat.search( + testQuery, + testExtras, + new SearchCallback() { + @Override + public void onError(String query, Bundle extras) { + latch.countDown(); + } + }); + + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + } + @Test public void search_error() throws Exception { String testQuery = SEARCH_QUERY_ERROR; @@ -607,8 +688,9 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest @Test public void rootBrowserHints_searchNotSupported_reportsSearchNotSupported() throws Exception { Bundle connectionHints = new Bundle(); - connectionHints.putBoolean( - MockMediaLibraryService.CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE_LIBRARY_SEARCH, true); + connectionHints.putInt( + MockMediaLibraryService.CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE, + SessionCommand.COMMAND_CODE_LIBRARY_SEARCH); connectAndWait(connectionHints); boolean isSearchSupported = diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaLibraryService.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaLibraryService.java index 745182949b..e906b0ffe5 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaLibraryService.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaLibraryService.java @@ -89,12 +89,12 @@ public class MockMediaLibraryService extends MediaLibraryService { "CONNECTION_HINTS_CUSTOM_LIBRARY_ROOT"; /** - * Key used in connection hints to instruct the mock service to remove {@link - * SessionCommand#COMMAND_CODE_LIBRARY_SEARCH} from the available commands in {@link + * Key used in connection hints to instruct the mock service to remove a {@link SessionCommand} + * identified by its command code from the available commands in {@link * MediaSession.Callback#onConnect(MediaSession, ControllerInfo)}. */ - public static final String CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE_LIBRARY_SEARCH = - "CONNECTION_HINTS_KEY_REMOVE_SEARCH_SESSION_COMMAND"; + public static final String CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE = + "CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE"; private static final String TEST_IMAGE_PATH = "media/png/non-motion-photo-shortened.png"; @@ -198,11 +198,11 @@ public class MockMediaLibraryService extends MediaLibraryService { SessionCommands.Builder builder = connectionResult.availableSessionCommands.buildUpon(); builder.add(new SessionCommand(CUSTOM_ACTION, /* extras= */ Bundle.EMPTY)); builder.add(new SessionCommand(CUSTOM_ACTION_ASSERT_PARAMS, /* extras= */ Bundle.EMPTY)); - if (controller - .getConnectionHints() - .getBoolean( - CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE_LIBRARY_SEARCH, /* defaultValue= */ false)) { - builder.remove(SessionCommand.COMMAND_CODE_LIBRARY_SEARCH); + Bundle connectionHints = controller.getConnectionHints(); + int commandCodeToRemove = + connectionHints.getInt(CONNECTION_HINTS_KEY_REMOVE_COMMAND_CODE, /* defaultValue= */ -1); + if (commandCodeToRemove != -1) { + builder.remove(commandCodeToRemove); } return MediaSession.ConnectionResult.accept( /* availableSessionCommands= */ builder.build(),