Treat KEYCODE_MEDIA_PLAY as play-only to stop repeated Android Auto pauses

[joashrajin]

Description

Wireless Android Auto users report that playback repeatedly pauses a few seconds after pressing play (PCDROID-516, #3543). Logs show the head unit (com.google.android.projection.gearhead) sending redundant KEYCODE_MEDIA_PLAY key events while playback is already running. Since #3297 routed KEYCODE_MEDIA_PLAY into the same single-tap play/pause toggle as KEYCODE_MEDIA_PLAY_PAUSE, each redundant play event toggled playback into a pause — producing the play/pause loop.

This PR gives KEYCODE_MEDIA_PLAY its explicit play-only semantics: a single tap from it is ignored when playback is already running, in both Media3SessionCallback and the legacy MediaSessionManager. The key stays in the MediaEventQueue multi-tap system, so the headset double/triple-tap skip behavior introduced by #3297 (and the Pixel Buds spurious-event suppression) is unchanged, and KEYCODE_MEDIA_PLAY_PAUSE / KEYCODE_HEADSETHOOK still toggle as before.

This should fix the repeated-pause loop; it needs on-device verification with a wireless Android Auto head unit (see TODOs below).

Fixes PCDROID-516

Testing Instructions

Unit tests:

• Run ./gradlew :modules:services:repositories:testDebugUnitTest --tests "*Media3SessionCallbackTest*"
• Verify the new tests pass (KEYCODE_MEDIA_PLAY is ignored while already playing, KEYCODE_MEDIA_PLAY_PAUSE still toggles while playing)

On device:

⚠️ TODO (manual): Needs a car/head unit with wireless Android Auto to reproduce.

• Connect to a car with wireless Android Auto (Bluetooth enabled) and start playing an episode
• Verify playback no longer pauses on its own every few seconds
• Pause and resume via the steering wheel / head unit controls and the Android Auto screen
• Verify play, pause, and resume all still work
• With regular Bluetooth headphones, single/double/triple-tap the play button
• Verify play/pause, skip forward, and skip back still work per the headphone controls settings

Screenshots or Screencast

n/a — no UI changes.

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have considered whether it makes sense to add tests for my changes — added 3 unit tests in Media3SessionCallbackTest
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml — n/a, no strings
• Any jetpack compose components I added or changed are covered by compose previews — n/a, no UI
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics — n/a, no analytics changes

I have tested any UI changes...

n/a — no UI changes.

[claude]

Claude finished @joashrajin's task in 1m 45s —— View job

---

Code Review

• Read changed files & surrounding context
• Analyze correctness, edge cases, and consistency
• Post review feedback

I traced the change through Media3SessionCallback, MediaSessionManager, MediaEventQueue, and PlaybackManager. Overall this is a clean, well-targeted fix with good tests. Details below.

---

✅ What's correct

• The core logic is right. PlaybackManager.playPause() (line 504–510) toggles based on isPlaying(). Guarding with if (playOnly && playbackManager.isPlaying()) return gives KEYCODE_MEDIA_PLAY true play-only semantics: while paused it still resumes (isPlaying()==false → falls through to playPause() → playQueue()), and while playing it's a no-op instead of a pause. That precisely breaks the redundant-play → pause loop.
• Keeping KEYCODE_MEDIA_PLAY in the MediaEventQueue is the right call — the Pixel Buds spurious-event suppression (multiTapJob?.isActive) and the double/triple-tap skip behaviour from Fix skip forward/back on some remote devices such as wired and wireless headsets #3297 are preserved. The playOnly guard only affects the SingleTap output, so multi-tap skips are untouched.
• playOnly reflects the correct tap. In a multi-tap sequence the SingleTap/DoubleTap output is returned by the first tap's SingleTapJob coroutine, so keyEvent.keyCode captured in that coroutine is the first tap's — the flag is only ever consulted for the genuine single-tap case. No aliasing bug there.
• Symmetry with KEYCODE_MEDIA_PAUSE, which already has explicit pause-only handling (pauseSuspend). This brings MEDIA_PLAY in line.
• Good, focused unit tests covering both the ignore-while-playing and the still-toggle (PLAY_PAUSE) cases.

---

🟡 Suggestions (non-blocking)

1. The legacy MediaSessionManager change has no new test coverage.
The identical guard was added to MediaSessionManager.handleMediaButtonSingleTap (line 1284), but the new tests only cover Media3SessionCallback. The only MediaSessionManagerTest is an androidTest (instrumentation) file, so this branch ships untested. Since it's the exact same logic, consider whether the legacy path warrants a mirrored test or is now dead enough not to matter — worth a sentence in the PR either way.

2. Duplicated logic across two callbacks.
The guard block (comment + isPlaying() check + log + early return) is copy-pasted verbatim into both files. That's acceptable given they're independent callback classes, but it's the kind of thing that drifts. A tiny shared helper (e.g. on PlaybackManager, fun playIfPaused(...)) would keep them in sync. Low priority.

3. Minor: isPlaying() is now called twice on the play path.
isPlaying() does a blocking RxJava read (playbackStateRelay.blockingFirst()), and playPause() calls it again internally. This runs on a coroutine dispatcher (not main), and it's one extra cheap blocking read, so it's negligible — flagging only for completeness.

---

Verdict

The fix is correct and minimal, and the reasoning in the PR body matches what the code does. The only thing I'd genuinely want addressed before merge is a note on the legacy MediaSessionManager path (item 1) — the rest are optional polish. As the PR notes, the fix still needs on-device verification with a wireless Android Auto head unit since that path isn't exercised by unit tests.

[claude]

Claude finished @joashrajin's task in 2m 5s —— View job

---

Code Review

• Read changed files & surrounding context
• Analyze correctness, edge cases, and consistency
• Post review feedback

Nice, focused fix. The root cause analysis is sound: since #3297 routed KEYCODE_MEDIA_PLAY into the same SingleTap → playPause() toggle path, a redundant play event from the head unit toggled a playing session into paused. Gating the toggle behind playOnly && isPlaying() gives KEYCODE_MEDIA_PLAY the play-only semantics the Android spec intends, while leaving the multi-tap routing (and the double/triple-tap skip + Pixel Buds suppression) untouched. 👍

Correctness ✅

• Behavior is right in all the interesting states: playing → ignored; paused → playPause() resumes (else-branch of playPause calls playQueue); buffering/loading → worst case a redundant playQueue, never a pause — so the reported loop can't recur even during the transition window.
• KEYCODE_MEDIA_PLAY_PAUSE and KEYCODE_HEADSETHOOK still toggle as before (playOnly = false), and multi-tap still works because playOnly is only consulted on the SingleTap output.
• Both the modern (Media3SessionCallback) and legacy (MediaSessionManager) paths were updated identically — good that the two didn't drift.

Suggestions / things worth noting

1. Test coverage is asymmetric. The 3 new tests cover Media3SessionCallback only; the identical change in MediaSessionManager is untested. There's no MediaSessionManagerTest, so I understand the friction, but the legacy path is exactly where a future refactor could silently regress. Worth at least a note in the PR that the legacy branch relies on the Media3 tests for equivalence — or a small test if the harness allows.

2. Duplicated logic + comment across two files. The guard block (comment, isPlaying() check, log line, early return) is copy-pasted verbatim into both callbacks. If the play-only semantics ever change, both must change in lockstep. Not blocking, but a shared helper (or a small extension on PlaybackManager, e.g. playIfNotPlaying/resumeOnly) would keep them from diverging.

3. isPlaying() is evaluated twice for the play-only path. handleMediaButtonSingleTap checks isPlaying(), then falls through to playPause() which checks isPlaying() again. isPlaying() does a blockingFirst() on the state relay (PlaybackManager.kt:367-368), so it's a (cheap) blocking read done twice, with a tiny window for the state to change between them. Harmless in practice — just flagging that calling playbackManager.play(...)/playQueue(...) directly in the play-only branch would be marginally more direct than routing back through the toggle. Current approach is fine too.

4. Behavioral edge (expected, not a bug): a non-spec-compliant device that uses KEYCODE_MEDIA_PLAY as its toggle button would no longer be able to pause via that key. This is correct per the Android convention (dedicated play key is play-only; pause should arrive as KEYCODE_MEDIA_PAUSE), and it's the whole point of the fix — just calling it out so it's a conscious tradeoff.

Nits

• CHANGELOG entry is under 8.17 — confirm that's the intended target version.
• The PR is still marked Draft and the on-device verification TODOs are unchecked; the pause-loop can only be truly confirmed on a wireless Android Auto head unit, so that manual pass is the key remaining gate before merge.

Overall this looks correct, minimal, and low-risk. The main thing I'd want before merge is the on-device confirmation and a decision on the legacy-path test gap.

[claude]

Claude finished @joashrajin's task in 2m 3s —— View job

---

Code Review

• Read changed files & surrounding context (Media3SessionCallback, MediaSessionManager, MediaEventQueue, PlaybackManager)
• Analyze correctness, edge cases, and consistency
• Post review feedback

I traced the redundant KEYCODE_MEDIA_PLAY all the way from onMediaButtonEvent → MediaEventQueue.consumeEvent → handleMediaButtonSingleTap → PlaybackManager.playPause. The fix is correct and minimal. 👍

✅ Correctness

• Root cause & fix are right. playPause() toggles on isPlaying() (PlaybackManager.kt:504). Gating the single-tap behind playOnly && isPlaying() gives KEYCODE_MEDIA_PLAY true play-only semantics: playing → no-op (breaks the loop), paused → resumes (playPause → playQueue), buffering → at worst a redundant playQueue, never a pause. KEYCODE_MEDIA_PLAY_PAUSE/KEYCODE_HEADSETHOOK still toggle (playOnly=false).
• playOnly is captured from the correct tap. In MediaEventQueue, a SingleTap output is only ever returned by the first tap's SingleTapJob when counter == 1 (MediaEventQueue.kt:67-68); multi-tap increments return null. So keyEvent.keyCode in that closure is genuinely the single tap's — no aliasing across the double/triple-tap window, and the Pixel Buds spurious-event suppression (multiTapJob?.isActive) is untouched.
• No ANR risk from the extra isPlaying(). It calls blockingFirst() (PlaybackManager.kt:368), but both callbacks dispatch on Dispatchers.Default (MediaSessionManager.kt:154), not Main.
• Both the Media3 and legacy paths were updated identically, so they don't drift today.

🟡 Suggestions (non-blocking)

1. Duplicated guard across two files — left an inline comment proposing a shared PlaybackManager.playIfNotPlaying(...) helper. It also collapses the double isPlaying() read (guard + playPause()) and its tiny TOCTOU window into one.
2. Legacy path is untested. The 3 new tests cover Media3SessionCallback only; the identical MediaSessionManager branch has no unit test (its only test is androidTest). Worth a one-line note in the PR that the legacy branch relies on the Media3 tests for equivalence — or extracting the helper above so a single test covers both.
3. Behavioural tradeoff (expected, not a bug): a non-spec-compliant device using KEYCODE_MEDIA_PLAY as its toggle can no longer pause via that key — pause is expected to arrive as KEYCODE_MEDIA_PAUSE. This is the intended semantics; just flagging it's a conscious choice.

Nits

• CHANGELOG entry is under 8.17 — confirm that's the intended target version.
• On-device verification with a wireless Android Auto head unit is still the key remaining gate (as the PR notes) — the pause-loop can't be confirmed by unit tests alone.

Verdict

Correct, focused, and low-risk. Nothing blocking in the code; the two things I'd want before merge are the on-device confirmation and a decision on the legacy-path test gap (item 2).

[claude]

This guard is identical to the one just added in Media3SessionCallback.handleMediaButtonSingleTap (comment, isPlaying() check, log line, early return). Since the two callbacks must stay behaviourally in lockstep for play-only semantics, consider hoisting this into a single shared helper — e.g. an extension on PlaybackManager:

fun PlaybackManager.playIfNotPlaying(sourceView: SourceView) {
    if (isPlaying()) return
    playQueue(sourceView)
}

Then both single-tap handlers become if (playOnly) playbackManager.playIfNotPlaying(source) else playbackManager.playPause(source). This also removes the double isPlaying() read (guard + playPause()) and the tiny TOCTOU window between them. Non-blocking — the current duplication is correct, just drift-prone.

[Copilot]

Pull request overview

Adjusts media-button handling so KEYCODE_MEDIA_PLAY behaves as a play-only command (instead of a play/pause toggle) to prevent wireless Android Auto head units from repeatedly pausing playback when they emit redundant play events.

Changes:

• Route single-tap handling with a playOnly flag when the originating key is KEYCODE_MEDIA_PLAY (Media3 + legacy MediaSessionCompat paths).
• Ignore KEYCODE_MEDIA_PLAY single taps when playback is already running (prevents accidental toggling into pause).
• Add unit tests covering the new KEYCODE_MEDIA_PLAY play-only behavior and ensuring KEYCODE_MEDIA_PLAY_PAUSE still toggles.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/Media3SessionCallbackTest.kt	Adds tests for play-only semantics and ensures play/pause toggle remains unchanged for KEYCODE_MEDIA_PLAY_PAUSE.
modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/MediaSessionManager.kt	Applies play-only semantics to legacy MediaSessionCompat single-tap handling for KEYCODE_MEDIA_PLAY.
modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/Media3SessionCallback.kt	Applies play-only semantics to Media3 session callback single-tap handling for KEYCODE_MEDIA_PLAY.
CHANGELOG.md	Documents the wireless Android Auto repeated-pausing fix under 8.17 bug fixes.

[joashrajin]

Addressed the review feedback in 13a6806: extracted PlaybackManager.playIfNotPlaying() so the play-only guard is shared by both the Media3 and legacy callbacks (no more duplicated logic or double isPlaying() read), and added PlaybackManagerPlayIfNotPlayingTest covering the helper directly — so the legacy path now relies on tested shared logic rather than untested copy-pasted code. The CHANGELOG target (8.17) is confirmed correct: 8.16 is already at rc. Remaining gate before marking ready: on-device verification with a wireless Android Auto head unit.

[claude]

Claude finished @joashrajin's task in 1m 11s —— View job

---

Code Review — follow-up on 13a6806

• Read changed files & surrounding context
• Verify the refactor addresses prior review feedback
• Analyze correctness, edge cases, and consistency

The latest commit resolves the three non-blocking items raised in the earlier reviews. Nice, clean follow-up.

✅ Feedback addressed

• Duplicated guard eliminated. The copy-pasted block (comment + isPlaying() + log + early return) is now a single PlaybackManager.playIfNotPlaying() (PlaybackManager.kt:518), and both callbacks call it via handleMediaButtonSingleTap(playOnly = keyEvent.keyCode == KEYCODE_MEDIA_PLAY). The two paths can no longer drift.
• Double isPlaying() read + TOCTOU window closed. The play-only path is now a single isPlaying() check inside playIfNotPlaying → playQueue(), instead of guard-then-playPause()-rechecks. Marginally more direct, as suggested.
• Legacy path now covered by tested shared logic. PlaybackManagerPlayIfNotPlayingTest exercises the helper directly (playing → no-op; paused → playQueue), and both callbacks route through it — so the MediaSessionManager branch is no longer untested copy-paste.

✅ Correctness (unchanged from before, re-verified)

• Behaviour is preserved: playing → no-op (breaks the loop), paused → resumes, PLAY_PAUSE/HEADSETHOOK → still toggle (playOnly = false), multi-tap skip + Pixel Buds suppression untouched (playOnly only consulted on SingleTap).
• playIfNotPlaying semantics match the old inline guard exactly, so no functional regression from the extraction.
• Test assertions line up with playQueue's 2-arg signature (sourceView, showedStreamWarning) — playQueue(eq(...), any()) / never().playQueue(any(), any()) are correct.

Minor / non-blocking

• playIfNotPlaying logs at LogBuffer.i on the ignored path — good for diagnosing this exact head-unit issue in future logs. No concern, just noting it's intentional and useful.
• The behavioural tradeoff still stands (a non-spec device using KEYCODE_MEDIA_PLAY as its toggle can no longer pause via that key) — this is the intended Android semantics, called out in prior reviews as a conscious choice.

Remaining gate

• On-device verification with a wireless Android Auto head unit is still the key manual step before merge — the pause-loop can only be confirmed there, as the PR notes. Unit tests + CI (green so far) cover the logic.
• CHANGELOG under 8.17 confirmed correct per the author (8.16 at rc).

Verdict

The code is correct, minimal, and the earlier suggestions are all resolved. Nothing blocking in the diff; the only outstanding item is the manual Android Auto confirmation.
• pcdroid-516-wireless-android-auto-repeatedly-pauses-when-bluetooth-is

[wpmobilebot]

Version 8.17 has now entered code-freeze, so the milestone of this PR has been updated to 8.18.

[claude]

Claude finished @joashrajin's task in 1m 1s —— View job

---

Code Review

• Read changed files & surrounding context
• Analyze correctness, edge cases, and consistency
• Post review feedback

Re-reviewed after the latest push (merge of origin/main + CHANGELOG moved to the 8.18 section). The fix remains correct and the earlier review feedback is fully addressed.

✅ Correctness (re-verified)

• playIfNotPlaying has the right semantics. PlaybackManager.kt:634 — playing → no-op (breaks the redundant-play → pause loop), paused → playQueue() resumes. KEYCODE_MEDIA_PLAY_PAUSE / KEYCODE_HEADSETHOOK still route to playPause() (playOnly = false), so they toggle as before.
• playOnly is captured from the correct tap. Both callbacks compute playOnly = keyEvent.keyCode == KEYCODE_MEDIA_PLAY on the MediaEvent.SingleTap branch, which MediaEventQueue only emits for a genuine single tap. Multi-tap skip routing and the Pixel Buds spurious-event suppression are untouched.
• Shared helper eliminates drift. Both the Media3 (Media3SessionCallback.kt:280) and legacy (MediaSessionManager.kt:1284) paths funnel through the single playIfNotPlaying, so the two can't diverge, and the earlier double-isPlaying()/TOCTOU concern is gone.

✅ Tests

• Media3SessionCallbackTest asserts KEYCODE_MEDIA_PLAY → playIfNotPlaying and never playPause, plus KEYCODE_MEDIA_PLAY_PAUSE still toggles while playing.
• PlaybackManagerPlayIfNotPlayingTest exercises the helper directly (playing → no-op; paused → playQueue), so the legacy path is covered by tested shared logic.

🟢 Notes (non-blocking)

• CHANGELOG entry is now correctly under the unreleased/8.18 Bug Fixes section (8.17 is in code-freeze). 👍
• Behavioural tradeoff (intended): a non-spec device using KEYCODE_MEDIA_PLAY as a toggle can no longer pause via that key — pause is expected as KEYCODE_MEDIA_PAUSE. This is the deliberate Android semantics and the point of the fix.
• Remaining gate: on-device verification with a wireless Android Auto head unit is still the key manual step — the pause loop can only be confirmed there, as the PR notes. Unit tests + CI cover the logic.

Verdict

Correct, minimal, and low-risk. Nothing blocking in the diff; the only outstanding item is the manual Android Auto confirmation.

[joashrajin]

Superseded by #5645, which includes this PR’s explicit play-only handling and adds the immediate-resume fix for #5631 while preserving multi-tap handling and spurious-event suppression. Closing this PR so review can continue on the consolidated fix.