Proposal: Allow use of system clipboard

[djpate]

Currently the copy to clipboard is done using OSC52 which is not supported by some linux terminals like gnome-terminal or xfce4-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

[imsnif]

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!

[tlinford]

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.

[djpate]

Perfect. Thanks for the feedback. I'll try to work on it this weekend.

[djpate]

Updated.

[tlinford]

Cool!
Tested it on wayland, and I had to install wl-clipboard to get it working (was getting Error: thread 'screen' panicked at 'called Result::unwrap()on anErr value: NoBinary': zellij-server/src/tab/mod.rs:1926 without).

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?

[djpate]

@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.

[tlinford]

Hey @djpate,
we talked about this in our weekly meeting, and would like to merge this great feature asap.
The one thing blocking this is the panic when wl-clipboard is not found.

The best way to work around this potential problem would be to make the statusbar plugin display an error message if DisplayServer::select().try_context() gives an Err. This could be done by replicating the way the 'Text copied to clipboard' message is displayed.

If you want to go this route, you could check out to how Event::CopyToClipboard is used.

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 :)

[djpate]

@tlinford I'll look into outputting the error in the status bar.

[djpate]

@tlinford I think that should do it but I can't repro to test... could you take a look?

[tlinford]

@djpate close, and my bad I pointed you at the wrong line, the error is coming from here:

clipboard_context
                .set_contents(selection.to_owned())
                .unwrap();

Although it's probably a good idea at this point to handle both error cases.

[tlinford]

 let res = DisplayServer::select()
                .try_context()
                .ok_or_else(|| "could not get clipboard provider".into())
                .and_then(|mut ctx| ctx.set_contents(selection.to_owned()));

            if res.is_err() {
                system_clipboard_failure = true
            }

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 :)

[djpate]

Awesome. Updated the code

[djpate]

@tlinford @imsnif Pipeline is fixed. I had to add a couple of ubuntu packages for it to pass.

Everything should be good now.

[imsnif]

Hey @djpate - could you briefly explain about those ubuntu packages you had to add?

[djpate]

@imsnif I believe they are required for copypasta-ext to compile. They are packages for xcb which is x11 C bindings.

[imsnif]

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 :/

[djpate]

I'm not adding it to macosx pipeline and it compiled fine.

Anyone compiling on Linux is gonna need those dependencies.

[imsnif]

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:

1. Find out if we can somehow require these at runtime and then give an error similar to what we did with Wayland above
2. Find a different solution/crate that interacts with the clipboard in a different way (not sure this exists or is possible?)

What do you think? Or maybe you have another idea?

[djpate]

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 :)

[djpate]

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

[tlinford]

damn :/
looks like it's a runtime dependency as well, get this on a ubuntu server vm:

./zellij: error while loading shared libraries: libxcb-xfixes.so.0: cannot open shared object file: No such file or directory

[imsnif]

damn :/ looks like it's a runtime dependency as well, get this on a ubuntu server vm:

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.

[djpate]

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 😢

[tlinford]

One last shot going with copypasta (I'm testing this out right now) - we can disable the x11-fork feature and use x11-bin instead. That way we should get the same behaviour as with wayland, and copying on x11 will rely on xclip and xsel being installed.

[tlinford]

no luck unfortunately, tried using copypasta-ext with:

copypasta-ext = { version = "0.3.7", default-features = false, features = [
    "x11-bin",
    "wayland-bin",
] }

but it still ends up pulling in x11-clipboard.

[imsnif]

Is it something we can fix upstream with a PR in copypasta?

[tlinford]

Hey @djpate - sorry that the copypasta way turned out to be problematic, really appreciate all the effort.
Another idea: we could take insipiration from tmux once again, and go for something like copy-command (https://github.com/tmux/tmux/wiki/Clipboard#how-to-configure---tmux-32-and-later).

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.
This way we don't need to depend on anything, and we can keep the awesome error reporting. what do you say?

[djpate]

@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.

[djpate]

So I got this to work using xclip.

seems like it would be trivial to set it up with wl-copy or others.

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: xclip and the args to that command -sel and clip so that means in the config file we could do something like

system_clipboard:
  command: xclip
  args:
    - "-sel"
    - clip

but that would make it very tricky to configure it from the cli's perspective.

Any thoughts?

[tlinford]

@djpate I'm not very familiar with structopt, but maybe quoting the command with it's args works?
something like --copy-command "xclip -sel clip"

[djpate]

@tlinford @imsnif It's me again, I've switched it use --copy-command

Seems to work fine with xclip and wl-copy.

The issue I have is that I can't get the error to display for me.

zellij options --copy-command "foobar"

does not display the failure message for some reason.

[tlinford]

Looks like we're getting there at last :)

does not display the failure message for some reason.

Did a quick test with
❯ cargo make run options --copy-command "does-not-exist"
and I'm seeing the expected error, while it seems to be also working great using wl-copy or wl-copy -p, so I'm not sure why you're not seeing the error.

[djpate]

@tlinford no you're right it's working. Not sure what I was doing wrong.

Okay I think we're there :) @imsnif what do you think?

[imsnif]

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 use_system_clipboard flag now that we're explicitly passing the command, right?

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.

[djpate]

@imsnif That's correct we only have a new option called --copy-command. If not supplied we use OSC52.

I got rid of copypasta dependency.

[tlinford]

@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
@djpate LGTM! Thanks again for this.

[djpate]

@tlinford @imsnif Thanks a lot.

Amazing app you have built.