🧵 Is this the correct way to control state of an external component (non-composable)?

Looks okay, except you need to wrap this code:

mediaPlayerComponent.mediaPlayer().controls().let { controls ->
        if (playerState.isPlay()) {
            controls.play()
        }

        if (playerState.isPause()) {
            controls.pause()
        }
    }

into this:

val isPlay = playerState.isPlay()
val isPause = playerState.isPause()

SideEffect {
    mediaPlayerComponent.mediaPlayer().controls().let { controls ->
        if (isPlay) {
            controls.play()
        }

        if (isPause) {
            controls.pause()
        }
    }
}

As Composable can be run from any thread in the future. Also, check if calling controls.play on every recomposition checks internally that it is already in the play state. Otherwise, you need to check it yourself.

Understood. makes sense. Thanks 🙇

I would probably make a few changes: Representing state You’re representing a Boolean as two separate Booleans. As that code grows, it could be confusing to have both

isPlaying

and

isPaused

properties - what if they’re both true? What if you want to add more states, eg

isStopped

? Which property is the actual state? Better to expose a single property of either a Boolean or an enum/sealed class. Eg for now, just make your

playPause

property public and maybe call it

isPlaying

to make it clear what a true value means. (The name “playPause” leaves the question, does true mean playing or paused?) This would simplify your state class too since you wouldn’t need to write the getters yourself. State vs events/actions It’s a bit awkward to be calling

controls.play

and

controls.pause

on every composition. • It’s a bit of an odd indirection that calling

PlayerState.play

doesn’t actually tell the player to play but just sets some state that will actually perform the action later. If you’re lucky those calls will be idempotent, as Igor mentioned, but if they’re not then this won’t work. • Another case that is common in this general pattern (although it might not apply to this particular case) is that the actions could be suspend functions. There’s a more flexible way to do this: 1. Make your PlayerState aware of the current

CallbackMediaPlayerComponent

. That will be null initially, and only non-null while a

VideoPlayer

composable is actively consuming the state. To support the multiple consumer case, just make this a list. 2. In

VideoPlayer

, use a

DisposableEffect

to set/add the CMPC on your video state when it enters the composition, and remove it (set back to null or remove it from the list) when it leaves. 3.

VideoPlayer

`play`/`pause` methods check if there’s a current CMPC and forward the action directly to it. If it’s null, do nothing. State synchronization I’m guessing your media player component probably has its own internal state for whether it’s paused or playing. By creating your own

MutableState

to hold that, you’ve introduced a second source of truth and now bugs can creep in where those states get out of sync - eg if there’s any other way for the media player to change state that doesn’t happen through this code path, eg if someone uses their Bluetooth headphones to pause the video. You probably want to watch for state changes on the media player (I’m assuming it implements the observer pattern somehow, so there’s callbacks you can register?) and update your

MutableState

when it changes, instead of setting it yourself as the direct result of your

PlayerState

`play`/`pause` methods.

@Zach Klippenstein (he/him) [MOD]

You’re representing a Boolean as two separate Booleans ...

Sorry. I did that to quickly make the gist. The real

PlayerState

class will have a better and meaningful structure.

It’s a bit awkward to be calling controls.play and controls.pause ...

Understood. Will fix it.

There’s a more flexible way to do this ...

Wow, that sounds better. Will try to implement this.

State synchronization..

Makes sense. I haven’t explored the

vlcj

library that much, but there should be some callbacks to expose the playerState. Thank you so much reviewing the code and your detailed feedback 🙂 I’ll try to improve the solution based on above points.