Re: [emms-help] More mpv player code review

[Mike Kazantsev]

On Wed, 06 Jun 2018 13:55:45 +0200
Pierre Neidhardt <address@hidden> wrote:

> Sorry for the long delay, I've finally spent some time review the mpv
> code.
> It's pretty neat I think, I like it.
> I've fixed a few nits already.

Thanks!

Probably should've mentioned this visual code trick wrt previous
change to long prefixes, in case you might want to try it out:
https://gist.github.com/mk-fg/c2eaa08d965801860b5c83169da3c99a

Haven't had a chance to use it myself, so not sure if it makes things
better or worse, but seem to work pretty well for shortening long
prefixes on the editor side.

When used with non-typeable prefix in particular, you can't really
mis-type manually it without copy-paste or completion either.


> Some questions:
> 
> - In `emms-player-mpv-proc-stop': (setq emms-player-mpv-proc nil) should
>   be done at the end I think, or in case of interrupt ("C-g") at the
>   wrong time the variable will be nil while the player will not be
>   stopped.  Further calls to emms-player-mpv-proc-stop won't be able to
>   stop the player.

Indeed, see no good reason why it should not be done after "let" at the
very end, probably didn't think about interrupt there.


> - In `emms-player-mpv-ipc-connect-fifo':
> 
> --8<---------------cut here---------------start------------->8---
> (format "cat > '%s'"
>  (replace-regexp-in-string "'" "'\"'\"'" emms-player-mpv-ipc-socket t t))
> --8<---------------cut here---------------end--------------->8---
> 
> That looks like proper ugly shell! :p I haven't tried, but couldn't we
> use `shell-quote-argument' instead?

Yes, I think it should be better with shell-quote-argument for non-bash
in particular.

Do you know if maybe there is some generic way to create fifo and pipe
process for it on arbitrary systems, by the way?

Given that subprocess is started for this already, wonder if maybe
there can be some "fake fifo pipe" process on e.g. windows or a general
routine for other platforms which maybe don't have "cat" or fifos.


Did test and commit both fixes.


-- 
Mike Kazantsev // fraggod.net