-
Notifications
You must be signed in to change notification settings - Fork 921
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
iOS: Never queue application-level events #3905
Conversation
b81218b
to
fb9cd26
Compare
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.
Commit should specify that it fixes specific issue you're talking about and you should have a changelog entry for it.
fb9cd26
to
b4772ac
Compare
I would treat the fact that it fixes #3714 as more of a curiosity, not a true fix, there's still a bunch of things I need to do to fully fix that issue. But I have added a changelog entry. |
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 would treat the fact that it fixes #3714 as more of a curiosity, not a true fix, there's still a bunch of things I need to do to fully fix that issue. But I have added a changelog entry.
In this case it shouldn't matter and it's better without it. And saying that it fixes it should be removed, since it's not as you say.
b4772ac
to
cab5487
Compare
Sure, I've updated the comment |
Does the changelog entry really matter at this point? Maybe just drop it, since it's an implementation detail. I thought that you've fixed the issue meaning that you'd close it with a changelog entry, but if it's not fixed, then changelog entry doesn't really matter. |
That's why I didn't have a changelog entry in the first place ;) But I think there is value in keeping the entry, while this is an implementation detail, it can be useful for users to know if something were to go wrong with it. |
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 just don't see how it's actionable or solves issues, and I've asked for changelog only because you've said that it fixes some old bug. I'll leave it up to you though.
cab5487
to
26db97a
Compare
Events like `resumed`, `new_events`, `about_to_wait`, and so on will never happen as a result of programmer action, so we'll never need to queue those. This allows us to remove the need for the old `Event` struct in the iOS backend. Furthermore, we can now remove `InUserCallback`, since that state is already stored inside `EventHandler`. I've tried to otherwise keep the semantics as close to the original by calling `handle_nonuser_events(mtm, [])`, which flushes pending events.
26db97a
to
9984443
Compare
Well, at the very least it should improve performance (once #3816 is also resolved), just because we do less. So just for that reason, the changelog entry is nice. |
Events like
resumed
,new_events
,about_to_wait
, and so on will never happen as a result of programmer action, so we'll never need to queue those. This allows us to remove the need for the oldEvent
struct in the iOS backend.Furthermore, we can now remove
InUserCallback
, since that state is already stored insideEventHandler
.I've tried to otherwise keep the semantics as close to the original by calling
handle_nonuser_events(mtm, [])
, which flushes pending events.