j-holub / Node-MPV

A NodeJs Module for MPV Player
https://www.npmjs.com/package/node-mpv
MIT License
117 stars 73 forks source link

Add a stimulus to start function/Update events (idle -> idle-active) #81

Closed leonekmi closed 4 years ago

leonekmi commented 4 years ago

Closes #79 by sending a stimulus to test the socket (we can imagine a loop to wait for idle-active to be true).

The error event may be useful to configure on the timeposition intervalled function, related to #78 You can also make the possibility to pass an error handler in options

j-holub commented 4 years ago

I just checked out the branch locally to have a deeper look at the changes. First of all, thank you for getting down to the root of the issue and finding that race condition. It's a nasty thing to find but just as important.

To test if MPV is already running you send a get_property command to idle-active, I think until recently this has been idle so if a user is using an older version of mpv this will result in an error message, right? This means, that if in that case, the race condition happens and we miss the idle message, we will have the same problem right? Because there will be no error=success in the response from the first command. Or am I mistaken?

Otherwise, it looks like a really nice fix. Thanks again

j-holub commented 4 years ago

I did some testing and even MPV 0.20.0 on Debian knows about idle-active so that's fine for me.

However, what I did realise is that the code reacts to the stimulus message and the result in message.data is false yet it resolves the promise. That means allthough MPV is not yet idling, it's resolving the promise which sounds wrong to me.I Shouldn't the promise only resolve if the player is actually idling?

j-holub commented 4 years ago

Merged this, since it seems to work and I don't have too much time looking into it right now. Thanks a lot for the contribution