From 3822197830faf5537088a1af6f2d327420ba42ab Mon Sep 17 00:00:00 2001 From: davotoula Date: Thu, 26 Feb 2026 18:22:28 +0000 Subject: [PATCH] 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 --- .../playback/composable/RenderVideoPlayer.kt | 15 ++-- .../controls/RenderCenterButtons.kt | 12 ++-- .../composable/controls/RenderTopButtons.kt | 71 ++++++++++--------- .../composable/controls/SkipButton.kt | 25 ++----- 4 files changed, 56 insertions(+), 67 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/RenderVideoPlayer.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/RenderVideoPlayer.kt index 7476629e3..be94e52bd 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/RenderVideoPlayer.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/RenderVideoPlayer.kt @@ -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.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? { var videoSize = Size(player.videoSize.width.toFloat(), player.videoSize.height.toFloat()) @@ -90,19 +91,18 @@ fun RenderVideoPlayer( onTap = { controllerVisible.value = !controllerVisible.value }, onDoubleTap = { offset -> if (!isLive) { - val skipSeconds = computeSkipSeconds(controllerState.controller.duration) val isLeftSide = offset.x < containerSize.value.width / 2 if (isLeftSide) { val newPosition = - (controllerState.controller.currentPosition - skipSeconds * 1000) + (controllerState.controller.currentPosition - SKIP_MILLIS) .coerceAtLeast(0) controllerState.controller.seekTo(newPosition) } else { val duration = controllerState.controller.duration - val newPosition = - (controllerState.controller.currentPosition + skipSeconds * 1000) - .coerceAtMost(duration) - controllerState.controller.seekTo(newPosition) + val newPosition = controllerState.controller.currentPosition + SKIP_MILLIS + controllerState.controller.seekTo( + if (duration > 0) newPosition.coerceAtMost(duration) else newPosition, + ) } } }, @@ -143,7 +143,6 @@ fun RenderVideoPlayer( controllerVisible = controllerVisible, modifier = Modifier.align(Alignment.Center), isLiveStream = isLive, - videoDurationMs = controllerState.controller.duration, ) RenderAnimatedBottomInfo(controllerState, controllerVisible, Modifier.align(Alignment.BottomCenter)) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderCenterButtons.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderCenterButtons.kt index 085381d4c..f2e5a6dbf 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderCenterButtons.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderCenterButtons.kt @@ -32,7 +32,7 @@ import androidx.compose.ui.unit.dp import androidx.media3.common.util.UnstableApi import androidx.media3.ui.compose.state.rememberPlayPauseButtonState 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 @OptIn(UnstableApi::class) @@ -42,10 +42,8 @@ fun RenderCenterButtons( controllerVisible: MutableState, modifier: Modifier, isLiveStream: Boolean = false, - videoDurationMs: Long = 0L, ) { val state = rememberPlayPauseButtonState(controllerState.controller) - val skipSeconds = computeSkipSeconds(videoDurationMs) Row( modifier = modifier, @@ -56,9 +54,8 @@ fun RenderCenterButtons( AnimatedSkipButton( controllerVisible = controllerVisible, 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) } } @@ -71,11 +68,10 @@ fun RenderCenterButtons( AnimatedSkipButton( controllerVisible = controllerVisible, isForward = true, - skipSeconds = skipSeconds, ) { val duration = controllerState.controller.duration - val newPosition = (controllerState.controller.currentPosition + skipSeconds * 1000).coerceAtMost(duration) - controllerState.controller.seekTo(newPosition) + val newPosition = controllerState.controller.currentPosition + SKIP_MILLIS + controllerState.controller.seekTo(if (duration > 0) newPosition.coerceAtMost(duration) else newPosition) } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt index 6010bb47a..4ae27026c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt @@ -25,6 +25,7 @@ import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Row import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -123,6 +124,10 @@ fun RenderTopButtons( val context = LocalContext.current val shareDialogVisible = remember { mutableStateOf(false) } + LaunchedEffect(controllerVisible.value) { + if (!controllerVisible.value) shareDialogVisible.value = false + } + Row(modifier) { if (onZoomClick != null) { FullScreenButton( @@ -137,39 +142,41 @@ fun RenderTopButtons( toggle = onMuteClick, ) - AnimatedOverflowMenuButton( - controllerVisible = controllerVisible, - showShare = true, - showSave = !isLive, - showPip = pipSupported, - onShareClick = { shareDialogVisible.value = true }, - onSaveClick = { - accountViewModel.saveMediaToGallery(mediaData.videoUri, mediaData.mimeType, context) - }, - onPipClick = onPictureInPictureClick, - ) - } + Box { + AnimatedOverflowMenuButton( + controllerVisible = controllerVisible, + showShare = true, + showSave = !isLive, + showPip = pipSupported, + onShareClick = { shareDialogVisible.value = true }, + onSaveClick = { + accountViewModel.saveMediaToGallery(mediaData.videoUri, mediaData.mimeType, context) + }, + onPipClick = onPictureInPictureClick, + ) - if (shareDialogVisible.value) { - ShareMediaAction( - popupExpanded = shareDialogVisible, - videoUri = mediaData.videoUri, - postNostrUri = mediaData.callbackUri, - blurhash = null, - dim = null, - hash = null, - mimeType = mediaData.mimeType, - onDismiss = { shareDialogVisible.value = false }, - content = - MediaUrlVideo( - url = mediaData.videoUri, + if (shareDialogVisible.value) { + ShareMediaAction( + popupExpanded = shareDialogVisible, + videoUri = mediaData.videoUri, + postNostrUri = mediaData.callbackUri, + blurhash = null, + dim = null, + hash = null, mimeType = mediaData.mimeType, - artworkUri = mediaData.artworkUri, - authorName = mediaData.authorName, - description = mediaData.title, - uri = mediaData.callbackUri, - ), - accountViewModel = accountViewModel, - ) + onDismiss = { shareDialogVisible.value = false }, + content = + MediaUrlVideo( + url = mediaData.videoUri, + mimeType = mediaData.mimeType, + artworkUri = mediaData.artworkUri, + authorName = mediaData.authorName, + description = mediaData.title, + uri = mediaData.callbackUri, + ), + accountViewModel = accountViewModel, + ) + } + } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/SkipButton.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/SkipButton.kt index 2d37909e5..b7776ac32 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/SkipButton.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/SkipButton.kt @@ -38,6 +38,7 @@ import androidx.compose.ui.graphics.Color import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp 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.theme.BitcoinOrange import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn @@ -50,11 +51,7 @@ private val FadeOut = fadeOut() fun SkipBackButtonPreview() { ThemeComparisonColumn { Box(Modifier.background(BitcoinOrange)) { - SkipButton( - isForward = false, - skipSeconds = 10, - onClick = {}, - ) + SkipButton(isForward = false, onClick = {}) } } } @@ -64,11 +61,7 @@ fun SkipBackButtonPreview() { fun SkipForwardButtonPreview() { ThemeComparisonColumn { Box(Modifier.background(BitcoinOrange)) { - SkipButton( - isForward = true, - skipSeconds = 10, - onClick = {}, - ) + SkipButton(isForward = true, onClick = {}) } } } @@ -77,7 +70,6 @@ fun SkipForwardButtonPreview() { fun AnimatedSkipButton( controllerVisible: State, isForward: Boolean, - skipSeconds: Int = 10, modifier: Modifier = Modifier, onClick: () -> Unit, ) { @@ -87,18 +79,13 @@ fun AnimatedSkipButton( enter = FadeIn, exit = FadeOut, ) { - SkipButton( - isForward = isForward, - skipSeconds = skipSeconds, - onClick = onClick, - ) + SkipButton(isForward = isForward, onClick = onClick) } } @Composable fun SkipButton( isForward: Boolean, - skipSeconds: Int = 10, onClick: () -> Unit, ) { IconButton( @@ -109,9 +96,9 @@ fun SkipButton( imageVector = if (isForward) Icons.Default.Forward10 else Icons.Default.Replay10, contentDescription = if (isForward) { - stringRes(R.string.skip_forward, skipSeconds) + stringRes(R.string.skip_forward, SKIP_SECONDS) } else { - stringRes(R.string.skip_back, skipSeconds) + stringRes(R.string.skip_back, SKIP_SECONDS) }, tint = Color.White, modifier = Modifier.size(32.dp),