-
Notifications
You must be signed in to change notification settings - Fork 919
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
Introductory docs on event loop #2743
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 72e1aa4.
Anybody keen to review this? I'd love to get something merged while I have some time on my hands |
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.
Hey @jmaargh, sorry that no-one has responded to your PR! I've tried to give a bit of feedback, but in general this is definitely an improvement, and I'd very much like to get it merged.
docs/event-loop.svg
Outdated
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: I dislike the duplicate "main" loop, perhaps that could be merged into one somehow (not sure how though)
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.
I wasn't a big fan initially, but decided it was best to clearly communicate the actual current behaviour, even if it's a little less "tidy" (since this is a result of the actual behaviour being less "tidy", at least when I originally wrote this, apologies if it's changed in the meantime).
The aim of the diagram was supposed to be illustrative (for pedagogical purposes for new users), not necessarily comprehensive. This is not least because I don't know every corner case to cover for a comprehensive one.
docs/event-loop.svg
Outdated
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.
NewEvents(Init)
should just be a "loop stage" event, a green circle background isn't even in the legend.
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.
Happy to change this if this is the consensus. However, I used the standard flowchart shapes for "start" and "end" and paired with commonly understood traffic-light colours for a bit of visual flair.
As noted in an earlier comment, the diagram was supposed to be illustrative and pedagogical (and always read with the accompanying prose), rather than strictly formal or comprehensive. Of course this (like anything else here) can be changed :)
Thanks for the changes! I think @kchibisov is working on changing how things work in #2896, so I'd like to hear from them and @rib before moving forwards (let's give them a week or so). |
There certainly are some changes in the pipeline that are likely to affect this but, so long as we feel like this isn't immutable then maybe it's ok to land and we can update it when things change. I just tried opening the diagram in https://app.diagrams.net/ and although I guess it's a proprietary web app it seems simple enough to use. Main changes I'm hoping to see though would be: RedrawEventsCleared can probably just be removed, since the concept doesn't really make sense imho if we no longer guarantee an explicit ordering for when we emit redraw events relative to other 'main' events. With the direction discussed for the pump_events/run_ondemand changes then MainEventsCleared is becoming more like an LoopDestroyed should probably be renamed to LoopExiting as was discussed in relation with the run_ondemand changes. |
I'd also like to reiterate the thanks for looking at this @jmaargh, having some visual docs for this seems like it would be really nice to have, sorry for dropping the ball with giving feedback after the initial issue discussion |
I'm sort of concerned that the diagram isn't really accurate, given that we have backends delivering I'm also not even sure that we'll keep |
I think, if it's not too much work for you @jmaargh, we should shift to Mermaid, which has native support in GitHub, Rustdoc integration and MdBook integration. It's completely open source and has an online editor that's fairly easy to use. |
I'm afraid I don't understand how this makes the diagram (or the text) inaccurate? If |
No worries, I'll get that done soon. |
@rib As the behaviour changes, I'm very happy for this documentation to change with it. I'd be very happy to be involved with making those updates if that's helpful (unfortunately I can't guarantee an SLA though!) Unless there's a definite major change to the API/behaviour that's imminent, my preference would be to get this merged as describing the current behaviour. The docs can always (and should) change as the API does. |
Fixes #2736
CHANGELOG.md
if knowledge of this change could be valuable to usersI ended up changing more text than I'd originally planned at the root level of the docs. Happy to make large changes/reversions if the maintainers would prefer - but to me this reads as a much better "what I would have wanted to know before starting" than what there was.
I also took the opportunity to fix up some docs I found along the way which I thought could use some love, specifically:
map_user_event()
Event::DeviceEvent