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

Fix clipboard on Wayland #1613

Merged
merged 1 commit into from
May 16, 2022
Merged

Fix clipboard on Wayland #1613

merged 1 commit into from
May 16, 2022

Conversation

Detegr
Copy link
Contributor

@Detegr Detegr commented May 9, 2022

arboard advertises that it works with Wayland, but in reality it only works with Wayland terminal applications. To make the clipboard work with applications that draw Wayland surfaces, arboard isn't going to work.

Copypasta does support Wayland's graphical clipboard, but the usage isn't documented. However, for the reasons mentioned in #1474 the move from Copypasta to arboard makes sense.

To resolve the issue, this commit brings in an optional dependency smithay-clipboard, that is a crate that correctly handles the Wayland clipboard in graphical applications. It is used by default if a Wayland window handle is found. If for some reason the handle to the Wayland window handle cannot be fetched, arboard is used as a backup.

It feels a bit wrong adding the wayland_display argument to from_pixels_per_point, but I couldn't figure out any better alternatives. Maybe pass the whole window to it?

Fixes #1612

@emilk
Copy link
Owner

emilk commented May 11, 2022

wouldn't it make more sense to make this PR to arboard?

@Detegr
Copy link
Contributor Author

Detegr commented May 11, 2022

Maybe. I'll try preparing and submitting a PR to them. Anyway, egui needs a modification to pass the wayland display handle to the clipboard crate, whatever it might be, so I'd leave this open for now.

egui-winit/Cargo.toml Outdated Show resolved Hide resolved
@emilk
Copy link
Owner

emilk commented May 13, 2022

Maybe. I'll try preparing and submitting a PR to them. Anyway, egui needs a modification to pass the wayland display handle to the clipboard crate, whatever it might be, so I'd leave this open for now.

Let's merge this fix into egui-winit now and then, later, we can hopefully remove it when this gets upstreamed to arboard.

@Detegr Detegr force-pushed the master branch 3 times, most recently from 901fe64 to 489bf15 Compare May 15, 2022 06:51
Comment on lines 61 to 64
#[cfg(unix)]
let wayland_display = window.wayland_display();
#[cfg(not(unix))]
let wayland_display = None;
Copy link
Contributor Author

@Detegr Detegr May 15, 2022

Choose a reason for hiding this comment

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

I think this needs to be wrapped in conditional compilation too, as Windows won't have the wayland_display() function that comes from a platform dependent trait. I'll try compiling this on a Windows machine later today.

Copy link
Owner

Choose a reason for hiding this comment

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

I added Windows and Max to the CI matrix (in #1631), so if you merge in master to this PR branch the CI will test for you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! A welcome change. Rebased master. I guess I need a maintainer approval to get it the CI running as I'm a first-time contributor.

@emilk
Copy link
Owner

emilk commented May 15, 2022

So it seems the new CI checks works! The problem with #[cfg(not(unix))] is that it includes macos!

@enomado
Copy link
Contributor

enomado commented May 15, 2022

cargo check --target wasm32-unknown-unknown

emits

69 |         Ok(Clipboard { platform: PlatformClipboard::new()? })
   |                                  ^^^^^^^^^^^^^^^^^ use of undeclared type `PlatformClipboard`

@Detegr
Copy link
Contributor Author

Detegr commented May 15, 2022

I see now why #[cfg(unix)] is problematic. Let's use the same OS listing as winit does for the conditional compilation, that should work I guess.

@Detegr
Copy link
Contributor Author

Detegr commented May 16, 2022

cargo check --target wasm32-unknown-unknown

emits

69 |         Ok(Clipboard { platform: PlatformClipboard::new()? })
   |                                  ^^^^^^^^^^^^^^^^^ use of undeclared type `PlatformClipboard`

I think this issue is unrelated to this PR. The same happens with the master branch too.

arboard advertises that it works with Wayland, but in reality it only
works with Wayland terminal applications. To make the clipboard work
with applications that draw Wayland surfaces, arboard isn't going to
work.

Copypasta does support Wayland's graphical clipboard, but the usage
isn't documented. However, for the reasons mentioned in emilk#1474 the move
from Copypasta to arboard makes sense.

To resolve the issue, this commit brings in an optional dependency
smithay-clipboard, that is a crate that correctly handles the Wayland
clipboard in graphical applications. It is used by default if a Wayland
window handle is found. If for some reason the handle to the Wayland
window handle cannot be fetched, arboard is used as a backup.
@emilk
Copy link
Owner

emilk commented May 16, 2022

I would prefer if you made new commits instead of squashing. New commits means I can see the changes since the previous one! I always squash on merge anyway, so don't worry about making a pretty history.

@Detegr
Copy link
Contributor Author

Detegr commented May 16, 2022

You can use the Compare button to see changes. But if you're squashing them on merge anyway, I'll do separate commits in the future :)

@emilk
Copy link
Owner

emilk commented May 16, 2022

You can use the Compare button to see changes

Today I learned! thanks :) Still, commits has the benefit of giving a description of the change as well. But it seems all good now!

@emilk emilk merged commit b8a5924 into emilk:master May 16, 2022
@complexspaces
Copy link

Hi, primary maintainer of arboard here 👋. I got pointed here by an issue mentioning egui used the crate. Its very cool to see 😄

wouldn't it make more sense to make this PR to arboard?

Maybe. I'll try preparing and submitting a PR to them. ...

@Detegr I think that we'd be willing to accept a similar contribution to this that lets users control the window surface used for both X11 and Wayland in arboard. The details might be somewhat different though as I'm hesitant to use smithay-clipboard due to its bias worker implementation.

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.

Clipboard support is broken on Wayland
4 participants