Feat/download notifications (#62 #902 #345)#913
Open
eddyizm wants to merge 16 commits into
Open
Conversation
…ded. Exacerbated by emulator testing
…s to maintain parity with db
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDownload notifications now expose pause, resume, and cancel controls, track progress and completion state, and display download metadata. Repository and cache changes support URI persistence and notification filenames, while completed Media3 downloads are exported to configured external directories. ChangesDownload notification controls and lifecycle
Download orchestration and metadata lookup
External export and cache behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DownloaderService
participant DownloadControlReceiver
participant Media3DownloadService
participant ExternalAudioWriter
DownloaderService->>DownloadControlReceiver: publish pause, resume, or cancel action
DownloadControlReceiver->>Media3DownloadService: issue download control command
DownloaderService->>DownloaderService: update notification progress and completion state
DownloaderService->>ExternalAudioWriter: export completed download by media id
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java (1)
373-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the declared notification-id constants instead of raw literals.
DownloaderServicedeclaresFOREGROUND_NOTIFICATION_ID=1,PAUSED_NOTIFICATION_ID=2,COMPLETE_NOTIFICATION_ID=3, but several call sites hardcode the numbers. They match today, so there's no live bug, but any future id change silently desyncs cancel/notify calls.
app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java#L373-L377: replacecancel(2)/notify(2)withPAUSED_NOTIFICATION_IDandcancel(1)withFOREGROUND_NOTIFICATION_ID.app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java#L187: replacemanager.notify(3, …)withCOMPLETE_NOTIFICATION_ID.app/src/main/java/com/cappielloantonio/tempo/broadcast/receiver/DownloadControlReceiver.java#L50: replacecancel(2)with the shared paused-notification id constant (expose it fromDownloaderService).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java` around lines 373 - 377, Replace hardcoded notification IDs with the declared constants: in DownloaderService lines 373-377, use PAUSED_NOTIFICATION_ID for cancel/notify calls and FOREGROUND_NOTIFICATION_ID for cancel(1); in DownloaderService line 187, use COMPLETE_NOTIFICATION_ID for manager.notify(3, …); in DownloadControlReceiver line 50, use the shared PAUSED_NOTIFICATION_ID exposed from DownloaderService.app/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.java (1)
64-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getDownloadByIdduplicatesgetDownload; the Javadoc rationale is inaccurate.Both methods construct
GetDownloadThreadSafe→downloadDao.getOne(id), andgetOneisSELECT * FROM download WHERE id = :idwith no state filter. SogetDownloadByIdis byte-for-byte equivalent togetDownload, and the comment claiminggetDownloadis "subject to the pending/completed filter used elsewhere" does not match the DAO. Consider delegating to avoid drift and fix the doc.♻️ Delegate to remove duplication
public Download getDownloadById(String id) { - Download download = null; - - GetDownloadThreadSafe getDownloadThreadSafe = new GetDownloadThreadSafe(downloadDao, id); - Thread thread = new Thread(getDownloadThreadSafe); - thread.start(); - - try { - thread.join(); - download = getDownloadThreadSafe.getDownload(); - } catch (InterruptedException e) { - e.printStackTrace(); - } - - return download; + return getDownload(id); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.java` around lines 64 - 85, Update getDownloadById to delegate to the existing getDownload implementation instead of duplicating the GetDownloadThreadSafe/thread handling, and revise its Javadoc to accurately describe the shared getOne lookup without claiming a state filter.app/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.java (1)
121-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated, divergent filename sanitization.
This sanitizer's char class includes a backslash (
[\\/:*?"<>|]), butExternalAudioWriter.sanitizeFileNameandExternalAudioReader.sanitizeFileNamedo not include backslash. The predicted externaldownload_uri/notification name computed here can therefore differ from the name the file is actually written under for titles containing\. Consider reusing the singleExternalAudioWriter.sanitizeFileName(now package-visible) to keep prediction, export, and matching in lockstep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.java` around lines 121 - 130, Update the filename construction in DownloaderManager to reuse ExternalAudioWriter.sanitizeFileName instead of maintaining the local replaceAll sanitization logic. Preserve the existing baseName construction and fallback behavior, and ensure the predicted download URI uses the same sanitized name as ExternalAudioWriter and ExternalAudioReader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.java`:
- Around line 233-249: The display-name logic in getDisplayFileName must avoid
deriving “download” from internal MusicUtil URIs. Detect the internal download
path and construct the notification name from the track title and artist, while
retaining the current URI-based filename behavior for external exports and the
existing title/extension fallback.
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java`:
- Around line 184-213: Update the notification construction in
DownloaderService, including the completion, paused, and downloading title/text
paths (and the corresponding occurrence near line 345), to use localized
strings.xml resources instead of hardcoded English. Add a quantity plural
resource for the tracks-downloaded success message and format all dynamic counts
through resource arguments, preserving the existing notification content and
behavior.
- Around line 205-213: Update the active-download title in DownloaderService so
the displayed current index, derived from completed + 1, is capped at
batchTotal. Preserve the paused title and the existing speed suffix and
zero-total behavior.
In `@app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.java`:
- Around line 155-180: The export failure path in ExternalAudioWriter must
remove targetFile so streaming errors or early failures cannot leave partial
downloads; apply this in the catch and any early failure path such as the out ==
null branch. In
app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.java lines
155-180, update the cleanup around the export task while preserving successful
writes. In
app/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioReader.java lines
185-196, keep the unconditional complete-file behavior synchronized with this
writer cleanup guarantee; no separate size verification is needed.
---
Nitpick comments:
In
`@app/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.java`:
- Around line 64-85: Update getDownloadById to delegate to the existing
getDownload implementation instead of duplicating the
GetDownloadThreadSafe/thread handling, and revise its Javadoc to accurately
describe the shared getOne lookup without claiming a state filter.
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.java`:
- Around line 121-130: Update the filename construction in DownloaderManager to
reuse ExternalAudioWriter.sanitizeFileName instead of maintaining the local
replaceAll sanitization logic. Preserve the existing baseName construction and
fallback behavior, and ensure the predicted download URI uses the same sanitized
name as ExternalAudioWriter and ExternalAudioReader.
In `@app/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.java`:
- Around line 373-377: Replace hardcoded notification IDs with the declared
constants: in DownloaderService lines 373-377, use PAUSED_NOTIFICATION_ID for
cancel/notify calls and FOREGROUND_NOTIFICATION_ID for cancel(1); in
DownloaderService line 187, use COMPLETE_NOTIFICATION_ID for manager.notify(3,
…); in DownloadControlReceiver line 50, use the shared PAUSED_NOTIFICATION_ID
exposed from DownloaderService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34810407-b26e-4bcc-83b8-1756a572d658
📒 Files selected for processing (16)
app/src/main/AndroidManifest.xmlapp/src/main/java/com/cappielloantonio/tempo/broadcast/receiver/DownloadControlReceiver.javaapp/src/main/java/com/cappielloantonio/tempo/database/dao/DownloadDao.javaapp/src/main/java/com/cappielloantonio/tempo/repository/DownloadRepository.javaapp/src/main/java/com/cappielloantonio/tempo/service/DownloaderManager.javaapp/src/main/java/com/cappielloantonio/tempo/service/DownloaderService.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/bottomsheetdialog/DownloadedBottomSheetDialog.javaapp/src/main/java/com/cappielloantonio/tempo/util/Constants.ktapp/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioReader.javaapp/src/main/java/com/cappielloantonio/tempo/util/ExternalAudioWriter.javaapp/src/main/java/com/cappielloantonio/tempo/util/ExternalDownloadMetadataStore.javaapp/src/main/res/values/strings.xmlapp/src/test/java/com/cappielloantonio/tempo/model/DownloadModelTest.ktapp/src/test/java/com/cappielloantonio/tempo/repository/DownloadRepositoryTest.ktapp/src/test/java/com/cappielloantonio/tempo/service/DownloaderManagerNotificationTest.ktapp/src/test/java/com/cappielloantonio/tempo/util/ExternalAudioWriterFilenameTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #62 with a major update of the legacy download notification feature.
partially addresses #902 which was discussed in #345 giving the option to pause, resume and cancel
also partially resolves #755 although it does not give a full new dialog/page fragment. We will put some more features, in a phase 2 after this gets rolled into production. Per a previous discussion, notification improvement could possibly handle all of it. We will see once we get more feedback and usage.
Summary by CodeRabbit
New Features
Bug Fixes
Tests