Make absolutely sure subprocess is not killed #14

Merged

eNV25 merged 2 commits from pr/process_group into main

2024-05-20 21:06:27 +02:00

eNV25 commented

2024-05-14 19:12:20 +02:00

(Migrated from github.com)

cargo fmt
Make absolutely sure subprocess is not killed

I added code to set the process group. The docs mention it, even though it looks like it's not needed.

Also added a platform specific dependency to windows-rs, rather than hard-coding CREATE_BREAKAWAY_FROM_JOB.

- **cargo fmt** - **Make absolutely sure subprocess is not killed** I added code to set the process group. The docs mention it, even though it looks like it's not needed. Also added a platform specific dependency to windows-rs, rather than hard-coding CREATE_BREAKAWAY_FROM_JOB.

ryze312 (Migrated from github.com) reviewed 2024-05-14 19:12:20 +02:00

ryze312 commented

2024-05-19 17:26:08 +02:00

(Migrated from github.com)

Thanks for the PR!

The docs mention it, even though it looks like it's not needed

I've tested it myself and it doesn't seem to make any difference on my system, but it's good to include it anyway.

Thanks for the PR! > The docs mention it, even though it looks like it's not needed I've tested it myself and it doesn't seem to make any difference on my system, but it's good to include it anyway.

ryze312 (Migrated from github.com) requested changes 2024-05-19 17:36:59 +02:00

src/command.rs

ryze312 (Migrated from github.com) commented

2024-05-19 17:32:47 +02:00

I think it would be better to extract that into a function.

impl Command {
    // ...

    #[cfg(target_family = "unix")]
    fn detach_player(command: &mut Command) {}
  
    #[cfg(target_family = "windows")]
    fn detach_player(command: &mut Command) {}
 }

I think it would be better to extract that into a function. ```rust impl Command { // ... #[cfg(target_family = "unix")] fn detach_player(command: &mut Command) {} #[cfg(target_family = "windows")] fn detach_player(command: &mut Command) {} } ```

ryze312 (Migrated from github.com) commented

2024-05-19 17:36:48 +02:00

Might as well include that in use

use std::os::windows::process::CommandExt;
use windows::Win32::System::Threading::CREATE_BREAKAWAY_FROM_JOB;

command.creation_flags(CREATE_BREAKAWAY_FROM_JOB.0);

Might as well include that in use ```rust use std::os::windows::process::CommandExt; use windows::Win32::System::Threading::CREATE_BREAKAWAY_FROM_JOB; command.creation_flags(CREATE_BREAKAWAY_FROM_JOB.0); ```

eNV25 (Migrated from github.com) reviewed 2024-05-19 19:59:04 +02:00

src/command.rs

eNV25 (Migrated from github.com) commented

2024-05-19 19:59:04 +02:00

By command: &mut Command I assume you mean std::process::Command and not ff2mpv_rust::command::Command. The function call would look like:

impl Command {
    fn launch_mpv(command: String, args: Vec<String>, url: &str) -> Result<(), io::Error> {
        let mut command = ...;
        Command::detach_player(&mut command);
        ...;
    }
}

Is that what you mean?

By `command: &mut Command` I assume you mean `std::process::Command` and not `ff2mpv_rust::command::Command`. The function call would look like: ```rust impl Command { fn launch_mpv(command: String, args: Vec<String>, url: &str) -> Result<(), io::Error> { let mut command = ...; Command::detach_player(&mut command); ...; } } ``` Is that what you mean?

eNV25 (Migrated from github.com) reviewed 2024-05-19 20:18:35 +02:00

src/command.rs

eNV25 (Migrated from github.com) commented

2024-05-19 20:18:35 +02:00

IMO, the current approach is cleaner. It also removes the need for #[cfg(not(any(unix, windows)))]

IMO, the current approach is cleaner. It also removes the need for `#[cfg(not(any(unix, windows)))]`

ryze312 (Migrated from github.com) reviewed 2024-05-19 20:25:33 +02:00

src/command.rs

ryze312 (Migrated from github.com) commented

2024-05-19 20:25:33 +02:00

Is that what you mean?

Yes exactly!

It also removes the need for #[cfg(not(any(unix, windows)))]

There is no need to have it, other platforms are not supported at the moment anyway. if anything the compilation would just fail in such case, so it is safe.

> Is that what you mean? Yes exactly! > It also removes the need for #[cfg(not(any(unix, windows)))] There is no need to have it, other platforms are not supported at the moment anyway. if anything the compilation would just fail in such case, so it is safe.