code review fixes:

Close the listener race window by re-snapshotting tracks inside the DisposableEffect, hoist the openDialog remember above the early returns so the hook count stays stable when the player loses its video group, return ImmutableList from buildQualityChoices for Compose stability, and remember the TextButton ButtonColors copy. Consolidate VideoQualitySheet into VideoQualityButton using the existing PlaybackSpeedPopUpButton Popup pattern. Derives selection state from tracks instead of duplicating it, drops the dead QualityOption.Auto case, gates listener-driven UI behind early returns, and reuses the existing call_settings_video_quality string. Net -168 lines.
2026-04-14 10:11:41 +02:00
parent 4fc739ad7e
commit 606d976b57
5 changed files with 144 additions and 308 deletions
@@ -31,7 +31,6 @@ import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.tooling.preview.Preview
-import androidx.media3.common.Player
 import com.google.accompanist.permissions.ExperimentalPermissionsApi
 import com.vitorpamplona.amethyst.commons.richtext.MediaUrlVideo
 import com.vitorpamplona.amethyst.service.playback.composable.DEFAULT_MUTED_SETTING
@@ -85,7 +84,6 @@ fun RenderTopButtons(

    RenderTopButtons(
        mediaData = mediaData,
-        player = controllerState.controller,
        controllerVisible = controllerVisible,
        startingMuteState = controllerState.controller.volume < 0.001,
        isLive = isLive,
@@ -105,6 +103,12 @@ fun RenderTopButtons(
                    it()
                }
            },
+        qualityButton = {
+            VideoQualityButton(
+                player = controllerState.controller,
+                controllerVisible = controllerVisible,
+            )
+        },
        modifier = modifier,
        accountViewModel = accountViewModel,
    )
