From 6e41c538d34f31ef3d5cb3abeef5c879dd640367 Mon Sep 17 00:00:00 2001 From: Justin Weir Date: Mon, 11 Apr 2022 18:22:49 +0000 Subject: [PATCH 1/2] Remove logging and nullable return type from getCustomAction There is no need to check boundary conditions and log potential errors if no index is passed in. Nulls are not added to the resulting list, so there's no reason for the nullable return type. Bug: 224749799 Test: Cleanup only, so ran MediaDataManagerTest Change-Id: Ie1d7eb8ce72a62a94da838e5f5589ef4c46cad8c --- .../systemui/media/MediaDataManager.kt | 42 +++++++------------ 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index 0ad15facee66b..d77f4ead2efe3 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -173,10 +173,6 @@ class MediaDataManager( // Maximum number of actions allowed in expanded view @JvmField val MAX_NOTIFICATION_ACTIONS = MediaViewHolder.genericButtonIds.size - - /** Maximum number of [PlaybackState.CustomAction] buttons supported */ - @JvmField - val MAX_CUSTOM_ACTIONS = 4 } private val themeText = com.android.settingslib.Utils.getColorAttr(context, @@ -821,14 +817,11 @@ class MediaDataManager( val nextButton = getStandardAction(controller, state.actions, PlaybackState.ACTION_SKIP_TO_NEXT) - // Then, check for custom actions - val customActions = MutableList(MAX_CUSTOM_ACTIONS) { null } - var customCount = 0 - for (i in 0..(MAX_CUSTOM_ACTIONS - 1)) { - getCustomAction(state, packageName, controller, customCount)?.let { - customActions[customCount++] = it - } - } + // Then, create a way to build any custom actions that will be needed + val customActions = state.customActions.asSequence().filterNotNull().map { + getCustomAction(state, packageName, controller, it) + }.iterator() + fun nextCustomAction() = if (customActions.hasNext()) customActions.next() else null // Finally, assign the remaining button slots: play/pause A B C D // A = previous, else custom action (if not reserved) @@ -838,12 +831,11 @@ class MediaDataManager( MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_PREV) == true val reserveNext = controller.extras?.getBoolean( MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_NEXT) == true - var customIdx = 0 actions.prevOrCustom = if (prevButton != null) { prevButton } else if (!reservePrev) { - customActions[customIdx++] + nextCustomAction() } else { null } @@ -851,13 +843,13 @@ class MediaDataManager( actions.nextOrCustom = if (nextButton != null) { nextButton } else if (!reserveNext) { - customActions[customIdx++] + nextCustomAction() } else { null } - actions.custom0 = customActions[customIdx++] - actions.custom1 = customActions[customIdx++] + actions.custom0 = nextCustomAction() + actions.custom1 = nextCustomAction() } return actions } @@ -938,18 +930,12 @@ class MediaDataManager( state: PlaybackState, packageName: String, controller: MediaController, - index: Int - ): MediaAction? { - if (state.customActions.size <= index || state.customActions[index] == null) { - if (DEBUG) { Log.d(TAG, "not enough actions or action was null at $index") } - return null - } - - val it = state.customActions[index] + customAction: PlaybackState.CustomAction + ): MediaAction { return MediaAction( - Icon.createWithResource(packageName, it.icon).loadDrawable(context), - { controller.transportControls.sendCustomAction(it, it.extras) }, - it.name, + Icon.createWithResource(packageName, customAction.icon).loadDrawable(context), + { controller.transportControls.sendCustomAction(customAction, customAction.extras) }, + customAction.name, null ) } From 5304fa7896597de7a78803123decdfdfe81bfd93 Mon Sep 17 00:00:00 2001 From: Justin Weir Date: Tue, 12 Apr 2022 19:58:08 +0000 Subject: [PATCH 2/2] Allow null prev/next buttons to be INVISIBLE instead of GONE Added an overload method for setVisibleAndAlpha that allows callers to specify the value for null as INVISIBLE instead of GONE and called that instead for the prev/next buttons. Added booleans to MediaButton to allow it to say whether to reserve the space for the prev/next buttons when they are null. Made the MediaButton class immutable. Fixes: 224749799 Test: tested manually and added 2 tests to MediaControlPanelTest Change-Id: I5ccac017eccfb2dc2ecdd42a1e6517d4787c68b6 --- .../systemui/media/MediaControlPanel.java | 16 ++- .../com/android/systemui/media/MediaData.kt | 18 ++- .../systemui/media/MediaDataManager.kt | 127 +++++++++--------- .../systemui/media/MediaControlPanelTest.kt | 58 ++++++++ .../systemui/media/MediaDataManagerTest.kt | 3 + 5 files changed, 155 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 c956783110932..a86ed40306a0d 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -786,7 +786,14 @@ public class MediaControlPanel { scrubbingTimeViewsEnabled(semanticActions) && hideWhenScrubbing && mIsScrubbing; boolean visible = mediaAction != null && !shouldBeHiddenDueToScrubbing; - setVisibleAndAlpha(expandedSet, buttonId, visible); + int notVisibleValue; + if ((buttonId == R.id.actionPrev && semanticActions.getReservePrev()) + || (buttonId == R.id.actionNext && semanticActions.getReserveNext())) { + notVisibleValue = ConstraintSet.INVISIBLE; + } else { + notVisibleValue = ConstraintSet.GONE; + } + setVisibleAndAlpha(expandedSet, buttonId, visible, notVisibleValue); setVisibleAndAlpha(collapsedSet, buttonId, visible && showInCompact); } @@ -1191,7 +1198,12 @@ public class MediaControlPanel { } private void setVisibleAndAlpha(ConstraintSet set, int actionId, boolean visible) { - set.setVisibility(actionId, visible ? ConstraintSet.VISIBLE : ConstraintSet.GONE); + setVisibleAndAlpha(set, actionId, visible, ConstraintSet.GONE); + } + + private void setVisibleAndAlpha(ConstraintSet set, int actionId, boolean visible, + int notVisibleValue) { + set.setVisibility(actionId, visible ? ConstraintSet.VISIBLE : notVisibleValue); set.setAlpha(actionId, visible ? 1.0f : 0.0f); } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaData.kt b/packages/SystemUI/src/com/android/systemui/media/MediaData.kt index bc8cca55154d8..f6d531b5b9d69 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaData.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaData.kt @@ -149,23 +149,31 @@ data class MediaButton( /** * Play/pause button */ - var playOrPause: MediaAction? = null, + val playOrPause: MediaAction? = null, /** * Next button, or custom action */ - var nextOrCustom: MediaAction? = null, + val nextOrCustom: MediaAction? = null, /** * Previous button, or custom action */ - var prevOrCustom: MediaAction? = null, + val prevOrCustom: MediaAction? = null, /** * First custom action space */ - var custom0: MediaAction? = null, + val custom0: MediaAction? = null, /** * Second custom action space */ - var custom1: MediaAction? = null + val custom1: MediaAction? = null, + /** + * Whether to reserve the empty space when the nextOrCustom is null + */ + val reserveNext: Boolean = false, + /** + * Whether to reserve the empty space when the prevOrCustom is null + */ + val reservePrev: Boolean = false ) { fun getActionById(id: Int): MediaAction? { return when (id) { diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index d77f4ead2efe3..0d65514bddc29 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -791,67 +791,74 @@ class MediaDataManager( */ private fun createActionsFromState(packageName: String, controller: MediaController): MediaButton? { - val actions = MediaButton() - controller.playbackState?.let { state -> - // First, check for standard actions - actions.playOrPause = if (isConnectingState(state.state)) { - // Spinner needs to be animating to render anything. Start it here. - val drawable = context.getDrawable( - com.android.internal.R.drawable.progress_small_material) - (drawable as Animatable).start() - MediaAction( - drawable, - null, // no action to perform when clicked - context.getString(R.string.controls_media_button_connecting), - context.getDrawable(R.drawable.ic_media_connecting_container), - // Specify a rebind id to prevent the spinner from restarting on later binds. - com.android.internal.R.drawable.progress_small_material - ) - } else if (isPlayingState(state.state)) { - getStandardAction(controller, state.actions, PlaybackState.ACTION_PAUSE) - } else { - getStandardAction(controller, state.actions, PlaybackState.ACTION_PLAY) - } - val prevButton = getStandardAction(controller, state.actions, - PlaybackState.ACTION_SKIP_TO_PREVIOUS) - val nextButton = getStandardAction(controller, state.actions, - PlaybackState.ACTION_SKIP_TO_NEXT) - - // Then, create a way to build any custom actions that will be needed - val customActions = state.customActions.asSequence().filterNotNull().map { - getCustomAction(state, packageName, controller, it) - }.iterator() - fun nextCustomAction() = if (customActions.hasNext()) customActions.next() else null - - // Finally, assign the remaining button slots: play/pause A B C D - // A = previous, else custom action (if not reserved) - // B = next, else custom action (if not reserved) - // C and D are always custom actions - val reservePrev = controller.extras?.getBoolean( - MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_PREV) == true - val reserveNext = controller.extras?.getBoolean( - MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_NEXT) == true - - actions.prevOrCustom = if (prevButton != null) { - prevButton - } else if (!reservePrev) { - nextCustomAction() - } else { - null - } - - actions.nextOrCustom = if (nextButton != null) { - nextButton - } else if (!reserveNext) { - nextCustomAction() - } else { - null - } - - actions.custom0 = nextCustomAction() - actions.custom1 = nextCustomAction() + val state = controller.playbackState + if (state == null) { + return MediaButton() } - return actions + // First, check for} standard actions + val playOrPause = if (isConnectingState(state.state)) { + // Spinner needs to be animating to render anything. Start it here. + val drawable = context.getDrawable( + com.android.internal.R.drawable.progress_small_material) + (drawable as Animatable).start() + MediaAction( + drawable, + null, // no action to perform when clicked + context.getString(R.string.controls_media_button_connecting), + context.getDrawable(R.drawable.ic_media_connecting_container), + // Specify a rebind id to prevent the spinner from restarting on later binds. + com.android.internal.R.drawable.progress_small_material + ) + } else if (isPlayingState(state.state)) { + getStandardAction(controller, state.actions, PlaybackState.ACTION_PAUSE) + } else { + getStandardAction(controller, state.actions, PlaybackState.ACTION_PLAY) + } + val prevButton = getStandardAction(controller, state.actions, + PlaybackState.ACTION_SKIP_TO_PREVIOUS) + val nextButton = getStandardAction(controller, state.actions, + PlaybackState.ACTION_SKIP_TO_NEXT) + + // Then, create a way to build any custom actions that will be needed + val customActions = state.customActions.asSequence().filterNotNull().map { + getCustomAction(state, packageName, controller, it) + }.iterator() + fun nextCustomAction() = if (customActions.hasNext()) customActions.next() else null + + // Finally, assign the remaining button slots: play/pause A B C D + // A = previous, else custom action (if not reserved) + // B = next, else custom action (if not reserved) + // C and D are always custom actions + val reservePrev = controller.extras?.getBoolean( + MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_PREV) == true + val reserveNext = controller.extras?.getBoolean( + MediaConstants.SESSION_EXTRAS_KEY_SLOT_RESERVATION_SKIP_TO_NEXT) == true + + val prevOrCustom = if (prevButton != null) { + prevButton + } else if (!reservePrev) { + nextCustomAction() + } else { + null + } + + val nextOrCustom = if (nextButton != null) { + nextButton + } else if (!reserveNext) { + nextCustomAction() + } else { + null + } + + return MediaButton( + playOrPause, + nextOrCustom, + prevOrCustom, + nextCustomAction(), + nextCustomAction(), + reserveNext, + reservePrev + ) } /** 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 a58a28e3920b2..21fabeb8c5448 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt @@ -436,6 +436,64 @@ public class MediaControlPanelTest : SysuiTestCase() { verify(expandedSet).setVisibility(R.id.action4, ConstraintSet.GONE) } + @Test + fun bindSemanticActions_reservedPrev() { + val icon = context.getDrawable(android.R.drawable.ic_media_play) + val bg = context.getDrawable(R.drawable.qs_media_round_button_background) + + // Setup button state: no prev or next button and their slots reserved + val semanticActions = MediaButton( + playOrPause = MediaAction(icon, Runnable {}, "play", bg), + nextOrCustom = null, + prevOrCustom = null, + custom0 = MediaAction(icon, null, "custom 0", bg), + custom1 = MediaAction(icon, null, "custom 1", bg), + false, + true + ) + val state = mediaData.copy(semanticActions = semanticActions) + + player.attachPlayer(viewHolder) + player.bindPlayer(state, PACKAGE) + + assertThat(actionPrev.isEnabled()).isFalse() + assertThat(actionPrev.drawable).isNull() + verify(expandedSet).setVisibility(R.id.actionPrev, ConstraintSet.INVISIBLE) + + assertThat(actionNext.isEnabled()).isFalse() + assertThat(actionNext.drawable).isNull() + verify(expandedSet).setVisibility(R.id.actionNext, ConstraintSet.GONE) + } + + @Test + fun bindSemanticActions_reservedNext() { + val icon = context.getDrawable(android.R.drawable.ic_media_play) + val bg = context.getDrawable(R.drawable.qs_media_round_button_background) + + // Setup button state: no prev or next button and their slots reserved + val semanticActions = MediaButton( + playOrPause = MediaAction(icon, Runnable {}, "play", bg), + nextOrCustom = null, + prevOrCustom = null, + custom0 = MediaAction(icon, null, "custom 0", bg), + custom1 = MediaAction(icon, null, "custom 1", bg), + true, + false + ) + val state = mediaData.copy(semanticActions = semanticActions) + + player.attachPlayer(viewHolder) + player.bindPlayer(state, PACKAGE) + + assertThat(actionPrev.isEnabled()).isFalse() + assertThat(actionPrev.drawable).isNull() + verify(expandedSet).setVisibility(R.id.actionPrev, ConstraintSet.GONE) + + assertThat(actionNext.isEnabled()).isFalse() + assertThat(actionNext.drawable).isNull() + verify(expandedSet).setVisibility(R.id.actionNext, ConstraintSet.INVISIBLE) + } + @Test fun bind_seekBarDisabled_seekBarVisibilityIsSetToInvisible() { whenever(seekBarViewModel.getEnabled()).thenReturn(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 858249960a6ed..7ec31a7ae8299 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt @@ -838,6 +838,9 @@ class MediaDataManagerTest : SysuiTestCase() { assertThat(actions.custom1).isNotNull() assertThat(actions.custom1!!.contentDescription).isEqualTo(customDesc[1]) + + assertThat(actions.reserveNext).isTrue() + assertThat(actions.reservePrev).isTrue() } @Test