-
Notifications
You must be signed in to change notification settings - Fork 912
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
Allow specifying an initialization closure in EventLoop::run
#3895
base: master
Are you sure you want to change the base?
Conversation
9827aff
to
d713d52
Compare
3c05c28
to
f6a520e
Compare
f6a520e
to
478b4df
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'm not sure how it could work given that the window creation is generally async and the intent was to move to such an async window creation.
It's also not yet decided where we store the window at the end of the day, and if it'll be in winit it'll be redundant again.
One option we could have is to allow swapping the application states on the fly, but that's all I can think of at the given time.
As for surface, it's generally won't always work when you use just Surface
without Option
since you may fail to create a surface, or surface could be temporally destroyed.
Specifically replying to:
Anything that moves Note that the Android system creates these |
Thanks for the responses, in general, I accept that I may be wrong about when to do surface initialization, I'll change the PR to accommodate once I've thought a bit about it, but I do believe that this closure is the correct time to do window initialization.
Well, perhaps we will in the future tell the user to initialize their surfaces in
Regardless of where we store the window, the user is still going to want some extra state associated with that window, and that state still has to be initialized. The closure I propose provides a place to do that in a type-state safe manner.
Not sure what you mean here, the user would implement
Sure, and that's also what the
Hmm, okay, I was really hoping we could do window and surface initialization in one step, it's just much easier to teach. Did you consider the approach I outlined (not initializing the application before the window is ready, i.e. skipping the lifecycle events that are emitted until the first surface is initialized)? It's kind of a hack, but it would allow the integration for the user to get Android to work to be more of an "add-on" rather than something that every user has to deal with from the onset, even if they don't support Android?
But... The programmer would still be in control of which ones are created, right? And would still be in control of when they close? |
I'm not sure how that could work, unless we push on with an idea where Specifically on Android this would hide the fact that the users'
Not sure and yes? Assume a case where I'm browsing my photos gallery, select a photo, and click on "share". If my Rust app declares an We could always call |
I am playing around with a different approach: to expose traits that together explicitly implement the state machine that winit already suggests you have today. Here is what an application would look like. enum Application {}
struct Uninitialized {
window_attributes: winit::window::WindowAttributes,
}
struct Resumed {
window_attributes: winit::window::WindowAttributes,
#[allow(unused)]
window: winit::window::Window,
}
struct Suspended {
window_attributes: winit::window::WindowAttributes,
}
struct Exited;
type Error = Box<dyn std::error::Error>;
impl<TUserEvent: 'static> winit_ext::Application<TUserEvent> for Application {
type Uninitialized = Uninitialized;
type Resumed = Resumed;
type Suspended = Suspended;
type Exited = Exited;
type Error = Error;
}
impl<TUserEvent: 'static> winit_ext::ApplicationUninitialized<TUserEvent> for Uninitialized {
type Application = Application;
fn initialize(self, event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Resumed, Error> {
let Self { window_attributes } = self;
let window = event_loop.create_window(window_attributes.clone())?;
Ok(Resumed {
window_attributes,
window,
})
}
}
impl<TUserEvent: 'static> winit_ext::ApplicationResumed<TUserEvent> for Resumed {
type Application = Application;
fn suspend(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Suspended, Error> {
let Self {
window_attributes, ..
} = self;
Ok(Suspended { window_attributes })
}
fn exit(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Exited, Error> {
Ok(Exited)
}
}
impl<TUserEvent: 'static> winit_ext::ApplicationSuspended<TUserEvent> for Suspended {
type Application = Application;
fn resume(self, event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Resumed, Error> {
let Self { window_attributes } = self;
let window = event_loop.create_window(window_attributes.clone())?;
Ok(Resumed {
window_attributes,
window,
})
}
fn exit(self, _event_loop: &winit::event_loop::ActiveEventLoop) -> Result<Exited, Error> {
Ok(Exited)
}
}
fn main() {
let event_loop = winit::event_loop::EventLoop::new().expect("event loop error");
let Exited = winit_ext::run(
event_loop,
Uninitialized {
window_attributes: winit::window::WindowAttributes::default(),
},
)
.expect("event loop error")
.expect("application error");
} This achieves a couple of things:
The downside is that the user is forced to implement multiple traits. However, I would argue that any applications that goes into production will have to deal with these basic states, unless you do not want to support suspend/resume (android, ...). The trait definitions are as follows (yes, the pub trait Application<TUserEvent: 'static = ()>: Sized {
type Uninitialized: ApplicationUninitialized<TUserEvent, Application = Self>;
type Resumed: ApplicationResumed<TUserEvent, Application = Self>;
type Suspended: ApplicationSuspended<TUserEvent, Application = Self>;
type Exited;
type Error;
}
pub trait ApplicationUninitialized<TUserEvent: 'static = ()>: Sized {
type Application: Application<TUserEvent, Uninitialized = Self>;
fn initialize(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
) -> Result<
<Self::Application as Application<TUserEvent>>::Resumed,
<Self::Application as Application<TUserEvent>>::Error,
>;
}
pub trait ApplicationResumed<TUserEvent: 'static = ()>: Sized {
type Application: Application<TUserEvent, Resumed = Self>;
fn handle(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
event: EventResumed<TUserEvent>,
) -> Result<Self, <Self::Application as Application<TUserEvent>>::Error> {
if let EventResumed::WindowEvent {
window_id: _,
event: winit::event::WindowEvent::CloseRequested,
} = event
{
event_loop.exit()
}
Ok(self)
}
fn suspend(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
) -> Result<
<Self::Application as Application<TUserEvent>>::Suspended,
<Self::Application as Application<TUserEvent>>::Error,
>;
fn exit(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
) -> Result<
<Self::Application as Application<TUserEvent>>::Exited,
<Self::Application as Application<TUserEvent>>::Error,
>;
}
pub trait ApplicationSuspended<TUserEvent: 'static = ()>: Sized {
type Application: Application<TUserEvent, Suspended = Self>;
fn handle(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
event: EventSuspended<TUserEvent>,
) -> Result<Self, <Self::Application as Application<TUserEvent>>::Error> {
let _ = (event_loop, event);
Ok(self)
}
fn resume(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
) -> Result<
<Self::Application as Application<TUserEvent>>::Resumed,
<Self::Application as Application<TUserEvent>>::Error,
>;
fn exit(
self,
event_loop: &winit::event_loop::ActiveEventLoop,
) -> Result<
<Self::Application as Application<TUserEvent>>::Exited,
<Self::Application as Application<TUserEvent>>::Error,
>;
} I wrote an adapter that converts a type implementing |
Summary
Allow windows and surfaces to be created before the user's application state, avoiding
Option
around these in user code.This is paired with using the
Drop
impl ofApplicationHandler
instead ofexited
to communicate that the event loop is done (composes much better).See this example for how things now look:
Motivation
One of the most common problems that users encounter in
v0.30
is that their window and surface needs to be wrapped in anOption
, because they can no longer initialize windows at the start ofmain
, and the method they're expected to initialize it in (resumed
inv0.30
,can_create_surfaces
onmaster
) isn't called before the application state is already created.See #3447 for the original change, it tackles many real bugs, such as:
EventLoop::run
on macOS and iOS.create_window
being called in a non-sync fashion on Wayland.We were aware that the solution was overly restrictive (as also noted in #3626 and the changelog), and provided the deprecated
EventLoop::create_window
to ease the transition. This API has since been removed in #3826 to prepare forv0.30
, but users are still left with no real solution to the ergonomic issues.Implementation details
We haven't really discussed this officially, but I think it's fair to say that Winit's mobile platforms are somewhat second-class citizens (and we could consider stating so in #3887). That is not to say that our support for them cannot be improved, #3833 is a great example of this, but I think that we should remember that our user-base is by far most concerned with desktop (and to some extent web).
With that in mind, I believe that the solution we've chosen for surface recreation is complicating our API, as it is a problem that purely exists on Android (also discussed in #3765). Instead, I believe that we should strive to make surface creation easy, and surface recreation possible. Furthermore, the current solution is probably also incorrect in that we're pushing users towards re-creating
Window
itself, while it really is only the surface that needs to be (from my understanding, unsure?).My proposed solution to this is to instead add a closure to the initial
EventLoop::run
call (keepingEventLoop::run_app
as deprecated fallback), which allows access toActiveEventLoop
before creating the application. On most platforms, this closure will simply be called before the event loop starts, but on macOS/iOS, it will be called at key initialization steps in the application's lifecycle, namely[NS|UI]ApplicationDidFinishLaunchingNotification
. On Android, we do roughly the same thing, except that we delay initialization even further, all the way up untilInitWindow
/onNativeWindowCreated
/surfaceCreated
is called, and then emit the lifecycle events that have come before it. I am not yet sure about web, but I think it's mostly the same there.To be clear, this changes what the user should do on Android from:
Initialize inside
can_create_surfaces
, destroy the surface indestroy_surfaces
, recreate it incan_create_surfaces
.To:
Initialize inside the main closure, destroy the surface in
destroy_surfaces
, recreate it inrecreate_surfaces
.I've implemented this in the
window
example, should be fairly self-explanatory.This will require some boxing to be made object-safe, see this playground, but that's the cost of going the trait route (which we've discussed at length, so I won't belabour the point ;) ).
This PR was my original motivation for doing #3721, and resolves my primary concerns from #2903.
Prior art
The SDL uses basically the old
poll_events
API that we moved away from a long time ago. To tackle iOS, it spawns a new thread and handles the user's call back in that thread (which is questionably thread safe). On macOS, to allow e.g. handling events while resizing, it provides the extraSDL_SetEventFilter
which handles events immediately in a callback.SDL3 provides the same API for backwards compat, but also allows a new design with
SDL_AppInit
/SDL_AppQuit
, see this documentation. This is literally just a poor mans static/global closure, i.e. effectively the same as what we're doing in this PR.Possible future work
Make using
surface_destroyed
andrecreate_surfaces
easier to use correctly.ApplicationHandler
between different type-states, similar to that described in Encode event lifecycle in the type-system #2903.FnMut
instead ofFnOnce
for the initialization closure. This depends on at what point in the lifecycle the surface creation callbacks are called, which I'm not really sure of.TODO
EventLoopExtRunOnDemand::run_app_on_demand
. I think it is?EventLoopExtWeb::spawn_app
?EventLoopExtPumpEvents::pump_app_events
. Should we just ignore the issue there?suspended()
andresumed()
events and patch up platform-specific documentation #3786