Exoplayer2 - Move AndroidMediaPlayerImpl creation + Listeners binding to collaborator

[juankysoriano]

Problem

In a previous PR #38 we discussed about removing the setForwarder method from AndroidMediaPlayerFacade

Solution

Done that, what required moving the binding of the listeners done in AndroidMediaPlayerImpl to a collaborator class; AndroidMediaPlayerImplCreator.

Test(s) added

Yes, the AndroidMediaPlayerImplCreator is fully tested, moving the tests existing on AndroidMediaPlayerImpl to it.

Screenshots

No UI changes

Paired with

Nobody, but talked with @joetimmins about a potential solution.

Note In a future PR (if this succeed) I will do the same for ExoPlayer2Impl

[joetimmins]

Should these be methods on the Player interface?

[juankysoriano]

It won't be used by any of the clients of Player, and very likely not be needed for the other implementation.

[joetimmins]

Can we enforce the forwarder's listeners to not be null?

[juankysoriano]

If the question is:

Can the player of the library force this listeners to be null? The question is no, because in the worst case scenario the forwarder will have a no-op implementation contained.

But it is possible that we, developers, by mistake do something that causes it hold a null. You can consider the thrown below as a DeveloperError

[joetimmins]

Okay cool, just wondering! 👍

[joetimmins]

I'm not as up to speed with the exoplayer stuff as other people on the project, but this is an improvement IMO on what was there before. Better separation of concerns between configuring the player impl and using it.

[ouchadam]

I'm a fan of the separate creator but not totally sold on the setup happening inside of it.

Would prefer that logic to remain inside the impl under a impl.initialise method which would be used internally via the PlayerFactory

    private Player createMediaPlayer() {
        AndroidMediaPlayerImpl player = androidMediaPlayerImplCreator.create(context);
        player.initialise();
        return player;
    }

what do you think?

[juankysoriano]

@ouchadam let me check if that will still remove the original concern: having a setForwarder method on the facade

[juankysoriano]

@ouchadam 8195032

[ouchadam]

this can be final 😸

[ouchadam]

👍

[ouchadam]

I like it! It's probably worth making a similar change to ExoPlayerImpl as well in another PR 👻

[juankysoriano]

Yes I will do it in exoplayer!