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

Move ControlFlow as methods on EventLoopWindowTarget #3042

Closed
daxpedda opened this issue Aug 24, 2023 · 7 comments · Fixed by #3056
Closed

Move ControlFlow as methods on EventLoopWindowTarget #3042

daxpedda opened this issue Aug 24, 2023 · 7 comments · Fixed by #3056
Labels
C - in progress Implementation is proceeding smoothly

Comments

@daxpedda
Copy link
Member

daxpedda commented Aug 24, 2023

On Web there are currently a bunch of ways to implement ControlFlow::Poll, they each have different tradeoffs. Unfortunately there is currently no way to add platform-specific functionality to ControlFlow the way usually Winit does: with extension traits.

My proposal to solve this problem is to make ControlFlow a struct, internally it would probably still use an enum, but the point is to make that private so we can add platform-specific functionality through extension traits. It already provides methods to set the ControlFlow.

Happy to do the PR for all platforms.

EDIT: See #3042 (comment), which is an even better solution imo.

@daxpedda daxpedda added the C - needs discussion Direction must be ironed out label Aug 24, 2023
@madsmtm
Copy link
Member

madsmtm commented Aug 24, 2023

In favour, though I actually think we should be removing it, and make its functionality available on the event loop that's passed to the closure instead.

@daxpedda
Copy link
Member Author

Even better! Let's do that.
@kchibisov's would appreciate your input on this as well.

@daxpedda daxpedda changed the title Make ControlFlow a struct Move ControlFlow as methods on EventLoopWindowTarget Aug 24, 2023
@madsmtm
Copy link
Member

madsmtm commented Aug 24, 2023

I think to do it properly though, we need to change EventLoopWindowTarget. I suggest we do something like the below.

impl EventLoop<T> {
    fn run(self, event_handler: impl FnMut(Event<T>, RunningEventLoop<'a, T>)) -> Result<(), Error>;
}

impl RunningEventLoop<'a, T> {
    fn set_wait(&mut self);
    fn set_poll(&mut self);
    fn set_exit(&mut self, code: ...);
    // ... Other `ControlFlow` methods
}

// Stuff that can be done both before and after the application starts
//
// Should ideally be #[inherent], but that's not possible yet.
trait EventLoopTraitBikeShed: Sealed {
    pub fn available_monitors(&self) -> ...;
    pub fn primary_monitor(&self) -> ...;
    pub fn listen_device_events(&self, ...) -> ...;
}

impl EventLoopTraitBikeShed for EventLoop<T> {}
impl EventLoopTraitBikeShed for RunningEventLoop<'a, T> {}

impl Window {
    // Should be changed to `RunningEventLoop` in future, as `EventLoop` doesn't actually work on macOS
    fn new(event_loop: &mut impl EventLoopTraitBikeShed);
}

This would, apart from removing ControlFlow, resolve at least these gripes I have with the current design (and would push us closer to #2903).

  • The platform-specific event loop has to contain the top-level EventLoopWindowTarget to be able to Deref to it, which is inconvenient (and can be confusing for the user).
  • There's unnecessary shared references. &mut when creating windows would be nice for Wayland, if I understand correctly @kchibisov?
  • Disallowing creating windows outside the event loop on macOS would be easy in the future.

@daxpedda
Copy link
Member Author

Is this really required to get rid of ControlFlow? I like the idea, but would like to handle it separately.

@madsmtm
Copy link
Member

madsmtm commented Aug 24, 2023

I remember experimenting with it before, I think it kinda is if you want to avoid interior mutability on the control flow. But maybe not, if not, feel free to do that first!

@notgull
Copy link
Member

notgull commented Aug 25, 2023

I would support moving these methods to the window target.

@daxpedda daxpedda added C - in progress Implementation is proceeding smoothly and removed C - needs discussion Direction must be ironed out labels Aug 26, 2023
@kchibisov
Copy link
Member

We already have the set_wait() like methods on the control_flow struct itself, just move it to tho EventLoopWindowTarget and call it a day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - in progress Implementation is proceeding smoothly
Development

Successfully merging a pull request may close this issue.

4 participants