Proposal: Allow use of system clipboard #996

djpate · 2022-01-07T14:48:37Z

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!

zellij-server/src/screen.rs

tlinford · 2022-01-07T16:46:27Z

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 · 2022-01-08T01:22:31Z

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

djpate · 2022-01-10T20:00:10Z

Updated.

tlinford · 2022-01-10T20:31:56Z

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 · 2022-01-10T21:20:15Z

@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 · 2022-01-11T17:48:10Z

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 · 2022-01-11T18:55:58Z

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

djpate · 2022-01-11T20:32:11Z

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

tlinford · 2022-01-11T20:48:59Z

@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 · 2022-01-11T21:09:17Z

 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 · 2022-01-11T21:30:46Z

Awesome. Updated the code

zellij-server/src/tab/mod.rs

djpate · 2022-01-12T14:41:05Z

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

Everything should be good now.

imsnif · 2022-01-12T15:43:00Z

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

djpate · 2022-01-12T15:47:50Z

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

imsnif · 2022-01-12T15:50:22Z

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 · 2022-01-12T16:03:35Z

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

Anyone compiling on Linux is gonna need those dependencies.

imsnif · 2022-01-12T16:09:56Z

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:

Find out if we can somehow require these at runtime and then give an error similar to what we did with Wayland above
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 · 2022-01-12T16:11:28Z

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 · 2022-01-12T16:16:32Z

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 · 2022-01-12T16:30:42Z

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 · 2022-01-12T16:34:53Z

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 · 2022-01-12T20:07:01Z

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 · 2022-01-12T20:32:35Z

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 · 2022-01-13T08:36:07Z

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 · 2022-01-13T09:24:56Z

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

tlinford · 2022-01-13T15:27:55Z

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 · 2022-01-13T15:30:26Z

@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 · 2022-01-13T21:03:23Z

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 · 2022-01-14T10:30:33Z

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

Usage: zellij options --copy-command "xclip -sel clip"

djpate · 2022-01-14T18:22:34Z

@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 · 2022-01-14T21:41:11Z

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 · 2022-01-14T21:54:56Z

@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 · 2022-01-14T22:16:24Z

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 · 2022-01-14T22:47:22Z

@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 · 2022-01-15T11:38:28Z

@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 · 2022-01-15T13:31:35Z

@tlinford @imsnif Thanks a lot.

Amazing app you have built.

…1019)

imsnif requested changes Jan 7, 2022

View reviewed changes

zellij-server/src/screen.rs Outdated Show resolved Hide resolved

djpate force-pushed the main branch 2 times, most recently from 5577a2a to e21b22b Compare January 10, 2022 19:59

djpate force-pushed the main branch from 71f98a6 to ffee5b1 Compare January 11, 2022 21:30

tlinford approved these changes Jan 12, 2022

View reviewed changes

zellij-server/src/tab/mod.rs Outdated Show resolved Hide resolved

zellij-server/src/tab/mod.rs Outdated Show resolved Hide resolved

djpate force-pushed the main branch from 5111bff to a3655a7 Compare January 12, 2022 14:37

Add a copy command option

e0568f3

Usage: zellij options --copy-command "xclip -sel clip"

djpate force-pushed the main branch from b154a23 to e0568f3 Compare January 14, 2022 18:19

tlinford approved these changes Jan 15, 2022

View reviewed changes

tlinford merged commit 9cc2645 into zellij-org:main Jan 15, 2022

tlinford mentioned this pull request Jan 18, 2022

Can't select text with Zellij, Alacritty's URL recognition doesn't work #627

Open

JCallicoat added a commit to JCallicoat/zellij that referenced this pull request Jan 20, 2022

Add option to default config for copy_command (PR zellij-org#996)

3e8983d

JCallicoat mentioned this pull request Jan 20, 2022

Add option to default config for copy_command (PR #996) #1019

Merged

JCallicoat added a commit to JCallicoat/zellij that referenced this pull request Jan 20, 2022

Add option to default config for copy_command (PR zellij-org#996)

7b96e51

tlinford pushed a commit that referenced this pull request Jan 20, 2022

doc(config): add copy-command examples to default config (PR #996) (#…

5f86dc4

…1019)

tlinford mentioned this pull request Feb 10, 2022

Zellij server stability discussion #1050

Closed

ardentperf mentioned this pull request Jan 12, 2025

no ability to copy multiline blocks from zellij inside alacritty basecamp/omakub#363

Open

Proposal: Allow use of system clipboard #996

Proposal: Allow use of system clipboard #996

Conversation

djpate commented Jan 7, 2022 • edited Loading

imsnif left a comment

Choose a reason for hiding this comment

tlinford commented Jan 7, 2022

djpate commented Jan 8, 2022

djpate commented Jan 10, 2022

tlinford commented Jan 10, 2022

djpate commented Jan 10, 2022

tlinford commented Jan 11, 2022

djpate commented Jan 11, 2022

djpate commented Jan 11, 2022

tlinford commented Jan 11, 2022

tlinford commented Jan 11, 2022

djpate commented Jan 11, 2022

djpate commented Jan 12, 2022

imsnif commented Jan 12, 2022

djpate commented Jan 12, 2022

imsnif commented Jan 12, 2022

djpate commented Jan 12, 2022

imsnif commented Jan 12, 2022

djpate commented Jan 12, 2022

djpate commented Jan 12, 2022

tlinford commented Jan 12, 2022

imsnif commented Jan 12, 2022 • edited Loading

djpate commented Jan 12, 2022

tlinford commented Jan 12, 2022

tlinford commented Jan 13, 2022

imsnif commented Jan 13, 2022 • edited Loading

tlinford commented Jan 13, 2022

djpate commented Jan 13, 2022

djpate commented Jan 13, 2022 • edited Loading

tlinford commented Jan 14, 2022

djpate commented Jan 14, 2022

tlinford commented Jan 14, 2022 • edited Loading

djpate commented Jan 14, 2022

imsnif commented Jan 14, 2022

djpate commented Jan 14, 2022 • edited Loading

tlinford commented Jan 15, 2022

djpate commented Jan 15, 2022

djpate commented Jan 7, 2022 •

edited

Loading

imsnif commented Jan 12, 2022 •

edited

Loading

imsnif commented Jan 13, 2022 •

edited

Loading

djpate commented Jan 13, 2022 •

edited

Loading

tlinford commented Jan 14, 2022 •

edited

Loading

djpate commented Jan 14, 2022 •

edited

Loading