ui: Enhance download actions bottom sheet#15
Conversation
|
@coderabbitai review |
|
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 refactors the download action bottom sheet UI by replacing MaterialButtons with custom LinearLayouts containing ImageViews and TextViews for interactive actions. New vector drawable icons for pause, resume, and delete actions are introduced. Related strings are updated for capitalization, and the download status message is slightly revised. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BottomSheet
participant DownloadManager
User->>BottomSheet: Opens Download Action Bottom Sheet
BottomSheet->>User: Shows actions (Pause/Resume, Delete, Cancel) with icon+label
User->>BottomSheet: Taps Pause/Resume/Delete/Cancel (via LinearLayout)
BottomSheet->>DownloadManager: Executes corresponding action
DownloadManager-->>BottomSheet: Updates download state
BottomSheet-->>User: Updates UI (icon and label) accordingly
Possibly related PRs
Poem
🪧 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 (
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActions.kt (1)
109-119: 🛠️ Refactor suggestionHard-coded status strings hinder localisation & introduce inconsistency
getCurrentDownloadStatus()now returns raw English strings (e.g."Downloading Paused").
Problems:
- Breaks localisation – the rest of the UI uses string resources.
- Inconsistent wording: strings.xml has
"Download Paused"(title) while code returns"Downloading Paused".- Future edits require touching code instead of translations.
-import com.tpstreams.player.R // already implied elsewhere +import com.tpstreams.player.R … -return when { - downloadTracker.isDownloaded(uri) -> "Downloaded" - downloadTracker.isDownloading(uri) -> "Downloading" - downloadTracker.isPaused(uri) -> "Download Paused" - else -> "Download" -} +val res = view.context.resources +return when { + downloadTracker.isDownloaded(uri) -> res.getString(R.string.downloaded) // add to strings.xml + downloadTracker.isDownloading(uri) -> res.getString(R.string.downloading) + downloadTracker.isPaused(uri) -> res.getString(R.string.download_paused) // reuse existing + else -> res.getString(R.string.download) +}You’ll need to add the missing
downloadedanddownloadingentries (lower-case) tostrings.xml.
🧹 Nitpick comments (8)
tpstreams-android-player/src/main/res/drawable/ic_pause_download.xml (1)
1-5: Remove fixed tint to stay theme-awareSame concern as for
ic_delete.xml: the explicitandroid:tint="#000000"means the asset cannot automatically adopt accent / dark-mode colors. Consider removing the attribute and letting the hosting view tint it.tpstreams-android-player/src/main/res/drawable/ic_resume_download.xml (1)
1-5: Follow consistent vector-drawable conventionsFor consistency with other drawables (and future recoloring), drop the inline tint here too. Using one convention across all icons avoids surprises.
tpstreams-android-player/src/main/res/values/strings.xml (1)
31-40: Inconsistent casing & duplicate semanticsGreat move to sentence-case the action labels, but a few follow-ups:
download_buttonremains all-caps (DOWNLOAD) while related actions switched to title-case – consider aligning.downloadvs newdelete,pause_downloadetc. mix verb and noun forms; prefer consistent prefixes (download_*oraction_*).- Added
"resume_download"yet code returns"Downloading"and"Download Paused"– make sure every label has a matching status string to avoid confusion.tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt (4)
16-16: Remove staleMaterialButtonimport
MaterialButtonis no longer referenced after migrating toLinearLayout-based controls. Keeping the import will produce an “unused import” warning and may fail a strict lint build.-import com.google.android.material.button.MaterialButton
68-74: Duplicate “pause / resume” wiring – extract into a helperThe two branches that configure the pause/resume control are almost identical, differing only in drawable, text, and callback. Duplicated
findViewByIdcalls insideupdateUI():
- increase method length / cognitive load,
- allocate view look-ups every time state changes,
- invite accidental divergence between the two blocks.
A small private helper keeps the intent clear and prevents drift:
private fun setupPauseResume(icon: Int, textRes: Int, onClick: () -> Unit) { val container = view!!.findViewById<LinearLayout>(R.id.pause_resume_container) val iconView = view!!.findViewById<ImageView>(R.id.pause_resume_icon) val textView = view!!.findViewById<TextView>(R.id.pause_resume_text) iconView.setImageResource(icon) textView.text = getString(textRes) container.setOnClickListener { onClick(); dismiss() } }Then, inside
updateUI():Download.STATE_DOWNLOADING -> { setupPauseResume( R.drawable.ic_pause_download, R.string.pause_download ) { listener?.onPauseDownloadConfirmed() } } Download.STATE_STOPPED -> { setupPauseResume( R.drawable.ic_resume_download, R.string.resume_download ) { listener?.onResumeDownloadConfirmed() } }Also applies to: 86-92
68-74: Cache view references or use ViewBinding
findViewByIdon everyupdateUI()invocation is cheap but unnecessary. Holding the three pause/resume views aslateinitvals (or switching to ViewBinding) removes repeated look-ups and enforces non-nullability at compile-time.
135-147: User-visible controls lack accessibility role
LinearLayoutcontainers are now acting as buttons. Consider addingViewCompat.setAccessibilityDelegate(container, object : AccessibilityDelegateCompat() { override fun onInitializeAccessibilityNodeInfo(v: View, info: AccessibilityNodeInfoCompat) { super.onInitializeAccessibilityNodeInfo(v, info) info.roleDescription = context.getString(R.string.role_button) } })or, in XML,
android:roleDescription="button"to ensure TalkBack users recognise these views as actionable elements.tpstreams-android-player/src/main/res/layout/layout_download_action_bottom_sheet.xml (1)
55-81: ClickableLinearLayoutbuttons need semantic hintsThe containers are declared clickable/focusable but have no:
android:contentDescriptionfor theImageView(screen-reader announces only the icon’s resource name otherwise).android:roleDescription="button"orandroid:accessibilityHeading.At minimum, set the icon’s
android:contentDescription="@string/delete"(etc.) or callandroid:importantForAccessibility="no"on the icon so that TalkBack reads the accompanying label only once.Also applies to: 96-123, 126-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActionBottomSheet.kt(4 hunks)tpstreams-android-player/src/main/java/com/tpstreams/player/DownloadActions.kt(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_delete.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_pause_download.xml(1 hunks)tpstreams-android-player/src/main/res/drawable/ic_resume_download.xml(1 hunks)tpstreams-android-player/src/main/res/layout/layout_download_action_bottom_sheet.xml(2 hunks)tpstreams-android-player/src/main/res/values/strings.xml(1 hunks)
tpstreams-android-player/src/main/res/layout/layout_download_action_bottom_sheet.xml
Outdated
Show resolved
Hide resolved
syed-tp
changed the title
syed-tp
force-pushed
the
ui/change_dowloading_action_btm_sht
branch
from
8713c4d to
8754dba
Compare
1 participant
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes