Remove lifetime from the Event
#2981
Conversation
There was a problem hiding this comment.
I wouldn't say it's an "uncontroversial" change, see e.g. #939 where alternatives to a lifetime was discussed.
In any case, while I agree that the lifetime is not ideal from a user's perspective, it does act as sort of "speedbump" for handling events outside of the event loop, which, even though most events don't have a lifetime, is generally not a good idea.
I.e. I think we should consider this change in that light as well, e.g. it's just not just about "what can users now do more easily", but also "what pitfalls will they now fall into more easily".
Approved the iOS and macOS impls.
| @@ -46,7 +46,7 @@ fn main() -> Result<(), impl std::error::Error> { | |||
|
|
|||
| println!("parent window: {parent_window:?})"); | |||
|
|
|||
| event_loop.run(move |event: Event<'_, ()>, event_loop, control_flow| { | |||
| event_loop.run(move |event: Event<()>, event_loop, control_flow| { | |||
There was a problem hiding this comment.
Do we have no examples that use ScaleFactorChanged?
That seems like an oversight, and makes it hard for us to judge how the API will be used by users.
There was a problem hiding this comment.
I guess the main case is to write when changing to some size the user wants so you don't have a window jumping in sizes, but not sure that we really want to demonstrate it.
In the current state of things this design is bad, because we tie event to the types it shouldn't be tied to. Yes this is sort of bad, but the lifetime is still there and we provided all the things to do event handling outside the event loop before with What I think we should change is to split the event callback handler into small ones, so we can have immediate actions encoded in a different way, like with the return type on the callback or some other handler structs. Events are just data in the end, they shouldn't be used to perform actions. So the current design is bad, and we already have workaround for it, so it's just justifies its removal. |
I fully agree, but we're not there right now, and it may take us a while to get there. So I fear that in the meantime, more people will be incentivized to handle events outside the callback, which will make it even harder for them to transition to a more fully callback-based model. Anyhow, I am somewhat just playing the devil's advocate, I think the trade-off by accepting this PR is in general okay, I just want to highlight that it is indeed a trade-off. |
They already do that with |
kchibisov
force-pushed
the
no-lifetime
branch
4 times, most recently
from
58107fd to
2e6e2da
Compare
Lifetimes don't work nicely when dealing with multithreaded environments in the current design of the existing winit's event handling model, so remove it in favor of `InnerSizeWriter` fences passed to client, so they could try to update the size. Fixes rust-windowing#1387.
|
I'll note down @Osspial's positive attitude towards the previous PR, that basically resolves my remaining concerns for why we shouldn't do this. |
Lifetimes don't work nicely when dealing with multithreaded environments in the current design of the existing winit's event handling model, so remove it in favor of
Weakfences passed to client, so they could try to update the size.Fixes #1387.
CHANGELOG.mdif knowledge of this change could be valuable to users--
The changes are uncontroversial and basically removed one concept with another. If backend maintainers think that they can do better now due to event being without a lifetime they should do that in the follow up PR.