-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Proposal: Allow use of system clipboard #996
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, on the whole this looks great - I'm especially impressed since you mentioned you don't know Rust - so kudos on getting into the code-base so easily!
I left one minor nitpick. Otherwise, we use rustfmt
to lint our code and clippy for some additional fixes. There are instructions about how to run them in CONTRIBUTING.md (that's why part of the tests here are failing).
Lastly - I haven't tested this yet, so would like to manually do so (I'll get to it over the weekend), and assuming the tests pass (at the time of writing they're still running) - this should be good to go.
Thanks very much for taking the time to do this!
Awesome, thanks for working on this! There is one thing that needs sorting out, it looks like for now this will work on linux with x11, but not macos, windows, or linux with wayland. As an example here is how alacritty is doing the setup: https://github.com/alacritty/alacritty/blob/531e494cf9a90de91dbbb84cb6cfc3158b78f1f2/alacritty/src/clipboard.rs#L8. |
Perfect. Thanks for the feedback. I'll try to work on it this weekend. |
5577a2a
to
e21b22b
Compare
Updated. |
Cool! I guess this is because the alacritty fork of copypasta doesn't work in a terminal program, so you went with copypasta-ext instead? Ideally I'd prefer not to have to depend on an external binary being present, but IMO this is an acceptable tradeoff (as long as we document it) for the time being - @imsnif thoughts on this? |
@tlinford thanks for testing it out. Works on x11 without additional libraries but yeah I guess wayland is different. I tried with copypasta initially but I couldn't get it to work. No errors just nothing in the copy buffer. |
Hey @djpate, The best way to work around this potential problem would be to make the statusbar plugin display an error message if If you want to go this route, you could check out to how As a simpler alternative if you feel like that is too much work, for now it would be fine to just swallow the error and move on, would be better than crashing :) |
@tlinford I'll look into outputting the error in the status bar. |
@tlinford I think that should do it but I can't repro to test... could you take a look? |
@djpate close, and my bad I pointed you at the wrong line, the error is coming from here:
Although it's probably a good idea at this point to handle both error cases. |
Did some poking around, and came up with this to handle both the Option and then the result, the message in status bar looks great :) |
Awesome. Updated the code |
Hey @djpate - could you briefly explain about those ubuntu packages you had to add? |
@imsnif I believe they are required for copypasta-ext to compile. They are packages for xcb which is x11 C bindings. |
Does that mean they will be a requirement for Zellij now on all platforms, or is this just a headless thing due to the CI? Because the former is a bit of a dealbreaker, I'm afraid :/ |
I'm not adding it to macosx pipeline and it compiled fine. Anyone compiling on Linux is gonna need those dependencies. |
Right - so I'm really sorry this came up only at this late stage, but we can't go forward with such a requirement. This would make it more difficult for new users to install the app. The way I see it, we have a couple of options to move forward:
What do you think? Or maybe you have another idea? |
I understand. This is deep in the copypasta crate so I'm not sure I can figure that one out. I'll go back to the drawing board but I don't have have a good idea yet :) |
I can't see the copypasta people requiring this package in their GitHub pipelines. Maybe I'm missing something. I'll try to take another look |
damn :/
|
This is good, actually. If we can bypass the compile time requirements, catch this error at runtime and communicate it to the user we're gold. |
ok I tracked it down to [](https://github.com/quininer/x11-clipboard which) is a dependency of copypasta. Not much more I can do with my limited knowledge of Rust. I'll use my fork for me in the mean time haha. Feel free to close the PR 😢 |
One last shot going with copypasta (I'm testing this out right now) - we can disable the |
no luck unfortunately, tried using copypasta-ext with:
but it still ends up pulling in x11-clipboard. |
Is it something we can fix upstream with a PR in copypasta? |
Hey @djpate - sorry that the copypasta way turned out to be problematic, really appreciate all the effort. Basically we would need to allow the user to set a command that the selection can be piped into - also kind of similar to how copypasta-ext was handling things on wayland. |
@tlinford I really like this idea. Seems to be very flexible. Let me try to see if I figure this one out. Thanks for the mentoring on this. |
So I got this to work using seems like it would be trivial to set it up with use std::io::prelude::*;
use std::process::{Command, Stdio};
pub struct SystemClipboard {
command: String,
}
impl SystemClipboard {
pub fn new(command: String) -> Self {
SystemClipboard {
command
}
}
pub fn set(&self, value: String) -> bool {
let process = match Command::new(self.command.clone())
.args(["-sel", "clip"])
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.spawn() {
Err(why) => {
eprintln!("couldn't spawn {}: {}", self.command, why);
return false;
},
Ok(process) => process,
};
match process.stdin.unwrap().write_all(value.to_string().as_bytes()) {
Err(why) => panic!("couldn't write to {} stdin: {}", self.command, why),
Ok(_) => println!("copied"),
}
return true
}
} the only thing I'm wondering about it is how to configure it. for xclip you need to configure the command: system_clipboard:
command: xclip
args:
- "-sel"
- clip but that would make it very tricky to configure it from the cli's perspective. Any thoughts? |
@djpate I'm not very familiar with structopt, but maybe quoting the command with it's args works? |
Usage: zellij options --copy-command "xclip -sel clip"
Looks like we're getting there at last :)
Did a quick test with |
I haven't taken a look yet, but if @tlinford says it's good to go, I'm happy. Just to make sure: we've gotten rid of the Otherwise, once we merge this we need to make sure to update our documentation. Might be nice to add it to the troubleshooting section. Very cool that this is happening! Thank you all for your work. I think it's a really cool solution. |
@imsnif That's correct we only have a new option called I got rid of copypasta dependency. |
@imsnif - just a note, the e2e ci tests seems to be flaky again, for me they are always passing locally but had to do multiple runs on this: https://github.com/zellij-org/zellij/actions/runs/1699020751/attempts/3 |
Currently the copy to clipboard is done using OSC52 which is not supported by some linux terminals like
gnome-terminal
orxfce4-term
etc.The proposal here is to allow users to set a specific copy command.
zellij options --copy-command "xclip -sel clip"
or
zellij options --copy-command "wl-copy"
would be two examples for x11 and wayland.
This is my first ever rust code so I'm happy to hear any feedback.
This was tested on Ubuntu using
gnome-terminal