feat(playback): add video playback support#3230
Draft
kairosci wants to merge 3 commits intoMetrolistGroup:mainfrom
Draft
feat(playback): add video playback support#3230kairosci wants to merge 3 commits intoMetrolistGroup:mainfrom
kairosci wants to merge 3 commits intoMetrolistGroup:mainfrom
Conversation
Collaborator
|
Merge conflicts, lovely. |
kairosci
force-pushed
the
feature/video-playback-clean
branch
from
e63c7c5 to
49d1db9
Compare
kairosci
force-pushed
the
feature/video-playback-clean
branch
from
a626c0f to
a894b05
Compare
Member
|
holy shit, good luck with the conflicts lmao |
kairosci
force-pushed
the
feature/video-playback-clean
branch
2 times, most recently
from
b1947e1 to
495f20a
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kairosci
force-pushed
the
feature/video-playback-clean
branch
from
abc12dd to
3480824
Compare
Replace AutoMigration with manual migration to handle cases where columns might already exist from previous development builds or partial migrations. This fixes SQLiteException 'duplicate column name: videoId' on app launch.
Contributor
|
I see too many force pushes hope everything is alright 😭 |
Member
|
remember to not force push that much. to update your branch you should rebase. i do it like this |
✅ Actions performedReview triggered.
|
Member
|
@coderabbitai review |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce video playback support to the music player application. A media3-ui dependency is added, SongEntity is extended with video metadata fields (videoId, artworkUrl, squareThumbnailUrl), database migrations are updated, and new preference keys control video playback. A VideoPlayer composable component is created alongside UI controls for toggling between song and video modes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt (1)
344-360: LGTM with a minor style suggestion.The
toggleVideoPlayback()function correctly manipulates track selection parameters to enable/disable video playback. The error handling is appropriate for this use case.Consider importing
androidx.media3.common.Cat the top of the file (it's already imported at line 12-16 area for otherCconstants) rather than using the fully qualified name on line 352.♻️ Optional: Use the existing C import
- val isVideoDisabled = currentParameters.disabledTrackTypes.contains(androidx.media3.common.C.TRACK_TYPE_VIDEO) + val isVideoDisabled = currentParameters.disabledTrackTypes.contains(C.TRACK_TYPE_VIDEO) exoPlayer.trackSelectionParameters = currentParameters .buildUpon() - .setTrackTypeDisabled(androidx.media3.common.C.TRACK_TYPE_VIDEO, !isVideoDisabled) + .setTrackTypeDisabled(C.TRACK_TYPE_VIDEO, !isVideoDisabled) .build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt` around lines 344 - 360, The code uses the fully qualified androidx.media3.common.C constant inside toggleVideoPlayback(); please add an import for androidx.media3.common.C at the top of the file and replace occurrences of androidx.media3.common.C.TRACK_TYPE_VIDEO in the toggleVideoPlayback() function with the simple C.TRACK_TYPE_VIDEO to match the existing import style and keep imports consistent (refer to function toggleVideoPlayback and constant TRACK_TYPE_VIDEO).app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt (2)
750-758: Unused animation states.
songBackgroundandvideoBackgroundare animated but never applied to any UI elements. The Box backgrounds on lines 764 and 778 use conditional expressions directly instead of these animated values.🧹 Remove unused code
- val songBackground by animateDpAsState( - targetValue = if (!isVideoSelected) 0.dp else 4.dp, - animationSpec = tween(200), - label = "songBackground" - ) - val videoBackground by animateDpAsState( - targetValue = if (isVideoSelected) 0.dp else 4.dp, - animationSpec = tween(200), - label = "videoBackground" - ) - Box(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt` around lines 750 - 758, The two animateDpAsState values songBackground and videoBackground are never used — the Box backgrounds still use conditional expressions directly; replace those conditional dp values with the animated state variables so the animated values drive the UI. Locate the animateDpAsState declarations (songBackground, videoBackground) and update the corresponding Box background modifiers that currently compute background corner radii or paddings with if (isVideoSelected) ... to instead reference songBackground and videoBackground so the animation affects the displayed Boxes.
160-162: Consider logging or handling the caught exception.The exception is caught but silently swallowed. While this provides resilience, logging the error would help with debugging playback issues.
🛡️ Suggested fix
val currentMediaItem = try { player.currentMediaItem - } catch (e: Exception) { null } + } catch (e: Exception) { + timber.log.Timber.w(e, "Failed to get currentMediaItem") + null + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt` around lines 160 - 162, The currentMediaItem try/catch silently swallows exceptions from player.currentMediaItem; update the catch in Thumbnail.kt to log or handle the exception instead of ignoring it — capture the caught Exception (e) and call your app's logging facility (e.g., Log.e, Timber.e, or the existing logger) with a concise message like "Failed to get currentMediaItem" plus the exception, or perform an alternative error handling path before returning null so playback issues are observable; locate the try block around player.currentMediaItem and add the logging/handling in the catch clause.app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt (1)
805-811: Consider removing unused Migration34To35 class.The
@Suppress("unused")annotation and empty implementation suggest this class should be removed rather than kept "for reference." Dead code increases maintenance burden and can confuse future developers.🧹 Remove the unused class
-@Suppress("unused") // Kept for reference, replaced by manual MIGRATION_34_35 -class Migration34To35 : AutoMigrationSpec { - override fun onPostMigrate(db: SupportSQLiteDatabase) { - // This class is no longer used, kept for schema compatibility - // All migration logic moved to MIGRATION_34_35 - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt` around lines 805 - 811, Remove the unused Migration34To35 class: delete the class declaration "class Migration34To35 : AutoMigrationSpec { override fun onPostMigrate(db: SupportSQLiteDatabase) { ... } }" and its `@Suppress`("unused") annotation so the dead code is eliminated; ensure any references to Migration34To35 elsewhere (e.g., registration lists or migration constants) are also removed or updated to use MIGRATION_34_35 to avoid unresolved symbol errors.app/src/main/kotlin/com/metrolist/music/models/MediaMetadata.kt (1)
140-142: Redundant assignment:videoId = setVideoIdduplicates existing field.In
SongItem.toMediaMetadata(), settingvideoId = setVideoId(line 140) while also settingsetVideoId = setVideoId(line 134) creates redundancy. Both fields will always have the same value from this mapping.This may be intentional to ensure
resolvedVideoIdworks consistently, but consider documenting why both are needed or simplifying if one field is sufficient forSongItemsources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/models/MediaMetadata.kt` around lines 140 - 142, The mapping in SongItem.toMediaMetadata() redundantly assigns the same value to both videoId and setVideoId (videoId = setVideoId), causing duplicate fields; either remove the redundant videoId assignment or explicitly document why both fields must mirror each other (and add a comment). Locate the SongItem.toMediaMetadata() mapping and either delete the videoId = setVideoId line and rely on setVideoId (and resolvedVideoId if used), or keep it but add a concise comment explaining that videoId must equal setVideoId for downstream consumers (mentioning videoId, setVideoId, and resolvedVideoId to make the intent clear).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt`:
- Around line 164-193: The fixed thread pools created in createBuilder via
setTransactionExecutor and setQueryExecutor leak because they are never shut
down; change to reuse cached/shared executors (create singleton Executors or use
Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()) or a
shared WorkStealing/SharedPool) and store references to those ExecutorService
instances, then ensure MusicDatabase closes them when the DB is closed
(override/extend the InternalDatabase/MusicDatabase close/shutdown path to call
shutdownNow()/shutdown on the stored executors). Update createBuilder to accept
or reference the shared executors instead of creating new ones inline (symbols:
createBuilder, setTransactionExecutor, setQueryExecutor, MusicDatabase,
newInstance).
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt`:
- Around line 374-383: The UI updates isVideoShown immediately after calling
playerConnection.toggleVideoPlayback(), which can desync if the operation fails;
change the flow so the UI state is updated only after confirming success —
either modify playerConnection.toggleVideoPlayback() to return a boolean/result
and set isVideoShown based on that, or call a synchronous/asynchronous
playerConnection.getVideoPlaybackState()/isVideoPlaying() after toggle (and in
error catch blocks) to re-sync isVideoShown with the actual player state; ensure
SongVideoToggle's onToggle uses the new return value or the re-read result and
handles exceptions by logging and keeping the UI consistent.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt`:
- Around line 805-811: Remove the unused Migration34To35 class: delete the class
declaration "class Migration34To35 : AutoMigrationSpec { override fun
onPostMigrate(db: SupportSQLiteDatabase) { ... } }" and its `@Suppress`("unused")
annotation so the dead code is eliminated; ensure any references to
Migration34To35 elsewhere (e.g., registration lists or migration constants) are
also removed or updated to use MIGRATION_34_35 to avoid unresolved symbol
errors.
In `@app/src/main/kotlin/com/metrolist/music/models/MediaMetadata.kt`:
- Around line 140-142: The mapping in SongItem.toMediaMetadata() redundantly
assigns the same value to both videoId and setVideoId (videoId = setVideoId),
causing duplicate fields; either remove the redundant videoId assignment or
explicitly document why both fields must mirror each other (and add a comment).
Locate the SongItem.toMediaMetadata() mapping and either delete the videoId =
setVideoId line and rely on setVideoId (and resolvedVideoId if used), or keep it
but add a concise comment explaining that videoId must equal setVideoId for
downstream consumers (mentioning videoId, setVideoId, and resolvedVideoId to
make the intent clear).
In `@app/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.kt`:
- Around line 344-360: The code uses the fully qualified
androidx.media3.common.C constant inside toggleVideoPlayback(); please add an
import for androidx.media3.common.C at the top of the file and replace
occurrences of androidx.media3.common.C.TRACK_TYPE_VIDEO in the
toggleVideoPlayback() function with the simple C.TRACK_TYPE_VIDEO to match the
existing import style and keep imports consistent (refer to function
toggleVideoPlayback and constant TRACK_TYPE_VIDEO).
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt`:
- Around line 750-758: The two animateDpAsState values songBackground and
videoBackground are never used — the Box backgrounds still use conditional
expressions directly; replace those conditional dp values with the animated
state variables so the animated values drive the UI. Locate the animateDpAsState
declarations (songBackground, videoBackground) and update the corresponding Box
background modifiers that currently compute background corner radii or paddings
with if (isVideoSelected) ... to instead reference songBackground and
videoBackground so the animation affects the displayed Boxes.
- Around line 160-162: The currentMediaItem try/catch silently swallows
exceptions from player.currentMediaItem; update the catch in Thumbnail.kt to log
or handle the exception instead of ignoring it — capture the caught Exception
(e) and call your app's logging facility (e.g., Log.e, Timber.e, or the existing
logger) with a concise message like "Failed to get currentMediaItem" plus the
exception, or perform an alternative error handling path before returning null
so playback issues are observable; locate the try block around
player.currentMediaItem and add the logging/handling in the catch clause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71f6649d-626d-4ea9-837d-62510db3457a
📒 Files selected for processing (13)
app/build.gradle.ktsapp/schemas/com.metrolist.music.db.InternalDatabase/35.jsonapp/schemas/com.metrolist.music.db.InternalDatabase/36.jsonapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/db/MusicDatabase.ktapp/src/main/kotlin/com/metrolist/music/db/entities/SongEntity.ktapp/src/main/kotlin/com/metrolist/music/models/MediaMetadata.ktapp/src/main/kotlin/com/metrolist/music/playback/PlayerConnection.ktapp/src/main/kotlin/com/metrolist/music/ui/component/VideoPlayer.ktapp/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/PlayerSettings.ktapp/src/main/res/values/metrolist_strings.xmlgradle/libs.versions.toml
Comment on lines
+164
to
+193
| fun createBuilder(context: Context): RoomDatabase.Builder<InternalDatabase> = | ||
| Room.databaseBuilder(context, InternalDatabase::class.java, DB_NAME) | ||
| .addMigrations( | ||
| MIGRATION_1_2, | ||
| MIGRATION_21_24, | ||
| MIGRATION_22_24, | ||
| MIGRATION_24_25, | ||
| MIGRATION_34_35, | ||
| ) | ||
| .fallbackToDestructiveMigration(dropAllTables = true) | ||
| .setJournalMode(RoomDatabase.JournalMode.WRITE_AHEAD_LOGGING) | ||
| .setTransactionExecutor(java.util.concurrent.Executors.newFixedThreadPool(4)) | ||
| .setQueryExecutor(java.util.concurrent.Executors.newFixedThreadPool(4)) | ||
| .addCallback(object : RoomDatabase.Callback() { | ||
| override fun onOpen(db: SupportSQLiteDatabase) { | ||
| super.onOpen(db) | ||
| try { | ||
| db.query("PRAGMA busy_timeout = 60000").close() | ||
| db.query("PRAGMA cache_size = -16000").close() | ||
| db.query("PRAGMA wal_autocheckpoint = 1000").close() | ||
| db.query("PRAGMA synchronous = NORMAL").close() | ||
| } catch (e: Exception) { | ||
| Timber.tag("MusicDatabase").e(e, "Failed to set PRAGMA settings") | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| fun newInstance(context: Context): MusicDatabase = | ||
| MusicDatabase( | ||
| delegate = | ||
| Room | ||
| .databaseBuilder(context, InternalDatabase::class.java, DB_NAME) | ||
| .addMigrations( | ||
| MIGRATION_1_2, | ||
| MIGRATION_21_24, | ||
| MIGRATION_22_24, | ||
| MIGRATION_24_25, | ||
| ) | ||
| .fallbackToDestructiveMigration(dropAllTables = true) | ||
| .setJournalMode(RoomDatabase.JournalMode.WRITE_AHEAD_LOGGING) | ||
| .setTransactionExecutor(java.util.concurrent.Executors.newFixedThreadPool(4)) | ||
| .setQueryExecutor(java.util.concurrent.Executors.newFixedThreadPool(4)) | ||
| .addCallback(object : RoomDatabase.Callback() { | ||
| override fun onOpen(db: SupportSQLiteDatabase) { | ||
| super.onOpen(db) | ||
| try { | ||
| db.query("PRAGMA busy_timeout = 60000").close() | ||
| db.query("PRAGMA cache_size = -16000").close() | ||
| db.query("PRAGMA wal_autocheckpoint = 1000").close() | ||
| db.query("PRAGMA synchronous = NORMAL").close() | ||
| } catch (e: Exception) { | ||
| Timber.tag("MusicDatabase").e(e, "Failed to set PRAGMA settings") | ||
| } | ||
| } | ||
| }) | ||
| .build(), | ||
| delegate = createBuilder(context).build() |
There was a problem hiding this comment.
Thread pool executors are never shut down.
The fixed thread pools created at lines 175-176 are never shut down when the database is closed, which can lead to resource leaks if multiple database instances are created over the app lifecycle.
Additionally, using a hardcoded pool size of 4 may not be optimal for all Android devices with varying core counts.
💡 Consider caching executors or using shared executors
companion object {
const val DB_NAME = "song.db"
+
+ // Use shared executors to avoid creating new pools per instance
+ private val sharedExecutor by lazy {
+ java.util.concurrent.Executors.newFixedThreadPool(
+ Runtime.getRuntime().availableProcessors().coerceIn(2, 4)
+ )
+ }
fun createBuilder(context: Context): RoomDatabase.Builder<InternalDatabase> =
Room.databaseBuilder(context, InternalDatabase::class.java, DB_NAME)
.addMigrations(
MIGRATION_1_2,
MIGRATION_21_24,
MIGRATION_22_24,
MIGRATION_24_25,
MIGRATION_34_35,
)
.fallbackToDestructiveMigration(dropAllTables = true)
.setJournalMode(RoomDatabase.JournalMode.WRITE_AHEAD_LOGGING)
- .setTransactionExecutor(java.util.concurrent.Executors.newFixedThreadPool(4))
- .setQueryExecutor(java.util.concurrent.Executors.newFixedThreadPool(4))
+ .setTransactionExecutor(sharedExecutor)
+ .setQueryExecutor(sharedExecutor)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt` around lines 164
- 193, The fixed thread pools created in createBuilder via
setTransactionExecutor and setQueryExecutor leak because they are never shut
down; change to reuse cached/shared executors (create singleton Executors or use
Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()) or a
shared WorkStealing/SharedPool) and store references to those ExecutorService
instances, then ensure MusicDatabase closes them when the DB is closed
(override/extend the InternalDatabase/MusicDatabase close/shutdown path to call
shutdownNow()/shutdown on the stored executors). Update createBuilder to accept
or reference the shared executors instead of creating new ones inline (symbols:
createBuilder, setTransactionExecutor, setQueryExecutor, MusicDatabase,
newInstance).
Comment on lines
+374
to
+383
| if (isVideoSong && isSongVideoToggleEnabled && !isListenTogetherGuest) { | ||
| SongVideoToggle( | ||
| isVideoSelected = isVideoShown, | ||
| onToggle = { | ||
| playerConnection.toggleVideoPlayback() | ||
| isVideoShown = !isVideoShown | ||
| }, | ||
| textColor = textBackgroundColor, | ||
| modifier = Modifier.align(Alignment.CenterEnd) | ||
| ) |
There was a problem hiding this comment.
Potential state desync if toggleVideoPlayback() fails.
The UI state isVideoShown is updated immediately after calling playerConnection.toggleVideoPlayback(), regardless of whether the toggle operation succeeded. If the toggle fails (exception is caught and logged in PlayerConnection), the UI state will be out of sync with the actual player state.
Consider either:
- Having
toggleVideoPlayback()return a success indicator - Re-syncing state after toggle by re-reading from the player
♻️ Suggested improvement
SongVideoToggle(
isVideoSelected = isVideoShown,
onToggle = {
playerConnection.toggleVideoPlayback()
- isVideoShown = !isVideoShown
+ // Re-read actual state from player to ensure sync
+ val currentParams = playerConnection.player.trackSelectionParameters
+ isVideoShown = !currentParams.disabledTrackTypes.contains(C.TRACK_TYPE_VIDEO)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/player/Thumbnail.kt` around lines
374 - 383, The UI updates isVideoShown immediately after calling
playerConnection.toggleVideoPlayback(), which can desync if the operation fails;
change the flow so the UI state is updated only after confirming success —
either modify playerConnection.toggleVideoPlayback() to return a boolean/result
and set isVideoShown based on that, or call a synchronous/asynchronous
playerConnection.getVideoPlaybackState()/isVideoPlaying() after toggle (and in
error catch blocks) to re-sync isVideoShown with the actual player state; ensure
SongVideoToggle's onToggle uses the new return value or the re-read result and
handles exceptions by logging and keeping the UI consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Users were unable to play YouTube video content within the app, limiting the app's functionality to audio-only playback.
Cause
The app lacked video playback capabilities and the necessary infrastructure to handle video streams from YouTube Music.
Solution
Testing
Related Issues
Summary by CodeRabbit