Drop application handler on run loop exit

[madsmtm]

Part of #2903, split out from #3895. This is the motivation for doing #3721.

Calling the Drop impl of the user's ApplicationHandler is important on macOS, iOS and Web, since they don't necessarily return from EventLoop::run_app. This PR fixes that.

And now that we reliably call Drop, the ApplicationHandler::exited event/callback is unnecessary; using Drop composes much better (open files etc. stored in the app state will be automatically flushed), and prevents weirdness like attempting to create a new window while exiting. So this PR also removes that method.

• Tested on all platforms changed
  • macOS
  • iOS
  • Web
  • Wayland/X11/Windows/Android are trivial
• Added an entry to the changelog module if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality

[kchibisov]

I'm in favor of doing so, however there's a case for rnn_app_on_demand and pump_app_events API.

What do you think about keeping the exiting callback, so users have a special place to run maybe some fail-able logic and e.g. clean-up things in case of on_demand/pump_event, but also guarantee to call drop on the State when we consume user state?

Also, there're cases where the user doesn't really want to drop their entire state in case of e.g. run_on_demand, but they may want to drop only part of it.

To sum up, for run_app the drop is fine, probably. For run_on_demand and pump_app_events they might need an exiting callback, which I think, is fine to keep anyway, just for compat sake.

[madsmtm]

For run_app_on_demand and pump_app_events, the user is free to pass &mut app; that way, their state won't get dropped. For both of these, they'll be able to "detect" the exiting of the event loop by simply running code after:

let mut app = App::default();
event_loop.run_app_on_demand(&mut app)?;
// exited()
event_loop.run_app_on_demand(&mut app)?;
// exited()

loop {
    let status = event_loop.pump_app_events(Some(Duration::ZERO), &mut app);

    if let PumpStatus::Exit(exit_code) = status {
        // exited()
        break ExitCode::from(exit_code as u8);
    }

    sleep(Duration::from_millis(16));
}

[kchibisov]

I've also thought about this, however run_app_on_demand implies that user should clear the state related to windows before the loop returns back to the user. It could be done with other events though, but I generally don't see an issue with keeping the event. Though, it could be a general run_app_on_demand issue that it doesn't ensure that your window will be closed, or that it closes all windows, etc, etc.

For pump_events it'll likely fine though, the api a bit sketchy.

[madsmtm]

I've also thought about this, however run_app_on_demand implies that user should clear the state related to windows before the loop returns back to the user.

Hmm, is that actually required by any backends? We don't drop the window in the run_on_demand.rs example.

If so, this could still be handled by the user with:

struct PartialApp<'app> {
    app: &'app App,
    windows: Vec<Box<dyn Window>>,
}

let mut app = App::default();
let partial_app = PartialApp { persistent: &mut app, windows: vec![] };
event_loop.run_app_on_demand(partial_app)?;

let partial_app = PartialApp { persistent: &mut app, windows: vec![] };
event_loop.run_app_on_demand(partial_app)?;

Of course, that's more verbose, but IMO also clearer what happens state-wise.

It could be done with other events though

Indeed, window cleanup should generally be done in WindowEvent::CloseRequested/WindowEvent::Destroyed. I have a larger issue about this in draft (we need WindowEvent::Created too), but of course that's all smokes and mirrors until I get around to posting it ;).

[madsmtm]

Responding to #4146 (comment) (posted in the wrong place).

Hmm, is that actually required by any backends? We don't drop the window in the run_on_demand.rs example.

On macOS it simply doesn't really work if you use run_app_on_demand and don't wait for all windows to close(macOS tells you that your app is stuck). In general, we kind of say that everything should be dropped in the docs.

See #4135

I suspect that's just a bug of the current implementation (run_app_on_demand is kinda hackily written NVMD, I think it's fine, it's the pump_app_events impl that I have a problem with), but I guess we can wait until that's resolved.

[madsmtm]

Looked in to #4135 now, and we can indeed do the cleanup correctly there, by force-closing windows before exiting (which will trigger WindowEvent::Destroyed and give the user extra time to respond if they need it).

[madsmtm]

Notes from meeting: It may have issues with the display needing to be shut down between run_app_on_demand, but it should be fine with well-behaved display servers. So probably fine to move forwards with this.

[Legend-Master]

Hi, I was about to upstream the change made in tauri-apps/tao#1126 but realized that the ApplicationHandler::exited callback no longer exits, if I understand this correctly, the users are expected to run their clean ups in Drop right? But in the case of WM_ENDSESSION (emitted on shutdown and by Restart Manager usually called from installers), Windows will terminate us after we return from it, so Drop will not be called in that case
Is there any recommended approach for that? Or we just don't support this use case (we don't support this on any platforms yet I think?) and leave that to the user (manually through with_msg_hook)

[kchibisov]

Can windows backend try to drop, etc things somehow from that handler? I think it should be, no?

[Legend-Master]

I'll give that a try

[Legend-Master]

After looking into it, I don't think it's possible to do with the current structure, the ApplicationHandler's ownership is held inside EventLoop::run_app and could even be held by the user if they're passing in &mut app or if they're using run_on_demand or pump_app_events

And in the system shutdown case, we can't really return to caller after receiving WM_ENDSESSION unless we tell Windows that we want to block the shutdown

[madsmtm]

After looking into it, I don't think it's possible to do with the current structure, the ApplicationHandler's ownership is held inside EventLoop::run_app and could even be held by the user if they're passing in &mut app or if they're using run_on_demand or pump_app_events

That's similar to the other platforms. Maybe you can just call self.runner.loop_destroyed(), and then maybe change move_state_to to call self.event_handler.take() when getting the Destroyed event?

[Legend-Master]

I'm not sure I get it, EventLoopRunner currently doesn't own the ApplicationHandler, how could we drop it?
Do you mean we something like changing the set_app function to take mut app: impl ApplicationHandler (instead of app: &'app mut (dyn ApplicationHandler + 'app)) so we could own it if the user passed us the ownership (e.g. event_loop.run_app(app) not event_loop.run_app(&mut app))?

[madsmtm]

I'm not sure I get it, EventLoopRunner currently doesn't own the ApplicationHandler, how could we drop it?

It contains event_handler: Rc<EventHandler>, and EventHandler = Cell<Option<&'static mut (dyn ApplicationHandler + 'static)>>. So it can drop it using interior mutability.

[madsmtm]

Wait, I see what you mean. Yeah, we'll need to change set_app as well to take Box<dyn ApplicationHandler>.