winit icon indicating copy to clipboard operation
winit copied to clipboard

Add initialization closure

Open madsmtm opened this issue 1 year ago • 10 comments

Summary

Allow windows to be created before the user's application state, avoiding Option around these in user code.

This is paired with using the Drop impl of ApplicationHandler instead of exited to communicate that the event loop is done (composes much better).

See this example for how things now look:

struct App {
    window: Box<dyn Window>,
    surface: Option<wgpu::Surface<...>>,
    in_flight_network_requests: Vec<...>,
}

impl Drop for App {
    fn drop(&mut self) {
        self.in_flight_network_requests.cancel();
        // And other such cleanup logic previously done in `exited`
    }
}

impl ApplicationHandler for App {
    fn can_create_surfaces(&mut self, _event_loop: &dyn ActiveEventLoop) {
        self.surface = Some(...);
    }

    // ...
}

fn main() -> Result<()> {
    let event_loop = EventLoop::new()?;
    event_loop.run(|event_loop| {
        // We have access to `&dyn ActiveEventLoop` here, and can initialize windows.
        // We _could_ potentially also initialize the surface here, if we didn't want to support Android.
        let window = event_loop.create_window(...).unwrap();
        // Users are expected to return their now initialized `ApplicationHandler` here.
        App { window, surface: None, network_connections: Vec::new() }
    })?;
    Ok(())
}

Motivation

One of the most common problems that users encounter in v0.30 is that their window (and surface) needs to be wrapped in an Option, because they can no longer initialize windows at the start of main, and the method they're expected to initialize it in (resumed in v0.30, can_create_surfaces on master) isn't called before the application state is already created.

See https://github.com/rust-windowing/winit/pull/3447 for the original change, it tackles many real bugs, such as:

  • The window being in a weird partially initialized state when created before EventLoop::run on macOS and iOS.
  • create_window being called in a non-sync fashion on Wayland.
  • Surfaces can be destroyed and must be recreated on Android.

We were aware that the solution was overly restrictive (as also noted in https://github.com/rust-windowing/winit/issues/3626 and the changelog), and provided the deprecated EventLoop::create_window to ease the transition. This API has since been removed in https://github.com/rust-windowing/winit/pull/3826 to prepare for v0.30, but users are still left with no real solution to the ergonomic issue!

Furthermore, the current design is also pushing the user towards re-creating Window, but that is incorrect, it really is only the surface that needs to be re-created on Android - @MarijnS95 stated it well:

Anything that moves Window lifetime closer to Surface lifetime is a step backwards.

Proposed solution

We add a closure to the initial EventLoop::run call (keeping EventLoop::run_app as deprecated fallback), which allows access to ActiveEventLoop 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 NSApplicationDidFinishLaunchingNotification/UIApplicationDidFinishLaunchingNotification.

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 https://github.com/rust-windowing/winit/pull/3721, and resolves my primary concerns from https://github.com/rust-windowing/winit/issues/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 extra SDL_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 can_create_surfaces and surface_destroyed easier to use correctly.

  • One solution would be to transition the ApplicationHandler between different type-states, similar to that described in https://github.com/rust-windowing/winit/issues/2903.
  • Crazy idea for this would be to use FnMut instead of FnOnce 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.
  • Previous iteration of this PR tried to merge the initialization closure and can_create_surfaces, see the comment history for details. There might still be a space to explore there.

TODO

  • [ ] Discuss whether we want to go this direction.
    • I especially want input from @MarijnS95 on whether he thinks this is a step forwards or backwards?
  • [x] Update example to help users understand how to use this correctly.
  • [ ] Write proper documentation and update changelog entry.
  • [ ] Figure out if this is also desired for EventLoopExtRunOnDemand::run_app_on_demand. I think it is?
  • [ ] Figure out if this is also desired for EventLoopExtWeb::spawn_app?
  • [ ] Figure out if this is also desired for EventLoopExtPumpEvents::pump_app_events. Should we just ignore the issue there?
  • [ ] Do we still want [StartCause::Init]?
  • [ ] Implement on all platforms:
    • [x] Android
    • [ ] iOS/UIKit
    • [ ] macOS/AppKit
    • [x] Orbital
    • [x] Wayland
    • [ ] Web
    • [ ] Windows
    • [x] X11
  • [ ] Test on all platforms (other maintainers, feel free to edit this if you have done so):
    • [ ] Android
    • [ ] iOS/UIKit
    • [ ] macOS/AppKit
    • [ ] Orbital
    • [ ] Wayland
    • [ ] Web
    • [ ] Windows
    • [ ] X11

madsmtm avatar Sep 01 '24 12:09 madsmtm

I especially want input from @MarijnS95 on whether he thinks this is a step forwards or backwards?

Specifically replying to:

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?).

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

MarijnS95 avatar Sep 01 '24 21:09 MarijnS95

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.


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.

Well, perhaps we will in the future tell the user to initialize their surfaces in WindowEvent::Created, or with async or something, I don't want to get derailed with that here, but the user still has to trigger the window creation itself, and that still has to be done inside the event loop.

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.

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.

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.

Not sure what you mean here, the user would implement ApplicationHandler for several newtypes, and we'd somehow switch between them?

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.

Sure, and that's also what the window example now showcases (which it didn't before).

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

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?

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

But... The programmer would still be in control of which ones are created, right? And would still be in control of when they close?

madsmtm avatar Sep 01 '24 22:09 madsmtm

Anything that moves Window lifetime closer to Surface lifetime is a step backwards. On Android, we definitely have an analogy of Windows (Activitys) which persist much longer than the Surface (buffer producer) backing their visual output.

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?

I'm not sure how that could work, unless we push on with an idea where raw-window-handle deals with surface lifetime events internally and pretends there's always an Optional user-initialized Surface held by it. Then we have a 1:1 relation between Window and Surface again? (and this might help with arbitrary "non-`Window subsurfaces" which could also have lifetime callbacks on Android).

Specifically on Android this would hide the fact that the users' Surface disappears (and requires a surface/swapchain recreate on the graphics side) every time a user navigates away from the app, turns off the screen, etc.

Note that the Android system creates these Activitys "asynchronously" for you somewhat like Wayland (except that it doesn't need to be an async reply to a user request, but is in most cases - such as initial startup - from another component in the system).

But... The programmer would still be in control of which ones are created, right? And would still be in control of when they close?

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 <intent-filter> that allows my Activity to start with a "data URL to files with picture MIME types" (or something like that), the Android system will start (another instance of, or in parallel to other Activitys already running within our Rust Application/process) our Rust Activity with the specified data (stored inside an Intent) when I wish to share via the Rust app. And there are numerous other (deep linking) examples where external factors (re)start (or launch "within" an already-running) an Activity.

We could always call .finish() to close it though.

MarijnS95 avatar Sep 11 '24 09:09 MarijnS95

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:

  • it is now easy to forward errors out of the event loop
  • some transitions that should never occur can be removed
  • resume and suspend transitions are automatically deduplicated
  • the library consumer can take self by value to implement an application specific sub state machine

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 handle method needs to be flattened into separate methods but I haven't for brevity):


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 Application<T> to winit::application::ApplicationHandler<T>. winit does not need to change for people to program against this interface. The complete code can be found at mickvangelderen/winit-ext. But if winit is considering modifying the interface anyway, I thought I should chip in.

mickvangelderen avatar Oct 04 '24 16:10 mickvangelderen

Thanks for the input @MarijnS95! You're right that I was mistaken in my original solution, and I've changed the approach in the PR (only add initialization closure, don't touch can_create_surfaces), see the updated PR description. I've hidden the now outdated comments for clarity. The same goes for @kchibisov's comments, since I felt that I've resolved them with https://github.com/rust-windowing/winit/pull/3895#issuecomment-2323516705, and because I've re-done the PR, but feel free to re-open / continue the discussion of it.

@mickvangelderen: I've linked to your comment in https://github.com/rust-windowing/winit/issues/2903#issuecomment-2514308561, but I think it's out of scope for this PR, so I've hidden the comment.

madsmtm avatar Dec 03 '24 11:12 madsmtm

In general I'd prefer exploring type state patterns during runtime then just having a closure, since it won't really solve e.g. android stuff or anything that has an option to remove windows and keep running.

kchibisov avatar Dec 03 '24 12:12 kchibisov

In general I'd prefer exploring type state patterns during runtime then just having a closure

What do you mean by this? Something like https://github.com/rust-windowing/winit/pull/3710?

it won't really solve e.g. android stuff or anything that has an option to remove windows and keep running.

Indeed, this won't solve Android stuff. And for a lot of "real world" applications, you'll want multiple windows, and then the point is a bit moot anyways, true.

But a lot of uses of Winit are not full-fledged applications, and there it does make sense to allow more easily creating windows.

madsmtm avatar Dec 03 '24 12:12 madsmtm

impl ApplicationHandler for Inactive {}
impl ApplicationHandler for Active {}

and a way to switch between them during a runtime with something on the event loop.

kchibisov avatar Dec 03 '24 13:12 kchibisov

impl ApplicationHandler for Inactive {}
impl ApplicationHandler for Active {}

and a way to switch between them during a runtime with something on the event loop.

Interesting, could you give more details on how this would work? How do you imagine you would switch between these? And how does it differ from the ideas in https://github.com/rust-windowing/winit/issues/2903?

In any case, I feel like that perhaps relates more to the Android surface stuff, not so much this specific PR?

madsmtm avatar Dec 03 '24 13:12 madsmtm

I'd assume that you store all of them in the big struct and then borrow a field, then you'd have a way to get back to your big wrapper. The problem is that it's self referencial, so requires a bit of unsafe.

struct Foo {
     state1: Option<State1>,
     state2: Option<State2>,
}

EventLoop {
     active: &mut dyn AppHandler, // either state1 or state2.
     original_state: Foo // You just store it for reference.
}

kchibisov avatar Dec 03 '24 13:12 kchibisov