j-holub / Node-MPV

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

Windows: you need to wrap path in quotes #18

Closed vankasteelj closed 7 years ago

vankasteelj commented 7 years ago

I encountered issues with the module when trying to spawn MPV from C:\Program Files. At first I thought it was some kind of permission issue, Windows being weird with it's Program Files folder, but it turns out it was way dumber: the spaces in the path made the entire thing go south.

I'll PR a fix that shouldn't change anything for linux/osx, but make it more solid on Windows

j-holub commented 7 years ago

Thanks for pointing that out. I using Mac and Linux for 6 years now, so I've really no idea about Windows development. Gonna look at those PRs and merge them.

vankasteelj commented 7 years ago

lol, I called mpv 'vlc' on c38bb07 commit message.

j-holub commented 7 years ago

Yes I noticed that :D

vankasteelj commented 7 years ago

I can rebase if you want :)

j-holub commented 7 years ago

I don't mind too much, but if you'd like to, sure, go for it ^^

vankasteelj commented 7 years ago

i've been using the modified v2 for a day, and it's working as it should, fyi

j-holub commented 7 years ago

Glad to hear that. I was thinking about adding a little more error handling when starting the player in the reject part of the promise and then publishing it.

vankasteelj commented 7 years ago

I did notice something strange, if I pass by mistake for example mpv.loadFile({filename: 'this.mkv', path:'foo/bar'}) it just fails without warning me of anything

vankasteelj commented 7 years ago

And was also wondering why you used require('promise') instead of standard Promise(), almost every nodejs version has it by now

vankasteelj commented 7 years ago

and I think I also need something like mpv.on('seek', newPosition) but I can wrap something around with timeposition tracking

j-holub commented 7 years ago

The synthax for loadFile is just

mpv.loadFile('path/to/this.mpv');

Why it fails without any message? Hmm I have a guess. The function takes a second parameter called mode , you can see its effect in the mpv documentation:

bildschirmfoto 2017-08-06 um 15 57 25

I assume that it takes this.mkv as the path, and foo/baras the mode, sends that to mpv and the error happens there.

But yes I should definitely add more error handling on that part.

As for the promise thing. Yeah I wasn't too familiar with promises at that time, I'll have a look into it. The fewer dependencies the better, thanks.

vankasteelj commented 7 years ago

I can PR replacing the promise module, it shouldn't take more than a few min

j-holub commented 7 years ago

The seek thing seems like a great idea actually. I will have to look into mpv's API to see how one could accomplish that though.

Sure if you want to :)