Skip to content
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

macOS: Fix pump_app_events #4155

Open
wants to merge 1 commit into
base: madsmtm/apple-remove-panic
Choose a base branch
from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Mar 2, 2025

The event loop was reworked in #2767, but unfortunately I didn't know enough about run loops back then to know how to do this kind of stuff correctly. I think I have a pretty good idea now, in short, it works as follows:

AppKit retains a queue of events, which can be popped from using -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:], and then passed to -[NSApplication sendEvent:] (though you're free to filter them if you so desire). This allows us to process events one at a time. It is suboptimal because certain actions like resizing will enter a "modal" mode (similar to this issue), which will prevent events from being delivered while that is ongoing; but that's the cost of pump_app_events.

The implementation is now similar to the implementation we had all the way back in v0.19, and is very close to SDL's approach.

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened DS - macos labels Mar 2, 2025
@madsmtm madsmtm force-pushed the madsmtm/janky-pump-events branch 2 times, most recently from 07bdc91 to 42ea395 Compare March 2, 2025 07:58
Use the system's mechanisms for pumping events, namely
`nextEventMatchingMask:untilDate:inMode:dequeue:`. This still has a few
problems as detailed in the documentation for `pump_app_events` (which
is why we switched away from it all the way back in Winit v0.20), but it
doesn't have nearly as many problems as the current implementation does.
@madsmtm madsmtm force-pushed the madsmtm/janky-pump-events branch from 42ea395 to 48974ed Compare March 3, 2025 07:47
@rib
Copy link
Contributor

rib commented Mar 7, 2025

Out of interest I'd be curious to understand what's broken with what we did for #2767? (the issue description just says that you didn't know how to do it correctly before, but doesn't point to or describe some concrete issues that will be addressed by the change).

It sounds like the blocking on resize events could be a pretty serious issue with this API, that looks like it was only addressed previously by EventLoop 2.0: #219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - macos
Development

Successfully merging this pull request may close these issues.

2 participants