rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.76k stars 892 forks source link

Change `run_app(app: &mut A)` to `run_app(app: A)` #3721

Closed madsmtm closed 2 months ago

madsmtm commented 3 months ago

Since https://github.com/rust-windowing/winit/pull/3709, we do a blanket impl of ApplicationHandler for &mut references. This means that we can now change run_app_x methods to consume the application instead of taking a reference to it.

This allows the user more control over how they pass their application state to Winit.

Internally, each backend is free to convert the app to an &mut app (or &mut dyn ApplicationHandler) if that's easier for them to pass around.

kchibisov commented 3 months ago

The API is intentionally that way because we want to &mut dyn in the future, which I don't think we'll be able to otherwise without some kind of boxing of the entire state.

madsmtm commented 3 months ago

Hmm, I think you can still convert it internally to &mut dyn, if need be?

E.g. (playground):

trait ApplicationHandler {}

fn run_app<A: ApplicationHandler>(mut app: A) {
    run_app_inner(&mut app)
}

fn run_app_inner(app: &mut dyn ApplicationHandler) {
    // ...
}

But perhaps I'm missing something?

kchibisov commented 3 months ago

You can not really have generic on the trait, which you want to make &mut dyn.

What's the end goal with this?

madsmtm commented 3 months ago

You can not really have generic on the trait, which you want to make &mut dyn.

Hmm, from what I understood, it's the ActiveEventLoop that you want to use in dyn, right? I.e. use &mut dyn ActiveEventLoop in the methods on ApplicationHandler?

What's the end goal with this?

It's twofold:

  1. Moving Event::NewEvents(StartCause::Init) to an initialization step, something like:

    struct MyApp {
        window: Window, // Doesn't need to be `Option` any more
    }
    impl ApplicationHandler for MyApp { ... }
    
    let attrs = WindowAttributes::new();
    // Pass closure that runs once in `applicationDidFinishLaunching:` and initializes the application
    event_loop.run_app_with_init(|event_loop: &ActiveEventLoop| {
        let window = event_loop.create_window(attrs);
        MyApp { window }
    })?;
  2. Removing exiting, and using Drop for this instead. Normal cleanup that users expect to be able to do in Drop (e.g. close open files and flush buffers, ...) currently doesn't happen on iOS, you have to do it manually inside exiting.

(Not directly proposing either of those here, in particular the story around number 1 and Android isn't fleshed out, but that's at least how it ought to work on iOS (i.e. the current "drop window on Suspended" is wrong there)).

madsmtm commented 3 months ago

Resolution from meeting: We want to do this, though the path is a bit more unclear for pump events. I'll try to flesh out the PR some more.

Things noted in our meeting document:

  1. Box<dyn A> vs &mut dyn A.
  2. run_on_demand and pump_events should take and return the state instead of &mut, so it's consistent internally.
madsmtm commented 3 months ago

Responding to @kchibisov's concerns:

Box<dyn A> vs &mut dyn A.

Whether to use Box or a reference is an implementation detail for each backend. In the AppKit and UIKit implementations, I will probably end up using Box<dyn A> in the future, since it allows me to drop the application state when the event loop is terminating (which it otherwise won't be).

run_on_demand and pump_events should take and return the state instead of &mut, so it's consistent internally.

I changed these to have the same interface as run_app, and from a type-system perspective it seems to work fine?

For run_on_demand I definitely think it makes sense to allow passing in the application state, as the user might e.g. want to run an application twice, without sharing state between these runs.

I agree that usually the user would probably want to pass &mut app to pump_events, but that is not a strict requirement, and I think being generic here is desirable? But it's not a hill I want to die on, and I guess the compiler can guide the user better if we require &mut.