Add `ApplicationHandler` trait

[madsmtm]

Part of https://github.com/rust-windowing/winit/issues/2903.

Builds upon https://github.com/rust-windowing/winit/pull/3056, so see the last commit(s) for the changes in this PR.

Add an ApplicationHandler<T> trait that replaces the Event<T> enum (should probably be kept around for at least one release, but be marked deprecated) by encoding these events as callbacks on a user-specified mutable application object instead. This allows us to fix many outstanding issues with the current event loop design, including ensuring that windows are only created after initialization, and helping users to properly suspended/resume their application (also here: recreating surfaces and such).

TODO:

• [ ] Figure out what to do about timing.
• [x] Experiment with returning a closure on suspend, instead of having to manually specify the suspended data.
  • https://github.com/rust-windowing/winit/pull/3074
• [ ] Should we do application initialization differently?
• [ ] How exactly should pump_events work?
• [ ] The user currently has to type a lot compared to the old closure, can we make things even nicer?
• [ ] How do we do platform-specific extensions?
• [ ] Naming things.
• [ ] Test on all platforms that the events are actually delivered in the order we want.
• [ ] Changelog entry and update examples.
• [ ] ...

[daxpedda]

Sorry about the delay in #3056, will try to finish it up today.

[notgull]

Now that I see it in real life, I'm not that sure about this one. It's still possible for users to just keep the Window around regardless of what the type system says. So it's a handful of extra complexity that may not provide strong enough guarantees.

[madsmtm]

For my part, this PR is mostly about ensuring that the application is initialized before you try to create windows, which solves sooo many issues on macOS and iOS, and unblocks a lot of my other work (it's really hard to make sure that e.g. fullscreen-ing works correctly when you basically have to work with the window in two states). This would also be possible with other solutions, e.g. nested closures in EventLoop::run, but that would still be ugly.

It's still possible for users to just keep the Window around

True, but there are ways to fix that; one approach could be, drum roll, you guessed it, lifetimes!

Queue the most general example I could conjure:

mod winit {
    pub struct Window<'running> { ... }

    impl<'running> Window<'running> {
        pub fn new(elwt: &'running EventLoopWindowTarget) -> Self { ... }
    }

    pub trait ApplicationHandler<'suspended: 'running, 'running, T = ()> {
        type Suspended: 'suspended;

        // Note that binding the elwt here to `'running` is non-trivial, because then we
        // have to allow it to escape. But should be doable, in one form or another.
        fn resume(suspended: Self::Suspended, elwt: &'running EventLoopWindowTarget) -> Self;

        fn suspend(self) -> Self::Suspended;
    }
}

struct State<'suspended> { ... }

struct App<'suspended, 'running> {
    window: Window<'running>,
    state: State<'suspended>,
}

impl<'suspended: 'running, 'running> ApplicationHandler<'suspended, 'running> for App<'suspended, 'running> {
    type Suspended = State<'suspended>;

    fn resume(
        state: Self::Suspended,
        elwt: &'running EventLoopWindowTarget,
    ) -> Self {
        Self {
            window: Window::new(elwt),
            state,
        }
    }

    fn suspend(self) -> Self::Suspended {
        self.state
    }
}

But whether or not we decide to do that, I still think moving to a trait like this is the way forwards.

(Again, the above is still possible using closures, but it becomes a nested mess. One closure is nice, three nested closures is quite enough thank you very much).

[dhardy]

The trait in the PR looks usable, but I think I'd just set type Suspended = Self and continue using this:

struct WindowData<S: WindowSurface> {
    window: winit::window::Window,
    #[cfg(all(wayland_platform, feature = "clipboard"))]
    wayland_clipboard: Option<smithay_clipboard::Clipboard>,
    surface: S,
    // ...
}

/// Per-window data
pub struct Window<A: AppData, S: WindowSurface> {
    widget: kas::Window<A>,
    ev_state: EventState,
    // ...
    window: Option<WindowData<S>>,
}

For any app with multiple windows this strategy probably makes more sense than using a separate Suspended type.

Regarding the lifetimes idea, I don't think it enforces anything (unless you are deliberately not passing elwt into other methods, thus forcing users to store a copy in their app state). Regardless, it looks more painful than useful, so please don't.

[madsmtm]

The trait in the PR looks usable, but I think I'd just set type Suspended = Self and continue using this:

struct WindowData<S: WindowSurface> {
    window: winit::window::Window,
    #[cfg(all(wayland_platform, feature = "clipboard"))]
    wayland_clipboard: Option<smithay_clipboard::Clipboard>,
    surface: S,
    // ...
}

/// Per-window data
pub struct Window<A: AppData, S: WindowSurface> {
    widget: kas::Window<A>,
    ev_state: EventState,
    // ...
    window: Option<WindowData<S>>,
}

For any app with multiple windows this strategy probably makes more sense than using a separate Suspended type.

The idea was that winit would allow you to know statically that a window has graphics attached; e.g. for your case, it'd look something like this (and you'd avoid the Option around the window data):

struct WindowData<S: WindowSurface{
    window: winit::window::Window,
    #[cfg(all(wayland_platform, feature = "clipboard"))]
    wayland_clipboard: Option<smithay_clipboard::Clipboard>,
    surface: S,
    // ...
}

pub struct Window<A: AppData, S: WindowSurface{
    widget: kas::Window<A>,
    ev_state: EventState,
    // ...
}

struct App {
    windows: Vec<(Window<A, S>, WindowData<S>)>,
}

struct Suspended {
    windows: Vec<Window<A, S>>,
}

Regarding the lifetimes idea, I don't think it enforces anything (unless you are deliberately not passing elwt into other methods, thus forcing users to store a copy in their app state). Regardless, it looks more painful than useful, so please don't.

No I definitely agree, we'd want something that was actually understandable and usable, the idea I sent was just to prove the concept (if you replace the Suspended type with App<'suspended, 'running>;, and fix the suspend/resume methods, it does fail to compile, because the Suspended type must be longer than 'suspended, which in turn must be longer than 'running). I've edited the comment to make that slightly more clear.

---

But again, I'm definitely on the pragmatic side here, fixing the Suspend/Resume stuff is not that important to me atm, but there are things this trait would be more useful for moving forwards. So I'm fine with ripping it out, at least in the first iteration.

[dhardy]

(if you replace the Suspended type with App<'suspended, 'running>;, and fix the suspend/resume methods, it does fail to compile, because the Suspended type must be longer than 'suspended, which in turn must be longer than 'running).

This works with your App, but it's still possible to get around the restriction by using only a single lifetime: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3d8728a864a3f9f119d77388592d2473

[madsmtm]

it's still possible to get around the restriction by using only a single lifetime

Oh yeah... I suspect there's probably still a way to do it, but let's handle that when we get there.