:thread: Is this the correct way to control state ...

# compose-desktop

[theapache64]

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

[theapache64]

I am trying to implement a video player using vlcj . To play/pause, am using a class called PlayerState which contains a boolean State . Here’s the full implementation : https://gist.github.com/theapache64/99d5582418dba8b8aee65edacd14e338

This is my first time controlling a swing-panel frame directly. would be great if someone can confirm the approach and if there’s anything else I need to improve or careful about 🙏

[Igor Demin]

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.

[theapache64]

Understood. makes sense. Thanks 🙇

[Zach Klippenstein (he/him) [MOD]]

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.

[theapache64]

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