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

Encode event lifecycle in the type-system #2903

Open
madsmtm opened this issue Jun 24, 2023 · 10 comments
Open

Encode event lifecycle in the type-system #2903

madsmtm opened this issue Jun 24, 2023 · 10 comments
Labels
B - bug Dang, that shouldn't have happened C - needs discussion Direction must be ironed out I - BREAKING CHANGE S - api Design and usability

Comments

@madsmtm
Copy link
Member

madsmtm commented Jun 24, 2023

We have a lot of invariants in how our events are delivered, but these are only vaguely documented. Examples of things that commonly go wrong:

  • Initializing windows outside NewEvents(Init)/Resumed
  • Not suspending / resetting the draw buffers on Suspended

To remedy this situation, I propose we change the run method(s) to use a trait object instead of just a closure.

Initial draft for what typical usage will look like (this will need a lot of tweaking, especially with regards to ControlFlow, and possibly we should consider a SimpleApplication trait that handles some of this for you):

struct MyApp {
    window: Window,
}

struct SuspendedApp {
    // ...
}

impl ApplicationHandler for MyApp {
    type SuspendedState = SuspendedApp;

    fn init(event_loop: &EventLoopWindowTarget) -> Self {
        let window = WindowBuilder::new().build(event_loop);
        Self { window }
    }

    fn suspend(self) -> Self::SuspendedState {
        todo!()
    }

    fn resume(state: Self::SuspendedState, event_loop: &EventLoopWindowTarget) -> Self {
        todo!()
    }

    fn on_event(
        &mut self,
        event: Event<'_>,
        event_loop: &EvenLoopWindowTarget,
        control_flow: &mut ControlFlow,
    ) {
         // The usual event handling in here
    }
}

impl Drop for MyApp { ... } // LoopDestroyed

fn main() {
    let event_loop = EventLoop::new();
    event_loop.run::<MyApp>();
}

The closure we have now is really nice though, we loose the easiness offered by that, but this is strictly more correct, which I think is important!

Again, this will need a lot of tweaking, opening as an issue first to gather opinions.

Relates to #2900, #2010, and many, many more.

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened S - api Design and usability C - needs discussion Direction must be ironed out I - BREAKING CHANGE labels Jun 24, 2023
@notgull
Copy link
Member

notgull commented Jun 24, 2023

I generally like this idea, especially since the ApplicationHandler trait could probably be implemented on top of a closure to provide compatibility.

@kchibisov
Copy link
Member

@madsmtm so you do you want more or less the API Wayland backend has internally, you can look at state and e.g pointer handlers to get a grasp on how it works? Since what you suggest is exactly how the API behaves on Wayland, it could simplify stuff for the wayland backend for sure, for example, but it could be hard for other backends like Windows and X11 where they have just one closure to handle all of that.

@kchibisov
Copy link
Member

cc @rib since it sort of interacts with the way we do event loop APIs and I'd like to hear what you think about all of that.

@madsmtm
Copy link
Member Author

madsmtm commented Jun 24, 2023

@madsmtm so you do you want more or less the API Wayland backend has internally, you can look at state and e.g pointer handlers to get a grasp on how it works?

Hmm, from a quick look, yeah, pretty closely.

it could be hard for other backends like Windows and X11 where they have just one closure to handle all of that.

Yeah, internally the backends would need some kind of enum with the different states that the application can be in, something like (possibly a shared definition, possibly platform-specific):

enum State<T: ApplicationHandler> {
    #[default]
    Uninitialized,
    Suspended(T::SuspendedState),
    Running(T),
    Dropped,
};

(Possibly using Box<dyn T> instead, with some magic for associated types).

@notgull notgull mentioned this issue Jun 25, 2023
17 tasks
@rib
Copy link
Contributor

rib commented Jun 26, 2023

As a first impression - yeah implementing a trait on your App state sounds like it could work pretty nicely - though the devil will probably be in the details.

Previously I think when the idea of splitting up the event callback has come up then I think the challenge with ergonomics has been with the way that you would currently capture your app state in the event closure and it becomes awkward to share app state across multiple closures.

I think probably a trait on the user's app state addresses that concern.

cc @rib since it sort of interacts with the way we do event loop APIs and I'd like to hear what you think about all of that.

I think it could hopefully be more-or-less orthogonal because this is about how you organize the app code that responds to events and that should be able to work consistently, regardless of exactly which way the event loop gets run.

E.g. you could conceivably call event_loop.run_ondemand::<MyApp>(); or you could define an external loop that could do something like:

    let mut event_loop = EventLoop::new();
    let mut app = App::default();

    'main: loop {
        let status = event_loop.pump_events(&app);
        if let PumpStatus::Exit(exit_code) = status {
            break 'main ExitCode::from(exit_code as u8);
        }

        println!("External Update()");
        app.update();
    }

To allow for pump_events() to be able to drive event processing for the app incrementally we'd maybe also need to lets apps create their App struct themselves and window could be an Option<Window>.

My initial impression is that this trait based approach would mainly just let us modularize the monolithic event closure and it would be easier to expose events that require the application to return values to Winit.

Good first candidates to split out from the general event callback could be any events that may need to be synchronized with the windowing system - such as suspend/resume already in the example above, but probably also render / draw.

I might not have fully followed the idea with the enum for application states and I'm not quite sure what the ideas are with SuspendedState. I think I'd expect the App type would exist for the full lifetime of the app (across suspend/resume) and in suspend I would just want to drop rendering surfaces, which can be recreated in resume

It could be good to think about extensibility of this approach though. E.g. what options do we have to apps to implement extension traits that maybe just certain backends deal with.

It's not the focus here but I'd really like to replace / remove ControlFlow from Winit's design. We could have an ELWT::quit() API perhaps and it would be good if Winit supported a timers API that could be used by orthogonal components instead of just allowing one thing to specify a timeout.

@madsmtm
Copy link
Member Author

madsmtm commented Jun 27, 2023

more-or-less orthogonal because this is about how you organize the app code that responds to events and that should be able to work consistently, regardless of exactly which way the event loop gets run.

Agreed.

My initial impression is that this trait based approach would mainly just let us modularize the monolithic event closure and it would be easier to expose events that require the application to return values to Winit.

Good first candidates to split out from the general event callback could be any events that may need to be synchronized with the windowing system - such as suspend/resume already in the example above, but probably also render / draw.

My evil scheme is to do something similar for window events (esp. drawing) later on (have a WindowHandler trait) - but I believe the proposal is still good by itself.

I might not have fully followed the idea with the enum for application states and I'm not quite sure what the ideas are with SuspendedState. I think I'd expect the App type would exist for the full lifetime of the app (across suspend/resume) and in suspend I would just want to drop rendering surfaces, which can be recreated in resume

The idea was that you'd be able to statically tell that "the render surfaces are currently valid", instead of relying on an Option for doing that.

In the example I gave, the suspended state and the running state would be the same, since the window itself would persist.

It could be good to think about extensibility of this approach though. E.g. what options do we have to apps to implement extension traits that maybe just certain backends deal with.

Good point!

Let me enumerate a few options:

  1. Put cfgs on the trait methods. Easy for the user to make a mistake and not put the cfgs on themselves, which we've tried hard in the past to avoid.
  2. Platform-specific traits + platform-specific run methods that require that trait.
trait ApplicationHandlerMacOS: ApplicationHandler {
  fn method();
}

trait EventLoopExtMacOS {
  fn run_with_platform<T: ApplicationHandlerMacOS>(...);
}

It's not the focus here but I'd really like to replace / remove ControlFlow from Winit's design. We could have an ELWT::quit() API perhaps and it would be good if Winit supported a timers API that could be used by orthogonal components instead of just allowing one thing to specify a timeout.

Agreed, and it would vastly simplify the amount of code users have to write under this proposal.

@madsmtm
Copy link
Member Author

madsmtm commented Aug 30, 2023

I've put up a PR with an initial implementation here: #3073

@madsmtm
Copy link
Member Author

madsmtm commented Jan 12, 2024

Blocked on #3432, which is part of the work to move to a handler trait.

this trait based approach would mainly just let us modularize the monolithic event closure and it would be easier to expose events that require the application to return values to Winit.

Re-reading, this is actually pretty much on-spot for what we're doing now half a year later.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 3, 2024

A similar proposal was also posted here.

@mickvangelderen
Copy link

I just want to note that providing an API like this can be done through a separate crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened C - needs discussion Direction must be ironed out I - BREAKING CHANGE S - api Design and usability
Development

No branches or pull requests

5 participants