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 copy command blocking #553

Merged
merged 1 commit into from
Aug 24, 2024
Merged

Conversation

juliamertz
Copy link
Contributor

@juliamertz juliamertz commented Aug 22, 2024

Fixes #538. It turns out that child.wait_with_output() blocks for a long time when either stdin or stderr is piped. I’ve tried multiple workarounds, but they all yield the same result.

This fix resolves the blocking issue, though we won't be able to read any output, making error messages completely useless.

Interestingly, when there is actually an error (e.g., passing invalid arguments), it doesn’t block and correctly reads from stderr.
I can share a small reproduction of this issue if that would be helpful.

@aome510
Copy link
Owner

aome510 commented Aug 24, 2024

It turns out that child.wait_with_output() blocks for a long time when either stdin or stderr is piped

Do you mean either stdout or stderr? This PR doesn't seem to change stdin. I wonder why this is the case, CopyLink works fine for me on MacOS (using pbcopy afaict).

@juliamertz
Copy link
Contributor Author

Oops yes, i meant stdout, basically any attempt at reading the commands output causes this freeze for me on Linux. Works fine on MacOS for me aswell

@aome510
Copy link
Owner

aome510 commented Aug 24, 2024

Look like helix, which spotify_player clipboard implementation is based on, doesn't also pipe output

https://github.com/helix-editor/helix/blob/620dfceb849d6b68d41d4f7678bb4675009fef4d/helix-view/src/clipboard.rs#L372

@aome510 aome510 merged commit 9c47701 into aome510:master Aug 24, 2024
3 checks passed
@aome510
Copy link
Owner

aome510 commented Aug 24, 2024

Thanks for the fix @juliamertz!

@dashaw92
Copy link

dashaw92 commented Sep 1, 2024

Hey all,
I'm currently experiencing this exact issue in my build of master (9c47701), which includes this PR. After experimenting with xsel on the command line, I've determined that the --nodetach argument causes xsel to hang (at least on my box, no idea if there are any alternative versions around). After removing this argument from the slice, the copy command works and the UI continues functioning as expected.

Posting this more as a convenience for anyone else who might encounter this issue, hope this helps someone.

diff --git a/spotify_player/src/event/clipboard.rs b/spotify_player/src/event/clipboard.rs
index fb06cfa..8d14f68 100644
--- a/spotify_player/src/event/clipboard.rs
+++ b/spotify_player/src/event/clipboard.rs
@@ -100,7 +100,7 @@ pub fn get_clipboard_provider() -> Box<dyn ClipboardProvider> {
     } else if env_var_is_set("DISPLAY") && binary_exists("xsel") {
         Box::new(CommandProvider {
             paste_command: Command::new("xsel", &["-o", "-b"]),
-            copy_command: Command::new("xsel", &["--nodetach", "-i", "-b"]),
+            copy_command: Command::new("xsel", &["-i", "-b"]),
         })
     } else {
         #[cfg(target_os = "windows")]
$ xsel --version
xsel version 1.2.0 by Conrad Parker <conrad@vergenet.net>

@juliamertz
Copy link
Contributor Author

juliamertz commented Sep 1, 2024

@dashaw92 I can confirm the same happens on my system, i had previously only tested with xclip. the fix also seems to be working fine for me.

edit:
Helix doesn't pass --nodetach either. https://github.com/helix-editor/helix/blob/620dfceb849d6b68d41d4f7678bb4675009fef4d/helix-view/src/clipboard.rs#L127

@apprehensions
Copy link
Contributor

Thank you.

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.

Display Freezes after Copying
4 participants