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

update winit to 0.30.3 #4702

Closed
wants to merge 7 commits into from
Closed

update winit to 0.30.3 #4702

wants to merge 7 commits into from

Conversation

j-axa
Copy link
Contributor

@j-axa j-axa commented Jun 24, 2024

I liked the new way winit 0.30 handles the eventloop/window creation.

I updated the cargo.toml of relevant crates and fixed the errors that popped up.

It works for my usecase and the demo app seems to work but I can't verify further than that.

@emilk
Copy link
Owner

emilk commented Jun 25, 2024

Comment on lines +138 to +140
#rwh_06 = { package = "raw-window-handle", version = "0.6.2", optional = true, features = [
# "std",
#] }
Copy link
Owner

Choose a reason for hiding this comment

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

rm dead code

Comment on lines +87 to +89
#rwh_06 = { package = "raw-window-handle", version = "0.6.2", features = [
# "std",
#] }
Copy link
Owner

Choose a reason for hiding this comment

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

rm dead code


[[example]]
name = "pure_glow"
required-features = [
"winit",
"egui/default_fonts",
"winit/rwh_05", # glutin stuck on old version of raw-window-handle
"winit/rwh_06", # glutin stuck on old version of raw-window-handle
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"winit/rwh_06", # glutin stuck on old version of raw-window-handle
"winit/rwh_06",

@@ -56,6 +56,7 @@ x11 = ["winit?/x11"]
egui = { workspace = true, default-features = false, features = ["bytemuck"] }
egui-winit = { workspace = true, optional = true, default-features = false }

raw-window-handle.workspace = true
Copy link
Owner

Choose a reason for hiding this comment

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

please keep dependencies sorted

Comment on lines +1829 to +1832
WindowEvent::PinchGesture { .. } => "WindowEvent::TouchpadMagnify",
WindowEvent::RedrawRequested { .. } => "WindowEvent::RedrawRequested",
WindowEvent::SmartMagnify { .. } => "WindowEvent::SmartMagnify",
WindowEvent::TouchpadRotate { .. } => "WindowEvent::TouchpadRotate",
WindowEvent::DoubleTapGesture { .. } => "WindowEvent::SmartMagnify",
WindowEvent::RotationGesture { .. } => "WindowEvent::TouchpadRotate",
Copy link
Owner

Choose a reason for hiding this comment

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

this maps to the old names

create_winit_window_attributes(egui_ctx, event_loop, viewport_builder.clone());
let window = event_loop
.create_window(window_attributes)
.expect("failed to create window");
Copy link
Owner

Choose a reason for hiding this comment

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

Return an error instead

AccessKitActionRequest(accesskit_winit::Event),
}

impl Default for UserEvent {
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to implement Default? And why is the default RequestRepaint?

.build(
window
.window_handle()
.expect("failed to get window handle")
Copy link
Owner

Choose a reason for hiding this comment

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

We don't want crashes. Return errors instead. Here and everywhere.
Add a case to eframe::Error if needed

use rwh_05::HasRawWindowHandle as _; // glutin stuck on old version of raw-window-handle
w.raw_window_handle()
w.window_handle()
.expect("failed to get window handle")
Copy link
Owner

Choose a reason for hiding this comment

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

Return errors

@emilk
Copy link
Owner

emilk commented Jul 8, 2024

Please don't open PRs from your master branch. Read the PR template for why.

@emilk emilk marked this pull request as draft July 8, 2024 11:20
@emilk emilk mentioned this pull request Jul 15, 2024
@ArthurBrussee ArthurBrussee mentioned this pull request Jul 18, 2024
7 tasks
emilk added a commit that referenced this pull request Jul 31, 2024
* Closes #1918
* Closes #4437
* Closes #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:

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

#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!

# Tested on
* [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))


# TODO
* [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 <mattcampbell@pobox.com>
Co-authored-by: j-axa <josef.axa@gmail.com>
Co-authored-by: DataTriny <datatriny@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@emilk
Copy link
Owner

emilk commented Jul 31, 2024

@emilk emilk closed this Jul 31, 2024
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.

2 participants