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.

Sign up for GitHub

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

Jump to bottom

Replace cli-clipboard with arboard #2067

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Replace cli-clipboard with arboard #2067

merged 1 commit into from
Jul 10, 2024

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented May 31, 2024 •
edited
Loading

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 LecrisUT mentioned this pull request May 31, 2024
Draft
@LecrisUT LecrisUT force-pushed the dependency-arboard branch from 84176e8 to 1b55540 Compare May 31, 2024 12:14
@LecrisUT
Copy link
Contributor Author

LecrisUT commented May 31, 2024 •
edited
Loading

@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

LecrisUT
LecrisUT commented May 31, 2024
View reviewed changes
crates/atuin/Cargo.toml
Comment on lines -90 to +93
[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.

@LecrisUT LecrisUT force-pushed the dependency-arboard branch from 1b55540 to 6c8d224 Compare May 31, 2024 12:55
LecrisUT
LecrisUT commented May 31, 2024
View reviewed changes
crates/atuin/src/command/client/search/interactive.rs Outdated
Comment on lines 1177 to 1187
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 LecrisUT force-pushed the dependency-arboard branch from 6c8d224 to e2a1a05 Compare May 31, 2024 13:06
@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)

@ovv ovv mentioned this pull request Jun 5, 2024
Closed
1 task
ellie
ellie previously approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

I'm not sure happy with the clipboard state on linux, but this seems fine for now.

If we face any more weird edge cases, I'd rather disable the clipboard feature by default on Linux than either have unreliable behaviour or a sleep introduced.

@ellie ellie force-pushed the dependency-arboard branch from e2a1a05 to 1398241 Compare July 10, 2024 15:49
ellie
ellie previously approved these changes Jul 10, 2024
View reviewed changes
@ellie ellie enabled auto-merge (squash) July 10, 2024 15:49
@ellie ellie force-pushed the dependency-arboard branch from 1398241 to 910ff1d Compare July 10, 2024 16:10
@ellie ellie disabled auto-merge July 10, 2024 16:10
@ellie ellie enabled auto-merge (squash) July 10, 2024 16:11
@ellie ellie merged commit 33925e3 into atuinsh:main Jul 10, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellie ellie ellie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Copy to clipboard unsuccessful
2 participants
@LecrisUT @ellie