theapache64
08/21/2021, 7:17 AMvlcj
. 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
08/21/2021, 9:08 AMmediaPlayerComponent.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
08/21/2021, 9:09 AMZach Klippenstein (he/him) [MOD]
08/21/2021, 3:05 PMisPlaying
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
08/21/2021, 3:24 PMYou’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.