Added quit functionality

[KeyserSoze1]

Not sure if this is honestly the best way to handle this.

Added quit function and added settings so the socket won't reconnect and the mpv process won't respawn.
The Mpv instance is no longer valid and can no longer be used after quit is called.

This was brought on because I've had issues with the mpv process hanging my app and not exiting properly. I'm hoping that if this isn't merged, similar functionality will be brought in to handle this issue.

[j-holub]

Sorry for the late reply.

Yes I have a similar functionality in a local branch, with the similar, somewhat "dirty" approach you have. I was actually working on the module about a week ago.

As discussed in #11 I am using the ipc only verbose method. This will send me some message like "Ipc Ready", when mpv has successfully created the ipc socket and is listening to it.
I wanna listen to that and then construct a promise on which's resolve I start mpv. This will get rid of all the Connection Refused Messages in the beginning.

After that is done I try to add a quit / restart feature based on that. I will have a closer look at your code and see if it can be merged, to give the functionality until I have implemented it properly.

I have a university project to hand in on May 3rd, so I don't know if I can work much on it before that, but I'm definitely on it.

Thank you very much :)

[KeyserSoze1]

No problem. This was just something to quickly get the issue I was seeing resolved. It may not necessarily be the best way to deal with the specific issue considering all things involved. Thanks for the work you've done so far!

[j-holub]

Just a quick update. I'm on it.

I already implemented the promise to detect wether MPV was able to connect to the IPC Socket or, which got already rid of all the Connection Refused messages.

Problem is though, since the resolving of the promise is asynchronous,

mpvPromise.then(function() {
    // rest of the code that is asynchronous
});

if you call some commands right after creating the mpv object, it will fail because that asynchronous part setting up stuff has not yet finished.

I could solve this by making the constructor return another promise, but that would break the API for everyone using it so far. I'm trying to find some way to make the constructor only return when everything is finished, but I didn't get that working yet.

Once that works, stoping and restarting mpv should not be that hard to implement :)

Just wanted to let you know, that I'm working on it.

[KeyserSoze1]

Thanks for the update!

[j-holub]

Sorry development is going slowly. The thing is I probably can't make this without breaking changes, that's why I'm very careful about it.

The constructor now takes some time to complete, since it waits until mpv created the socket, checks if that was successful and then connects to it.

Thus any code like for example player.loadFile('path/to/some/file.mp3') immediately after the constructor is called crashes, because the socket is not ready yet.

I thought about a few options

1

Wait for the stuff to be finished before returning. That would make the constructor blocking. That's why I don't like this idea

2

Give the constructor some ready callback. This would of course be a breaking change and the constructor would not return an instance of the mpv player.

3

Make the constructor return a promise. Slightly more modern than version 2 but it yields the same problem: the constructor does not return an instance but a promise.

4

Take the Socket thing out of the constructor and introduce new methods called start() and stop(), both returning a promise wether it was successful or not.
This is a breaking change as well and would change the following code

let mpv = new mpvAPI();
mpv.loadFile('path/to/some/file.mp3');
// further code

to

let mpv = new mpvAPI();
mpv.start().then( () => {
    mpv.loadFile('path/to/some/file.mp3');
   // further code
}, () => {
  // error handling when mpv couldn't start
});

So far I like this idea the best.

5 Get up my lazy ass and read into async and await

Kinda like 4 but maybe more modern?