@@ -114,7 +118,6 @@ fun RenderTopButtons(
@Composable
 fun RenderTopButtons(
    mediaData: MediaItemData,
-    player: Player? = null,
    controllerVisible: MutableState<Boolean>,
    startingMuteState: Boolean,
    isLive: Boolean,
@@ -124,6 +127,7 @@ fun RenderTopButtons(
    onZoomClick: (() -> Unit)?,
    modifier: Modifier,
    accountViewModel: AccountViewModel,
+    qualityButton: @Composable () -> Unit = {},
 ) {
    val shareDialogVisible = remember { mutableStateOf(false) }
    val saveAction =
@@ -145,12 +149,7 @@ fun RenderTopButtons(
            toggle = onMuteClick,
        )

-        if (player != null) {
-            VideoQualityButton(
-                player = player,
-                controllerVisible = controllerVisible,
-            )
-        }
+        qualityButton()

        Box {
            AnimatedOverflowMenuButton(
@@ -23,19 +23,12 @@ package com.vitorpamplona.amethyst.service.playback.composable.controls
 import androidx.media3.common.C
 import androidx.media3.common.Tracks

-/**
- * Checks if the current tracks contain multiple video renditions (HLS/DASH adaptive streams).
- * Returns false for single-rendition MP4s to avoid showing a useless quality menu.
- */
-fun hasMultipleRenditions(tracks: Tracks): Boolean {
-    val videoGroup = getVideoTrackGroup(tracks) ?: return false
-    return videoGroup.length > 1
-}
+fun getVideoTrackGroup(tracks: Tracks): Tracks.Group? = tracks.groups.firstOrNull { it.type == C.TRACK_TYPE_VIDEO && it.length > 0 }

-/**
- * Returns the first video track group from the tracks, or null if none exists.
- */
-fun getVideoTrackGroup(tracks: Tracks): Tracks.Group? =
-    tracks.groups.firstOrNull { group ->
-        group.type == C.TRACK_TYPE_VIDEO && group.length > 0
+fun getCurrentPlayingHeight(tracks: Tracks): Int? {
+    val group = getVideoTrackGroup(tracks) ?: return null
+    for (i in 0 until group.length) {
+        if (group.isTrackSelected(i)) return group.getTrackFormat(i).height
    }
+    return null
+}
@@ -24,14 +24,19 @@ import androidx.compose.animation.AnimatedVisibility
 import androidx.compose.animation.fadeIn
 import androidx.compose.animation.fadeOut
 import androidx.compose.foundation.background
+import androidx.compose.foundation.layout.Arrangement
 import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.foundation.shape.CircleShape
 import androidx.compose.material.icons.Icons
 import androidx.compose.material.icons.filled.Settings
+import androidx.compose.material3.ButtonDefaults
 import androidx.compose.material3.Icon
 import androidx.compose.material3.IconButton
 import androidx.compose.material3.MaterialTheme
+import androidx.compose.material3.Text
+import androidx.compose.material3.TextButton
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.DisposableEffect
 import androidx.compose.runtime.MutableState
@@ -42,28 +47,33 @@ import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.clip
+import androidx.compose.ui.text.font.FontWeight
+import androidx.compose.ui.window.Popup
+import androidx.compose.ui.window.PopupProperties
+import androidx.media3.common.C
 import androidx.media3.common.Player
+import androidx.media3.common.TrackSelectionOverride
 import androidx.media3.common.Tracks
 import com.vitorpamplona.amethyst.R
 import com.vitorpamplona.amethyst.ui.stringRes
 import com.vitorpamplona.amethyst.ui.theme.PinBottomIconSize
 import com.vitorpamplona.amethyst.ui.theme.Size20Modifier
 import com.vitorpamplona.amethyst.ui.theme.Size50Modifier
+import kotlinx.collections.immutable.ImmutableList
+import kotlinx.collections.immutable.toImmutableList
+import java.util.Locale

-/**
- * A gear icon button that opens the video quality picker.
- * Only visible when the video has multiple renditions (HLS/DASH adaptive streams).
- */
@Composable
 fun VideoQualityButton(
    player: Player,
    controllerVisible: MutableState<Boolean>,
    modifier: Modifier = Modifier,
 ) {
-    var tracks by remember { mutableStateOf(player.currentTracks) }
-    var showSheet by remember { mutableStateOf(false) }
+    var tracks by remember(player) { mutableStateOf(player.currentTracks) }
+    var openDialog by remember { mutableStateOf(false) }

    DisposableEffect(player) {
+        tracks = player.currentTracks
        val listener =
            object : Player.Listener {
                override fun onTracksChanged(newTracks: Tracks) {
@@ -74,10 +84,11 @@ fun VideoQualityButton(
        onDispose { player.removeListener(listener) }
    }

-    val hasMultiple = hasMultipleRenditions(tracks)
+    val videoGroup = getVideoTrackGroup(tracks) ?: return
+    if (videoGroup.length <= 1) return

    AnimatedVisibility(
-        visible = controllerVisible.value && hasMultiple,
+        visible = controllerVisible.value,
        modifier = modifier,
        enter = remember { fadeIn() },
        exit = remember { fadeOut() },
@@ -92,12 +103,12 @@ fun VideoQualityButton(
            )

            IconButton(
-                onClick = { showSheet = true },
+                onClick = { openDialog = true },
                modifier = Size50Modifier,
            ) {
                Icon(
                    imageVector = Icons.Default.Settings,
-                    contentDescription = stringRes(id = R.string.video_quality),
+                    contentDescription = stringRes(id = R.string.call_settings_video_quality),
                    tint = MaterialTheme.colorScheme.onBackground,
                    modifier = Size20Modifier,
                )
@@ -105,10 +116,112 @@ fun VideoQualityButton(
        }
    }

-    if (showSheet) {
-        VideoQualitySheet(
-            player = player,
-            onDismiss = { showSheet = false },
-        )
+    if (openDialog) {
+        Popup(
+            alignment = Alignment.BottomCenter,
+            onDismissRequest = { openDialog = false },
+            properties = PopupProperties(focusable = true),
+        ) {
+            VideoQualityChoices(
+                videoGroup = videoGroup,
+                currentHeight = getCurrentPlayingHeight(tracks),
+                isAuto = !hasVideoOverride(player),
+                onSelectAuto = {
+                    clearVideoOverride(player)
+                    openDialog = false
+                },
+                onSelectTrack = { trackIndex ->
+                    selectVideoTrack(player, videoGroup, trackIndex)
+                    openDialog = false
+                },
+            )
+        }
    }
 }
+
+@Composable
+private fun VideoQualityChoices(
+    videoGroup: Tracks.Group,
+    currentHeight: Int?,
+    isAuto: Boolean,
+    onSelectAuto: () -> Unit,
+    onSelectTrack: (Int) -> Unit,
+) {
+    val baseColors = ButtonDefaults.textButtonColors()
+    val contentColor = MaterialTheme.colorScheme.onBackground
+    val colors =
+        remember(baseColors, contentColor) {
+            baseColors.copy(contentColor = contentColor)
+        }
+
+    val choices: ImmutableList<QualityChoice> = remember(videoGroup) { buildQualityChoices(videoGroup) }
+
+    Column(
+        modifier = Modifier.background(MaterialTheme.colorScheme.background),
+        verticalArrangement = Arrangement.Center,
+        horizontalAlignment = Alignment.CenterHorizontally,
+    ) {
+        TextButton(colors = colors, onClick = onSelectAuto) {
+            val suffix = currentHeight?.let { " (${it}p)" } ?: ""
+            Text(
+                stringRes(R.string.video_quality_auto) + suffix,
+                fontWeight = if (isAuto) FontWeight(1000) else FontWeight(400),
+            )
+        }
+
+        choices.forEach { choice ->
+            TextButton(colors = colors, onClick = { onSelectTrack(choice.trackIndex) }) {
+                Text(
+                    "${choice.height}p  ${formatBitrate(choice.bitrate)}",
+                    fontWeight = if (!isAuto && currentHeight == choice.height) FontWeight(1000) else FontWeight(400),
+                )
+            }
+        }
+    }
+}
+
+private data class QualityChoice(
+    val trackIndex: Int,
+    val height: Int,
+    val bitrate: Int,
+)
+
+private fun buildQualityChoices(group: Tracks.Group): ImmutableList<QualityChoice> {
+    val choices = mutableListOf<QualityChoice>()
+    for (i in 0 until group.length) {
+        val format = group.getTrackFormat(i)
+        if (format.height > 0) {
+            choices.add(QualityChoice(i, format.height, format.bitrate))
+        }
+    }
+    return choices.sortedByDescending { it.height }.toImmutableList()
+}
+
+private fun formatBitrate(bitrate: Int): String =
+    when {
+        bitrate <= 0 -> ""
+        bitrate >= 1_000_000 -> String.format(Locale.US, "%.1f Mbps", bitrate / 1_000_000.0)
+        else -> String.format(Locale.US, "%.0f kbps", bitrate / 1_000.0)
+    }
+
+private fun hasVideoOverride(player: Player): Boolean = player.trackSelectionParameters.overrides.any { (key, _) -> key.type == C.TRACK_TYPE_VIDEO }
+
+private fun clearVideoOverride(player: Player) {
+    player.trackSelectionParameters =
+        player.trackSelectionParameters
+            .buildUpon()
+            .clearOverridesOfType(C.TRACK_TYPE_VIDEO)
+            .build()
+}
+
+private fun selectVideoTrack(
+    player: Player,
+    group: Tracks.Group,
+    trackIndex: Int,
+) {
+    player.trackSelectionParameters =
+        player.trackSelectionParameters
+            .buildUpon()
+            .setOverrideForType(TrackSelectionOverride(group.mediaTrackGroup, trackIndex))
+            .build()
+}
@@ -1,269 +0,0 @@
-/*
- * Copyright (c) 2025 Vitor Pamplona
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy of
- * this software and associated documentation files (the "Software"), to deal in
- * the Software without restriction, including without limitation the rights to use,
- * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
- * Software, and to permit persons to whom the Software is furnished to do so,
- * subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in all
- * copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
- * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
- * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
- * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
- * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
- */
-package com.vitorpamplona.amethyst.service.playback.composable.controls
-
-import androidx.compose.foundation.clickable
-import androidx.compose.foundation.layout.Arrangement
-import androidx.compose.foundation.layout.Column
-import androidx.compose.foundation.layout.Row
-import androidx.compose.foundation.layout.Spacer
-import androidx.compose.foundation.layout.fillMaxWidth
-import androidx.compose.foundation.layout.height
-import androidx.compose.foundation.layout.padding
-import androidx.compose.foundation.layout.size
-import androidx.compose.foundation.layout.width
-import androidx.compose.material.icons.Icons
-import androidx.compose.material.icons.filled.Check
-import androidx.compose.material3.ExperimentalMaterial3Api
-import androidx.compose.material3.Icon
-import androidx.compose.material3.MaterialTheme
-import androidx.compose.material3.ModalBottomSheet
-import androidx.compose.material3.Text
-import androidx.compose.material3.rememberModalBottomSheetState
-import androidx.compose.runtime.Composable
-import androidx.compose.runtime.DisposableEffect
-import androidx.compose.runtime.getValue
-import androidx.compose.runtime.mutableStateOf
-import androidx.compose.runtime.remember
-import androidx.compose.runtime.setValue
-import androidx.compose.ui.Alignment
-import androidx.compose.ui.Modifier
-import androidx.compose.ui.unit.dp
-import androidx.media3.common.C
-import androidx.media3.common.Player
-import androidx.media3.common.TrackSelectionOverride
-import androidx.media3.common.Tracks
-
-/**
- * Represents a quality option in the picker.
- */
-sealed class QualityOption {
-    data object Auto : QualityOption()
-
-    data class Specific(
-        val trackIndex: Int,
-        val height: Int,
-        val bitrate: Int,
-        val codecs: String?,
-    ) : QualityOption()
-}
-
-/**
- * Modal bottom sheet for selecting video quality (rendition).
- */
-@OptIn(ExperimentalMaterial3Api::class)
-@Composable
-fun VideoQualitySheet(
-    player: Player,
-    onDismiss: () -> Unit,
-) {
-    val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
-
-    var tracks by remember { mutableStateOf(player.currentTracks) }
-    var isAutoSelected by remember { mutableStateOf(!hasVideoOverride(player)) }
-    var currentlyPlayingHeight by remember { mutableStateOf(getCurrentPlayingHeight(player)) }
-
-    DisposableEffect(player) {
-        val listener =
-            object : Player.Listener {
-                override fun onTracksChanged(newTracks: Tracks) {
-                    tracks = newTracks
-                    currentlyPlayingHeight = getCurrentPlayingHeight(player)
-                }
-            }
-        player.addListener(listener)
-        onDispose { player.removeListener(listener) }
-    }
-
-    val videoGroup = getVideoTrackGroup(tracks)
-    if (videoGroup == null) {
-        onDismiss()
-        return
-    }
-
-    val options = buildQualityOptions(videoGroup)
-
-    ModalBottomSheet(
-        onDismissRequest = onDismiss,
-        sheetState = sheetState,
-        containerColor = MaterialTheme.colorScheme.surface,
-        contentColor = MaterialTheme.colorScheme.onSurface,
-    ) {
-        Column(
-            modifier =
-                Modifier
-                    .fillMaxWidth()
-                    .padding(horizontal = 16.dp)
-                    .padding(bottom = 32.dp),
-        ) {
-            Text(
-                text = "Quality",
-                style = MaterialTheme.typography.titleLarge,
-                color = MaterialTheme.colorScheme.onSurface,
-            )
-
-            Spacer(Modifier.height(16.dp))
-
-            // Auto option
-            QualityRow(
-                label = "Auto",
-                sublabel = currentlyPlayingHeight?.let { "${it}p" },
-                isSelected = isAutoSelected,
-                onClick = {
-                    clearVideoOverride(player)
-                    isAutoSelected = true
-                    onDismiss()
-                },
-            )
-
-            // Specific quality options
-            options.forEach { option ->
-                if (option is QualityOption.Specific) {
-                    QualityRow(
-                        label = "${option.height}p",
-                        sublabel = formatBitrate(option.bitrate) + (option.codecs?.let { "  $it" } ?: ""),
-                        isSelected = !isAutoSelected && currentlyPlayingHeight == option.height,
-                        onClick = {
-                            selectVideoTrack(player, videoGroup, option.trackIndex)
-                            isAutoSelected = false
-                            onDismiss()
-                        },
-                    )
-                }
-            }
-        }
-    }
-}
-
-@Composable
-private fun QualityRow(
-    label: String,
-    sublabel: String?,
-    isSelected: Boolean,
-    onClick: () -> Unit,
-) {
-    Row(
-        modifier =
-            Modifier
-                .fillMaxWidth()
-                .clickable(onClick = onClick)
-                .padding(vertical = 12.dp),
-        verticalAlignment = Alignment.CenterVertically,
-        horizontalArrangement = Arrangement.Start,
-    ) {
-        if (isSelected) {
-            Icon(
-                imageVector = Icons.Default.Check,
-                contentDescription = null,
-                tint = MaterialTheme.colorScheme.primary,
-                modifier = Modifier.size(24.dp),
-            )
-        } else {
-            Spacer(Modifier.size(24.dp))
-        }
-
-        Spacer(Modifier.width(16.dp))
-
-        Column {
-            Text(
-                text = label,
-                style = MaterialTheme.typography.bodyLarge,
-                color = MaterialTheme.colorScheme.onSurface,
-            )
-            if (sublabel != null) {
-                Text(
-                    text = sublabel,
-                    style = MaterialTheme.typography.bodySmall,
-                    color = MaterialTheme.colorScheme.onSurfaceVariant,
-                )
-            }
-        }
-    }
-}
-
-private fun buildQualityOptions(group: Tracks.Group): List<QualityOption> {
-    val options = mutableListOf<QualityOption>()
-
-    for (i in 0 until group.length) {
-        val format = group.getTrackFormat(i)
-        if (format.height > 0) {
-            options.add(
-                QualityOption.Specific(
-                    trackIndex = i,
-                    height = format.height,
-                    bitrate = format.bitrate,
-                    codecs = format.codecs,
-                ),
-            )
-        }
-    }
-
-    // Sort by height descending
-    return options.sortedByDescending { (it as QualityOption.Specific).height }
-}
-
-private fun formatBitrate(bitrate: Int): String =
-    if (bitrate > 0) {
-        val mbps = bitrate / 1_000_000.0
-        if (mbps >= 1.0) {
-            String.format("%.1f Mbps", mbps)
-        } else {
-            String.format("%.0f kbps", bitrate / 1000.0)
-        }
-    } else {
-        ""
-    }
-
-private fun hasVideoOverride(player: Player): Boolean =
-    player.trackSelectionParameters.overrides.any { (key, _) ->
-        key.type == C.TRACK_TYPE_VIDEO
-    }
-
-private fun getCurrentPlayingHeight(player: Player): Int? {
-    val videoGroup = getVideoTrackGroup(player.currentTracks) ?: return null
-    for (i in 0 until videoGroup.length) {
-        if (videoGroup.isTrackSelected(i)) {
-            return videoGroup.getTrackFormat(i).height
-        }
-    }
-    return null
-}
-
-private fun clearVideoOverride(player: Player) {
-    player.trackSelectionParameters =
-        player.trackSelectionParameters
-            .buildUpon()
-            .clearOverridesOfType(C.TRACK_TYPE_VIDEO)
-            .build()
-}
-
-private fun selectVideoTrack(
-    player: Player,
-    group: Tracks.Group,
-    trackIndex: Int,
-) {
-    player.trackSelectionParameters =
-        player.trackSelectionParameters
-            .buildUpon()
-            .setOverrideForType(
-                TrackSelectionOverride(group.mediaTrackGroup, trackIndex),
-            ).build()
-}
@@ -2076,7 +2076,7 @@
    <string name="profile_actions_dialog_title">Profile Actions</string>
    <string name="media_actions_dialog_title">Media Actions</string>
    <string name="playback_actions_dialog_title">Playback</string>
-    <string name="video_quality">Video quality</string>
+    <string name="video_quality_auto">Auto</string>
    <string name="pack_actions_dialog_title">Pack Actions</string>
    <string name="list_actions_dialog_title">List Actions</string>
    <string name="bookmark_item_actions_dialog_title">Bookmark Actions</string>