feat: Add non-drm download support - #5
Conversation
WalkthroughThis update introduces comprehensive offline video download and playback support to the Android player module. It adds new activities, UI layouts, utility classes, and download management components, enabling users to download, manage, and play videos offline. The player and its settings UI are enhanced for download integration, with supporting resources and documentation provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainActivity
participant DownloadsActivity
participant PlayerActivity
participant TPStreamsPlayer
participant DownloadUtils
participant VideoDownloadManager
participant DownloadService
User->>MainActivity: Click "Downloads"
MainActivity->>DownloadsActivity: Launch activity
DownloadsActivity->>DownloadUtils: Load downloads
DownloadUtils->>VideoDownloadManager: Query downloads
DownloadsActivity->>User: Show download list
User->>DownloadsActivity: Click "Download" on item
DownloadsActivity->>DownloadUtils: Start download
DownloadUtils->>VideoDownloadManager: Initiate download
VideoDownloadManager->>DownloadService: Start foreground download
DownloadService-->>User: Show notification
User->>DownloadsActivity: Click "Play" on downloaded item
DownloadsActivity->>PlayerActivity: Launch with contentId
PlayerActivity->>TPStreamsPlayer: Play offline content
TPStreamsPlayer->>DownloadUtils: Verify download
TPStreamsPlayer-->>User: Playback starts
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Nitpick comments (43)
tpstreams-android-player/src/main/res/drawable/ic_download_option.xml (1)
2-7: Use theme attribute for fillColor instead of tint for simpler theming.You can remove the
android:tintand apply the theme color directly to the path’sfillColor, improving clarity and backward compatibility:- <vector xmlns:android="http://schemas.android.com/apk/res/android" - android:width="24dp" - android:height="24dp" - android:viewportWidth="24" - android:viewportHeight="24" - android:tint="?attr/colorControlNormal"> + <vector xmlns:android="http://schemas.android.com/apk/res/android" + android:width="24dp" + android:height="24dp" + android:viewportWidth="24" + android:viewportHeight="24"> <path - android:fillColor="@android:color/white" + android:fillColor="?attr/colorControlNormal" android:pathData="M5,20h14v-2H5V20zM19,9h-4V3H9v6H5l7,7L19,9z"/> </vector>app/src/main/java/com/tpstreams/player/utils/NetworkUtils.kt (1)
18-23: Enhance network check with validated capability and safe cast.To reliably detect real internet access and avoid potential
ClassCastException, update as follows:- val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager + val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as? ConnectivityManager + ?: return false - return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) + return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) + && capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED)tpstreams-android-player/src/main/res/drawable/rounded_button_background.xml (2)
4-4: Use dynamic theme attribute
Instead of hardcoding@color/tpstreams_primary, consider using?attr/colorPrimaryor a custom theme attribute to align with theming and support dark mode.
2-6: Enable touch feedback on stateful views
Applying this shape as a button background will override the default ripple. Wrap it in a<ripple>element or useandroid:foreground="?attr/selectableItemBackgroundBorderless"to retain touch feedback.tpstreams-android-player/src/main/res/drawable/ic_download.xml (2)
7-7: Review vector tint usage for compatibility
android:tinton<vector>is only supported on API 21+. For wider support, consider usingapp:tintvia AppCompat or applyandroid:fillColor="?attr/colorControlNormal"directly on<path>.
9-10: Avoid hardcoded white fill
Using@android:color/whitemay conflict with dark themes. Replace with?attr/colorControlNormalor rely on the vector-level tint to ensure theme consistency.app/src/main/res/layout/activity_downloads.xml (3)
11-11: Use snake_case for view IDs
Rename@+id/recyclerViewto@+id/recycler_viewto follow Android’s resource naming conventions.
21-21: Use snake_case for view IDs
Rename@+id/emptyViewto@+id/empty_viewfor consistency.
10-17: Add design-time preview for RecyclerView
Includetools:listitem="@layout/item_download"on theRecyclerViewto visualize list items in the layout editor.app/src/main/res/layout/activity_main.xml (3)
7-7: Re-evaluatefitsSystemWindowsusage
Confirm whetherandroid:fitsSystemWindows="true"is required here; unnecessary use can lead to unexpected window insets behavior.
10-19: Simplify centering constraints
Instead of anchoring both top and bottom for vertical centering on awrap_contentview, use only those constraints withapp:layout_constraintVertical_bias="0.5"to reduce complexity.
35-36: Use explicit directional margins
Replaceandroid:layout_marginHorizontalandandroid:layout_marginVerticalwithlayout_marginStart,layout_marginEnd,layout_marginTop, andlayout_marginBottomto support pre-API 21 devices.app/src/main/res/layout/activity_player.xml (2)
10-17: Clarify view constraints
While the 16:9 ratio defines the player's height, explicitly constraining its bottom edge (e.g.,app:layout_constraintBottom_toTopOf="@id/content_title") can improve layout readability.
19-28: Add preview text for runtime-set views
Sincecontent_titleis set at runtime, includetools:text="Sample Title"for design-time preview without affecting runtime behavior.tpstreams-android-player/src/main/res/layout/layout_player_settings_bottom_sheet.xml (1)
154-233: Consider using different icons for download actions.Both download option and view downloads option use the same icon (
@drawable/ic_download_option). Consider using distinct icons for better UX differentiation (e.g., download icon vs. list/folder icon for viewing downloads).Otherwise, the layout structure follows existing patterns well and maintains consistency with other options.
tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt (5)
4-4: Verify necessity of new imports.Ensure all new imports are actually used in the implementation and consider organizing them consistently.
Apply this diff to group related imports:
import android.app.Dialog import android.content.Intent import android.os.Bundle import android.util.Log import android.view.LayoutInflater import android.view.View import android.view.ViewGroup import android.widget.FrameLayout import android.widget.ImageView import android.widget.LinearLayout import android.widget.TextView + import androidx.fragment.app.FragmentManager import androidx.media3.common.util.UnstableApi + import com.google.android.material.bottomsheet.BottomSheetBehavior import com.google.android.material.bottomsheet.BottomSheetDialog import com.google.android.material.bottomsheet.BottomSheetDialogFragment + import com.tpstreams.player.offline.DownloadListActivityAlso applies to: 11-11, 15-15, 19-19
21-21: Document the purpose of the UnstableApi annotation.The
@UnstableApiannotation indicates usage of experimental APIs. Consider adding a comment explaining why this annotation is necessary for this class.
87-99: Review download status UI logic.The download status logic correctly updates the UI based on the video's download state, but consider the user experience implications.
When a video is downloaded, showing "Downloaded" text with no chevron suggests no action is available. Consider if users might want to:
- Re-download in different quality
- Remove the download
- View download details
If additional actions are needed, you might want to show a different icon or maintain the chevron with different behavior:
if (isDownloaded) { - currentDownloadText.text = "Downloaded" - downloadChevron.visibility = View.GONE + currentDownloadText.text = "Downloaded" + // Keep chevron visible if download management actions are needed + downloadChevron.visibility = View.VISIBLE } else { currentDownloadText.text = "" downloadChevron.visibility = View.VISIBLE }
102-102: Clarify the reason for hiding view downloads option.The "View Downloads" option is explicitly hidden, but the reason isn't clear from the context. Consider adding a comment explaining why this option is hidden in this context.
-// Hide the view downloads option +// Hide the view downloads option - downloads are accessed through a different UI flow view.findViewById<LinearLayout>(R.id.view_downloads_option)?.visibility = View.GONE
122-126: Ensure consistent error handling in click listener.The download option click listener follows the same pattern as other options, which is good for consistency. However, consider what happens if the listener is null.
While the existing pattern is consistent, consider adding null safety for robustness:
view.findViewById<LinearLayout>(R.id.download_option)?.setOnClickListener { Log.d(TAG, "Download option clicked") - listener?.onDownloadSelected() - dismiss() + listener?.let { + it.onDownloadSelected() + dismiss() + } ?: Log.w(TAG, "No listener set for download option") }tpstreams-android-player/src/main/res/layout/activity_download_list.xml (1)
31-41: Consider accessibility improvements for empty state.The empty state TextView is functional but could be enhanced for better accessibility and user experience.
Consider adding content description and improving the empty state:
<TextView android:id="@+id/emptyView" android:layout_width="wrap_content" android:layout_height="wrap_content" + android:contentDescription="@string/no_downloads_description" android:text="No downloads available" + android:textColor="?android:attr/textColorSecondary" android:textSize="18sp" android:visibility="gone" + android:gravity="center" app:layout_constraintBottom_toBottomOf="parent" app:layout_constraintEnd_toEndOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@+id/toolbar" />tpstreams-android-player/build.gradle.kts (2)
67-69: Consider using version catalog for Compose dependencies.The Compose dependencies are hardcoded with specific versions. Consider adding them to the version catalog for consistent dependency management.
Add these to your version catalog and reference them consistently:
-// Compose dependencies for UI components -implementation("androidx.compose.material3:material3:1.2.0") -implementation("androidx.compose.material:material-icons-extended:1.6.3") +// Compose dependencies for UI components +implementation(libs.androidx.compose.material3) +implementation(libs.androidx.compose.material.icons.extended)
77-92: Review publishing configuration placement and versioning.The publishing configuration is functional but could be improved for maintainability.
Consider these improvements:
- Version management: The hardcoded version "1.0.2" should be managed centrally
- Configuration placement: Publishing logic could be moved to a separate script
afterEvaluate { publishing { publications { create<MavenPublication>("release") { groupId = "com.tpstreams" artifactId = "tpstreams-player" - version = "1.0.2" + version = project.findProperty("publishVersion") as String? ?: "1.0.2-SNAPSHOT" from(components["release"]) } } repositories { mavenLocal() + // Consider adding remote repository for releases } } }tpstreams-android-player/src/main/java/com/tpstreams/player/offline/README.md (1)
142-142: Consider using more concise wording.The phrase "not accessible" could be simplified for better readability.
Apply this diff:
-- Downloaded videos are stored in the app's internal storage and are not accessible to other apps +- Downloaded videos are stored in the app's internal storage and are inaccessible to other apps🧰 Tools
🪛 LanguageTool
[style] ~142-~142: Consider using “inaccessible” to avoid wordiness.
Context: ...d in the app's internal storage and are not accessible to other apps - Downloads are automatic...(NOT_ABLE_PREMIUM)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadListActivity.kt (1)
100-103: Optimize RecyclerView updates with DiffUtil.Using
notifyDataSetChanged()is inefficient as it redraws the entire list even for single item changes. This can cause visual glitches and poor performance with large download lists.Consider implementing DiffUtil for efficient updates:
fun updateDownloads(newDownloads: List<Download>) { + val diffCallback = object : DiffUtil.Callback() { + override fun getOldListSize() = downloads.size + override fun getNewListSize() = newDownloads.size + override fun areItemsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean { + return downloads[oldItemPosition].request.id == newDownloads[newItemPosition].request.id + } + override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean { + return downloads[oldItemPosition] == newDownloads[newItemPosition] + } + } + val diffResult = DiffUtil.calculateDiff(diffCallback) downloads = newDownloads - notifyDataSetChanged() + diffResult.dispatchUpdatesTo(this) }tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadService.kt (3)
29-29: Remove unused field or implement its intended functionality.The
activeDownloadsConcurrentHashMap is declared but never used throughout the service. Either implement the tracking functionality it was intended for or remove it to avoid confusion.- private val activeDownloads = ConcurrentHashMap<String, Float>()
52-52: Consider reusing DownloadNotificationHelper instance.Creating a new
DownloadNotificationHelperinstance for each notification might be inefficient. Consider creating it once during service initialization and reusing it.+ private lateinit var downloadNotificationHelper: DownloadNotificationHelper override fun onCreate() { super.onCreate() // ... existing code ... + downloadNotificationHelper = DownloadNotificationHelper(this) } override fun getForegroundNotification( downloads: List<androidx.media3.exoplayer.offline.Download>, notMetRequirements: Int ): Notification { - val downloadNotificationHelper = DownloadNotificationHelper(this) // ... rest of the method ... }
104-118: Enhance error handling and logging in companion method.The
startDownloadmethod has good basic error handling, but could benefit from more specific error types and user feedback mechanisms.Consider adding more specific error handling:
fun startDownload(context: Context, downloadRequest: androidx.media3.exoplayer.offline.DownloadRequest) { Log.d(TAG, "Starting download for: ${downloadRequest.id}, URI: ${downloadRequest.uri}") try { val intent = buildAddDownloadIntent( context, VideoDownloadService::class.java, downloadRequest, /* foreground= */ false ) Util.startForegroundService(context, intent) Log.d(TAG, "Download service started") - } catch (e: Exception) { + } catch (e: SecurityException) { + Log.e(TAG, "Security exception starting download service: missing permissions?", e) + } catch (e: IllegalStateException) { + Log.e(TAG, "Cannot start foreground service in current state", e) + } catch (e: Exception) { Log.e(TAG, "Error starting download service", e) } }app/src/main/java/com/tpstreams/player/PlayerActivity.kt (1)
36-36: Consider making SDK initialization configurable.The SDK is initialized with a hardcoded value
"9q94nm". Consider making this configurable through build variants, resources, or intent extras to support different environments.- TPStreamsPlayer.init("9q94nm", applicationContext) + TPStreamsPlayer.init(BuildConfig.TP_STREAMS_ORG_ID, applicationContext)Or retrieve from intent/resources:
+ val orgId = intent.getStringExtra(EXTRA_ORG_ID) ?: getString(R.string.default_org_id) + TPStreamsPlayer.init(orgId, applicationContext)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/TPStreamsPlayerDownloadExt.kt (1)
52-64: Consider more robust DRM detection.The current DRM detection only checks for
.mpdfile extensions, which may not catch all DRM-protected content. Some DRM content might use different file extensions or URL patterns.Consider a more comprehensive approach:
- // Check if this is a DRM-protected DASH stream (typically .mpd files) - if (videoUrl.endsWith(".mpd") || videoUrl.contains(".mpd?")) { + // Check if this is likely DRM-protected content + if (isDrmProtectedContent(videoUrl)) { Log.e(TAG, "Cannot download DRM-protected content: $videoUrl") // ... existing toast logic return } + private fun isDrmProtectedContent(url: String): Boolean { + return url.endsWith(".mpd") || + url.contains(".mpd?") || + url.contains("drm=") || + url.contains("widevine") || + url.contains("playready") + }tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadNotificationHelper.kt (1)
31-36: Prefer string resources over hard-coded notification text
setContentTitle/ContentTextcurrently use literal strings. Moving them tostrings.xmlkeeps UI copy consistent, enables localisation, and avoids future magic-string edits.- .setContentTitle("Downloading video") + .setContentTitle(context.getString(R.string.download_in_progress))Apply the same treatment for Download complete and Download failed messages.
Also applies to: 53-57, 67-72
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadQualityBottomSheet.kt (2)
67-69: Hard-coded UI string – use resources
"Select Download Quality"should come fromR.string.select_qualityto allow localisation and reuse.
80-84: Padding specified in raw pixels
setPadding(0, 24, 0, 24)uses raw px, resulting in inconsistent size across densities. Convert toTypedValue.applyDimensionor define adimenresource in dp.- setPadding(0, 24, 0, 24) + val vPadding = resources.getDimensionPixelSize(R.dimen.radio_vertical_padding) + setPadding(0, vPadding, 0, vPadding)tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt (3)
492-503: Consider handling empty video info more gracefully.The method logs warnings when video URL or content ID is empty but doesn't prevent subsequent operations that might fail. Consider either throwing an exception or setting a flag to indicate invalid state.
private fun updateVideoInfo() { val tpsPlayer = player as? TPStreamsPlayer ?: return val videoUrl = tpsPlayer.getVideoUrl() val contentId = tpsPlayer.getAssetId() Log.d("TPStreamsPlayerView", "Updating video info - URL: $videoUrl, ID: $contentId") if (videoUrl.isNotEmpty() && contentId.isNotEmpty()) { setVideoInfo(videoUrl, contentId) } else { - Log.w("TPStreamsPlayerView", "Video URL or content ID is empty") + Log.w("TPStreamsPlayerView", "Video URL or content ID is empty - URL: ${videoUrl.isEmpty()}, ID: ${contentId.isEmpty()}") + // Clear any existing video info to prevent stale data + currentVideoUrl = "" + currentContentId = "" + isDownloaded = false } }
662-678: Improve error handling specificity.The generic exception catch could mask specific issues. Consider catching more specific exceptions or at least logging the exception type.
private fun checkDownloadStatus() { if (currentContentId.isEmpty()) { Log.d("TPStreamsPlayerView", "Cannot check download status: Content ID is empty") return } try { // Initialize the download manager first to ensure it's ready TPStreamsPlayerDownloadExt.initializeDownloadManager(context) isDownloaded = TPStreamsPlayerDownloadExt.isDownloaded(context, currentContentId) Log.d("TPStreamsPlayerView", "Checked download status for $currentContentId: $isDownloaded") + } catch (e: IllegalStateException) { + Log.e("TPStreamsPlayerView", "Download manager not properly initialized", e) + isDownloaded = false } catch (e: Exception) { - Log.e("TPStreamsPlayerView", "Error checking download status", e) + Log.e("TPStreamsPlayerView", "Unexpected error checking download status: ${e.javaClass.simpleName}", e) isDownloaded = false } }
731-735: Consider making the status check delay configurable.The hardcoded 1-second delay might not be sufficient for all scenarios. Consider making it configurable or using a callback-based approach.
+ companion object { + private const val DOWNLOAD_STATUS_CHECK_DELAY_MS = 1000L + } + // Implementation of DownloadQualityBottomSheet.DownloadQualityListener override fun onDownloadQualitySelected(videoUrl: String, contentId: String, quality: String) { Log.d("TPStreamsPlayerView", "Starting download: $videoUrl, $contentId, $quality") TPStreamsPlayerDownloadExt.startDownload( context, videoUrl, contentId, selectedQuality = quality ) // Update download status after a delay to allow download to start postDelayed({ checkDownloadStatus() - }, 1000) + }, DOWNLOAD_STATUS_CHECK_DELAY_MS) }app/src/main/java/com/tpstreams/player/DownloadsActivity.kt (3)
236-238: Provide more specific error messages to users.The generic error message "Content not fully downloaded or download is invalid" might not help users understand what went wrong.
// First verify the download is complete and valid if (!DownloadUtils.verifyDownload(this, contentId)) { - Toast.makeText(this, "Content not fully downloaded or download is invalid", Toast.LENGTH_SHORT).show() + val downloadItem = adapter.downloads.find { it.contentId == contentId } + val message = when { + downloadItem == null -> "Download not found" + downloadItem.progress < 100 -> "Download is ${downloadItem.progress}% complete" + else -> "Download is corrupted. Please delete and re-download." + } + Toast.makeText(this, message, Toast.LENGTH_SHORT).show() return }
389-413: Simplify the updateDownloadProgress logic.The method has complex conditional logic that could be simplified for better readability.
fun updateDownloadProgress(updatedDownloads: List<DownloadUtils.DownloadItem>) { try { + // Check if the download list has changed (items added/removed) + val currentIds = downloads.map { it.contentId }.toSet() + val updatedIds = updatedDownloads.map { it.contentId }.toSet() + + if (currentIds != updatedIds) { + // Full refresh needed - list has changed + downloads.clear() + downloads.addAll(updatedDownloads) + notifyDataSetChanged() + return + } + // Update progress for existing items without full refresh updatedDownloads.forEach { updatedItem -> - val existingItem = downloads.find { it.contentId == updatedItem.contentId } - if (existingItem != null && - (existingItem.progress != updatedItem.progress || - existingItem.status != updatedItem.status || - existingItem.isComplete != updatedItem.isComplete)) { + val index = downloads.indexOfFirst { it.contentId == updatedItem.contentId } + if (index != -1) { + val existingItem = downloads[index] + val hasChanged = existingItem.progress != updatedItem.progress || + existingItem.status != updatedItem.status || + existingItem.isComplete != updatedItem.isComplete - // Update the ViewHolder if it's visible - viewHolders[updatedItem.contentId]?.updateProgress(updatedItem) + if (hasChanged) { + downloads[index] = updatedItem + // Update the ViewHolder if it's visible + viewHolders[updatedItem.contentId]?.updateProgress(updatedItem) + } } } - - // If the list has changed (items added/removed), do a full refresh - if (downloads.map { it.contentId }.toSet() != updatedDownloads.map { it.contentId }.toSet()) { - downloads.clear() - downloads.addAll(updatedDownloads) - notifyDataSetChanged() - } } catch (e: Exception) { Log.e(TAG, "Error updating download progress in adapter: ${e.message}", e) } }
431-436: Add null safety checks for findViewById.While unlikely, findViewById could return null if the layout doesn't contain the expected views. Consider using view binding for better type safety.
private val titleTextView: TextView = itemView.findViewById(R.id.titleTextView) private val statusTextView: TextView = itemView.findViewById(R.id.statusTextView) private val progressBar: ProgressBar = itemView.findViewById(R.id.progressBar) private val playButton: MaterialButton = itemView.findViewById(R.id.playButton) private val pauseButton: MaterialButton = itemView.findViewById(R.id.pauseButton) private val resumeButton: MaterialButton = itemView.findViewById(R.id.resumeButton) private val deleteButton: MaterialButton = itemView.findViewById(R.id.deleteButton) + + init { + // Verify all views were found + require(titleTextView != null) { "titleTextView not found in layout" } + require(statusTextView != null) { "statusTextView not found in layout" } + require(progressBar != null) { "progressBar not found in layout" } + require(playButton != null) { "playButton not found in layout" } + require(pauseButton != null) { "pauseButton not found in layout" } + require(resumeButton != null) { "resumeButton not found in layout" } + require(deleteButton != null) { "deleteButton not found in layout" } + }tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt (1)
35-37: Missing initialization path can return stale state
isDownloaded()bypassesinitializeDownloadManager()whereasgetDownloads()doesn’t.
If the first call in the app’s lifetime isisDownloaded(), the underlying manager may still be un-initialised, yielding a false negative.Consider invoking
TPStreamsPlayerDownloadExt.initializeDownloadManager(context)here as well.tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt (3)
372-374: Locale-dependentString.formattriggers detekt warning
String.formatwithout an explicitLocaleuses the device locale and can break on non-US settings (comma decimal separators, etc.).-String.format("%.1f MB/min", bitrateInMbps) +String.format(Locale.US, "%.1f MB/min", bitrateInMbps)Apply the same pattern to other occurrences (e.g. lines 448-450 in
formatBitrate).🧰 Tools
🪛 detekt (1.23.8)
[warning] 373-373: String.format("%.1f MB/min", bitrateInMbps) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
445-450: Same Locale issue informatBitrate-String.format("%.0f Kbps", bitrate / 1000.0) -… -String.format("%.1f Mbps", bitrate / 1000000.0) +String.format(Locale.US, "%.0f Kbps", bitrate / 1000.0) +… +String.format(Locale.US, "%.1f Mbps", bitrate / 1000000.0)🧰 Tools
🪛 detekt (1.23.8)
[warning] 448-448: String.format("%.0f Kbps", bitrate / 1000.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 449-449: String.format("%.1f Mbps", bitrate / 1000000.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
3-19: Several imports are unused
AlertDialog,Toast,DataSource,DataSpec,DefaultHttpDataSource,DownloadHelper,DefaultTrackSelector,MappingTrackSelector,Executors, andRare never referenced.
Remove them to avoid compilation warnings and reduce byte-code size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/com/tpstreams/player/DownloadsActivity.kt(1 hunks)app/src/main/java/com/tpstreams/player/MainActivity.kt(1 hunks)app/src/main/java/com/tpstreams/player/PlayerActivity.kt(1 hunks)app/src/main/java/com/tpstreams/player/PlayerUIViewModel.kt(1 hunks)app/src/main/java/com/tpstreams/player/utils/NetworkUtils.kt(1 hunks)app/src/main/res/drawable/rounded_button_background.xml(1 hunks)app/src/main/res/layout/activity_downloads.xml(1 hunks)app/src/main/res/layout/activity_main.xml(1 hunks)app/src/main/res/layout/activity_player.xml(1 hunks)app/src/main/res/layout/item_download.xml(1 hunks)tpstreams-android-player/build.gradle.kts(2 hunks)tpstreams-android-player/src/main/AndroidManifest.xml(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt(3 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayer.kt(6 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt(6 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadListActivity.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadNotificationHelper.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadQualityBottomSheet.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/README.md(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/SharedCacheUtil.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/TPStreamsPlayerDownloadExt.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadManager.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadService.kt(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_download.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_download_option.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/rounded_button_background.xml(1 hunks)tpstreams-android-player/src/main/res/layout/activity_download_list.xml(1 hunks)tpstreams-android-player/src/main/res/layout/item_download.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_download_quality_bottom_sheet.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_player_settings_bottom_sheet.xml(1 hunks)tpstreams-android-player/src/main/res/values/colors.xml(1 hunks)tpstreams-android-player/src/main/res/values/strings.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadService.kt (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadNotificationHelper.kt (1)
createNotificationChannel(93-105)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadQualityBottomSheet.kt (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt (1)
show(133-137)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadManager.kt (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadService.kt (1)
getDownloadManager(38-41)
tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadQualityBottomSheet.kt (1)
setDownloadQualityListener(31-33)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadNotificationHelper.kt (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadService.kt (1)
createNotificationChannel(80-91)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadListActivity.kt (1)
app/src/main/java/com/tpstreams/player/DownloadsActivity.kt (2)
showDeleteConfirmation(177-186)loadDownloads(152-164)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt (3)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/TPStreamsPlayerDownloadExt.kt (2)
isDownloaded(81-94)getDownloads(139-147)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadManager.kt (2)
isDownloaded(127-141)getDownloads(161-181)tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayer.kt (1)
playOfflineContent(417-486)
app/src/main/java/com/tpstreams/player/DownloadsActivity.kt (3)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt (3)
deleteDownload(126-128)pauseDownload(240-242)resumeDownload(249-251)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/TPStreamsPlayerDownloadExt.kt (2)
pauseDownload(163-175)resumeDownload(182-194)tpstreams-android-player/src/main/java/com/tpstreams/player/offline/VideoDownloadManager.kt (2)
pauseDownload(210-240)resumeDownload(242-271)
🪛 LanguageTool
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/README.md
[style] ~142-~142: Consider using “inaccessible” to avoid wordiness.
Context: ...d in the app's internal storage and are not accessible to other apps - Downloads are automatic...
(NOT_ABLE_PREMIUM)
🪛 detekt (1.23.8)
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt
[warning] 373-373: String.format("%.1f MB/min", bitrateInMbps) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 448-448: String.format("%.0f Kbps", bitrate / 1000.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 449-449: String.format("%.1f Mbps", bitrate / 1000000.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (26)
tpstreams-android-player/src/main/res/values/colors.xml (1)
1-4: Primary color resource looks good.Defining
tpstreams_primaryas#2196F3aligns with Material Blue 500 and is used consistently across new UI components.app/src/main/java/com/tpstreams/player/PlayerUIViewModel.kt (1)
26-32: LGTM: Proper resource cleanup in updatePlayer method.The method correctly releases the old player before assigning the new one, preventing memory leaks.
app/src/main/res/layout/item_download.xml (1)
1-126: LGTM: Well-structured download item layout.The layout follows Material Design guidelines with proper constraint relationships, appropriate button sizing, and good use of visibility controls for different download states. The control flow (Play → Pause → Resume) is intuitive and well-implemented.
tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt (1)
28-28: Excellent interface design for download functionality.The addition of
onDownloadSelected()andisVideoDownloaded()methods to theSettingsListenerinterface follows a clean, consistent pattern with the existing methods. This maintains the separation of concerns between the bottom sheet UI and the implementation logic.Also applies to: 32-32
app/src/main/AndroidManifest.xml (1)
29-39: Well-configured activity declarations.The new activity declarations follow Android best practices:
- Both activities are properly marked as
android:exported="false"for security- Parent activity relationships are correctly established
- Descriptive labels are provided
This configuration ensures proper navigation flow and prevents unauthorized external access to these activities.
tpstreams-android-player/src/main/res/layout/activity_download_list.xml (2)
1-43: Well-structured layout with proper Material Design implementation.The layout follows Android best practices with:
- Proper ConstraintLayout usage for responsive design
- Material Design Toolbar with appropriate theming
- RecyclerView with proper constraints for content area
- Empty state TextView with centered positioning
The constraint relationships ensure the layout adapts well to different screen sizes and orientations.
29-29:Details
✅ Verification successful
Verify the referenced list item layout exists.
The RecyclerView references
@layout/item_downloadin the tools:listitem attribute. Ensure this layout file exists and is properly implemented for the download items.
🏁 Script executed:
#!/bin/bash # Description: Check if the referenced item_download layout exists # Expected: Find the item_download.xml layout file fd "item_download.xml" --type fLength of output: 136
Referenced layout confirmed
I’ve verified that
item_download.xmlexists at both locations in the repo:
app/src/main/res/layout/item_download.xmltpstreams-android-player/src/main/res/layout/item_download.xmlNo further action needed.
tpstreams-android-player/build.gradle.kts (1)
4-5:Details
❓ Verification inconclusive
Verify necessity of new plugins.
The addition of
maven-publishand Kotlin Compose plugins should align with the actual usage in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Check if Compose is actually used in the codebase # Expected: Find Compose-related code in the module rg -A 3 "androidx.compose|@Composable" --type kotlinLength of output: 485
🏁 Script executed:
#!/bin/bash # Description: Check for publishing configuration in tpstreams-android-player module rg -n "publishing" -n tpstreams-android-player/build.gradle.kts rg -n "publications" -n tpstreams-android-player/build.gradle.ktsLength of output: 183
Review plugin additions align with actual usage
- Compose plugin declared in
tpstreams-android-player/build.gradle.kts(lines 4–5) alongside Compose dependencies (material3,material-icons-extended), but no@Composableorandroidx.compose.*references found in Kotlin source files.- Maven-publish plugin declared with a corresponding
publishing { publications { … } }block (lines 78–79), matching expected publishing configuration.Please confirm whether the Compose plugin is required now (or if it’s being added preemptively) and ensure the publishing setup is correct for your artifact lifecycle.
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/README.md (1)
1-147: Excellent documentation quality.The README provides comprehensive coverage of the download functionality with clear examples and practical guidance. The structure is logical and the code samples are helpful for developers integrating this feature.
🧰 Tools
🪛 LanguageTool
[style] ~142-~142: Consider using “inaccessible” to avoid wordiness.
Context: ...d in the app's internal storage and are not accessible to other apps - Downloads are automatic...(NOT_ABLE_PREMIUM)
app/src/main/java/com/tpstreams/player/MainActivity.kt (2)
47-83: Well-structured button handling with appropriate offline checks.The
setupButtonListeners()method demonstrates good separation of concerns with:
- Clear offline mode validation before streaming attempts
- Consistent user feedback via toast messages
- Proper intent creation with required extras
- Clean separation between different content types
42-44: 🛠️ Refactor suggestionReplace deprecated onBackPressed() with modern alternative.
The
onBackPressed()method is deprecated. Use the newerOnBackPressedDispatcherAPI for better lifecycle management.Apply this diff:
toolbar.setNavigationOnClickListener { - onBackPressed() + onBackPressedDispatcher.onBackPressed() }Likely an incorrect or invalid review comment.
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadListActivity.kt (2)
124-151: Comprehensive download state handling.The
bind()method effectively handles all download states with:
- Clear status text mapping for each
Download.STATE_*- Appropriate progress bar visibility and value updates
- Proper UI feedback for different download phases
The implementation provides good user experience with clear visual indicators for download progress and state.
87-93: Well-implemented delete flow with proper user confirmation.The delete functionality follows Android UX best practices with:
- Confirmation dialog to prevent accidental deletions
- Clear user feedback via toast message
- Immediate UI refresh after deletion
- Proper integration with the download management system
tpstreams-android-player/src/main/AndroidManifest.xml (1)
3-23: LGTM! Well-structured manifest additions for download functionality.The manifest changes properly support the new download feature:
- Appropriate permissions for network access and foreground services
- Correct foreground service type (
dataSync) for download operations- Proper intent filter for ExoPlayer's download service restart mechanism
- Security-conscious use of
android:exported="false"app/src/main/java/com/tpstreams/player/PlayerActivity.kt (3)
83-104: Excellent network connectivity handling.The dual network checks (offline mode and internet availability) provide robust protection against streaming failures in poor network conditions.
106-135: Robust offline playback implementation.Good practice to verify download validity before attempting playback, and the error handling provides clear user feedback when downloads are invalid or playback fails.
154-158: Proper resource cleanup in lifecycle method.Correctly releases the player and nullifies the reference to prevent memory leaks.
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/TPStreamsPlayerDownloadExt.kt (3)
20-31: Good initialization pattern with thread safety.The initialization check prevents multiple initialization attempts and includes proper error handling.
66-73: Excellent error handling and delegation pattern.The method properly validates inputs, ensures initialization, and delegates to the appropriate download task while handling exceptions gracefully.
152-156: Clean initialization pattern.The
ensureInitializedmethod provides a clean way to handle lazy initialization throughout the class.tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadNotificationHelper.kt (1)
78-88: Verify that the customOPEN_DOWNLOADSintent is actually handled
Intent().setPackage(...).action("com.tpstreams.player.action.OPEN_DOWNLOADS")will resolve to no activity unless an<intent-filter>is declared. Missing handling will cause thePendingIntentto be a no-op when the user taps the notification.Please confirm that an activity (e.g.
DownloadListActivity) is exported with the same action or switch to an explicit component class:val intent = Intent(context, DownloadListActivity::class.java)tpstreams-android-player/src/main/res/values/strings.xml (1)
23-28: Check for duplicateplayback_speedstring resource
playback_speedwas previously declared in the base strings; adding it again will cause a merge error at build time. Confirm removal of the old declaration or rename the new key.tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt (1)
101-105: LGTM!The lazy initialization of
downloadQualityBottomSheetfollows the same pattern as other bottom sheets in the class.tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayer.kt (2)
36-42: LGTM! Good addition of context and mutable properties.The constructor changes properly support offline functionality by maintaining context and making asset properties mutable for dynamic updates.
519-531: Excellent fallback handling for cache creation!The error handling with fallback to HTTP data source ensures the player continues to work even if cache initialization fails.
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt (1)
215-222: Hard-coded DASH quality indices rarely match real manifest
createDashQualities()fabricates period/renderer/track indices (0-4).
Unless the manifest happens to align exactly,StreamKeycreation indownloadVariantwill throwIllegalArgumentExceptionat runtime.Consider using
DownloadHelperto query the actual track groups instead of guessing indices, or fall back to full-manifest download when track mapping is unknown.
| <View | ||
| android:layout_width="32dp" | ||
| android:layout_height="4dp" | ||
| android:layout_gravity="center_horizontal" | ||
| android:layout_marginTop="8dp" | ||
| android:layout_marginBottom="16dp" | ||
| android:background="#CCCCCC" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded colors with theme attributes for dark-mode support.
Hardcoded hex values prevent proper theming. Use Material attributes instead:
- <View
- android:layout_width="32dp"
- android:layout_height="4dp"
- android:layout_gravity="center_horizontal"
- android:layout_marginTop="8dp"
- android:layout_marginBottom="16dp"
- android:background="#CCCCCC" />
+ <View
+ android:layout_width="32dp"
+ android:layout_height="4dp"
+ android:layout_gravity="center_horizontal"
+ android:layout_marginTop="8dp"
+ android:layout_marginBottom="16dp"
+ android:background="?attr/colorOnSurface" />
- <TextView
- android:id="@+id/title_text"
- android:layout_width="match_parent"
- android:layout_height="wrap_content"
- android:text="Select Download Quality"
- android:textSize="18sp"
- android:textStyle="bold"
- android:textColor="#212121"
- ...
- />
+ <TextView
+ android:id="@+id/title_text"
+ android:layout_width="match_parent"
+ android:layout_height="wrap_content"
+ android:text="Select Download Quality"
+ android:textSize="18sp"
+ android:textStyle="bold"
+ android:textColor="?attr/colorOnSurface"
+ ...
+ />
- <com.google.android.material.button.MaterialButton
- ...
- android:textColor="#FFFFFF"
- android:backgroundTint="@color/tpstreams_primary"
- ... />
+ <com.google.android.material.button.MaterialButton
+ ...
+ android:textColor="?attr/colorOnPrimary"
+ android:backgroundTint="@color/tpstreams_primary"
+ ... />Also applies to: 23-31, 57-63
🤖 Prompt for AI Agents
In
tpstreams-android-player/src/main/res/layout/layout_download_quality_bottom_sheet.xml
around lines 11 to 17, replace the hardcoded color value "#CCCCCC" used in the
android:background attribute with a theme attribute reference that supports dark
mode, such as "?attr/colorSurfaceVariant" or another appropriate Material theme
color. Apply the same replacement for hardcoded colors found in lines 23-31 and
57-63 to ensure consistent theming across the layout.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <shape xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:shape="rectangle"> | ||
| <solid android:color="#2196F3" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reference the primary color resource instead of hardcoding.
Use the defined tpstreams_primary to avoid duplication:
- <solid android:color="#2196F3" />
+ <solid android:color="@color/tpstreams_primary" />📝 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.
| <solid android:color="#2196F3" /> | |
| <solid android:color="@color/tpstreams_primary" /> |
🤖 Prompt for AI Agents
In app/src/main/res/drawable/rounded_button_background.xml at line 4, replace
the hardcoded color value "#2196F3" with a reference to the primary color
resource by using @color/tpstreams_primary. This change avoids duplication and
ensures consistency with the app's color scheme.
| android:id="@+id/emptyView" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:text="No downloads available" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract hardcoded string
Move "No downloads available" into strings.xml to support localization.
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_downloads.xml at line 23, the string "No
downloads available" is hardcoded in the layout file. To support localization,
move this string into the strings.xml resource file by creating a new string
resource with an appropriate name and then reference this string resource in the
layout XML using the @string/ resource syntax.
| <Button | ||
| android:id="@+id/drm_button" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:text="DRM Content" | ||
| android:layout_marginHorizontal="32dp" | ||
| android:layout_marginVertical="8dp" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract button texts
Move hardcoded texts ("DRM Content", "Non-DRM Content", "Downloads") into strings.xml for localization and consistency.
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_main.xml around lines 30 to 36, the button
text "DRM Content" is hardcoded. To support localization and maintain
consistency, move this text into the strings.xml resource file by creating a
string resource entry for "DRM Content" and then reference this string resource
in the button's android:text attribute instead of the hardcoded value. Repeat
this for other hardcoded button texts like "Non-DRM Content" and "Downloads" in
the layout files.
| android:text="Back to Home" | ||
| android:layout_marginTop="16dp" | ||
| app:layout_constraintTop_toBottomOf="@id/content_title" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract button label
Move "Back to Home" into strings.xml to support localization and maintain consistency with other UI text.
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_player.xml lines 34 to 38, the button label
"Back to Home" is hardcoded. To support localization and maintain consistency,
move this string into the strings.xml resource file by creating a new string
entry with an appropriate name, then reference this string resource in the
android:text attribute instead of the hardcoded text.
| // Add this property to the DownloadsActivity class | ||
| private val pausedDownloads = mutableMapOf<String, Boolean>() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define pause stop reason as a constant.
The pause/resume logic uses a magic number 1 as the stop reason for paused downloads. This should be defined as a named constant for clarity.
class DownloadsActivity : AppCompatActivity() {
private lateinit var binding: ActivityDownloadsBinding
private lateinit var adapter: DownloadAdapter
private val TAG = "DownloadsActivity"
+
+ companion object {
+ // Custom stop reason for paused downloads
+ private const val STOP_REASON_PAUSED = 1
+ }
// Map to track currently playing content
private val playersMap = mutableMapOf<String, TPStreamsPlayer>()Then update line 141:
- if (download.state == Download.STATE_STOPPED && download.stopReason == 1) {
+ if (download.state == Download.STATE_STOPPED && download.stopReason == STOP_REASON_PAUSED) {📝 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.
| // Add this property to the DownloadsActivity class | |
| private val pausedDownloads = mutableMapOf<String, Boolean>() | |
| // In DownloadsActivity.kt | |
| class DownloadsActivity : AppCompatActivity() { | |
| private lateinit var binding: ActivityDownloadsBinding | |
| private lateinit var adapter: DownloadAdapter | |
| private val TAG = "DownloadsActivity" | |
| companion object { | |
| // Custom stop reason for paused downloads | |
| private const val STOP_REASON_PAUSED = 1 | |
| } | |
| // Add this property to the DownloadsActivity class | |
| private val pausedDownloads = mutableMapOf<String, Boolean>() | |
| // Map to track currently playing content | |
| private val playersMap = mutableMapOf<String, TPStreamsPlayer>() | |
| // … other members and methods … | |
| // Around line 141, replace the magic number with the named constant: | |
| - if (download.state == Download.STATE_STOPPED && download.stopReason == 1) { | |
| + if (download.state == Download.STATE_STOPPED && download.stopReason == STOP_REASON_PAUSED) { | |
| // handle paused download | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/tpstreams/player/DownloadsActivity.kt around lines 51
to 52, define a named constant for the pause stop reason instead of using the
magic number 1. Declare a constant (e.g., PAUSE_STOP_REASON) with the value 1 at
the top of the class or companion object, then replace all occurrences of the
number 1 used as the pause stop reason, including line 141, with this constant
for better code clarity.
| fun getDownloadStatusString(download: Download): String { | ||
| return when (download.state) { | ||
| Download.STATE_COMPLETED -> "Completed" | ||
| Download.STATE_DOWNLOADING -> "Downloading ${download.percentDownloaded.toInt()}%" | ||
| Download.STATE_FAILED -> "Failed" | ||
| Download.STATE_QUEUED -> "Queued" | ||
| Download.STATE_REMOVING -> "Removing" | ||
| Download.STATE_RESTARTING -> "Restarting" | ||
| Download.STATE_STOPPED -> "Stopped" | ||
| else -> "Unknown" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Negative progress value leaks into UI strings
Download.STATE_DOWNLOADING may report percentDownloaded == -1;
the current formatting would show “Downloading -1%”.
-Download.STATE_DOWNLOADING -> "Downloading ${download.percentDownloaded.toInt()}%"
+Download.STATE_DOWNLOADING -> {
+ val pct = download.percentDownloaded
+ if (pct == C.PERCENTAGE_UNSET) "Downloading"
+ else "Downloading ${pct.toInt()}%"
+}Again, remember to import androidx.media3.common.C.
📝 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.
| fun getDownloadStatusString(download: Download): String { | |
| return when (download.state) { | |
| Download.STATE_COMPLETED -> "Completed" | |
| Download.STATE_DOWNLOADING -> "Downloading ${download.percentDownloaded.toInt()}%" | |
| Download.STATE_FAILED -> "Failed" | |
| Download.STATE_QUEUED -> "Queued" | |
| Download.STATE_REMOVING -> "Removing" | |
| Download.STATE_RESTARTING -> "Restarting" | |
| Download.STATE_STOPPED -> "Stopped" | |
| else -> "Unknown" | |
| } | |
| fun getDownloadStatusString(download: Download): String { | |
| return when (download.state) { | |
| Download.STATE_COMPLETED -> "Completed" | |
| Download.STATE_DOWNLOADING -> { | |
| val pct = download.percentDownloaded | |
| if (pct == C.PERCENTAGE_UNSET) "Downloading" | |
| else "Downloading ${pct.toInt()}%" | |
| } | |
| Download.STATE_FAILED -> "Failed" | |
| Download.STATE_QUEUED -> "Queued" | |
| Download.STATE_REMOVING -> "Removing" | |
| Download.STATE_RESTARTING -> "Restarting" | |
| Download.STATE_STOPPED -> "Stopped" | |
| else -> "Unknown" | |
| } | |
| } |
🤖 Prompt for AI Agents
In
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt
around lines 160 to 170, the getDownloadStatusString function can display a
negative progress value (-1) for Download.STATE_DOWNLOADING, causing incorrect
UI text like "Downloading -1%". Fix this by checking if percentDownloaded is
negative before formatting the string; if it is, display a default or
placeholder value (e.g., 0%) instead. Also, ensure to import
androidx.media3.common.C as required.
| if (download.state != Download.STATE_COMPLETED) { | ||
| Log.d(TAG, "Download not complete for contentId: $contentId, state: ${download.state}") | ||
| return false | ||
| } | ||
|
|
||
| // Check if download percentage is 100% | ||
| if (download.percentDownloaded < 100) { | ||
| Log.d(TAG, "Download not 100% for contentId: $contentId, progress: ${download.percentDownloaded}%") | ||
| return false |
There was a problem hiding this comment.
Completed downloads with percentDownloaded == -1 are rejected
Download.percentDownloaded is often C.PERCENTAGE_UNSET (-1) when the download is finished.
Requiring an explicit 100 makes every valid, completed download fail verification and therefore blocks offline playback.
-// Check if download percentage is 100%
-if (download.percentDownloaded < 100) {
+// If the percentage is known and not yet 100 — treat as incomplete.
+if (download.percentDownloaded != C.PERCENTAGE_UNSET &&
+ download.percentDownloaded < 100) {Import androidx.media3.common.C at the top of the file.
🤖 Prompt for AI Agents
In
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadUtils.kt
around lines 63 to 71, the check for download completion incorrectly rejects
downloads with percentDownloaded == -1 (C.PERCENTAGE_UNSET), which is valid for
completed downloads. Modify the condition to accept downloads where
percentDownloaded is either 100 or C.PERCENTAGE_UNSET (-1). Also, import
androidx.media3.common.C at the top of the file to access the constant.
| CoroutineScope(Dispatchers.Main).launch { | ||
| try { | ||
| // Try to fetch and parse the master playlist | ||
| val manifestContent = withContext(Dispatchers.IO) { | ||
| fetchManifestContent(videoUrl) | ||
| } | ||
|
|
||
| if (manifestContent.isNotEmpty()) { | ||
| Log.d(TAG, "Successfully fetched manifest content, length: ${manifestContent.length}") | ||
|
|
||
| val trackInfoList = parseM3U8Content(manifestContent, videoUrl) | ||
|
|
||
| if (trackInfoList.isNotEmpty()) { | ||
| Log.d(TAG, "Found ${trackInfoList.size} qualities from direct m3u8 parsing") | ||
|
|
||
| // Use the selected quality or default to highest | ||
| val selectedTrackInfo = if (selectedQuality != null) { | ||
| trackInfoList.find { it.quality.startsWith(selectedQuality) } | ||
| } else { | ||
| trackInfoList.firstOrNull() | ||
| } | ||
|
|
||
| if (selectedTrackInfo != null) { | ||
| downloadVariant(mediaItem, selectedTrackInfo) | ||
| } else { | ||
| downloadVariant(mediaItem, trackInfoList.first()) | ||
| } | ||
| } else { | ||
| Log.e(TAG, "No qualities found in the stream") | ||
| // Fallback to default qualities for HLS | ||
| val defaultQualities = createDefaultQualities(videoUrl) | ||
|
|
||
| // Use the selected quality or default to highest | ||
| val selectedTrackInfo = if (selectedQuality != null) { | ||
| defaultQualities.find { it.quality.startsWith(selectedQuality) } | ||
| } else { | ||
| defaultQualities.firstOrNull() | ||
| } | ||
|
|
||
| if (selectedTrackInfo != null) { | ||
| downloadVariant(mediaItem, selectedTrackInfo) | ||
| } else { | ||
| downloadVariant(mediaItem, defaultQualities.first()) | ||
| } | ||
| } | ||
| } else { | ||
| Log.e(TAG, "Failed to fetch manifest content") | ||
| // Fallback to default qualities | ||
| val defaultQualities = createDefaultQualities(videoUrl) | ||
|
|
||
| // Use the selected quality or default to highest | ||
| val selectedTrackInfo = if (selectedQuality != null) { | ||
| defaultQualities.find { it.quality.startsWith(selectedQuality) } | ||
| } else { | ||
| defaultQualities.firstOrNull() | ||
| } | ||
|
|
||
| if (selectedTrackInfo != null) { | ||
| downloadVariant(mediaItem, selectedTrackInfo) | ||
| } else { | ||
| downloadVariant(mediaItem, defaultQualities.first()) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error parsing manifest: ${e.message}", e) | ||
| // Fallback to default qualities | ||
| val defaultQualities = createDefaultQualities(videoUrl) | ||
|
|
||
| // Use the selected quality or default to highest | ||
| val selectedTrackInfo = if (selectedQuality != null) { | ||
| defaultQualities.find { it.quality.startsWith(selectedQuality) } | ||
| } else { | ||
| defaultQualities.firstOrNull() | ||
| } | ||
|
|
||
| if (selectedTrackInfo != null) { | ||
| downloadVariant(mediaItem, selectedTrackInfo) | ||
| } else { | ||
| downloadVariant(mediaItem, defaultQualities.first()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leaking CoroutineScope tied to no lifecycle
CoroutineScope(Dispatchers.Main).launch { … } is created ad-hoc and never cancelled.
If the calling component is destroyed (Activity/Service), the coroutine keeps running, risking leaks and crashes.
Wrap the launch in an Android-aware scope (e.g. lifecycleScope, viewModelScope) or accept a CoroutineScope parameter so the caller can manage its lifetime.
🤖 Prompt for AI Agents
In
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt
between lines 100 and 181, the coroutine is launched using
CoroutineScope(Dispatchers.Main).launch, which creates a scope not tied to any
Android lifecycle and can cause memory leaks or crashes if the component is
destroyed. To fix this, replace the ad-hoc CoroutineScope with an Android
lifecycle-aware scope such as lifecycleScope or viewModelScope if available, or
modify the function to accept a CoroutineScope parameter from the caller so the
coroutine's lifetime is properly managed and cancelled when the component is
destroyed.
| Log.d(TAG, "Fetching manifest from: $url") | ||
| val connection = URL(url).openConnection() as HttpURLConnection | ||
| connection.requestMethod = "GET" | ||
| connection.setRequestProperty("User-Agent", "TPStreamsPlayer") | ||
| connection.connectTimeout = 15000 | ||
| connection.readTimeout = 15000 | ||
|
|
||
| if (connection.responseCode == HttpURLConnection.HTTP_OK) { | ||
| val reader = BufferedReader(InputStreamReader(connection.inputStream)) | ||
| val content = reader.readText() | ||
| reader.close() | ||
| connection.disconnect() | ||
| content | ||
| } else { | ||
| Log.e(TAG, "Failed to fetch M3U8 manifest: HTTP ${connection.responseCode}") | ||
| "" | ||
| } |
There was a problem hiding this comment.
HttpURLConnection isn’t closed on non-200 or error paths
connection.disconnect() is executed only on the happy path; error and exception branches keep the socket open.
try {
…
- if (connection.responseCode == HttpURLConnection.HTTP_OK) {
+ val result = if (connection.responseCode == HttpURLConnection.HTTP_OK) {
…
- content
- } else {
- Log.e(TAG, "Failed to fetch M3U8 manifest: HTTP ${connection.responseCode}")
- ""
- }
+ content
+ } else {
+ Log.e(TAG, "Failed to fetch M3U8 manifest: HTTP ${connection.responseCode}")
+ ""
+ }
+ result
} finally {
connection.disconnect()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
tpstreams-android-player/src/main/java/com/tpstreams/player/offline/DownloadTask.kt
around lines 268 to 284, the HttpURLConnection is disconnected only when the
response code is HTTP_OK, leaving connections open on error or exception paths.
To fix this, ensure connection.disconnect() is called in all cases by moving it
to a finally block or using a try-finally structure so the connection is always
properly closed regardless of the response code or exceptions.
Summary by CodeRabbit
New Features
User Interface
Bug Fixes
Documentation
Chores