Code review fixes:

Skip-forward clamps against unknown duration
Share dropdown is no longer anchored to the overflow button
simplify: keep skip amount fixed at 10
This commit is contained in:
davotoula
2026-02-26 18:22:28 +00:00
parent ce76f8fb27
commit 3822197830
4 changed files with 56 additions and 67 deletions
@@ -48,7 +48,8 @@ import com.vitorpamplona.amethyst.service.playback.composable.wavefront.Waveform
import com.vitorpamplona.amethyst.service.playback.diskCache.isLiveStreaming import com.vitorpamplona.amethyst.service.playback.diskCache.isLiveStreaming
import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
internal fun computeSkipSeconds(durationMs: Long): Int = if (durationMs in 1..30000) 5 else 10 internal const val SKIP_SECONDS = 10
internal const val SKIP_MILLIS = SKIP_SECONDS * 1000L
private fun getVideoSizeDp(player: Player): Size? { private fun getVideoSizeDp(player: Player): Size? {
var videoSize = Size(player.videoSize.width.toFloat(), player.videoSize.height.toFloat()) var videoSize = Size(player.videoSize.width.toFloat(), player.videoSize.height.toFloat())
@@ -90,19 +91,18 @@ fun RenderVideoPlayer(
onTap = { controllerVisible.value = !controllerVisible.value }, onTap = { controllerVisible.value = !controllerVisible.value },
onDoubleTap = { offset -> onDoubleTap = { offset ->
if (!isLive) { if (!isLive) {
val skipSeconds = computeSkipSeconds(controllerState.controller.duration)
val isLeftSide = offset.x < containerSize.value.width / 2 val isLeftSide = offset.x < containerSize.value.width / 2
if (isLeftSide) { if (isLeftSide) {
val newPosition = val newPosition =
(controllerState.controller.currentPosition - skipSeconds * 1000) (controllerState.controller.currentPosition - SKIP_MILLIS)
.coerceAtLeast(0) .coerceAtLeast(0)
controllerState.controller.seekTo(newPosition) controllerState.controller.seekTo(newPosition)
} else { } else {
val duration = controllerState.controller.duration val duration = controllerState.controller.duration
val newPosition = val newPosition = controllerState.controller.currentPosition + SKIP_MILLIS
(controllerState.controller.currentPosition + skipSeconds * 1000) controllerState.controller.seekTo(
.coerceAtMost(duration) if (duration > 0) newPosition.coerceAtMost(duration) else newPosition,
controllerState.controller.seekTo(newPosition) )
} }
} }
}, },
@@ -143,7 +143,6 @@ fun RenderVideoPlayer(
controllerVisible = controllerVisible, controllerVisible = controllerVisible,
modifier = Modifier.align(Alignment.Center), modifier = Modifier.align(Alignment.Center),
isLiveStream = isLive, isLiveStream = isLive,
videoDurationMs = controllerState.controller.duration,
) )
RenderAnimatedBottomInfo(controllerState, controllerVisible, Modifier.align(Alignment.BottomCenter)) RenderAnimatedBottomInfo(controllerState, controllerVisible, Modifier.align(Alignment.BottomCenter))
@@ -32,7 +32,7 @@ import androidx.compose.ui.unit.dp
import androidx.media3.common.util.UnstableApi import androidx.media3.common.util.UnstableApi
import androidx.media3.ui.compose.state.rememberPlayPauseButtonState import androidx.media3.ui.compose.state.rememberPlayPauseButtonState
import com.vitorpamplona.amethyst.service.playback.composable.MediaControllerState import com.vitorpamplona.amethyst.service.playback.composable.MediaControllerState
import com.vitorpamplona.amethyst.service.playback.composable.computeSkipSeconds import com.vitorpamplona.amethyst.service.playback.composable.SKIP_MILLIS
import kotlinx.coroutines.delay import kotlinx.coroutines.delay
@OptIn(UnstableApi::class) @OptIn(UnstableApi::class)
@@ -42,10 +42,8 @@ fun RenderCenterButtons(
controllerVisible: MutableState<Boolean>, controllerVisible: MutableState<Boolean>,
modifier: Modifier, modifier: Modifier,
isLiveStream: Boolean = false, isLiveStream: Boolean = false,
videoDurationMs: Long = 0L,
) { ) {
val state = rememberPlayPauseButtonState(controllerState.controller) val state = rememberPlayPauseButtonState(controllerState.controller)
val skipSeconds = computeSkipSeconds(videoDurationMs)
Row( Row(
modifier = modifier, modifier = modifier,
@@ -56,9 +54,8 @@ fun RenderCenterButtons(
AnimatedSkipButton( AnimatedSkipButton(
controllerVisible = controllerVisible, controllerVisible = controllerVisible,
isForward = false, isForward = false,
skipSeconds = skipSeconds,
) { ) {
val newPosition = (controllerState.controller.currentPosition - skipSeconds * 1000).coerceAtLeast(0) val newPosition = (controllerState.controller.currentPosition - SKIP_MILLIS).coerceAtLeast(0)
controllerState.controller.seekTo(newPosition) controllerState.controller.seekTo(newPosition)
} }
} }
@@ -71,11 +68,10 @@ fun RenderCenterButtons(
AnimatedSkipButton( AnimatedSkipButton(
controllerVisible = controllerVisible, controllerVisible = controllerVisible,
isForward = true, isForward = true,
skipSeconds = skipSeconds,
) { ) {
val duration = controllerState.controller.duration val duration = controllerState.controller.duration
val newPosition = (controllerState.controller.currentPosition + skipSeconds * 1000).coerceAtMost(duration) val newPosition = controllerState.controller.currentPosition + SKIP_MILLIS
controllerState.controller.seekTo(newPosition) controllerState.controller.seekTo(if (duration > 0) newPosition.coerceAtMost(duration) else newPosition)
} }
} }
} }
@@ -25,6 +25,7 @@ import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.Row
import androidx.compose.runtime.Composable import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MutableState import androidx.compose.runtime.MutableState
import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember import androidx.compose.runtime.remember
@@ -123,6 +124,10 @@ fun RenderTopButtons(
val context = LocalContext.current val context = LocalContext.current
val shareDialogVisible = remember { mutableStateOf(false) } val shareDialogVisible = remember { mutableStateOf(false) }
LaunchedEffect(controllerVisible.value) {
if (!controllerVisible.value) shareDialogVisible.value = false
}
Row(modifier) { Row(modifier) {
if (onZoomClick != null) { if (onZoomClick != null) {
FullScreenButton( FullScreenButton(
@@ -137,39 +142,41 @@ fun RenderTopButtons(
toggle = onMuteClick, toggle = onMuteClick,
) )
AnimatedOverflowMenuButton( Box {
controllerVisible = controllerVisible, AnimatedOverflowMenuButton(
showShare = true, controllerVisible = controllerVisible,
showSave = !isLive, showShare = true,
showPip = pipSupported, showSave = !isLive,
onShareClick = { shareDialogVisible.value = true }, showPip = pipSupported,
onSaveClick = { onShareClick = { shareDialogVisible.value = true },
accountViewModel.saveMediaToGallery(mediaData.videoUri, mediaData.mimeType, context) onSaveClick = {
}, accountViewModel.saveMediaToGallery(mediaData.videoUri, mediaData.mimeType, context)
onPipClick = onPictureInPictureClick, },
) onPipClick = onPictureInPictureClick,
} )
if (shareDialogVisible.value) { if (shareDialogVisible.value) {
ShareMediaAction( ShareMediaAction(
popupExpanded = shareDialogVisible, popupExpanded = shareDialogVisible,
videoUri = mediaData.videoUri, videoUri = mediaData.videoUri,
postNostrUri = mediaData.callbackUri, postNostrUri = mediaData.callbackUri,
blurhash = null, blurhash = null,
dim = null, dim = null,
hash = null, hash = null,
mimeType = mediaData.mimeType,
onDismiss = { shareDialogVisible.value = false },
content =
MediaUrlVideo(
url = mediaData.videoUri,
mimeType = mediaData.mimeType, mimeType = mediaData.mimeType,
artworkUri = mediaData.artworkUri, onDismiss = { shareDialogVisible.value = false },
authorName = mediaData.authorName, content =
description = mediaData.title, MediaUrlVideo(
uri = mediaData.callbackUri, url = mediaData.videoUri,
), mimeType = mediaData.mimeType,
accountViewModel = accountViewModel, artworkUri = mediaData.artworkUri,
) authorName = mediaData.authorName,
description = mediaData.title,
uri = mediaData.callbackUri,
),
accountViewModel = accountViewModel,
)
}
}
} }
} }
@@ -38,6 +38,7 @@ import androidx.compose.ui.graphics.Color
import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.dp
import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.R
import com.vitorpamplona.amethyst.service.playback.composable.SKIP_SECONDS
import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.stringRes
import com.vitorpamplona.amethyst.ui.theme.BitcoinOrange import com.vitorpamplona.amethyst.ui.theme.BitcoinOrange
import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn
@@ -50,11 +51,7 @@ private val FadeOut = fadeOut()
fun SkipBackButtonPreview() { fun SkipBackButtonPreview() {
ThemeComparisonColumn { ThemeComparisonColumn {
Box(Modifier.background(BitcoinOrange)) { Box(Modifier.background(BitcoinOrange)) {
SkipButton( SkipButton(isForward = false, onClick = {})
isForward = false,
skipSeconds = 10,
onClick = {},
)
} }
} }
} }
@@ -64,11 +61,7 @@ fun SkipBackButtonPreview() {
fun SkipForwardButtonPreview() { fun SkipForwardButtonPreview() {
ThemeComparisonColumn { ThemeComparisonColumn {
Box(Modifier.background(BitcoinOrange)) { Box(Modifier.background(BitcoinOrange)) {
SkipButton( SkipButton(isForward = true, onClick = {})
isForward = true,
skipSeconds = 10,
onClick = {},
)
} }
} }
} }
@@ -77,7 +70,6 @@ fun SkipForwardButtonPreview() {
fun AnimatedSkipButton( fun AnimatedSkipButton(
controllerVisible: State<Boolean>, controllerVisible: State<Boolean>,
isForward: Boolean, isForward: Boolean,
skipSeconds: Int = 10,
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
onClick: () -> Unit, onClick: () -> Unit,
) { ) {
@@ -87,18 +79,13 @@ fun AnimatedSkipButton(
enter = FadeIn, enter = FadeIn,
exit = FadeOut, exit = FadeOut,
) { ) {
SkipButton( SkipButton(isForward = isForward, onClick = onClick)
isForward = isForward,
skipSeconds = skipSeconds,
onClick = onClick,
)
} }
} }
@Composable @Composable
fun SkipButton( fun SkipButton(
isForward: Boolean, isForward: Boolean,
skipSeconds: Int = 10,
onClick: () -> Unit, onClick: () -> Unit,
) { ) {
IconButton( IconButton(
@@ -109,9 +96,9 @@ fun SkipButton(
imageVector = if (isForward) Icons.Default.Forward10 else Icons.Default.Replay10, imageVector = if (isForward) Icons.Default.Forward10 else Icons.Default.Replay10,
contentDescription = contentDescription =
if (isForward) { if (isForward) {
stringRes(R.string.skip_forward, skipSeconds) stringRes(R.string.skip_forward, SKIP_SECONDS)
} else { } else {
stringRes(R.string.skip_back, skipSeconds) stringRes(R.string.skip_back, SKIP_SECONDS)
}, },
tint = Color.White, tint = Color.White,
modifier = Modifier.size(32.dp), modifier = Modifier.size(32.dp),