Skip to content

Fix onAudioSessionIdChanged() handling in (Forwarding)SimpleBasePlayer#2978

Open
nift4 wants to merge 3 commits intoandroidx:mainfrom
nift4:sessionidcb
Open

Fix onAudioSessionIdChanged() handling in (Forwarding)SimpleBasePlayer#2978
nift4 wants to merge 3 commits intoandroidx:mainfrom
nift4:sessionidcb

Conversation

@nift4
Copy link
Contributor

@nift4 nift4 commented Jan 2, 2026

Issue: #3140

@nift4
Copy link
Contributor Author

nift4 commented Jan 23, 2026

Hi @marcbaechinger, can you please check this PR? Thanks!

@icbaker
Copy link
Collaborator

icbaker commented Mar 25, 2026

Please can you add some tests for this

@nift4
Copy link
Contributor Author

nift4 commented Mar 25, 2026

@icbaker Thanks for the reminder, I added tests.

Weirdly, ForwardingSimpleBasePlayerTest.getterMethods_noOtherMethodCalls_returnCurrentStateFromWrappedPlayer() was testing ForwardingPlayer instead of ForwardingSimpleBasePlayer, which I think might've been a mistake, so I fixed that. And then a lot of asserts started failing due to wrong available commands so I fixed that too. Please take a look whether that was correct, as it feels a bit odd that it was testing the wrong class?

Edit: also, to be clear, there is already MediaControllerListenerTest.onAudioSessionIdChanged_isCalledAndUpdatesGetter() which covers MediaController, it seems controller itself never was affected and it just happens because both demo and my prod app use ForwardingSimpleBasePlayer.

copybara-service bot pushed a commit that referenced this pull request Mar 26, 2026
This was pointed out as part of Issue: #2978 but I'm sending it
separately to keep the code history clearer.

This CL also removes a stale comment that should have been removed as
part of bdf7e6f.

PiperOrigin-RevId: 889730426
@icbaker
Copy link
Collaborator

icbaker commented Mar 26, 2026

Weirdly, ForwardingSimpleBasePlayerTest.getterMethods_noOtherMethodCalls_returnCurrentStateFromWrappedPlayer() was testing ForwardingPlayer instead of ForwardingSimpleBasePlayer, which I think might've been a mistake, so I fixed that. And then a lot of asserts started failing due to wrong available commands so I fixed that too. Please take a look whether that was correct, as it feels a bit odd that it was testing the wrong class?

Thanks for flagging this! I agree it looks wrong. It seemed a bit messy to change the class-under-test in this PR (since it's not really related), so I submitted a separate change to make that fix: 4a32661

I've rebased & re-pushed this PR on top.

@icbaker icbaker changed the title Fixed bug where MediaController listener onAudioSessionIdChanged() was not called Fix onAudioSessionIdChanged() forwarding in ForwardingSimpleBasePlayer Mar 26, 2026
@icbaker icbaker changed the title Fix onAudioSessionIdChanged() forwarding in ForwardingSimpleBasePlayer Fix onAudioSessionIdChanged() forwarding in (Forwarding)SimpleBasePlayer Mar 26, 2026
@icbaker icbaker changed the title Fix onAudioSessionIdChanged() forwarding in (Forwarding)SimpleBasePlayer Fix onAudioSessionIdChanged() handling in (Forwarding)SimpleBasePlayer Mar 26, 2026
@icbaker
Copy link
Collaborator

icbaker commented Mar 26, 2026

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!

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.

3 participants