Skip to content

Conversation

johngray1965
Copy link

Fixes #2768

…nnected_dispatchesEvent to include KEYCODE_MEDIA_PLAY_PAUSE & KEYCODE_HEADSETHOOK.

Fixed a missing case for KEYCODE_HEADSETHOOK that caused the test above to fail.
@johngray1965
Copy link
Author

@marcbaechinger Here you go

@marcbaechinger
Copy link
Contributor

marcbaechinger commented Oct 1, 2025

Many thanks!

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@johngray1965
Copy link
Author

@marcbaechinger Thank you. I won't change anything unless I'm asked to.

@marcbaechinger
Copy link
Contributor

marcbaechinger commented Oct 1, 2025

Thanks John. I added a couple of test cases to make it obvious what your fix actually fixed.

The test that your change makes pass is the unit test I added in MediaSessionServiceTest. With your change, an Intent that is sent to onStartCommand() of the service is now handled just like a PLAY_PAUSE command. As you already explained, if playback resumption is triggered with a KEY_HEADSETHOOK instead of PLAY_PAUSE or PLAY this didn't trigger play() on the player. In such a case the service wouldn't be started into the foreground which may well be the reason for the remaining crashes we were seeing.

I've added the test cases and sent that for review. The PR will be merged as soon as this is done. Please continue not adding further changes. :)

Thanks a lot for that contribution. Very much appreciated!

@johngray1965
Copy link
Author

@marcbaechinger Thank you. I was tempted to add more tests myself, but I was trying to keep the PR as small as possible.

@johngray1965
Copy link
Author

@marcbaechinger the new tests look great, one suggestion, MediaButtonReceiver checks that the keyCode is KeyEvent.KEYCODE_MEDIA_PLAY, KeyEvent.KEYCODE_MEDIA_PLAY_PAUSE, or KeyEvent.KEYCODE_HEADSETHOOK. Why not have the parameterized test do all three (I believe its not doing KeyEvent.KEYCODE_MEDIA_PLAY). I imagine that covered elsewhere, but why not cover it here too for completeness.

@marcbaechinger
Copy link
Contributor

Yeah, can do. I'll do that in a follow up CL if I get the approval. It's a bit easier for me then routing everything through GitHub and back to our internal repo. Will do that just after I have submitted internally. How does that sound?

@johngray1965
Copy link
Author

@marcbaechinger sounds good. I feel like the tests not exercising all the options is how this originally slipped through the cracks.

@marcbaechinger
Copy link
Contributor

Found another warning, so here we go. :)

@copybara-service copybara-service bot merged commit 50b91a4 into androidx:main Oct 2, 2025
1 check passed
@johngray1965 johngray1965 deleted the main branch October 3, 2025 20:23
nift4 pushed a commit to nift4/media that referenced this pull request Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants