Open Fix missing KEYCODE_HEADSETHOOK caseMain

[johngray1965]

Fixes #2768

[johngray1965]

@marcbaechinger Here you go

[marcbaechinger]

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]

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

[marcbaechinger]

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]

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

[johngray1965]

@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]

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]

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

[marcbaechinger]

Found another warning, so here we go. :)