event_loop: group core API in EventLoopProvider#4221
event_loop: group core API in EventLoopProvider#4221
EventLoopProvider#4221Conversation
8368762 to
09211f8
Compare
09211f8 to
0d0fd39
Compare
madsmtm
left a comment
There was a problem hiding this comment.
Thanks, this is what I thought of in #4208 (comment).
This helps with portability and defines some top-level structure around the event loop, so in the future, backends can get an idea of what API to use. This also changes the API to be object safe by using `dyn` throughout.
0d0fd39 to
8d39dbf
Compare
| - `run_app_on_demand`/`pump_app_events` now accept `&mut dyn ApplicationHandler` instead of generic. | ||
| - Moved common `EventLoop` methods like `run_app` into `EventLoopProvider` trait. | ||
| - Moved `event_loop::EventLoop` into `platform::event_loop::EventLoop` keeping the old re-export in place. | ||
| - `EventLoopProvider::run_app` now takes `Box<dyn ApplicationHandler` instead of owned generic. |
| /// [`set_control_flow()`]: ActiveEventLoop::set_control_flow() | ||
| /// [`run_app()`]: Self::run_app() | ||
| #[inline] | ||
| #[cfg(not(all(web_platform, target_feature = "exception-handling")))] |
There was a problem hiding this comment.
It's not that clean, but until we land #4165, I think you have to keep this cfg, otherwise it won't build?
There was a problem hiding this comment.
Ah, It'll fail to compile on a different level, thus I can not do much about it...
Maybe the run APIs on their own should be separated into their own Traits, so the impl will be present only when it's actually possible?
There was a problem hiding this comment.
Maybe the
runAPIs on their own should be separated into their own Traits, so the impl will be present only when it's actually possible?
Possibly? I'm not sure at the moment.
madsmtm
left a comment
There was a problem hiding this comment.
Actually, I spent some more time thinking this over, and I no longer think a trait is warranted here.
Places where it could be useful:
- If Winit wanted to do
Box<dyn EventLoopProvider>- but we do not, the top-level crate is always going to know the set of backends that it supports (i.e. that's anenum). - If some downstream library wanted to do
fn my_fn(event_loop: impl EventLoopProvider). The only functions that you can call here though are:run_app, in which casemy_fnshould instead return aimpl ApplicationHandlercreate_proxy, in which casemy_fnshould just takeEventLoopProxy.listen_device_events, in which case you'd probably wantimpl ActiveEventLoopinstead.set_control_flow, in which case you'd probably wantimpl ActiveEventLoopinstead.
So I think we should close this PR, and move forwards with #4208 without.
Apologies for making you submit this PR, and then ending up changing my mind!
While that is true, there's another side of the issue. Given that those Also, the user of the API wants to store |
Yeah, I can see that point, though that is kinda also solvable with better docs.
Which is why I think it's fine for us to sidestep the issue and wait with introducing a trait here. |
This helps with portability and defines some top-level structure around the event loop, so in the future, backends can get an idea of what API to use.
This also changes the API to be object safe by usingdynthroughout.--
I think that should help with core split.