Feature/compose downloads #1675
Conversation
Use coil to display the downloads thumbnail with error handling
…em to storage This commit removes the local storage and loading of download thumbnails. Instead, thumbnails are now loaded directly from the server when displaying the downloads list. The `DownloadUtils` no longer downloads and saves thumbnails locally. The `DownloadsAdapter` now constructs the thumbnail URL using the `ApiClient` and loads it via Coil. The `DownloadEntity` no longer stores the local thumbnail path. Layout adjustments were made in `fragment_downloads.xml` and `download_item.xml` to accommodate this change and improve the display of download items. A new dimension `movie_thumbnail_list_size` was added for consistent thumbnail sizing. Note: Code to delete thumbnails is still in place to handle currently installed clients who may have existing downloads.
…oadsScreenRoot` Composable. Removed unused fragment related code
- Move `DownloadsViewModel` to the `ui.screens.downloads` package. - Remove unused drawable `ic_local_movies_white_64.xml`. - Use `NotificationHelper` to build download completed notifications in `JellyfinDownloadService`. - Add preview for DownloadsScreen
|
|
||
| fun deleteDownload(mediaSourceId: String) { | ||
| viewModelScope.launch { | ||
| val downloadEntity: DownloadEntity = requireNotNull(downloadDao.get(mediaSourceId)) |
There was a problem hiding this comment.
Deleting the download should really be handled somewhere else, but moving the code here is no different than having it in MainActivity like is was before.
|
Marked the PR as draft as it depends on another one. |
|
@cameocoder now that the base PR was merged, could you please rebase this PR and fix the conflicts? Thanks! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis change migrates the downloads feature from a traditional Android View-based implementation to a Jetpack Compose-based UI. It removes legacy RecyclerView adapters, XML layouts, and ViewModel code, introducing new Compose screens, dialogs, and a refactored ViewModel. Gradle dependencies and library versions are updated to support Compose tooling and fragment integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DownloadsFragment
participant DownloadsViewModel
participant DownloadsScreen (Compose)
participant DownloadDao
participant ApiClient
participant DownloadService
User->>DownloadsFragment: Opens Downloads screen
DownloadsFragment->>DownloadsScreen: Launches Compose UI with ViewModel
DownloadsScreen->>DownloadsViewModel: Observes downloads state
DownloadsViewModel->>DownloadDao: Fetch all downloads (Flow)
DownloadDao-->>DownloadsViewModel: Emits list of downloads
DownloadsViewModel-->>DownloadsScreen: Updates UI state with downloads
DownloadsScreen-->>User: Displays list of downloads
User->>DownloadsScreen: Clicks download item
DownloadsScreen->>DownloadsViewModel: openDownload(mediaSourceId)
DownloadsViewModel->>ApiClient: Prepares playback options
DownloadsViewModel-->>DownloadsScreen: Emits event to open native player
User->>DownloadsScreen: Long-presses download item
DownloadsScreen->>DownloadsScreen: Shows DeleteDownloadConfirmationDialog
User->>DownloadsScreen: Confirms deletion
DownloadsScreen->>DownloadsViewModel: deleteDownload(mediaSourceId)
DownloadsViewModel->>DownloadDao: Remove download entry
DownloadsViewModel->>DownloadService: Remove media and subtitles
DownloadsViewModel-->>DownloadsScreen: Updates UI state
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
app/src/main/java/org/jellyfin/mobile/app/AppModule.kt (1)
80-81: InjectContextviaget<Context>()rather thanandroidContext()for testability.
androidContext()hard-codes the Android app context, which makes it harder to swap in a mocked context during instrumented or JVM unit tests. Using the normal Koin lookup keeps the binding flexible:- viewModel { DownloadsViewModel(androidContext()) } + viewModel { DownloadsViewModel(get()) }This still resolves to the same object in production but can be overridden in a
testKoin module.app/build.gradle.kts (1)
136-137: Addui-tooling-previewtoimplementationfor release-safe previews.The common setup is:
implementation(libs.compose.ui.tooling.preview) debugImplementation(libs.compose.ui.tooling)
ui.tooling.previewis stripped by R8/ProGuard, whileui.toolingstays in debug only. Keeping previews in the main artifact avoids “unresolved symbol” errors in release builds while not bloating APK size.app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DeleteDownloadConfirmationDialog.kt (1)
30-37: CallonDismiss()after a positive confirmation to close the dialog.Currently the dialog remains open until the caller’s state update propagates. For immediate UX feedback, invoke the dismiss callback right after
onConfirm:- onConfirm(mediaSourceId) + onConfirm(mediaSourceId) + onDismiss()This makes the dialog disappear instantly and avoids a redundant recomposition.
app/src/main/java/org/jellyfin/mobile/downloads/DownloadsFragment.kt (2)
7-10: Importing the Composable ViewModel directly couples UI and DI layers.If
DownloadsScreenRootis ever reused outside the fragment (e.g., in an Activity), consider exposing the ViewModel as a@ComposablehiltViewModel()/koinViewModel()inside the composable instead of injecting it in the fragment. This keeps the fragment truly lightweight.
16-20:content {}is still marked@ExperimentalFragmentComposeApi.Add an explicit opt-in to avoid build-time warnings:
@OptIn(ExperimentalFragmentComposeApi::class) override fun onCreateView(...): View = content { ... }This makes the experimental status explicit for future maintainers.
app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsScreen.kt (3)
75-85: Hard-coded strings & empty content description hurt i18n and accessibility
TopAppBartitle uses"Downloads"literal and the back-arrow’scontentDescriptionis an empty string.-title = { Text("Downloads") } +title = { Text(stringResource(id = R.string.downloads_title)) } -Icon(..., contentDescription = "") +Icon(..., contentDescription = stringResource(id = R.string.back))Please add the missing string resources; screen-reader users will thank you.
168-171:combinedClickablewithoutindicationremoves default rippleWhen wrapping a
ListItemwithcombinedClickable, the default ripple is lost unless anindication = LocalIndication.currentis provided..combinedClickable( onClick = { onSelectItem(...) }, onLongClick = { onDeleteItem(mediaSourceId) }, + indication = null, // < add proper indication or null intentionally + interactionSource = remember { MutableInteractionSource() }, )If visual feedback is desired, consider restoring ripple or migrate to
Modifier.clickable {}+Modifier.pointerInput {}for long-press.
188-203: Fallback icon isn’t tint-aware
rememberVectorPainter(Icons.Default.LocalMovies)returns an untinted painter; on dark background it may end up invisible. UsepainterResource+tintor setcolorFilter:error = rememberVectorPainter(Icons.Default.LocalMovies) .apply { /* tint with LocalContentColor.current */ }Minor, but improves visual consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/build.gradle.kts(2 hunks)app/src/main/java/org/jellyfin/mobile/app/AppModule.kt(3 hunks)app/src/main/java/org/jellyfin/mobile/downloads/DownloadDiffCallback.kt(0 hunks)app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt(0 hunks)app/src/main/java/org/jellyfin/mobile/downloads/DownloadsFragment.kt(1 hunks)app/src/main/java/org/jellyfin/mobile/downloads/DownloadsViewModel.kt(0 hunks)app/src/main/java/org/jellyfin/mobile/downloads/JellyfinDownloadService.kt(1 hunks)app/src/main/java/org/jellyfin/mobile/events/ActivityEvent.kt(0 hunks)app/src/main/java/org/jellyfin/mobile/events/ActivityEventHandler.kt(0 hunks)app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DeleteDownloadConfirmationDialog.kt(1 hunks)app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsScreen.kt(1 hunks)app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsViewModel.kt(1 hunks)app/src/main/java/org/jellyfin/mobile/utils/SystemUtils.kt(0 hunks)app/src/main/res/layout/activity_main.xml(0 hunks)app/src/main/res/layout/download_item.xml(0 hunks)app/src/main/res/layout/fragment_downloads.xml(0 hunks)gradle/libs.versions.toml(4 hunks)
💤 Files with no reviewable changes (9)
- app/src/main/java/org/jellyfin/mobile/events/ActivityEventHandler.kt
- app/src/main/res/layout/activity_main.xml
- app/src/main/java/org/jellyfin/mobile/events/ActivityEvent.kt
- app/src/main/res/layout/fragment_downloads.xml
- app/src/main/java/org/jellyfin/mobile/downloads/DownloadDiffCallback.kt
- app/src/main/res/layout/download_item.xml
- app/src/main/java/org/jellyfin/mobile/downloads/DownloadsViewModel.kt
- app/src/main/java/org/jellyfin/mobile/utils/SystemUtils.kt
- app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/org/jellyfin/mobile/downloads/DownloadsFragment.kt (1)
app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsScreen.kt (1)
DownloadsScreenRoot(47-71)
🔇 Additional comments (6)
app/src/main/java/org/jellyfin/mobile/app/AppModule.kt (1)
38-40: Correct package switch for the new ViewModel – looks good.The import update to the new
ui.screens.downloadsnamespace correctly removes the dependency on the deleted legacy class.app/build.gradle.kts (1)
122-123: Confirmandroidx.fragment:fragment-composeversion compatibility.
fragment-composeis still in alpha; make sure the version pulled in bylibs.androidx.fragment.composematches your current Fragment/Compose Compiler versions to avoid ClassNotFoundException at runtime.app/src/main/java/org/jellyfin/mobile/downloads/JellyfinDownloadService.kt (1)
80-85: Verify the parameter order forbuildDownloadCompletedNotification.The ExoPlayer API signatures changed between 1.x and 2.x lines. Check that the parameters you pass (
context, icon, null, Util.fromUtf8Bytes(...)) map to(Context, Int, PendingIntent?, CharSequence?)in your dependency version; otherwise you’ll get a type-mismatch compilation error or a notification without content intent.If the method expects
contentIntentthird and message fourth, swap the arguments:- notificationHelper.buildDownloadCompletedNotification( - context, - R.drawable.ic_notification, - null, - Util.fromUtf8Bytes(download.request.data), - ) + notificationHelper.buildDownloadCompletedNotification( + context, + R.drawable.ic_notification, + Util.fromUtf8Bytes(download.request.data), + null, + )app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsViewModel.kt (1)
61-71: Hard-coded stream indices may break playback
audioStreamIndex = 1andsubtitleStreamIndex = -1assume a very specific media layout. If a download has only one audio track (index 0) or embedded subtitles, playback will fail.Consider deriving the first available audio/subtitle index from
mediaSource.audioStreams/subtitleStreamson theDownloadEntityinstead of hard-coding.gradle/libs.versions.toml (2)
107-117:compose-ui-toolingshould never leak into release builds
compose-ui-toolingadds ~1 MB of methods and should bedebugImplementationonly. Ensurelibs.compose.ui.toolingis referenced exclusively from the debug configuration (it is inbuild.gradle.ktscurrently).No action needed if that constraint is already respected; just highlighting because the new library entry appears in the general
librariesblock.
165-166: Bundle now pulls incoil-composefor all consumersAdding
coil-composeto thecoilbundle is convenient, but every consumer ofbundles.coilnow transitively depends on Compose, even non-UI modules. Double-check that this bundle isn’t used by pure JVM / instrumentation-only modules to avoid unnecessary dependencies.
| viewModelScope.launch { | ||
| val downloadEntity: DownloadEntity = requireNotNull(downloadDao.get(mediaSourceId)) | ||
| val downloadDir = File(downloadEntity.mediaSource.localDirectoryUri) | ||
| downloadDao.delete(mediaSourceId) | ||
| downloadDir.deleteRecursively() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
requireNotNull will crash when the entity is missing
If the DB is already cleaned up (e.g. by another process) downloadDao.get(mediaSourceId) can return null, leading to an immediate crash.
Either:
val downloadEntity = downloadDao.get(mediaSourceId) ?: return@launchor emit an error event so the UI can show feedback.
🤖 Prompt for AI Agents
In
app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsViewModel.kt
around lines 75 to 80, the use of requireNotNull on
downloadDao.get(mediaSourceId) can cause a crash if the entity is missing. To
fix this, replace requireNotNull with a safe call and handle the null case by
either returning early from the coroutine or emitting an error event for the UI.
For example, assign downloadEntity using a safe call with ?: return@launch to
avoid crashing when the entity is null.
| viewModelScope.launch { // this: CoroutineScope | ||
| downloadDao.getAllDownloads().flowOn(Dispatchers.IO).collect { downloadEntities: List<DownloadEntity> -> | ||
| val downloadModels = downloadEntities.map { it.asDownloadModel(applicationContext) } | ||
| _uiState.value = DownloadsUiState.ShowDownloads(downloadModels) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Collecting on Main thread may cause UI jank under large data sets
flowOn(Dispatchers.IO) moves the DAO query off-thread, but collect executes in the parent scope (viewModelScope, i.e. main). Mapping every entity to a DownloadModel for each emission could still become non-trivial when the list grows.
-viewModelScope.launch {
- downloadDao.getAllDownloads()
- .flowOn(Dispatchers.IO)
- .collect { downloadEntities ->
- _uiState.value = ...
- }
-}
+viewModelScope.launch(Dispatchers.Default) { // off-main for mapping
+ downloadDao.getAllDownloads()
+ .flowOn(Dispatchers.IO) // off-main for Room query
+ .collect { downloadEntities ->
+ _uiState.value = DownloadsUiState.ShowDownloads(
+ downloadEntities.map { it.asDownloadModel(applicationContext) }
+ )
+ }
+}Using a background dispatcher for both the Room fetch and the mapping avoids blocking the main thread while still updating _uiState atomically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| viewModelScope.launch { // this: CoroutineScope | |
| downloadDao.getAllDownloads().flowOn(Dispatchers.IO).collect { downloadEntities: List<DownloadEntity> -> | |
| val downloadModels = downloadEntities.map { it.asDownloadModel(applicationContext) } | |
| _uiState.value = DownloadsUiState.ShowDownloads(downloadModels) | |
| } | |
| } | |
| } | |
| fun observeDownloads() { | |
| - viewModelScope.launch { // this: CoroutineScope | |
| - downloadDao.getAllDownloads().flowOn(Dispatchers.IO).collect { downloadEntities: List<DownloadEntity> -> | |
| - val downloadModels = downloadEntities.map { it.asDownloadModel(applicationContext) } | |
| - _uiState.value = DownloadsUiState.ShowDownloads(downloadModels) | |
| - } | |
| - } | |
| + viewModelScope.launch(Dispatchers.Default) { // off-main for mapping | |
| + downloadDao.getAllDownloads() | |
| + .flowOn(Dispatchers.IO) // off-main for Room query | |
| + .collect { downloadEntities -> | |
| + _uiState.value = DownloadsUiState.ShowDownloads( | |
| + downloadEntities.map { it.asDownloadModel(applicationContext) } | |
| + ) | |
| + } | |
| + } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsViewModel.kt
around lines 42 to 48, the flow collection and mapping are done on the main
thread, which can cause UI jank with large data sets. To fix this, move both the
Room fetch and the mapping operations to a background dispatcher by applying
flowOn(Dispatchers.Default) or another appropriate background dispatcher before
collect, ensuring the mapping does not block the main thread while still
updating _uiState atomically on the main thread.
| fun deleteDownload(mediaSourceId: String) { | ||
| viewModelScope.launch { | ||
| val downloadEntity: DownloadEntity = requireNotNull(downloadDao.get(mediaSourceId)) | ||
| val downloadDir = File(downloadEntity.mediaSource.localDirectoryUri) | ||
| downloadDao.delete(mediaSourceId) | ||
| downloadDir.deleteRecursively() | ||
|
|
||
| val mediaSource = downloadEntity.mediaSource | ||
|
|
||
| val contentId = mediaSource.itemId.toString() | ||
| // Remove media file | ||
| DownloadService.sendRemoveDownload( | ||
| applicationContext, | ||
| JellyfinDownloadService::class.java, | ||
| contentId, | ||
| false, | ||
| ) | ||
|
|
||
| // Remove subtitles | ||
| mediaSource.externalSubtitleStreams.forEach { | ||
| DownloadService.sendRemoveDownload( | ||
| applicationContext, | ||
| JellyfinDownloadService::class.java, | ||
| "$contentId:${it.index}", | ||
| false, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
File & DB deletion run on main thread – move to Dispatchers.IO
deleteDownload touches the DB, performs File.deleteRecursively(), and calls DownloadService.sendRemoveDownload(...). All of that currently executes on the main thread because the surrounding launch {} inherits the default dispatcher from viewModelScope.
-viewModelScope.launch {
+viewModelScope.launch {
+ withContext(Dispatchers.IO) {
val downloadEntity = downloadDao.get(mediaSourceId) ?: return@withContext
val downloadDir = File(downloadEntity.mediaSource.localDirectoryUri)
downloadDao.delete(mediaSourceId)
downloadDir.deleteRecursively()
}
// Media3 API calls can stay on main
...
}Off-loading heavy I/O keeps UI responsive and prevents ANRs, especially when deleting large folders.
🤖 Prompt for AI Agents
In
app/src/main/java/org/jellyfin/mobile/ui/screens/downloads/DownloadsViewModel.kt
between lines 74 and 101, the deleteDownload function performs database
operations, file deletion, and service calls on the main thread due to the
default dispatcher in viewModelScope.launch. To fix this, wrap the entire
coroutine block inside viewModelScope.launch(Dispatchers.IO) to offload these
heavy I/O tasks to the IO dispatcher, ensuring the UI remains responsive and
preventing potential ANRs.
|
Please fix the merge requests so we can proceed with reviews |
4 participants
Convert Downloads screen to Compose
Changes
DownloadsFragment is now just an empty shell used to display the Compose DownloadsScreen
These changes are built on top of update/dependency-coil3
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes