-
Notifications
You must be signed in to change notification settings - Fork 879
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
Deprecate window creation with stale event loop #3447
Conversation
7914260
to
a612cc3
Compare
That's based on #3299 In general, I have the following questions:
|
a612cc3
to
caa0d5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Should we add `ActiveEventLoop` getter for `pump_events` trait? The API is kind of weird and requiring it to be normal doesn't sound right.
I don't think this works out.
2. Maybe some types like `CustomCursor` should accept `EventLoop` instead?
Maybe we should keep the EventLoopWindowTarget
type around for this reason.
3. Do we need threaded rendering example...? softbuffer is a bit tricky around threads.
I'd agree.
4. Do we need something else in window example? I've added pretty much everything, though I don't print just all events, since it's a bit noisy.
Nah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #3400 is the better way forwards for this, as it allows us to still support the "create event loop while it's not running" use-case, but allows us to mark it as deprecated for a release cycle.
/// # use winit::window::Window; | ||
/// # let mut event_loop = EventLoop::new().unwrap(); | ||
/// # let window = Window::new(&event_loop).unwrap(); | ||
/// # fn scope(window: &Window) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe?
/// # fn scope(window: &Window) { | |
/// # let window: &Window = unimplemented!(); |
Since the examples are no_run
anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about it since all the code in examples will be kind of dead with such pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, on the other hand, if someone forgets no_run
, then the example will still mistakenly run, it'll just do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still probably go with fn
for now.
Do you mean |
caa0d5b
to
6f463f6
Compare
I've applied it for now the way you've suggested. Though, I haven't touched the |
952f9a0
to
32e0f77
Compare
I think the way forward would be to move |
Proposal for custom cursors we discussed at the meeting today: impl EventLoop {
fn create_cursor(...);
}
impl ActiveEventLoop {
fn create_cursor(...);
} |
2df6c05
to
e593b66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the combined example!
Should we add
ActiveEventLoop
getter forpump_events
trait? The API is kind of weird and requiring it to be normal doesn't sound right.
I'm very unsure, but let's punt on it for now, as users still have the option of using EventLoop::create_window
.
Do we need threaded rendering example...?
Would be kinda nice at some point, but it can definitely wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orbital changes are good
@daxpedda I believe this is waiting mostly on your review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's actually a lot of changes in one PR.
LGTM!
e593b66
to
ec87160
Compare
661d15d
to
ec4fafe
Compare
Creating window when event loop is not running generally doesn't work, since a bunch of events and sync OS requests can't be processed. This is also an issue on e.g. Android, since window can't be created outside event loop easily. Thus deprecate the window creation when event loop is not running, as well as other resource creation to running event loop. Given that all the examples use the bad pattern of creating the window when event loop is not running and also most example existence is questionable, since they show single thing and the majority of their code is window/event loop initialization, they wore merged into a single example 'window.rs' example that showcases very simple application using winit. Fixes #3399.
Replace the `CustomCursorBuilder` with the `CustomCursorSource` and perform the loading of the cursor via the `EventLoop::create_custom_cursor` instead of passing it to the builder itself. This follows the `EventLoop::create_window` API.
649c71e
to
ba0e8c5
Compare
Creating window when event loop is not running generally doesn't work,
since a bunch of events and sync OS requests can't be processed. This
is also an issue on e.g. Android, since window can't be created outside
event loop easily.
Thus deprecate the window creation when event loop is not running,
as well as other resource creation to running event loop.
Given that all the examples use the bad pattern of creating the window
when event loop is not running and also most example existence is
questionable, since they show single thing and the majority of their
code is window/event loop initialization, they wore merged into
a single example 'window.rs' example that showcases very simple
application using winit.
Fixes #3399.
Replaces #3299 and #3400.
CHANGELOG.md
if knowledge of this change could be valuable to users--
@daxpedda could you wire the web example into
window.rs
example somehow? We maybe also want android there as well, @MarijnS95 , just without drawing, I guess?