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

Make EventLoopWindowTarget<T> independent of the user event type T #3053

Closed
8 of 9 tasks
madsmtm opened this issue Aug 27, 2023 · 11 comments · Fixed by #3298
Closed
8 of 9 tasks

Make EventLoopWindowTarget<T> independent of the user event type T #3053

madsmtm opened this issue Aug 27, 2023 · 11 comments · Fixed by #3298
Labels
S - api Design and usability S - enhancement Wouldn't this be the coolest?

Comments

@madsmtm
Copy link
Member

madsmtm commented Aug 27, 2023

Requested in #2900 (comment).

Should be possible from an API perspective, though might be a bit tricky to change in the backends.

Platforms that just use PhantomData<T> in the type (and thus don't need the actual user event type):

Finish:

@notgull
Copy link
Member

notgull commented Aug 27, 2023

This would be nice for #3014. The generic type makes it difficult to use the distributed function pointer strategy for event handling that I use in that PR.

@nerditation
Copy link
Contributor

initially, in this comment, I thought the Windows backend is doable but might take some efforts.

after I played a little bit with the Windows backend, it turns out way easier than I thought. I managed to get rid of the generic type T in platform_imp::EventLoopWindowTarget<T> and replace it with a PhantomData (just to keep the API). surprizingly, the change is relatively minor. here's a patch file I created:

elwt-windows.patch

there's two commits in the patch, because I was expecting the changes would be very large and I want to make check points after each small step. I did a quick cargo test --all-features on my machine and it seems nothing is broken (I hope). I also checked some of the examples, all seems fine.

@madsmtm
Copy link
Member Author

madsmtm commented Aug 28, 2023

Nice! Could you put up a Pull Request? That makes it easier to review and discuss the changes

@nerditation
Copy link
Contributor

Could you put up a Pull Request?

sure, #3061 is created. this is slight different from the patch I posted above, even fewer changes to the existing code, I also added more comments explaining why I did it this way. it's intentionally to be minimum as I'm not very familiar with the code base, but can serve as a start point.

@daxpedda
Copy link
Member

Unfortunately this isn't going to be easy on Web, but I have started working on it now.

@kchibisov
Copy link
Member

I'm not sure how it'll work long term for the Wayland, if I move all the winit state into the EventLoopWindowTarget and it'll be passed around and also sort of carry user events stuff, maybe user events will need a different abstraction in general here, but I can't say more at the moment.

In general, we could always fallback to dynamic dispatch for the user events, hence winit doesn't need to know what is coming in them, it just needs to pass them around.

@nerditation
Copy link
Contributor

I'm not sure how it'll work long term for the Wayland, if I move all the winit state into the EventLoopWindowTarget and it'll be passed around and also sort of carry user events stuff, maybe user events will need a different abstraction in general here, but I can't say more at the moment.

I don't know much about the history of API evolution, the EventLoopWindowTarget type always feels a bit odd API design to me. I didn't really notice its existance for very long (even I used winit many times, I only used it for simple applications with single window.)

initially I was under the impression that all windows must be created before entering the event loop. the introduction section of the document doesn't mention EventLoopWindowTarget at all. the example code passes a reference of EventLoop to create the window, and the event handler completely ignores the second argument:

winit/src/lib.rs

Lines 52 to 54 in bb9b629

//! let window = WindowBuilder::new().build(&event_loop).unwrap();
//!
//! event_loop.run(move |event, _, control_flow| {

when I first discovered the EventLoopWindowTarget, I was very confused by the naming: what is a window target any way? the document simply says:

it associates windows with an EventLoop

This type exists to allow you to create new windows while Winit executes your callback.

from that I assumed it was probably related to some weird API limitations, and maybe OS limitations too (e.g. GUI must be done on main thread, etc).

from the high level API of winit, my mental model of the architecture was pictured like this (from my experience of Windows API, I don't know much about other systems):

  • cross-platform EventLoop wraps platform specific backend EventLoop, which calls polls/drives the OS message queue
  • backends provide an OS specific callback procedure to translate system IO messages into cross platform Events
  • cross-platform EventLoopWindowTarget wraps backend EventLoopWindowTarget, which manages resources needed by the OS window creation API.
    • on Win32, it could possibly include any of the parameters of CreateWindowExW, notably hInstance, hMenu, hWndParent
    • or maybe lpClassName too, i.e. make it responsible to call RegisterClassExW lazily.
  • cross-platform EventLoopProxy maintain a thread safe side channel with the cross-platform EventLoop for user event
  • backend EventLoop can be waken/notified by the EventLoopProxy using system specific mechanism.
    • on Win32, presumably using PostThreadMessage with message id above WM_USER

there's some major difference between my mental model and how it was actually implemented. for example:

  • I thought the backend only processes platform specific messages, the entire backend can be agnostic about the user event type.
    • the system defines the message callback type, we'll have to erase the user event type and do dynamic dispatch anyway, why not do it in the front end?
  • I thought EventLoopWindowTarget only serves the window creation purpose, why do we require the backend EventLoop contains a root ELWT?
  • instead of leaking the user event type from front end to backend, if the backend simply provide an abstract wakeup mechanism, maybe we can use that to drive an async executor?
    • some backends may have more efficient way to integrate future polling in the backends than round trip though the front end, like, e.g. how winit-block-on uses a Event<Signal<T>> wrapping an Event<T> to deliver reactor waker callbacks.

@nerditation
Copy link
Contributor

nerditation commented Aug 30, 2023

Unfortunately this isn't going to be easy on Web, but I have started working on it now.

@daxpedda I just looked at the web backend, and applied my Windows backend technique to it, and it seems doable. here's a little proof-of-concept patch if you want to take a look (I literally just applied my method for the Windows backend to the web to see if it compiles, and it seems it compiles fine)

EDIT:

I explained the rationale about why I change the code in this way in #3061, it's merely a start point to isolate the user type dependent and independent portion so later cleanups can be done incrementally.

POC-web.patch

DISCLAIMER

I'm not a web developer, I don't understand how the web backend works, this is me just trying to proof the EventLoopWindowTarget (and probably the whole backend around the Runner thingy) is indeed independet of the user event type. after some thinking, it's obvious to me that the backend is only processing IO in a platform dependent way, it had nothing to do with the user event type.

@daxpedda
Copy link
Member

Nice, that's a really neat idea @nerditation. Thanks!

@madsmtm
Copy link
Member Author

madsmtm commented Aug 30, 2023

@nerditation actually one more thing to add: Ideally you shouldn't create windows with the EventLoop, you should only do it using the EventLoopWindowTarget given to you. The reason we even support the former is that it'd be very inconvenient for the user, but I am working on that in #2903.

the system defines the message callback type, we'll have to erase the user event type and do dynamic dispatch anyway, why not do it in the front end?

Because we want it to integrate well with the platform's native waiting mechanism, as well as emit Init(WaitCancelled) events and such before and after the user's own event.

If we didn't want to do any of that, we could just not have the functionality at all, and let the user use a channel themselves.

instead of leaking the user event type from front end to backend, if the backend simply provide an abstract wakeup mechanism, maybe we can use that to drive an async executor?

Might be possible, biggest blocker is that it'll take quite a bit of work.

@daxpedda
Copy link
Member

daxpedda commented Sep 3, 2023

Web is done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability S - enhancement Wouldn't this be the coolest?
Development

Successfully merging a pull request may close this issue.

5 participants