Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues in video playback by refining how volume and audio attributes are set in VideoPlayer, and disables video autoplay in the profile media tab.
- Refactored volume and audio-attribute logic using
DisposableEffectand removed an unused import. - Wrapped media items in
ProfileScreenwith aCompositionLocalProviderto setvideoAutoplay = NEVER.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| shared/ui/component/src/androidMain/kotlin/dev/dimension/flare/ui/component/VideoPlayer.kt | Removed unused import; switched from LaunchedEffect to DisposableEffect for volume and audio-attribute management. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/ProfileScreen.kt | Added CompositionLocalProvider to override videoAutoplay for media items in the profile media tab. |
Comments suppressed due to low confidence (2)
desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/ProfileScreen.kt:206
- [nitpick] Only
videoAutoplayis overridden while inheriting all other fields from the current appearance. Ensure that inheriting other settings won’t introduce unintended styling or behavior in this context.
LocalComponentAppearance.current.copy(
shared/ui/component/src/androidMain/kotlin/dev/dimension/flare/ui/component/VideoPlayer.kt:116
- [nitpick] The
.applyblock duplicates volume assignment already handled by theDisposableEffect, so you can remove it to simplify initialization and avoid redundancy.
}
| player.setAudioAttributes(audioAttributes, true) | ||
| } | ||
| onDispose { | ||
| player.setAudioAttributes(audioAttributes, false) |
There was a problem hiding this comment.
Currently only the audio attributes are reset on dispose, but the player’s volume remains unchanged. Consider resetting the volume on dispose as well to avoid lingering state if this player instance is reused.
| player.setAudioAttributes(audioAttributes, false) | |
| player.setAudioAttributes(audioAttributes, false) | |
| player.volume = 1f // Reset volume to default |
| .clickable { | ||
| val content = item.status.content | ||
| if (content is UiTimeline.ItemContent.Status) { | ||
| CompositionLocalProvider( |
There was a problem hiding this comment.
[nitpick] The CompositionLocalProvider is applied for each media item, which could add overhead. Consider hoisting it to a higher scope so it wraps the entire list rather than individual items.
| .clickable { | ||
| val content = item.status.content | ||
| if (content is UiTimeline.ItemContent.Status) { | ||
| CompositionLocalProvider( |
There was a problem hiding this comment.
[nitpick] This nested CompositionLocalProvider adds extra indentation and nested braces. You might extract this into a small helper composable to improve readability and reduce nesting.
No description provided.