Skip to content

Commit

Permalink
Upgrade winit to 0.30.2 (emilk#4849)
Browse files Browse the repository at this point in the history
* Closes emilk#1918
* Closes emilk#4437
* Closes emilk#4709
* [x] I have followed the instructions in the PR template

Hiya,

I need new winit for a specific fix for a android_native_actvity. There
are already two PRs, but both don't seem to have a lot of movement, or
are entirely complete:

emilk#4466
Seems to have gone stale & is missing some bits.

emilk#4702
Also seems stale (if less so), and is missing a refactor to
run_on_demand. I also *think* the accesskit integration has a mistake
and can't be enabled. I've marked them as a co-author on this as I
started from this branch. (I think! Haven't done that on git before...).

Sorry for the wall of text but just dumping some details / thoughts
here:

- There's an issue with creating child windows in winit 0.30.1 and up on
macOS. The multiple_viewports, "create immediate viewport" example
crashes on anything later 0.30.1, with a stack overflow in unsafe code.
I've create [a winit
issue](rust-windowing/winit#3800), it *might*
already be fixed in 0.31.0 but I can't test as 0.31 will likely require
another refactoring. For now I have just pinned things to 0.30.0 exatly.

- Winit has deprecated run_on_demand, instead requiring the
ApplicationHandler interface. In 0.31.0 run_on_demand is removed. I've
refactored both the integration and the WinitApp trait to follow this
pattern. I've left user_events a bit more opaque, as it seems 0.31.0 is
doing a rework of UserEvents too.

- I've used the new lazy init approach for access kit from this branch
https://github.com/mwcampbell/egui/tree/accesskit-new-lazy-init and
marked Matt as co-author, thanks Matt!

- There was very similair but not quite the same code for run_and_return
and run_and_exit. I've merged them, but looking at the github issues
graveyard it seems vey finnicky. I *hope* this is more robust than
before but it's a bit scary.

- when receiving new_events this also used to check the redraw timing
dictionary. That doesn't seem necesarry so left this out, but that is a
slight behaviour change?

- I have reeneabled serial_windows on macOS. I wondered whether it was
fixed after this PR and does seem to be! However, even before this PR it
seems to work, so maybe winit has sorted things out before that...
Windows also works fine now without the extra hack.

- I've done a very basic test of AccessKit on Windows and screen reader
seems ok but I'm really not knowleadgable enough to say whether it's all
good or not.

- I've tested cargo tests & all examples on Windows & macOS, and ran a
basic Android app. Still, testing native platforms is wel... hard so if
anyone can test linux / iOs / older mac versions / windows 10 would
probably be a good idea!

- For consistencys sake I've made all event like functions in WinitApp
return a `Result<EventResult>`. There's quite a bit of Ok-wrapping now,
maybe too annoying? Not sure.

Thank you for having a look!

* [x] macOS
* [x] Windows
* [x] Wayland (thanks [SiebenCorgie](https://github.com/SiebenCorgie))
* [x] X11 (thanks
[crumblingstatue](https://github.com/crumblingstatue)!,
[SiebenCorgie](https://github.com/SiebenCorgie))

* [x] Fix "follow system theme" not working on initial startup (winit
issue, pinning to 0.30.2 for now).
* [x] Fix `request_repaint_after`

---------

Co-authored-by: mwcampbell <[email protected]>
Co-authored-by: j-axa <[email protected]>
Co-authored-by: DataTriny <[email protected]>
Co-authored-by: Emil Ernerfeldt <[email protected]>
  • Loading branch information
5 people authored and 486c committed Oct 9, 2024
1 parent 6c0750c commit a9d4216
Show file tree
Hide file tree
Showing 25 changed files with 1,275 additions and 1,124 deletions.
669 changes: 421 additions & 248 deletions Cargo.lock

Large diffs are not rendered by default.

9 changes: 5 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ bytemuck = "1.7.2"
criterion = { version = "0.5.1", default-features = false }
document-features = " 0.2.8"
glow = "0.13"
glutin = "0.31"
glutin-winit = "0.4"
glutin = "0.32.0"
glutin-winit = "0.5.0"
image = { version = "0.25", default-features = false }
log = { version = "0.4", features = ["std"] }
nohash-hasher = "0.2"
Expand All @@ -85,16 +85,17 @@ ron = "0.8"
raw-window-handle = "0.6.0"
serde = { version = "1", features = ["derive"] }
thiserror = "1.0.37"
web-time = "0.2" # Timekeeping for native and web
web-time = "1.1.0" # Timekeeping for native and web
wasm-bindgen = "0.2"
wasm-bindgen-futures = "0.4"
web-sys = "0.3.58"
wgpu = { version = "22.0.0", default-features = false, features = [
# Make the renderer `Sync` even on wasm32, because it makes the code simpler:
"fragile-send-sync-non-atomic-wasm",
] }
winit = { version = "0.29.4", default-features = false }

# Currently can't upgrade above 0.30.2 due to https://github.com/rust-windowing/winit/issues/3837
winit = { version = "=0.30.2", default-features = false }

[workspace.lints.rust]
unsafe_code = "deny"
Expand Down
13 changes: 1 addition & 12 deletions crates/eframe/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,7 @@ android-native-activity = ["egui-winit/android-native-activity"]
default_fonts = ["egui/default_fonts"]

## Use [`glow`](https://github.com/grovesNL/glow) for painting, via [`egui_glow`](https://github.com/emilk/egui/tree/master/crates/egui_glow).
glow = [
"dep:egui_glow",
"dep:glow",
"dep:glutin-winit",
"dep:glutin",
"dep:rwh_05",
"winit/rwh_05",
]
glow = ["dep:egui_glow", "dep:glow", "dep:glutin-winit", "dep:glutin"]

## Enable saving app state to disk.
persistence = [
Expand Down Expand Up @@ -141,10 +134,6 @@ web-time.workspace = true

egui_glow = { workspace = true, optional = true, default-features = false }
glow = { workspace = true, optional = true }
# glutin stuck on old version of raw-window-handle:
rwh_05 = { package = "raw-window-handle", version = "0.5.2", optional = true, features = [
"std",
] }
ron = { workspace = true, optional = true, features = ["integer128"] }
serde = { workspace = true, optional = true }

Expand Down
2 changes: 1 addition & 1 deletion crates/eframe/src/epi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use static_assertions::assert_not_impl_any;

#[cfg(not(target_arch = "wasm32"))]
#[cfg(any(feature = "glow", feature = "wgpu"))]
pub use winit::{event_loop::EventLoopBuilder, window::WindowBuilder};
pub use winit::{event_loop::EventLoopBuilder, window::WindowAttributes};

/// Hook into the building of an event loop before it is run
///
Expand Down
32 changes: 4 additions & 28 deletions crates/eframe/src/native/epi_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use web_time::Instant;

use std::path::PathBuf;
use winit::event_loop::EventLoopWindowTarget;
use winit::event_loop::ActiveEventLoop;

use raw_window_handle::{HasDisplayHandle as _, HasWindowHandle as _};

Expand All @@ -12,9 +12,9 @@ use egui_winit::{EventResponse, WindowSettings};

use crate::{epi, Theme};

pub fn viewport_builder<E>(
pub fn viewport_builder(
egui_zoom_factor: f32,
event_loop: &EventLoopWindowTarget<E>,
event_loop: &ActiveEventLoop,
native_options: &mut epi::NativeOptions,
window_settings: Option<WindowSettings>,
) -> ViewportBuilder {
Expand Down Expand Up @@ -95,10 +95,7 @@ pub fn apply_window_settings(
}
}

fn largest_monitor_point_size<E>(
egui_zoom_factor: f32,
event_loop: &EventLoopWindowTarget<E>,
) -> egui::Vec2 {
fn largest_monitor_point_size(egui_zoom_factor: f32, event_loop: &ActiveEventLoop) -> egui::Vec2 {
crate::profile_function!();

let mut max_size = egui::Vec2::ZERO;
Expand Down Expand Up @@ -229,27 +226,6 @@ impl EpiIntegration {
}
}

#[cfg(feature = "accesskit")]
pub fn init_accesskit<E: From<egui_winit::accesskit_winit::ActionRequestEvent> + Send>(
&self,
egui_winit: &mut egui_winit::State,
window: &winit::window::Window,
event_loop_proxy: winit::event_loop::EventLoopProxy<E>,
) {
crate::profile_function!();

let egui_ctx = self.egui_ctx.clone();
egui_winit.init_accesskit(window, event_loop_proxy, move || {
// This function is called when an accessibility client
// (e.g. screen reader) makes its first request. If we got here,
// we know that an accessibility tree is actually wanted.
egui_ctx.enable_accesskit();
// Enqueue a repaint so we'll receive a full tree update soon.
egui_ctx.request_repaint();
egui_ctx.accesskit_placeholder_tree_update()
});
}

/// If `true`, it is time to close the native window.
pub fn should_close(&self) -> bool {
self.close
Expand Down
54 changes: 54 additions & 0 deletions crates/eframe/src/native/event_loop_context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
use std::cell::Cell;
use winit::event_loop::ActiveEventLoop;

thread_local! {
static CURRENT_EVENT_LOOP: Cell<Option<*const ActiveEventLoop>> = Cell::new(None);
}

struct EventLoopGuard;

impl EventLoopGuard {
fn new(event_loop: &ActiveEventLoop) -> Self {
CURRENT_EVENT_LOOP.with(|cell| {
assert!(
cell.get().is_none(),
"Attempted to set a new event loop while one is already set"
);
cell.set(Some(event_loop as *const ActiveEventLoop));
});
Self
}
}

impl Drop for EventLoopGuard {
fn drop(&mut self) {
CURRENT_EVENT_LOOP.with(|cell| cell.set(None));
}
}

// Helper function to safely use the current event loop
#[allow(unsafe_code)]
pub fn with_current_event_loop<F, R>(f: F) -> Option<R>
where
F: FnOnce(&ActiveEventLoop) -> R,
{
CURRENT_EVENT_LOOP.with(|cell| {
cell.get().map(|ptr| {
// SAFETY:
// 1. The pointer is guaranteed to be valid when it's Some, as the EventLoopGuard that created it
// lives at least as long as the reference, and clears it when it's dropped. Only run_with_event_loop creates
// a new EventLoopGuard, and does not leak it.
// 2. Since the pointer was created from a borrow which lives at least as long as this pointer there are
// no mutable references to the ActiveEventLoop.
let event_loop = unsafe { &*ptr };
f(event_loop)
})
})
}

// The only public interface to use the event loop
pub fn with_event_loop_context(event_loop: &ActiveEventLoop, f: impl FnOnce()) {
// NOTE: For safety, this guard must NOT be leaked.
let _guard = EventLoopGuard::new(event_loop);
f();
}
Loading

0 comments on commit a9d4216

Please sign in to comment.