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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/egui-winit/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ All notable changes to the `egui-winit` integration will be noted in this file.


## Unreleased

* Fix unsafe API: remove `State::new_with_wayland_display`; change `Clipboard::new` to take `&EventLoopWindowTarget<T>`.

## 0.21.1 - 2023-02-12
* Fixed crash when window position is in an invalid state, which could happen e.g. due to changes in monitor size or DPI ([#2722](https://github.com/emilk/egui/issues/2722)).
Expand Down
41 changes: 29 additions & 12 deletions crates/egui-winit/src/clipboard.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::os::raw::c_void;
use winit::event_loop::EventLoopWindowTarget;

/// Handles interfacing with the OS clipboard.
///
Expand All @@ -25,8 +25,12 @@ pub struct Clipboard {
}

impl Clipboard {
#[allow(unused_variables)]
pub fn new(#[allow(unused_variables)] wayland_display: Option<*mut c_void>) -> Self {
/// Construct a new instance
///
/// # Safety
///
/// The returned `Clipboard` must not outlive the input `_event_loop`.
pub fn new<T>(_event_loop: &EventLoopWindowTarget<T>) -> Self {
Self {
#[cfg(all(feature = "arboard", not(target_os = "android")))]
arboard: init_arboard(),
Expand All @@ -41,7 +45,7 @@ impl Clipboard {
),
feature = "smithay-clipboard"
))]
smithay: init_smithay_clipboard(wayland_display),
smithay: init_smithay_clipboard(_event_loop),

clipboard: Default::default(),
}
Expand Down Expand Up @@ -132,15 +136,28 @@ fn init_arboard() -> Option<arboard::Clipboard> {
),
feature = "smithay-clipboard"
))]
fn init_smithay_clipboard(
wayland_display: Option<*mut c_void>,
fn init_smithay_clipboard<T>(
_event_loop: &EventLoopWindowTarget<T>,
) -> Option<smithay_clipboard::Clipboard> {
if let Some(display) = wayland_display {
tracing::debug!("Initializing smithay clipboard…");
#[allow(unsafe_code)]
Some(unsafe { smithay_clipboard::Clipboard::new(display) })
} else {
tracing::debug!("Cannot initialize smithay clipboard without a display handle");
// Note: ideally "smithay-clipboard" would imply "wayland", but it doesn't.
#[cfg(feature = "wayland")]
{
use winit::platform::wayland::EventLoopWindowTargetExtWayland as _;
if let Some(display) = _event_loop.wayland_display() {
tracing::debug!("Initializing smithay clipboard…");
#[allow(unsafe_code)]
Some(unsafe { smithay_clipboard::Clipboard::new(display) })
} else {
tracing::debug!("Cannot initialize smithay clipboard without a display handle");
None
}
}

#[cfg(not(feature = "wayland"))]
{
tracing::debug!(
"You need to enable the 'wayland' feature of 'egui-winit' to get a working clipboard"
);
None
}
}
35 changes: 6 additions & 29 deletions crates/egui-winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

#![allow(clippy::manual_range_contains)]

use std::os::raw::c_void;

#[cfg(feature = "accesskit")]
pub use accesskit_winit;
pub use egui;
Expand Down Expand Up @@ -85,11 +83,12 @@ pub struct State {
}

impl State {
/// Construct a new instance
///
/// # Safety
///
/// The returned `State` must not outlive the input `_event_loop`.
pub fn new<T>(event_loop: &EventLoopWindowTarget<T>) -> Self {
Self::new_with_wayland_display(wayland_display(event_loop))
}

pub fn new_with_wayland_display(wayland_display: Option<*mut c_void>) -> Self {
let egui_input = egui::RawInput {
has_focus: false, // winit will tell us when we have focus
..Default::default()
Expand All @@ -103,7 +102,7 @@ impl State {
current_cursor_icon: None,
current_pixels_per_point: 1.0,

clipboard: clipboard::Clipboard::new(wayland_display),
clipboard: clipboard::Clipboard::new(event_loop),

simulate_touch_screen: false,
pointer_touch_id: None,
Expand Down Expand Up @@ -871,28 +870,6 @@ fn translate_cursor(cursor_icon: egui::CursorIcon) -> Option<winit::window::Curs
}
}

/// Returns a Wayland display handle if the target is running Wayland
fn wayland_display<T>(_event_loop: &EventLoopWindowTarget<T>) -> Option<*mut c_void> {
#[cfg(feature = "wayland")]
#[cfg(any(
target_os = "linux",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
))]
{
use winit::platform::wayland::EventLoopWindowTargetExtWayland as _;
return _event_loop.wayland_display();
}

#[allow(unreachable_code)]
{
let _ = _event_loop;
None
}
}

// ---------------------------------------------------------------------------

/// Profiling macro for feature "puffin"
Expand Down