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

Deprecate window creation with stale event loop #3447

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented Jan 31, 2024

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.

  • Tested on all platforms changed // Kind of.
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

--

@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?

@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch 3 times, most recently from 7914260 to a612cc3 Compare January 31, 2024 21:28
@kchibisov
Copy link
Member Author

kchibisov commented Jan 31, 2024

That's based on #3299

In general, I have the following questions:

  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.
  2. Maybe some types like CustomCursor should accept EventLoop instead?
  3. Do we need threaded rendering example...? softbuffer is a bit tricky around threads.
  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.

@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch from a612cc3 to caa0d5b Compare January 31, 2024 21:33
Copy link
Member

@notgull notgull left a 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.

Copy link
Member

@madsmtm madsmtm left a 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Suggested change
/// # fn scope(window: &Window) {
/// # let window: &Window = unimplemented!();

Since the examples are no_run anyways.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@kchibisov
Copy link
Member Author

"create event loop while it's not running"

Do you mean Window here? I'm not so sure about it, since I'd rather force users here to more correct design and allow us to deal with soundness stuff more nicely. The issue is though, that pump_events is a bit out of place and I'm not sure how to enforce lifetimes for it, etc. Like the users are more likely to drop the event loop when using it all together which could lead to issues all over the place. But I guess pump_events stuff should be discussed separately and probably marked as unsafe in the first place?

@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch from caa0d5b to 6f463f6 Compare February 1, 2024 16:12
@kchibisov
Copy link
Member Author

I think #3400 is the better way forwards for this

I've applied it for now the way you've suggested. Though, I haven't touched the CustomCursor for example.

@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch 2 times, most recently from 952f9a0 to 32e0f77 Compare February 1, 2024 16:19
@kchibisov kchibisov changed the title Require event loop to run when creating Window Deprecate window creation with stale event loop Feb 1, 2024
@kchibisov
Copy link
Member Author

Maybe we should keep the EventLoopWindowTarget type around for this reason.

I think the way forward would be to move CustomCursor into the EventLoop instead of getting the &EventLoopWindowTarget.

@madsmtm
Copy link
Member

madsmtm commented Feb 2, 2024

Proposal for custom cursors we discussed at the meeting today:

impl EventLoop {
    fn create_cursor(...);
}

impl ActiveEventLoop {
    fn create_cursor(...);
}

Copy link
Member

@madsmtm madsmtm left a 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 for pump_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.

examples/window.rs Show resolved Hide resolved
examples/child_window.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
examples/pump_events.rs Show resolved Hide resolved
src/event_loop.rs Show resolved Hide resolved
src/window.rs Outdated Show resolved Hide resolved
src/window.rs Outdated Show resolved Hide resolved
src/window.rs Show resolved Hide resolved
@madsmtm
Copy link
Member

madsmtm commented Feb 9, 2024

@daxpedda I've opened #3473 for making the big example work on the web.

Copy link
Member

@jackpot51 jackpot51 left a 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

@madsmtm
Copy link
Member

madsmtm commented Feb 13, 2024

@daxpedda I believe this is waiting mostly on your review

Copy link
Member

@daxpedda daxpedda left a 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!

src/window.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch from e593b66 to ec87160 Compare February 18, 2024 01:10
@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch 3 times, most recently from 661d15d to ec4fafe Compare February 19, 2024 05:07
CHANGELOG.md Outdated Show resolved Hide resolved
examples/child_window.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
examples/pump_events.rs Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
src/window.rs Outdated Show resolved Hide resolved
src/window.rs Show resolved Hide resolved
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.
@kchibisov kchibisov force-pushed the kchibisov/restrict-window-to-loop branch from 649c71e to ba0e8c5 Compare February 21, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability S - docs Awareness, docs, examples, etc. S - enhancement Wouldn't this be the coolest?
Development

Successfully merging this pull request may close these issues.

Merge all examples showing one feature into a single example
6 participants