-
Notifications
You must be signed in to change notification settings - Fork 1.1k
event_loop: group core API in EventLoopProvider
#4221
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
base: master
Are you sure you want to change the base?
Conversation
8368762 to
09211f8
Compare
09211f8 to
0d0fd39
Compare
madsmtm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Outdated
| /// [`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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.