@KeyserSoze1, @iamale, @pussinboot what do you guys think? (If you're even still using my library :D )

Again sorry for the slow development.

I'm eager to here your opinions :)

Cheers

[notpushkin]

async/await isn't actually something really new, it's all about Promises. I. e.:

mpvAPI.create().then((mpv) => {
  ...
});

becomes:

let mpv = await mpvAPI.create();

I think new mpvAPI() wouldn't really be OK here if it returns a Promise as users would instead expect an actual instance. Then again, it doesn't make sense to separate initialization code to constructor and start() because why not just make it one function yeah.

[notpushkin]

Another possible solution is emitting a ready event (should be careful to not conflict with mpv's own events though).

[j-holub]

Exactly, the constructor should return an instance. I though about the ready event as well, it's similar to the ready callback I think.

As for the initialization. The constructor would still initialize mpv, set everything up and so on. It just wouldn't start the actual player yet. Maybe something that is even desired for some applications?

The goal is to include a start and stop (quit) functionality anyways. This would nicely blend with it.

I kind of like it more than letting the constructor return a promise, since it should return an instance. So either making start/stop return a promise or using async (which results in returning a promise I think) could be a good choice.

Either way I don't think this change can be done in an unbreaking way =/

Thanks for your thoughts :)

[pussinboot]

i like 4 the most. adding asynchronous code to starting an instance of mpv will break things either way, with start() and stop() functions it becomes explicit what's going on, and shifting any initialization code to within the promise's then() is easy.

[notpushkin]

I don't like

let mpv = new mpvAPI();
mpv.start() ...

because mpv here doesn't make any sense until it's started, and it is a bit error-prone (i. e. user might try to call mpv.play() before it's actually ready to handle it — totally user's fault (or bad docs), but still), but apart from that it's totally fine I guess.

[j-holub]

Good point, the thing that the user might be forgetting the start thing, (For the quit functionality there will be a quit and start functionality anyway though).

Is it common in JS to have constructors returning a promise or having something like a callback? Because that would be the alternative to the start / quit thing... kinda don't like it too much I have to say.

This is the part within the constructor that causes the asychronity

     // init code

      // Start the mpv player with a promise to see when the IPC socket is ready
	let mpvPromise = new Promise(function(resolve, reject) {

		this.mpvPlayer.stdout.on('data', function(data) {
			let output = data.toString();
			// "Listening to IPC socket" - message
			if(output.match(/Listening/)){

				resolve();
			}
			// "Could not bind IPC Socket" - message
			else if(output.match(/bind/)){
				reject();
			}

		});
	}.bind(this));

	mpvPromise.then(function() {

             // do the rest of the initialization 

      }

This could also be a point to tackle in order the keep the original API functionality to the outside. Any suggetions there?

[notpushkin]

TBH await new MPV() doesn't look as bad as I thought it would be. But no, I don't think anybody uses this in wild yet.

I'm not even sure why do we need this explicit new, probably because I'm used to new-less Python constructors. I bet it makes more sense in Java, but not in JS.

[j-holub]

Yeah I'm coming from an object oriented background, like C++ and Java. So it felt natural to me when I was creating this library.

How would you go about it in a different way? Without the new keyword?

Of course I could run the asynchronous part inside synchronously, it's matter of a few milliseconds, but making things blocking should never be the solution I think.

[j-holub]

I've been thinking about it and discussing about it with a friend who's really good with JavaScript here at my University.

I kinda came to the conclusion, that waiting for the asynchronous part inside of the constructor, making it synchronous isn't all that bad. We're talking about mere milliseconds here (it's actually already happening with all the Connection Refused error messages. Plus you usually create the node-mpv in the initialization part of your program, thus the blocking thing is actually no problem, cause intialization has to be finished first.

With the planned start and stop method's in mind I could go on and offer two ways to initialize node-mpv. The way it used to be with

let mpvPlayer = new mpv();

And that being synchronous / blocking.

And another version were it only initializes the player but does not start it. The player can then be started with something like a start method, which is asynchronous aka returning a Promise.

Anyhow, this will require some big code reorganization, been already doing that yesterday, might as well go on.

Again sorry you guys have to wait so long for this :(

[j-holub]

I've just committed the first Version with Promises in a local branch. I went for the version

let player = new mpv();
player.start()
    .then(() => {
        loadFile('best/mp3/file.mp3');
        // code goes on here
    })
    .catch((error) => {
        console.log(error);
    });

This immediately gets rid of all the Connected Error messages and if it fails to bind the IPC socket (for example because it's set to some path where only root is rights) this fails and gives an error message.

After thinking a lot about it I had to break the API unfortunately. I like this way the best and I will look into, if I can start the MPV player internally when calling loadFile() and the player is not yet started.

Because this breaks the API I will work towards a 2.0.0 release, with possibly a little more features.
The Code readability has also improved a lot. As soon as I have something to push I will push it into an Node-MPV-2 branch, so you guys can use it right away for development (If you're still using this module that is).

[j-holub]

In the end I decided to merge this PR anyway.

While working on the start and quit functionality I learned more about NodeJs, Sockets and all that stuff. I broke the API, changed quite a few things and decided to safe that for a Version 2.0.0, since it breaks the API, you know.
But I wanna take my time with 2.0.0 and don't want to rush it, possible ending up with something broken and bad.

But I still don't wanna make you wait any long (Again if you're still using this, somehow I doubt it :D). That's why I put a quit function inside version 1 anyways.
I figured my solution was not too different from yours @KeyserSoze1 , but I found out, that yours still has a few bugs. The EventListeners remained and still output information, such as the time for example.
I unbound them on quitting which made the status variables you introduced (exiting) obsolete.

I'm afraid there's not too much left of your code @KeyserSoze1 , very sorry about that :( . But I decided to merge this and work from it anyways, because you gave me the initial idea and kind of as a Thank You (well if you actually give a sh*t about being mentioned as a contributor, that is of course :) ).

Once again my apologies that this took so many month, especially because I ended up merging it anyway. I didn't wanna rush it and I am really happy with how Version 2 turned out so far. If anybody's interested I push a new branch with it, you can see it here.

Again, thank you all for your help :)