Move ControlFlow to EventLoopWindowTarget

[daxpedda]

This PR makes the following changes:

• Removes &mut ControlFlow from run() and adds a setter and getter to EventLoopWindowTarget instead.
• Exit was split off and now is exit() and is_exit() on EventLoopWindowTarget.
• User-defined exit codes were removed.

Fixes #3042.

[notgull]

Looks good from a brief look

[daxpedda]

@madsmtm could you check the iOS implementation?
I couldn't really figure out what to do with the state changes, maybe everything works as is, I don't really get what exactly is going on there 😅.

[madsmtm]

I don't really get what exactly is going on there

Honestly me neither, been meaning to rewrite it for a while now. I'll give it a short test drive, I suspect the situation is going to be somewhat improved (previously, there were certain events for which setting the control flow was ignored).

[madsmtm]

MacOS and iOS impl approved

[daxpedda]

I still need to go over the documentation again.
And we still need to figure out if we want getters. I'm in favor of at least adding a getter for exit. WDYT?

[dhardy]

Personally I like the previous model: it's possible to match control_flow { ... }, and it's clear what is going on.

Now: if I call elwt.set_poll() then elwt.set_wait_until(instant), does the latter override or does poll take priority?

I would at least like getters for poll, exit status (sufficient to adapt my current usage).

[daxpedda]

Personally I like the previous model: it's possible to match control_flow { ... }, and it's clear what is going on.

I think the Winit API in general is quite inconsistent with that, there are plenty of things that have setters but no getters. Events as well: some events can be queried, others can't.

I would argue that the API would be better if all setters have getters and I don't think it hurts to be able to query any event instead of having to record events in case you need to know what the last status was.

But that's a much bigger problem for another time.

Now: if I call elwt.set_poll() then elwt.set_wait_until(instant), does the latter override or does poll take priority?

Why would anything "take priority"? set_exit() is the only one that can't be overwritten which is documented appropriately.

[daxpedda]

Just some thought I wanna drop here:

The original motivation was to add platform-specific control flow variants. My thinking was to remove the enum in favor of setters to be able to add platform-specific methods through extension traits, which is the way Winit has continued to add platform-specific functionality until now.

I was quite frustrated with the fact that we removed the ControlFlow type, because I think it makes for a very clear API compared to any getters or setters we could possibly introduce: an enum clearly states that it can only be one of those variants, this way the code documents itself.

In addition, this is a problem not specific to ControlFlow, we have already added event variants that are platform-specific that don't make any sense to be there in a cross-platform API. The extension trait model we are currently using just doesn't work with enums.

If we want to continue using the extension trait model for platform-specific features, I came up with the following idea that we could use for enums:

enum Event {
    EventA,
    EventB(String),
    EventC {
        data: u16,
    },
    Platform(PlatformEvent),
}

impl PlatformEventExtWayland for PlatformEvent {
    fn event(&self) -> WaylandEvent {
        self.0
    }
}

// Not really needed, just a neat idea.
impl PlatformEventExtWindows for PlatformEvent {
    fn event(&self) -> ! {
        match self.0 { }
    }
}

An alternative of course could be to simply mark the enum #[non_exhausive] and add cfg-gated variants.

---

Personally I would be in favor of the first solution. We then could add a (set_)control_flow() method to EventLoopWindowTarget that returns/takes the ControlFlow type. Keeping the setters for convenience might still be a good idea. In any case, can be done in a follow-up PR.

WDYT?

[madsmtm]

I think this PR is still an improvement, I'd very much prefer to get rid of &mut ControlFlow, since it allows us more flexibility in how we store the information.

Whether the exact user-facing API uses elwt.[set_]control_flow(...) or distinct getter/setters is a little irrelevant to me, although I prefer the methods because of the easy extensibility (and slightly looser semantics, which is nice for platform implementers ;)) ).

I will say, I don't think Exit has a place in the enum, it's really a separate thing. If you want to have an enum, I think it should be something like enum NextEvent { Poll, Wait, WaitUntil(Instant) }.

[daxpedda]

Agreed, w/e further thoughts I'm dropping here, are just for follow-up.

I will say, I don't think Exit has a place in the enum, it's really a separate thing. If you want to have an enum, I think it should be something like enum NextEvent { Poll, Wait, WaitUntil(Instant) }.

I like that idea!

[rib]

I've never been a fan of Winit's &mut ControlFlow design due to how it only allows for one thing to implement timers based on WaitUntil, which in turn means you can't have portable timers unless a framework on top of winit provides timers, which I find far from ideal for a portable event loop abstraction. (I.e. right now it's not possible to write a utility crate that only depends on Winit and can schedule timers that won't conflict without how the application updates the control_flow state).

Timers are such a basic building block for applications so it's always seemed odd to me that Winit doesn't support them.

I'd love to see Winit provide a portable timer API which would replace the main use of ControlFlow (Poll is an special case of a zero timeout and Wait is a special case of an indefinite timeout because there are no timers), and then there could be some separate EventLoop/EventLoopWindowTarget API for requesting to exit the loop.

[kchibisov]

I'm not sure winit should do that directly though. Maybe in a separate lib, because you can clearly implement timers based on WaitUntil which alacritty does for example.

[dhardy]

Why would anything "take priority"? set_exit() is the only one that can't be overwritten which is documented appropriately.

This is an example of why control-flow is not a series of setters/getters: set_wait_until(..) and set_poll do not represent independent properties.

An alternative of course could be to simply mark the enum #[non_exhausive] and add cfg-gated variants.

Event should probably be #[non_exhaustive] anyway, just so that Winit v1.1 can add new events.

On the whole I am not a fan of the platform-extension-trait model. For an app targetting only one platform the model is fine, but that's mostly not why you'd use Winit anyway. A cross-platform API should require as little platform-specific code to use as possible. If, say, Event::PinchGesture is only generated on MacOS but exists everywhere, then removing unused handlers should be an optimisation problem only (i.e. the app should be able to just write a cross-platform handler for Event::PinchGesture). Adding #[cfg(macos_platform)] to this isn't a big deal if we must have a minimal API on each platform but is not necessary. (Further motivation for this is that if, later, PinchGesture were supported on Wayland, the only change needed for an app to enable it there too would be a one-line cfg, and only then if a cfg was used.)

[rib]

I'm not sure winit should do that directly though. Maybe in a separate lib, because you can clearly implement timers based on WaitUntil which alacritty does for example.

To my mind it really needs to be Winit that supports timers otherwise they aren't portable.

Even if you had a library that supported timers based on something like control_flow nothing is able to assume that that library is usable because nothing guarantees that every app based on Winit will use that common library.

Portability for timers is only really practical / usable when they are supported by Winit directly.

Handled in external libraries just means every app / framework has to come up with their own timer abstraction and although they could potentially use a utility crate for that, that in itself isn't really that beneficial (portability is more useful here imho than being able to share a small amount of utility code).

[notgull]

I'd like to point out that async-winit has a Timer primitive built on winit's set_wait_until functionality.

I like the idea of having a ControlFlow enum still, but just having it be get/set on the ELWT.

[dhardy]

@rib the issue I see there is that a "portable timer" is only really useful when a Wake event can be delivered to the thing requesting being woken. If multiple pieces of code interact with an event-loop directly then how does the main event handler know what to do with ResumeTimeReached? (Okay, attaching a Waker works when using an async scheduler for everything.)

The way I've handled it in Kas is by storing (roughly) a Vec<(Instant, Id)>, sorted by time, then sending Event::TimerUpdate to the accompanying Id.

[rib]

because you can clearly implement timers based on WaitUntil which alacritty does for example

yeah, although it's possible to write timers on top of a single, global timer, those timers are then only meaningful for Alacritty and it's not possible to write shared code for Winit applications that can utilize timers.

I find this odd because implementing timers is definitely something that an event loop needs to be involved with but from an application development pov you generally want support for orthogonal timers and Winit is currently doing the absolute bare minimum to expose the timeout for polling.

E.g. GLib mainloops have orthogonal timers as do libuv loops as does SDL...

It'd be nicer imho if middleware crates / shared code could just assume that any Winit based app can register timers, regardless of whether they are used in Alacritty or Bevy or Egui etc.

In this context I think having timers would essentially remove the need for ControlFlow, except for needing an ELWT API to request an exit().

[rib]

@rib the issue I see there is that a "portable timer" is only really useful when a Wake event can be delivered to the thing requesting being woken.

If Winit supported orthogonal timers then there would be some kind of handle for a timer that you create and you'd get an event like TimerElapsed(timer_handle) that tells you when a specific timer handle has elapsed its time.

[daxpedda]

This is ready now. I'm gonna update OP in a moment.

There was a bunch of this code in various backends:

match ... {
    ...
    ControlFlow::ExitWithCode(_) => unreachable!(),
}

I just removed them because I assume that they were there to exhaust ControlFlow variants, should I add an assert!() or something instead?

[dhardy]

Looks good other than the nits

[kchibisov]

The function changed, however it doesn't explain what it wants to do.

[daxpedda]

Did it change? Or do you just mean the documentation has to be adjusted to not say "set"?

[daxpedda]

I adjusted the documentation a bit.

[kchibisov]

The issue here is that we don't actually send anything, we basically say that we want to exit event loop.

[dhardy]

So terminate and is_terminating are better names?

[daxpedda]

I assume the problem here was the documentation, which I adjusted.

[kchibisov]

We have an event called LoopExiting, not LoopTerminating, so using the word terminate will be inconsistent with the event names.