Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MPV socket getting created in CWD #675

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

notpeelz
Copy link
Contributor

@notpeelz notpeelz commented Apr 7, 2024

Fixes #674

I haven't tested on Windows or MacOS.

syncplay/utils.py Outdated Show resolved Hide resolved
@notpeelz notpeelz force-pushed the fix-mpv-socket-path branch 2 times, most recently from 6e94def to 0690aa1 Compare April 7, 2024 21:55
syncplay/players/mpv.py Outdated Show resolved Hide resolved
@notpeelz notpeelz force-pushed the fix-mpv-socket-path branch 3 times, most recently from f6fec7c to 7280bb7 Compare April 7, 2024 22:51
@notpeelz notpeelz force-pushed the fix-mpv-socket-path branch from 7280bb7 to df33187 Compare April 8, 2024 18:12
@daniel-123
Copy link
Contributor

Overall I think the PR looks quite reasonable with one caveat: the python_mpv_jsonipc.py is technically vendored from https://github.com/iwalton3/python-mpv-jsonipc

It would be best if Syncplay didn't add too much of its own things to jsonipc, but on the other hand I don't think anybody else uses that library to begin with and we already made some changes to it. So it's not like this is a blocker of any kind, just something to be aware of and documented in this PR.

@notpeelz
Copy link
Contributor Author

notpeelz commented Apr 27, 2024

I could pass the socket path via the ipc_socket constructor parameter instead and leave the vendored library unmodified.

Edit: I updated the PR. Let me know if there's anything else.

@notpeelz notpeelz force-pushed the fix-mpv-socket-path branch from df33187 to 1f1cbc6 Compare April 27, 2024 21:33
@notpeelz notpeelz force-pushed the fix-mpv-socket-path branch from 1f1cbc6 to 6c5e3a1 Compare April 27, 2024 21:45
@daniel-123 daniel-123 self-assigned this Apr 28, 2024
@daniel-123 daniel-123 self-requested a review April 28, 2024 09:44
@Et0h Et0h self-requested a review April 29, 2024 17:49
Copy link
Contributor

@Et0h Et0h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested in Windows by confirming that mpv works and by using using Power Shell and [System.IO.Directory]::GetFiles("\\.\\pipe\\") to confirm that by default it will create a pipe along the lines of \\.\\pipe\\syncplay-mpv-94244417144407 but that if you add a player argument such as input-ipc-server=\\.\pipe\foobar it will also create a pipe of the specified name.

@Et0h
Copy link
Contributor

Et0h commented Apr 29, 2024

Thanks @notpeelz and @daniel-123 for your work on this. Time to accept those changes! :-)

@Et0h Et0h closed this Apr 29, 2024
@Et0h Et0h reopened this Apr 29, 2024
@Et0h Et0h merged commit 887131e into Syncplay:master Apr 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mpvSyncplaySocket created in $HOME
3 participants