Verify event order #3710
Draft
Verify event order #3710
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
madsmtm
added
B - bug
madsmtm
force-pushed
the
madsmtm/verify-event-order
branch
from
128fb21 to
14bedb6
Compare
When debug assertions are enabled, check that events are emitted in the expected order, and if not, log an error.
madsmtm
force-pushed
the
madsmtm/verify-event-order
branch
from
14bedb6 to
8968765
Compare
kchibisov
requested changes
Jun 6, 2024
There was a problem hiding this comment.
The general idea is good, though, we'd need something to run all of that on CI in a crossplatform way.
But as for Suspended event, I'm still not sure whether we should emit it like that. Probably it won't hurt, but we must ensure that it's semantically the same place on all the platforms, right now I don't think it's the case.
kchibisov
reviewed
Jun 6, 2024
Comment on lines
+307
to
+311
| fn memory_warning(&mut self, event_loop: &ActiveEventLoop) { | ||
| // TODO: What states are allowed when receiving this? | ||
| self.inner.memory_warning(event_loop); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's just a generic event which should be moved somewhere else, like to a separate trait. The ApplicationHandler today should represent the lifecycle of the event loop in the future, and real events should be on some other traits.
25 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The order that application handler methods are called in relation to each other is somewhat badly documented, and difficult to know for sure if correct. To remedy this, I've implemented a check that's enabled with debug assertions, and which logs if events are emitted in an unexpected order.
I went with using debug assertions, since I don't know of a better way to ensure that this will be tested, but there's of course a cost here in that if we don't catch all the issues, then downstream users will see these errors.
TODO:
Once done, also fixes #3206.