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

egui-winit: Fix unsafe API of Clipboard::new #2765

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented Feb 26, 2023

So, window_clipboard is not (very actively) maintained and I had a look around...

  • Winit doesn't yet have a Clipboard API
  • copypasta is maintained, supports the "primary" clipboard, but makes no attempt to provide a Wayland-X11 abstraction
  • arboard is maintained, supports HTML and uncompressed RGBA images, but makes no attempt to provide a Wayland-X11 abstraction
  • egui-winit does have this abstraction, but with an unsafe API

smithay_clipboard::Clipboard::new notes:

display must be a valid *mut wl_display pointer, and it must remain valid for as long as Clipboard object is alive.

This change addresses the first point (prevents calling Clipboard::new with an arbitrary pointer).

Addressing the second point with Rust's lifetimes would probably be very frustrating. I haven't tried. Addressing it by marking Clipboard::new and State::new as unsafe would be another approach, but would result in all code within State::new being compiled in unsafe mode, would violate your #[forbid(unsafe)] policy and is probably needlessly onerous (given that observing broken behaviour is unlikely), so I opted merely to add documentation here.


I ran sh/check but egui-glow fails cargo check --all-features (on X11). Nothing to do with me.

The old API allowed passing an arbitrary pointer. The new
API still breaks safety by allowing the object to outlive
the input, but is at least safer.
crates/egui-winit/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks good!

I think it would be good to document in crates/egui-winit/Cargo.toml that the wayland feature is required to get the clipboard working on wayland.

@dhardy
Copy link
Contributor Author

dhardy commented Mar 1, 2023

Without the wayland feature Winit won't support Wayland at all, so I don't think there's any point.

@emilk
Copy link
Owner

emilk commented Mar 29, 2023

needs cargo fmt

@emilk
Copy link
Owner

emilk commented Mar 29, 2023

cargo check --locked --all-features --all-targets fails

@dhardy
Copy link
Contributor Author

dhardy commented Mar 29, 2023

Updated. Wasn't expecting you to push to my branch, 😆 .

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Thanks!

@emilk emilk merged commit fb726aa into emilk:master Mar 30, 2023
@emilk emilk added the egui-winit porblems related to winit label Apr 18, 2023
@emilk emilk changed the title egui-winit: fix unsafe API of Clipboard::new egui-winit: Fix unsafe API of Clipboard::new Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui-winit porblems related to winit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants