Dialog RFD does not work with winit MACOS #2752

TimonPost · 2023-03-27T14:29:14Z

I am using RFD and the run_return function. If I open a dialog outside run-return it will open one frame and close directly i.e not block as expected. If I move the dialog into the run_return it stays open.

This behavior used to work on macos pre 0.27, so was wondering if there was a change that broke the behavior. And also it still works on windows!

If I log the FocusManager (link) in RFD, which is used here, I see the following:

Focus manager: false  true  true

With the log:

 log::warn!("Focus manager: {}  {}  {}", app.is_running(), app.get_window().is_some(), app.key_window().is_some());

Indicating the app is not running.

I have my repro example here:

https://github.com/rust-windowing/winit/compare/master...TimonPost:winit:timon/rfd-issue-run-return?expand=1

The text was updated successfully, but these errors were encountered:

msiglreith · 2023-03-28T16:31:44Z

Which OS does it affect? I'm unable reproduce it on Windows fwiw.

TimonPost · 2023-03-28T18:10:26Z

Ah sorry, forgot to mention but its macos as references in the links as well. Windows does indeed work!

rib · 2023-04-10T20:18:30Z

This is getting into some sketchy territory in the MacOS backend...

On MacOS you have a global NSApp and CFRunLoop that may exist beyond the lifetime of a Winit EventLoop and is accessible to this external event loop, and rfd outside of Winit's run_return.

Internally Winit's MacOS backend has some global static Lazy<Handle> state that effectively lets it track state associated with the NSApp, including temporarily tracking the callback given to run_return.

Additionally when you create a new EventLoop the MacOS backend installs a RunLoopObserver for tracking when the event loop blocks to wait for events and when it wakes up. This observer will also try and dispatch corresponding events via the global Handle if it can find a valid callback - otherwise it will panic!. This panic! happens inside a fail-safe stop_app_on_panic wrapper around catch_unwind that will aim to call [NSApp stop] for this panic.

I'm not 100% sure of exactly what's going wrong in this specific example, but I did try reproducing something similar while I was looking at implementing the run, run_ondemand and pump_events APIs that were discussed recently here: #2706

I currently see something like:

Winit doesn't appear to ever uninstall the RunLoopObserver and doesn't anticipate that something else could potentially run the RunLoop.
[NSApp stop] will have been called as a means to stop the event loop and return from run_return which means that rfd is then trying to do things within a non-running NSApp.
Modal dialogs have a special "runModal" way of running a RunLoop which is effectively ignoring that the top-level NSApp is currently not actually "running" and this then kicks off Winit's observer callbacks and panic, and then re-"stop" the NSApp.

Some of this behaviour looks like it's been like this in Winit for a while, so I'm not sure atm why this usage model might have worked in the past - I have a feeling that might have involved a pretty big dose of luck :)

Maybe it will be enough to try and be more careful in Winit to consider that the RunLoop might get run outside of Winit but switching between two crates driving the main RunLoop in different ways (one via [NSApp run] and the other via a modal run seems like it could be risky.

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: Linux TODO: split patch up

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: tidy up macos changes - probably lots of debug stuff left in atm TODO: Linux TODO: ask someone to test orbital changes, made blindly TODO: split patch up

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: X11/Wayland backends TODO: ask someone to test orbital changes, made blindly TODO: split patch up Second pass over the windows backend I had totally underestimated what was involved in updating the windows backend since I hadn't seen the special `wait_thread` that was also dispatching messages and implementing control_flow timeouts via MsgWaitForMultipleObjects, nor had I grokked all the special stuff being done with `WM_PAINT` messages. This "breaks" the MainEventsCleared -> RedrawRequested -> RedrawEventsCleared ordering guarantees, similar to how this is anyway inherently broken in the MacOS backend. The switches to using SetTimer to handle control_flow Wait timeouts and removes the need to use MsgWaitForMultipleObjects. Overall this is a pretty nice simplification of several things TODO: tidy up (lots of debug and misc changes + comments atm) stash

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: X11/Wayland backends TODO: ask someone to test orbital changes, made blindly TODO: split patch up

This was referenced Mar 28, 2023

Embark issues EmbarkStudios/rust-ecosystem#36

Open

RFD is broken with winit run_return PolyMeilex/rfd#116

Open

TimonPost changed the title ~~Dialog RFD does not work with winit~~ Dialog RFD does not work with winit MACOS Mar 28, 2023

msiglreith added the DS - macos label Mar 28, 2023

rib mentioned this issue Apr 12, 2023

Re-work event loop run APIs: adds run -> Result<>, run_ondemand and pump_events #2767

Merged

15 tasks

kchibisov closed this as completed in #2767 Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialog RFD does not work with winit MACOS #2752

Dialog RFD does not work with winit MACOS #2752

TimonPost commented Mar 27, 2023 •

edited

msiglreith commented Mar 28, 2023

TimonPost commented Mar 28, 2023 •

edited

rib commented Apr 10, 2023

Dialog RFD does not work with winit MACOS #2752

Dialog RFD does not work with winit MACOS #2752

Comments

TimonPost commented Mar 27, 2023 • edited

msiglreith commented Mar 28, 2023

TimonPost commented Mar 28, 2023 • edited

rib commented Apr 10, 2023

TimonPost commented Mar 27, 2023 •

edited

TimonPost commented Mar 28, 2023 •

edited