Correct the playing status logic.

[yschimke]

WHAT

Correct logic from #360

See #360

WHY

Loading state is a broken proxy for Buffering. So use the correct value.

HOW

Checklist 📋

• Add explicit visibility modifier and explicit return types for public declarations
• Run spotless check
• Run tests
• Update metalava's signature text files

[luizgrp]

question: is player.isPlaying not relevant at all for the state to be playing?

nit: if so, kdoc should be updated (maybe to a more generic text this time #360 (comment))

[yschimke]

playing is strictly whether time is moving forward. It's not relevant for the UI components, because it should be a subset of this logic.

[luizgrp]

nit: should this be placed in a broader package (I've been using com.google.android.horologist.test.toolbox.testdoubles) so it can be used by other tests in other packages? ignore this comment if this test double is specific to the tests in this package

[luizgrp]

nit: the tests in this file don't look like unit tests for PlayerStateMapper itself, they look like integration tests for media3.Player and PlayerStateMapper in the FakePlayer implementation. Which is odd given it looks like it is testing a test double implementation.

I'd question if this is necessary. If yes, then I would consider moving the test double to an individual module ("media-data-fixtures"), where the implementation is in main folder and this test is in the test folder.

Alternatively, as a quick win would be to just rename this file and move to another package (suggestion, prefix with integration so it is clear: integration.player.PlayerTest).

[luizgrp]

to clarify my previous comment, I think it's valid to have the test for the test double, if it is complex enough and we would like to provide it as fixture, where I suggested to check if it's "necessary" I meant to check if it's intended

[yschimke]

To me it's focused on PlayerStateMapper. I've introduced PlayerStateMapper and MediaItemPositionMapper, because they are evidently things that are discussion worthy, even a doc for one of them. Pulling it out because they don't appear to be just minor implementation details of PlayerRepositoryImpl, but standalone business logic.

I don't understand the comment about it testing the test double, it's hard to use the TestExoPlayer to test specific states like this (or if it's simple, then we can follow up on this PR). The test double just extends StubPlayer to return specific values, and I really want a test that documents the various cases for PlayerStateMapper. It's not a general purpose one, just enough to get specific player states.

[luizgrp]

totally agree with the benefits of extracting out those classes.

re "testing the test double", my apologies, I missed the call to PlayerStateMapper.map in line 52 (or this was added now after my comment) so I had the impression that this test class was only calling functions from FakePlayer class

nit: suggestion for clarity:

@Test
    fun `check playback state while playing`() {
        val player = fakeStatePlayer.overrideState(
            playbackState = Player.STATE_READY,
            playWhenReady = true
        )

        val state = PlayerStateMapper.map(fakeStatePlayer)

        assertThat(state).isEqualTo(PlayerState.Playing)
    }

it adds two extra lines to each test, but it makes clearer which function is being tested, specially now that the class has more than one public function