Repository navigation
fix(playback): put rewind and forward on either side of play in system controls - #1232
Conversation
ashwkun
added
the
user-impact-medium
 |
| Layer / File(s) | Summary |
|---|---|
Define button preferences core/playback/src/main/java/cx/aswin/boxlore/core/playback/service/PlaybackServicePlayerFactory.kt, core/playback/src/test/java/cx/aswin/boxlore/core/playback/service/PlaybackButtonSlotsTest.kt, core/catalog/src/main/res/drawable/rounded_skip_next_24.xml, core/catalog/src/main/res/values/strings.xml, core/playback/src/test/java/cx/aswin/boxlore/core/playback/service/PlaybackServicePlayerFactoryTest.kt |
The factory assigns rewind, forward, and Mark Complete to the back, forward, and overflow slots. It creates a skip-next button for the overflow slot and provides ordered preferences with Next episode as an optional button. Tests check slot assignments, optional button behavior, and custom seek lengths. |
Publish preferences during playback core/playback/src/main/java/cx/aswin/boxlore/core/playback/service/BoxLorePlaybackService.kt, core/playback/src/test/java/cx/aswin/boxlore/core/playback/service/BoxLorePlaybackServiceFallbackTest.kt, core/playback/README.md, release-notes/upcoming.md |
The playback service publishes preferences after session setup and updates them when relevant player events change next-item availability. The README describes the preferences and testing notes. The release note describes control order and when Next episode appears. |
Add skip-next to Android Auto core/playback/src/main/java/cx/aswin/boxlore/core/playback/service/auto/AutoBrowseContract.kt, core/playback/src/main/java/cx/aswin/boxlore/core/playback/service/auto/AutoBrowseLibraryHost.kt, core/playback/src/main/java/cx/aswin/boxlore/core/playback/service/auto/AutoBrowseLibraryCallback.kt, core/playback/src/test/java/cx/aswin/boxlore/core/playback/service/auto/AutoBrowseLibraryCallbackTest.kt |
The Android Auto callback exposes the skip-next command and includes its button only when a next item exists. The command advances the player to the next item. Tests check button availability and command handling. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Low
Merge Risk: 
52f26
-
Hardware keys seek by the configured interval rather than navigating episodes as documented. Queue-driven button updates also need regression coverage; these are bounded follow-ups.
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
The title uses the required Conventional Commits format and accurately describes the playback-control change, but it is 79 characters and exceeds the approximate 72-character limit. | Shorten the title to about 72 characters or fewer while keeping the imperative mood, for example: "fix(playback): place rewind and forward around play". |
| Unresolved Review Threads | |
Four review findings remain unresolved. Two posted CodeRabbit threads are marked discussion=unresolved: the source-size finding in BoxLorePlaybackService.kt and the regression-test finding in `Playb… |
Fix all four findings and mark each review thread resolved, or explicitly dismiss each finding with a short rationale. |
| Out of Scope Changes check |  |
The current diff adds a Next episode control that the PR does not describe. The PR description states the result is "rewind / play / forward everywhere, with Mark played in the overflow." The code add… | Remove the Next episode path: the skipNext button, the COMMAND_SKIP_NEXT command and handler, the nextEpisode parameter and its call sites, the skip-next drawable and string, and the related README and release-note lines. Keep rewind … |
| Docstring Coverage | |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check [ #1231] requires system controls in the order skip back, play/pause, skip forward. The current diff meets this in code.buildSeekButtonssetsSLOT_BACKon rewind andSLOT_FORWARDon forward. Mar…Architecture Compliance PASS. The authoritative diff changes only playback service/session code, playback tests/docs, catalog resources, and release notes. It adds no feature-to-feature import or dependency, no PostHog, Hilt… Module Readme Updated The PR changes production Kotlin under core/playback/src/main/. It also modifies the matching module README,core/playback/README.md, in the same PR. No other changed production Kotlin module lack…Jvm Tests For Changed Logic The PR adds hermetic Robolectric/JVM tests under core/playback/src/test.PlaybackButtonSlotsTestcovers slot assignment, queued and empty queue behavior, and custom skip lengths. `AutoBrowseLibrar…Description check The pull request description directly explains the playback-control changes, their motivation, affected system controls, implementation details, compatibility behavior, testing, and release notes. Full details: Linked Issues check
-
Explanation
[
#1231] requires system controls in the order skip back, play/pause, skip forward. The current diff meets this in code.buildSeekButtonssetsSLOT_BACKon rewind andSLOT_FORWARDon forward. Mark played usesSLOT_OVERFLOW.playbackButtonPreferencesfeedssetMediaButtonPreferencesinbuildMediaLibrarySessionand inAutoBrowseLibraryCallback.onConnect. The optional Next episode button uses overflow and does not take the back or forward slot, so it does not change the requested order. The PR states thatPlaybackButtonSlotsTestcovers slots with custom skip lengths; that test file was not re-read. Samsung Now Bar behavior is unverified on a device, and JVM tests do not cover the platform playback-state conversion. The code evidence supports compliance, so the verdict is PASS.
Full details: Architecture Compliance
-
Explanation
PASS. The authoritative diff changes only playback service/session code, playback tests/docs, catalog resources, and release notes. It adds no feature-to-feature import or dependency, no PostHog, Hilt, Koin, Dagger, or MockK usage, no second PlaybackRepository or AppContainer graph, and no identity or storage contract. No build or dependency files changed. The catalog additions are resources consumed by core:playback, which matches ARCHITECTURE.md's allowed playback → catalog direction.
Full details: Module Readme Updated
-
Explanation
The PR changes production Kotlin under
core/playback/src/main/. It also modifies the matching module README,core/playback/README.md, in the same PR. No other changed production Kotlin module lacks its matching README.
Full details: Jvm Tests For Changed Logic
-
Explanation
The PR adds hermetic Robolectric/JVM tests under
core/playback/src/test.PlaybackButtonSlotsTestcovers slot assignment, queued and empty queue behavior, and custom skip lengths.AutoBrowseLibraryCallbackTestcovers queued/empty Next episode visibility and the skip command. The fallback test exercises theSecurityExceptionpath. These tests cover the changed playback button logic.
-
Full details: Unresolved Review Threads
-
Explanation
Four review findings remain unresolved. Two posted CodeRabbit threads are marked discussion=unresolved: the source-size finding in
BoxLorePlaybackService.ktand the regression-test finding inPlaybackButtonSlotsTest.kt; both have Severity: Major. Two newly generated findings are also outstanding: the README hardware-key description is Severity: Minor, and the service-side button-update regression is Severity: Major. No finding is marked resolved or explicitly dismissed.Resolution
Fix all four findings and mark each review thread resolved, or explicitly dismiss each finding with a short rationale.
Full details: Out of Scope Changes check
-
Explanation
The current diff adds a Next episode control that the PR does not describe. The PR description states the result is "rewind / play / forward everywhere, with Mark played in the overflow." The code adds: a
skipNextCommandButtonwithSLOT_OVERFLOWandCOMMAND_SKIP_NEXT(PlaybackServicePlayerFactory.kt); anextEpisodeparameter inplaybackButtonPreferences; aCOMMAND_SKIP_NEXTsession command and handler inAutoBrowseLibraryCallback.kt; therounded_skip_next_24.xmldrawable; and theauto_skip_nextstring. [#1231] asks only for skip back, play/pause, and skip forward. The Next episode button adds a new user-visible control that the issue does not request. The release notes text in the README andrelease-notes/upcoming.mdmentions it, but the PR changelog copy does not. The prior out-of-scope PASS does not cover this addition.Resolution
Remove the Next episode path: the
skipNextbutton, theCOMMAND_SKIP_NEXTcommand and handler, thenextEpisodeparameter and its call sites, the skip-next drawable and string, and the related README and release-note lines. Keep rewind inSLOT_BACK, forward inSLOT_FORWARD, and Mark played inSLOT_OVERFLOW, as the PR description states. If the Next episode button is intended, move it to a separate PR. Then update the PR description, changelog copy, and release notes to describe it and to justify it against [#1231].
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 9 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
- update changelog
-
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Summary
Closes #1231
Motivation
What changed
buildSeekButtonssetsSLOT_BACKon rewind andSLOT_FORWARDon forward; Mark played getsSLOT_OVERFLOW.COMMAND_SKIP_NEXT,SLOT_OVERFLOW) comes last, after Mark played, while the player has a next item; system players that put one extra button on each side of the row (HyperOS) then show it on the right. It runs the session player'sseekToNext(the service'shandleSkipNext), so the skip setting (just skip / mark played and skip) applies. The service republishes the buttons when the timeline, current item, repeat or shuffle changes whether a next item exists, because skipping with nothing queued stops playback.playbackButtonPreferences, is set as media button preferences in all three places that set buttons: the session builder,updateSeekCommandButtonsafter a skip-length change, andAutoBrowseLibraryCallback.onConnect(previouslysetCustomLayout).ACTION_SKIP_TO_PREVIOUS/ACTION_SKIP_TO_NEXTout of the platform playback state and lists rewind and forward first among its custom actions (MediaSessionLegacyStubin 1.10.1), so system players place them around play/pause.core/playback/README.mdandrelease-notes/upcoming.mdupdated.Behavior & compatibility
AutoBrowseLibraryCallback.onMediaButtonEventturns them into seek forward/back.Impact (required)
user-impact-criticaluser-impact-highuser-impact-mediumuser-impact-lowno-user-impactListener impact
What changes in the user’s life:
The lock screen, notification and Samsung's Now Bar show rewind, play and forward in the usual order, so the right-hand button skips forward instead of rewinding.
backend-changeRelease copy (verbatim)
CHANGELOG.md (required developer copy)
Fixed
SLOT_BACK, forwardSLOT_FORWARD, Next episode and Mark playedSLOT_OVERFLOW) viaplaybackButtonPreferences, so system players show rewind, play, forward instead of previous, play, rewind when nothing is queued next; Next episode is offered only while a next item exists.Listener notes (required for user-impact changes)
release-notes/upcoming.mdgains one line under Fixed.Test plan
:core:playback:testDebugUnitTest(591 tests, including the newPlaybackButtonSlotsTest: order and slots, Next only with a next item, custom skip lengths; andAutoBrowseLibraryCallbackTestfor Next on connect and its skip),detekt,:core:playback:lintDebugNotes (optional)
MediaControllerto a session, so the platform playback-state conversion isn't covered by JVM tests; it's Media3's documented slot behavior and needs the device checks above.