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

Only run observers in the default run loop mode #3767

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jun 28, 2024

AppKit/UIKit has a concept of "run loops", which are effectively a queue of events that are repeatedly consumed from, and when empty, the consuming thread is std::thread::parked until new events appear.

Apple has locked the exact details of these down quite tightly, especially on iOS, which is part of the reason why everything must be in a callback there instead of the original pump_events API, but effectively this is the basis of our event loop.

We are still given the option to listen to certain events, however! And this is how we implement the new_events/about_to_wait events, as well as the proxy_wake_up event.

One somewhat odd thing about run loops is that they support being run re-entrantly / in a nested fashion! In this way, the system takes events out of the queue that the nested caller requests, and keeps the rest of the events queued up until the nested caller returns and the outer caller can now process the queued events. Internally, it looks roughly like the following:

'outer loop {
    match run_loop.get_event_matching(_) {
        ClickOnCornerOfWindow => {
            'inner loop {
                match run_loop.get_event_matching(ReleaseClick | MouseMoved) {
                    ReleaseClick => break 'inner,
                    MouseMoved => window.resized(),
                    _ => unreachable!(),
                }
            }
        }
        ClickOnCloseButton => {
            break 'outer;
        }
        _ => {} // Other events
    }
}

Notably, both calls to the fictional "get_event_matching" can end up parking the thread when there is no more work to be done. Now, what this means for us is that we're given basically two options for how to emit new_events/about_to_wait/proxy_wake_up:

  1. Emit it from inside each "get_event_matching" (kCFRunLoopCommonModes).
  2. Emit it only from the outer event loop's "get_event_matching" (kCFRunLoopDefaultMode).

Nested run loops like these are used notably in the following instances:
A. When opening e.g. a file dialog with the rfd crate.
B. When clicking the corner of the window and dragging to live-resize it.

I am unsure what the desired behaviour is for new_events/about_to_wait/proxy_wake_up? In case A, the file dialog is spawned from inside the event loop itself, and as such we cannot emit the event (since that would be re-entrant). But in case B, we might want to emit the new_events/about_to_wait/proxy_wake_up, even though that is not technically what is happening with our outer event loop.

I felt like the most consistent answer would be that we always use kCFRunLoopDefaultMode when originally writing the PR, though then again, maybe resizing should be considered an implementation detail here, and we should just paper over it (like we somewhat currently do)? CC @daxpedda @kchibisov, I'd like input on this! Would the user still expect new_events/about_to_wait to be emitted when live-resizing a window, even though the application is still somewhat "alive" at this stage?

(In any case, I will first have to fix calling request_redraw inside of RedrawRequested, since the approach of calling it inside about_to_wait would break resizing with this PR in its current form).

TODO:

  • Figure out what is actually the desired behaviour here.
  • Test.
  • Add entry to the changelog.
  • Update internal documentation.
  • Update public documentation (esp. also pump_events) with my learnings.
  • Write a proper commit message.

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened DS - macos DS - ios C - needs discussion Direction must be ironed out labels Jun 28, 2024
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 C - needs discussion Direction must be ironed out DS - ios DS - macos
Development

Successfully merging this pull request may close these issues.

1 participant