fix: Make "Play Next" actually play next under shuffle (#329)#910
Conversation
|
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:
📝 WalkthroughWalkthroughChangesPlay-next shuffle workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/BaseMediaService.kt`:
- Around line 179-223: Replace the single pending-request fields used by
requestPlayNextFixup and tryApplyPlayNextFixup with a FIFO queue of (insertPos,
count, target) requests. Enqueue every valid request, then have
tryApplyPlayNextFixup inspect the queue head and retain it until its target item
count is visible and the fixup is successfully handled; remove only the
processed or explicitly invalidated request, then continue applying any newly
eligible queued requests so overlapping Play Next operations are preserved.
In `@app/src/main/java/com/cappielloantonio/tempo/service/MediaManager.java`:
- Around line 357-380: Update insertPlayNext to retain the ListenableFuture
returned by browser.sendCustomCommand and attach failure handling that logs
rejected or failed fixup commands, including the throwable details. Keep the
existing timeline insertion and successful-command behavior unchanged, and use
the class’s established logging mechanism.
🪄 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: 86c48f3b-8ee5-4cba-9e9a-8a1e7998ac04
📒 Files selected for processing (4)
app/src/main/java/com/cappielloantonio/tempo/service/BaseMediaService.ktapp/src/main/java/com/cappielloantonio/tempo/service/BaseSessionCallback.ktapp/src/main/java/com/cappielloantonio/tempo/service/MediaManager.javaapp/src/main/java/com/cappielloantonio/tempo/util/Constants.kt
Summary
Applied the "Play Next under shuffle" fix (from @herrerad85 commit #c55440e3d9d55988995cdce759470d45962ac6e7 ) across 4 files, adapted to the current repo (which was in the patch's exact "before" state) resolves #329 :
Root cause it fixes: under shuffle, getNextMediaItemIndex() is shuffle-order-dependent and ExoPlayer randomizes a newly-inserted item's shuffle position — so "play next" landed at a random spot. Now the item is deterministically inserted at current+1 on the timeline and re-spliced into shuffle position current+1.