Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces comprehensive offline video download support to the Android player. It adds new UI components for download actions and options, integrates a download manager and supporting services, updates the player to enable offline playback, and expands resources and permissions to support the new functionality. Multiple new classes, layouts, and resources are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlayerView
participant DownloadOptionsBottomSheet
participant DownloadActionBottomSheet
participant DownloadController
participant DownloadTracker
participant TPSDownloadService
User->>PlayerView: Clicks Download in Settings
PlayerView->>DownloadTracker: Check download state for media URI
alt Not downloaded
PlayerView->>DownloadOptionsBottomSheet: Show resolution options
User->>DownloadOptionsBottomSheet: Selects resolution and clicks Download
DownloadOptionsBottomSheet->>PlayerView: onDownloadResolutionSelected(resolution)
PlayerView->>DownloadController: startDownload(mediaItem, resolution)
DownloadController->>TPSDownloadService: sendDownload(request)
else Downloading/Paused/Completed
PlayerView->>DownloadActionBottomSheet: Show download action sheet
User->>DownloadActionBottomSheet: Selects action (Pause/Resume/Delete/Cancel)
DownloadActionBottomSheet->>PlayerView: onPause/onResume/onDelete/onCancel
PlayerView->>DownloadTracker: pauseDownload/resumeDownload/removeDownload(uri)
DownloadTracker->>DownloadController: pauseDownload/resumeDownload/removeDownload(id)
DownloadController->>TPSDownloadService: pauseDownloads/resumeDownloads/removeDownload(id)
end
Poem
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 review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ 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: 10
🧹 Nitpick comments (19)
tpstreams-android-player/src/main/res/values/styles.xml (1)
10-10: Use a color resource or theme attribute instead of a hard-coded hex
Referencing#2196F3directly makes future palette changes error-prone. Extract this into a color resource (e.g.@color/colorAccent) or reference the theme attribute (?attr/colorAccent) for consistency.tpstreams-android-player/src/main/res/drawable/ic_download.xml (1)
1-13: Enable theme-driven icon tinting
The vector’sfillColoris hard-coded to black, which may prevent runtime tinting via Material components. Consider replacing it with a theme attribute (e.g.?attr/colorOnSurface) or omittingfillColorand relying on the host view’siconTint.tpstreams-android-player/src/main/res/layout/item_download_resolution.xml (1)
1-39: Extract hard-coded text styles and colors into resources
Inline text sizes, colors, and margins are repeated values. Define reusable styles (textAppearance) and color resources (@color/...) to maintain consistency and simplify theming.tpstreams-android-player/src/main/res/layout/layout_download_options_bottom_sheet.xml (1)
53-68: Extract button styling into a reusable style
Hard-codedbackgroundTint,iconTint, paddings, and textColor risk duplication. Define a style instyles.xmlor leverage theme attributes (?attr/colorPrimary,?attr/colorOnPrimary) to enforce consistency across buttons.tpstreams-android-player/src/main/res/layout/layout_download_action_bottom_sheet.xml (4)
11-17: Add contentDescription for decorative View
The drag handle is purely decorative and currently focusable by accessibility services. Addandroid:contentDescription="@null"to the<View>to mark it as non-interactive.
17-17: Replace hard-coded colors with resource references
Switch hex literals (e.g.#CCCCCC,#212121,#757575, button tints) to named color resources or theme attributes to support theming and maintain consistency.Also applies to: 27-27, 39-39, 61-61, 83-83, 94-94
12-17: Use dimension resources for margins and paddings
Extract inlinedpvalues (margins/paddings) into@dimen/resources to centralize spacing definitions and improve consistency.Also applies to: 28-30, 40-42
53-61: Consolidate MaterialButton styling
Extract repeated attributes (textColor, backgroundTint, layout params) into a shared style instyles.xmlto reduce duplication and simplify future style changes.Also applies to: 75-83, 86-94
tpstreams-android-player/src/main/res/drawable/ic_download_done.xml (1)
8-8: Use color resource instead of hex literal
Replaceandroid:fillColor="#FF000000"with a color resource (e.g.,@color/black) or theme attribute to allow easy theming.tpstreams-android-player/src/main/res/drawable/ic_download_progress.xml (1)
1-1: Replace hex tint with color resource
Use a named color resource (e.g.,@color/icon_tint) or theme attribute forandroid:tintinstead of#000000to support dark/light theming.tpstreams-android-player/src/main/res/layout/layout_player_settings_bottom_sheet.xml (3)
172-180: Remove unnecessary ID on static label
The TextView displaying the static "Download" label does not need an ID (@+id/download_text) since it isn’t referenced in code. Remove it to match other option layouts.
165-169: Make download icon decorative
Similar to other option icons, this ImageView does not require an ID and should be marked decorative. Remove@+id/download_iconand addandroid:contentDescription="@null".
183-189: Mark chevron icon as decorative & use theme tint
Addandroid:contentDescription="@null"to the chevron ImageView and replaceandroid:tint="#757575"with a theme attribute (e.g.,?attr/colorOnSurfaceVariant) for consistency.tpstreams-android-player/src/main/res/values/strings.xml (2)
23-24: Disambiguate resource name 'download'
The genericdownloadkey may collide with other uses. Consider renaming todownload_labeloraction_downloadfor clarity.
29-31: Use more specific name for 'delete' string
deleteis too generic and could conflict with other delete actions. Rename todelete_download_actionor similar for better context.tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadPermissionHandler.kt (1)
25-37: Clarify permission-request contract
requestNotificationPermission()returns a Boolean that signals whether the permission is already granted, yet the caller inTPStreamsPlayerViewignores the return value and proceeds to start the download immediately.
Consider:
- Rename the method to something like
ensureNotificationPermission()and drop the Boolean return, or- Keep the Boolean but make callers branch on it before starting any foreground-service work.
Using
registerForActivityResult(RequestPermission())would also modernise the flow and give you a callback with the user’s decision.tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt (1)
738-764: Cache download status while the sheet is open
getCurrentDownloadStatus()/getDownloadIcon()hitDownloadTrackeron every invocation.
The values change slowly; cache them when the settings sheet is shown and refresh via a listener fromDownloadTrackerinstead.tpstreams-android-player/src/main/java/com/tpstreams/player/download/TPSDownloadService.kt (1)
94-103: MakenotificationHelperthread-safe
notificationHelperis a mutablecompanion objectfield written fromonCreate(); if the service is started concurrently multiple times you risk a race.
Initialise it with Kotlin lazy (or@Volatile+ double-checked lock) to guarantee a single instance.tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt (1)
46-94: Avoid piling up click listeners inupdateUI()Every time
setDownloadState()is called,updateUI()re-attaches a newOnClickListenertopause_resume_button.
Use a single listener whose behaviour switches on the current state, or clear the existing listener before setting a new one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
tpstreams-android-player/src/main/AndroidManifest.xml(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadOptionsBottomSheet.kt(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(7 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt(6 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadPermissionHandler.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadTracker.kt(1 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/download/TPSDownloadService.kt(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_download.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_download_done.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_download_progress.xml(1 hunks)tpstreams-android-player/src/main/res/layout/item_download_resolution.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_download_action_bottom_sheet.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_download_options_bottom_sheet.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_player_settings_bottom_sheet.xml(7 hunks)tpstreams-android-player/src/main/res/values/strings.xml(1 hunks)tpstreams-android-player/src/main/res/values/styles.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt (2)
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadOptionsBottomSheet.kt (1)
show(170-174)tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt (1)
show(113-117)
tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt (5)
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadOptionsBottomSheet.kt (2)
setDownloadSelectionListener(47-49)setAvailableResolutions(51-62)tpstreams-android-player/src/main/java/com/tpstreams/player/AdvancedResolutionBottomSheet.kt (1)
setAvailableResolutions(33-38)tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt (1)
setDownloadActionListener(33-35)tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt (1)
startDownload(158-181)tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadTracker.kt (1)
startDownload(98-101)
🔇 Additional comments (6)
tpstreams-android-player/src/main/AndroidManifest.xml (2)
3-10: Validate storage permissions and scoped storage approach
UsingWRITE_EXTERNAL_STORAGE(maxSdkVersion=28) andREAD_EXTERNAL_STORAGE(maxSdkVersion=32) is deprecated on newer Android releases. Ensure you’ve adopted scoped storage (MediaStore or SAF) for API 29+ and that runtime request logic aligns with your storage strategy.
12-17: Verify boot completed handling and service registration
You’ve declaredTPSDownloadServiceand addedRECEIVE_BOOT_COMPLETEDpermission, but no receiver is defined here. Confirm that you handleBOOT_COMPLETEDevents elsewhere (e.g. a BroadcastReceiver) or remove the unused permission.tpstreams-android-player/src/main/res/layout/layout_download_options_bottom_sheet.xml (2)
1-8: LGTM – Root layout configuration
The bottom-sheet container, padding, and background match the design spec.
31-38: RecyclerView setup is appropriate
layout_weight="1"andoverScrollMode="never"provide the expected scrolling behavior.tpstreams-android-player/src/main/java/com/tpstreams/player/PlayerSettingsBottomSheet.kt (1)
69-110: Looks good – download option integrates cleanlyThe added view look-ups and listeners are concise and null-safe.
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadTracker.kt (1)
130-133:getExternalFilesDir()may be nullSame NPE risk as in
DownloadController; validate before using the path.
tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayerView.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadController.kt
Outdated
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadOptionsBottomSheet.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadTracker.kt
Outdated
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/download/DownloadTracker.kt
Show resolved
Hide resolved
tpstreams-android-player/src/main/java/com/tpstreams/player/TPStreamsPlayer.kt
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Chores