atuinsh / atuin

✨ Magical shell history
https://atuin.sh
MIT License
19.01k stars 527 forks source link

[Bug]: Copy to clipboard unsuccessful #2028

Closed ovv closed 1 week ago

ovv commented 2 months ago

What did you expect to happen?

Pressing ctrl-y in the search view should copy the selected line to the clipboard

What happened?

Pressing ctrl-y closes atuin but the clipboard content is not set

Atuin doctor output

atuin:
  version: 18.2.0
  sync:
    cloud: false
    records: true
    auto_sync: true
    last_sync: 2024-05-17 11:10:19.493442155 +00:00:00
shell:
  name: fish
  default: unknown
  plugins:
  - atuin
system:
  os: Fedora Linux
  arch: x86_64
  version: '39'
  disks:
  - name: /dev/mapper/luks-a71d0523-7b8b-4cc2-9d60-a73e65041027
    filesystem: btrfs
  - name: /dev/mapper/luks-a71d0523-7b8b-4cc2-9d60-a73e65041027
    filesystem: btrfs
  - name: /dev/nvme0n1p2
    filesystem: ext4
  - name: /dev/nvme0n1p1
    filesystem: vfat

Code of Conduct

ovv commented 2 months ago

Interestingly using this patch make it work. I'm quite confused how/why

diff --git a/crates/atuin/src/command/client/search/interactive.rs b/crates/atuin/src/command/client/search/interactive.rs
index 9185bc0f..5caedeb4 100644
--- a/crates/atuin/src/command/client/search/interactive.rs
+++ b/crates/atuin/src/command/client/search/interactive.rs
@@ -1174,7 +1174,8 @@ pub async fn history(
     any(target_os = "windows", target_os = "macos", target_os = "linux")
 ))]
 fn set_clipboard(s: String) {
-    cli_clipboard::set_contents(s).unwrap();
+    cli_clipboard::set_contents(s.to_owned()).unwrap();
+    assert_eq!(cli_clipboard::get_contents().unwrap(), s);
 }

 #[cfg(not(all(
tessus commented 1 month ago

I was not able to reproduce this on macOS.

ellie commented 1 month ago

I think this is only an issue on Linux. What window manager/graphical env are you using?

ovv commented 1 month ago

I think this is only an issue on Linux. What window manager/graphical env are you using?

I'm using KDE Plasma, it reproduces on plasma 5 and 6

The cli_clipboard crate documentation mention a different behavior on Linux.

This uses the platform default behavior for setting clipboard contents. Other users of the Wayland or X11 clipboard will only see the contents copied to the clipboard so long as the process copying to the clipboard exists. MacOS and Windows clipboard contents will stick around after your application exits.

I don't fully understand the so long as the process copying to the clipboard exists since it seems to work if the process doesn't exit instantly. This code work, removing the sleep makes it fail.

use cli_clipboard;
use std::time::Duration;
use std::thread::sleep;

fn main() {
    let the_string = "foobar";
    cli_clipboard::set_contents(the_string.to_owned()).unwrap();
    sleep(Duration::new(1, 0));
}

Looks like the bug is not directly related to atuin

LecrisUT commented 1 month ago

Hi @ellie, I am working on packaging atuin for Fedora (now there is only 1 dependency left, guess which), and I've encountered errors when testing cli_clipboard. Upstream appears to encounter similar test failures. They also point to a few alternatives, primarily arboard and copypasta. The former seems to be the most active right now. Both are packaged right now, so it could be that these work better for linux. Will you consider trying either of these out?

ellie commented 1 month ago

I'd be pretty happy to switch to arboard - I'll likely get around to doing so fairly soon, unless anyone fancies making a PR

ovv commented 1 month ago

@LecrisUT #2067 works fine, but it has the same issue. Removing the ctx.get_text().unwrap(); line result in the clipboard not being set.

LecrisUT commented 1 month ago

@LecrisUT #2067 works fine, but it has the same issue. Removing the ctx.get_text().unwrap(); line result in the clipboard not being set.

Indeed, but I am not sure what would be an appropriate fix. From what I've tested the issue is that the clipboard must be consumed somewhere for it to be saved on the system, otherwise even with set_text().unwrap() it doesn't seem to be propagated. Could be an upstream issue or it could be a misunderstanding on the API, not sure. I couldn't figure it out from reading the issues related to it, but maybe someone has better ideas.

But anything that does ctx.get_text().unwrap() should be fine and it could be something like checking that the clipboard is set, otherwise try again n times and give an error message that it failed afterwards. Dunno how to implement the latter part for that though

ovv commented 1 month ago

I can't test right now, but I'm curious to know if we need an explicity get_text or if a sleep does it

LecrisUT commented 1 month ago

From my testing, the sleep also works, but it's finicky and it makes it slugish. get_text is more responsive. In both cases though, there is no guarantee that the clipboard values was set (or even not overwritten) until we actually test for it

complexspaces commented 4 weeks ago

Hi there, maintainer/owner of arboard here 👋:

From my testing, the sleep also works...

It might be possible that the clipboard manager is taking longer to respond then the data serving thread is willing to wait currently. If you enable trace level logging for arboard, you should be able to see if that is happening by viewing the steps of clipboard handoff.

LecrisUT commented 4 weeks ago

I don't know about @ovv, but I am testing on wayland. Is there an equivalent there?

complexspaces commented 4 weeks ago

It doesn't look like wl-clipboard-rs has much logging it it, so I think the answer is no :(. Most of what that crate does is a black box to arboard for better or worse.