Conversation
Summary of ChangesHello @ExZyO, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a convenient new interaction for the video player, allowing users to quickly toggle 2x playback speed by holding their finger on the screen. This enhancement improves the user experience by providing a dynamic way to consume content faster, while also refining the existing long-press gesture to be context-aware, ensuring both speed control and screenshot functionality are readily available. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Hold screen, speed up time, Two times faster, content flies, Release, back to normal. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'hold to play at 2x speed' feature. The implementation involves detecting a long press gesture to increase playback speed and restoring it on release. A new UI indicator is added to show when 2x speed is active.
My review focuses on improving the reliability of the new UI indicator. I've found an issue where the 2x speed indicator can disappear while the feature is still active, which could be confusing for the user. Overall, the changes are good and the feature is a nice addition.
| fun DoubleSpeedIndicator( | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| var targetAlpha by remember { mutableFloatStateOf(1f) } | ||
|
|
||
| LaunchedEffect(Unit) { | ||
| delay(1500) | ||
| targetAlpha = 0.35f | ||
| delay(1500) | ||
| targetAlpha = 0f | ||
| } | ||
|
|
||
| val alpha by androidx.compose.animation.core.animateFloatAsState( | ||
| targetValue = targetAlpha, | ||
| animationSpec = androidx.compose.animation.core.tween(durationMillis = 500), | ||
| label = "doubleSpeedAlpha", | ||
| ) | ||
|
|
||
| if (alpha > 0f) { | ||
| PlayerUpdate(modifier.alpha(alpha)) { | ||
| Row(verticalAlignment = Alignment.CenterVertically) { | ||
| Icon( | ||
| imageVector = Icons.Filled.FastForward, | ||
| contentDescription = null, | ||
| tint = Color.White, | ||
| modifier = Modifier.size(14.dp), | ||
| ) | ||
| Spacer(modifier = Modifier.width(4.dp)) | ||
| Text( | ||
| text = "2x", | ||
| color = Color.White, | ||
| fontSize = 12.sp, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of DoubleSpeedIndicator uses a LaunchedEffect with delays to manage its visibility, causing it to disappear after 3 seconds even if the user is still long-pressing. This can be confusing as the playback speed remains at 2x until the press is released. The component's visibility should be solely controlled by its presence in the composition tree. The parent AnimatedVisibility in PlayerControls already handles the fade-in and fade-out animations. Simplifying this component to just display the indicator will fix the bug and make the code cleaner.
@Composable
fun DoubleSpeedIndicator(
modifier: Modifier = Modifier,
) {
PlayerUpdate(modifier) {
Row(verticalAlignment = Alignment.CenterVertically) {
Icon(
imageVector = Icons.Filled.FastForward,
contentDescription = null,
tint = Color.White,
modifier = Modifier.size(14.dp),
)
Spacer(modifier = Modifier.width(4.dp))
Text(
text = "2x",
color = Color.White,
fontSize = 12.sp,
)
}
}
}
Reviewer's guide (collapsed on small PRs)Reviewer's GuideImplements a hold-to-play-at-2x-speed gesture in the player, reusing the long-press gesture and wiring it into the player update UI via a new DoubleSpeedIndicator component, while preserving and restoring the original playback speed. Sequence diagram for hold-to-play-at-2x-speed gesturesequenceDiagram
actor User
participant GestureHandler
participant PlayerViewModel
participant MPVLib
participant PlayerControls
participant DoubleSpeedIndicator
User->>GestureHandler: longPress(screen)
GestureHandler->>PlayerViewModel: read paused
alt playback_paused
GestureHandler->>PlayerViewModel: pause()
GestureHandler->>PlayerViewModel: sheetShown.update(Sheets.Screenshot)
else playback_playing
GestureHandler->>MPVLib: getPropertyDouble(speed)
GestureHandler->>GestureHandler: store originalSpeed
GestureHandler->>MPVLib: setPropertyDouble(speed, 2.0)
GestureHandler->>PlayerViewModel: playerUpdate.update(PlayerUpdates.DoubleSpeed)
PlayerViewModel-->>PlayerControls: playerUpdate flow emits DoubleSpeed
PlayerControls->>DoubleSpeedIndicator: render()
end
Updated class diagram for player gesture and 2x speed indicatorclassDiagram
class GestureHandler {
+fun GestureHandler(viewModel, haptics, controlsShown)
-Boolean isLongPressing
-Float originalSpeed
}
class PlayerViewModel {
+MutableState<Float> playbackSpeed
+MutableState<Boolean> paused
+MutableState<Sheets> sheetShown
+MutableState<PlayerUpdates> playerUpdate
+fun pause()
+fun hideControls()
+fun showControls()
}
class MPVLib {
+static fun getPropertyDouble(name)
+static fun setPropertyDouble(name, value)
}
class PlayerControls {
+fun PlayerControls(viewModel)
-PlayerUpdates currentPlayerUpdate
}
class PlayerUpdates {
}
class DoubleSpeedIndicator {
+fun DoubleSpeedIndicator()
}
class Sheets {
<<enumeration>>
Screenshot
}
PlayerViewModel --> PlayerUpdates : playerUpdate
PlayerViewModel --> Sheets : sheetShown
GestureHandler --> PlayerViewModel : uses
GestureHandler --> MPVLib : controls speed
PlayerControls --> PlayerViewModel : observes
PlayerControls --> PlayerUpdates : uses
PlayerControls --> DoubleSpeedIndicator : renders
PlayerUpdates <|-- PlayerUpdates_DoubleSpeed
class PlayerUpdates_DoubleSpeed {
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Speed changes are now made directly via
MPVLib.setPropertyDouble("speed", ...)whileviewModel.playbackSpeedstill exists as the apparent source of truth; consider routing this through the same abstraction (e.g., a viewModel method that updates both MPV and state) so the UI state and underlying player don’t drift out of sync. - In the long-press handler, the
if (viewModel.paused.value)branch still callsviewModel.pause(); ifpause()is a toggle this could unexpectedly resume playback when long-pressing while paused—consider using an explicit pause/screenshot method or branching logic that doesn’t toggle state in the paused case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Speed changes are now made directly via `MPVLib.setPropertyDouble("speed", ...)` while `viewModel.playbackSpeed` still exists as the apparent source of truth; consider routing this through the same abstraction (e.g., a viewModel method that updates both MPV and state) so the UI state and underlying player don’t drift out of sync.
- In the long-press handler, the `if (viewModel.paused.value)` branch still calls `viewModel.pause()`; if `pause()` is a toggle this could unexpectedly resume playback when long-pressing while paused—consider using an explicit pause/screenshot method or branching logic that doesn’t toggle state in the paused case.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ExZyO
force-pushed
the
feature/hold-to-2x-speed
branch
2 times, most recently
from
23b16f0 to
add5417
Compare
ExZyO
force-pushed
the
feature/hold-to-2x-speed
branch
from
ca932c0 to
e788095
Compare
1 participant
Add a 👍 reaction to pull requests you find important.
Summary by Sourcery
Add long-press gesture support to temporarily play video at 2x speed and surface a corresponding player UI indicator.
New Features:
Enhancements: