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

added static ipc support and configurabe dynamic ipc support #4

Closed
wants to merge 1 commit into from

Conversation

JoschuaL
Copy link

@JoschuaL JoschuaL commented Feb 5, 2021

Added support for configuring weather a dynamic or static ipc socket should be used, with the abilitu to configure where
the dynamic ipc socket should be created. It would potentially also make sense to enable configuring the path of the static
ipc socket, though that seems less usefull to me since that is typically set to /tmp/mpvsocket.

I tested this on linux, in combination with SVP, and as long as both are configured the same way
(either both use /tmp/mpvsocket or both use /tmp/mpvSockets/$PID), they work flawlessly with each other.

I did not test on macos or windows, as I currently dont have these OS available to me.

macos should work out of the box, though windows might be tricky.

@tnychn tnychn linked an issue Feb 6, 2021 that may be closed by this pull request
}
opts.read_options(options, "discord")

local function get_temp_path()
Copy link
Owner

Choose a reason for hiding this comment

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

What does this get_temp_path() function do?

If it's for getting the system's temporary directory, I think it's better to handle it in main.go.

Copy link
Owner

@tnychn tnychn left a comment

Choose a reason for hiding this comment

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

I think it would be even better if we allow users to also customise the static path of the IPC socket, not just the dynamic path.

This can be done by using the use_static_socket_path option to determine whether the user wants to use a dynamic path or not. Then, the socket_path option will be regarded as the absoulute path of the IPC socket (e.g. /tmp/mpvsocket). Otherwise, the socket_path option will be used as the path prefix (which ponits to a directory) for placing the IPC socket with a dynamic filename containing the pid (e.g. mpv-discord-1234).

EDIT: This approach allows better flexibility to let users decide where the socket should be placed, and leaves the job of creating the dynamic_ipc_prefix directory to the users so that the code can be less complex as we don't have to deal with Windows filesystem compatibility.

@tnychn
Copy link
Owner

tnychn commented Feb 6, 2021

I'm gonna work on this myself to implement your suggestions. Thanks anyway for your ideas!

tnychn pushed a commit that referenced this pull request Feb 6, 2021
+ added two options to config
+ allow the IPC socket path to be static
@tnychn
Copy link
Owner

tnychn commented Feb 6, 2021

Closing this pull request as this has been implemeted at 1fd2498. Also check out the latest v1.3.1 release.

@tnychn tnychn closed this Feb 6, 2021
@tnychn tnychn added the enhancement New feature or request label Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PID in ipc socket path
2 participants