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

Replace cli-clipboard with arboard #2067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented May 31, 2024

Unfortunately I don't think this is covered by the tests, so I am not sure how correct it is. I've mostly followed the example here

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

closes #2028

@ovv Could you give it a try? the current fix here seems to work on my end, but I don't think it's reliable

@LecrisUT LecrisUT changed the title Replace cli-clipboard with arboard Replace cli-clipboard with arboard May 31, 2024
@LecrisUT
Copy link
Contributor Author

LecrisUT commented May 31, 2024

@ovv, seems that arboard has a similar issue?

Note that in my tests the wayland backend did not keep the clipboard contents after the process exited. (Although neither did the X11 backend on my Wayland setup).

Maybe the same fix you had in #2028 works here?

Edit: Well, I can confirm that having the sleep at least worked for me :/. The to_owned() didn't seem to do much

Comment on lines -90 to +94
[target.'cfg(any(target_os = "windows", target_os = "macos", target_os = "linux"))'.dependencies]
cli-clipboard = { version = "0.4.0", optional = true }
[target.'cfg(any(target_os = "windows", target_os = "macos"))'.dependencies]
arboard = { version = "3.4", optional = true }

[target.'cfg(target_os = "linux")'.dependencies]
arboard = { version = "3.4", optional = true, features = ["wayland-data-control"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one, but would want to have at least an option to enable the native wayland support.

Comment on lines 1177 to 1180
cli_clipboard::set_contents(s).unwrap();
let mut ctx = arboard::Clipboard::new().unwrap();
ctx.set_text(s.clone()).unwrap();
// Use the clipboard context to make sure it is saved
ctx.get_text().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_text() seems to make sure that the clipboard content is saved, solving #2028. But I think it should be in a short loop to check the contents are equal. Any preference on what loop to do here?

@LecrisUT
Copy link
Contributor Author

Note: I seem to encounter a bug where I can't copy something from a window (maybe XWayland), but if I try on another window (maybe native wayland) to copy it recovers. I can't seem to reliably reproduce, but it reminds me of a bug that used to happen in KeepassXC: keepassxreboot/keepassxc#8492 (comment)

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.

[Bug]: Copy to clipboard unsuccessful
1 participant