From aff4b082d7157e426c81c6844df739d9b9f09cbb Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 17 Oct 2026 15:23:58 -0700 Subject: [PATCH 1/3] Fix #341: on macOS, a quit from a visible window goes through `terminate:` Quit EffectCraft and Finder's Quit send `ViewportCommand::Close`, and the close button sends the window's close request. In each case eframe drops the window while AppKit's run loop is still running. On a Mac with a Touch Bar, AppKit's Touch Bar code then raises an uncaught Objective-C exception (`-[_NSTouchBarFinderObservation invalidate]`) and the app crashes (storytold/craft-libs#3). After the unsaved-changes check, `close_or_quit` calls `craft_quit::on_close_request` from craft-libs, pinned to the rev that craft-docpath already uses. When nothing holds the window open, it cancels the close and quits through `-[NSApplication terminate:]`, which still runs `on_exit` (window position, workspace, crash-recovery cleanup). The prompt for a modified project still holds the window. The window close after a lost graphics device takes the same route. On other platforms it does nothing. pdfcraft adds craft-quit the same way (storytold/pdfcraft#1533). A quit while the window is minimized or hidden still closes the window as before: eframe does not run `ui` then, and the unsaved-changes check and the new call both run from `ui`. `terminate:` ends the process before `eframe::run_native` returns, so the log line for the closing window moves from `main` into `on_exit`. --- Cargo.lock | 12 +++++++ Cargo.toml | 1 + apps/effectcraft/src/main.rs | 14 ++++---- crates/ui-egui/Cargo.toml | 1 + crates/ui-egui/src/lib.rs | 5 ++- crates/ui-egui/src/panels/unsaved.rs | 51 ++++++++++++++++++++++++++++ crates/ui-egui/tests/ui_gpu.rs | 7 ++++ 7 files changed, 83 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c2f0c927d..8c354978a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -982,6 +982,17 @@ dependencies = [ "thiserror 2.0.21", ] +[[package]] +name = "craft-quit" +version = "0.1.0" +source = "git+https://github.com/storytold/craft-libs?rev=82a24b006eefa036b43c09a67fb9a3fbd17ffe63#82a24b006eefa036b43c09a67fb9a3fbd17ffe63" +dependencies = [ + "dispatch2", + "egui", + "objc2 0.6.4", + "objc2-app-kit 0.3.2", +] + [[package]] name = "craft-ui" version = "0.1.0" @@ -1651,6 +1662,7 @@ dependencies = [ name = "effectcraft-ui-egui" version = "0.11.0" dependencies = [ + "craft-quit", "craft-ui", "effectcraft-color", "effectcraft-engine", diff --git a/Cargo.toml b/Cargo.toml index 5061bdfcd..60e97e13c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,7 @@ repository = "https://github.com/storytold/effectcraft" [workspace.dependencies] craft-pdf = { git = "https://github.com/storytold/craft-libs", rev = "ca50d63b773c99f96e75c77ce5a1d21e7c9cda0d", version = "=0.1.0", default-features = false, features = ["read"] } craft-docpath = { version = "0.1.0", git = "https://github.com/storytold/craft-libs", rev = "82a24b006eefa036b43c09a67fb9a3fbd17ffe63" } +craft-quit = { version = "0.1.0", git = "https://github.com/storytold/craft-libs", rev = "82a24b006eefa036b43c09a67fb9a3fbd17ffe63" } # internal effectcraft-time = { path = "crates/time" } effectcraft-geom = { path = "crates/geom" } diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 2f99bd5e0..4a8d5332a 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -327,14 +327,11 @@ fn main() -> eframe::Result { })) }), ); - match &result { - Ok(()) => log::info!("the window closed"), - Err(e) => { - if on_gl { - launch_guard::opengl_failed(effectcraft_host::config_dir().as_deref()); - } - startup_failed(e, !on_gl && cfg!(not(target_os = "macos"))); + if let Err(e) = &result { + if on_gl { + launch_guard::opengl_failed(effectcraft_host::config_dir().as_deref()); } + startup_failed(e, !on_gl && cfg!(not(target_os = "macos"))); } result } @@ -429,6 +426,9 @@ impl eframe::App for Desktop { } fn on_exit(&mut self) { + // On macOS the app quits through `-[NSApplication terminate:]`, and the process ends + // before `eframe::run_native` returns (#341). + log::info!("the window is closing"); self.launch.exited(); self.window.save(self.app.session.config.as_deref()); self.app.on_exit(); diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index be02fef5e..ea356b871 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -11,6 +11,7 @@ workspace = true [dependencies] craft-ui = { workspace = true } +craft-quit = { workspace = true, features = ["egui"] } effectcraft-time = { workspace = true } effectcraft-geom = { workspace = true } effectcraft-color = { workspace = true } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 105a075e7..938e828c7 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1657,7 +1657,7 @@ impl EffectcraftApp { self.run_script_tasks(&ctx); self.apply_prefs(&ctx); self.handle_events(&ctx); - panels::unsaved::on_close_requested(self, &ctx); + panels::unsaved::close_or_quit(self, &ctx); let title = panels::unsaved::window_title(self); if title != self.window_title { ctx.send_viewport_cmd(egui::ViewportCommand::Title(title.clone())); @@ -1903,6 +1903,9 @@ impl eframe::App for EffectcraftApp { fn ui(&mut self, ui: &mut egui::Ui, frame: &mut eframe::Frame) { if self.gpu_failure_applied == Some(true) { self.close_after_device_loss(ui.ctx()); + // On macOS the app quits through `-[NSApplication terminate:]` instead of closing the + // window while AppKit's run loop is running (#341). + craft_quit::on_close_request(ui.ctx()); return; } self.init_gpu(frame); diff --git a/crates/ui-egui/src/panels/unsaved.rs b/crates/ui-egui/src/panels/unsaved.rs index 05f719fa9..e509f889e 100644 --- a/crates/ui-egui/src/panels/unsaved.rs +++ b/crates/ui-egui/src/panels/unsaved.rs @@ -58,6 +58,15 @@ pub fn on_close_requested(app: &mut EffectcraftApp, ctx: &egui::Context) { } } +/// [`on_close_requested`], then on macOS a close that nothing held open quits through +/// `-[NSApplication terminate:]` instead: closing the window while AppKit's run loop is running +/// crashes on a Mac with a Touch Bar (#341). `on_exit` still runs. Returns whether that quit was +/// scheduled; other platforms close the window as before. +pub fn close_or_quit(app: &mut EffectcraftApp, ctx: &egui::Context) -> bool { + on_close_requested(app, ctx); + craft_quit::on_close_request(ctx) +} + /// The project's display name: its file name, or `Untitled Project.ecproj`. pub fn project_name(app: &EffectcraftApp) -> String { app.session @@ -207,6 +216,23 @@ mod tests { out.viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(&egui::ViewportCommand::CancelClose)) } + /// One frame of the dialogs with the window's close button pressed, through [`close_or_quit`] + /// as the app runs it; returns whether the close was canceled and whether the platform quit + /// was scheduled. + fn close_frame(app: &mut EffectcraftApp, ctx: &egui::Context) -> (bool, bool) { + let mut raw = egui::RawInput::default(); + raw.viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + let mut quits = false; + let mut out = ctx.run_ui(raw, |ui| { + app.auto.begin_frame(); + quits = close_or_quit(app, ui.ctx()); + crate::panels::dialogs::show(app, ui.ctx()); + }); + out.textures_delta.clear(); + let canceled = out.viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(&egui::ViewportCommand::CancelClose)); + (canceled, quits) + } + fn modified(app: &mut EffectcraftApp) { app.session.execute("comp.new", json!({"name": "Main", "width": 64, "height": 64})).unwrap(); assert!(app.session.is_dirty()); @@ -340,4 +366,29 @@ mod tests { assert!(!app.session.is_dirty()); assert!(!frame(&mut app, &ctx, true)); } + + /// Closing the window while AppKit's run loop is running crashes on a Mac with a Touch Bar + /// (#341). On macOS the close is canceled and the app quits through + /// `-[NSApplication terminate:]`. Other platforms close the window. + #[test] + fn closing_a_saved_project_quits_through_the_platform_on_macos() { + let (mut app, ctx) = app(); + let macos = cfg!(target_os = "macos"); + assert_eq!(close_frame(&mut app, &ctx), (macos, macos)); + assert!(app.dialog.is_none(), "nothing to ask"); + } + + #[test] + fn closing_a_modified_project_asks_before_the_platform_quit() { + let (mut app, ctx) = app(); + modified(&mut app); + assert_eq!(close_frame(&mut app, &ctx), (true, false), "the prompt holds the window and nothing quits"); + assert_eq!(app.dialog, Some(Dialog::UnsavedChanges)); + answer_with(&mut app, &ctx, Answer::Cancel); + assert!(app.session.is_dirty() && !app.dialog_state.unsaved.quitting); + assert_eq!(close_frame(&mut app, &ctx), (true, false), "asked again after Cancel"); + answer_with(&mut app, &ctx, Answer::Discard); + let macos = cfg!(target_os = "macos"); + assert_eq!(close_frame(&mut app, &ctx), (macos, macos), "Don't Save quits"); + } } diff --git a/crates/ui-egui/tests/ui_gpu.rs b/crates/ui-egui/tests/ui_gpu.rs index 906ac0cd1..355c35bf5 100644 --- a/crates/ui-egui/tests/ui_gpu.rs +++ b/crates/ui-egui/tests/ui_gpu.rs @@ -115,5 +115,12 @@ fn a_lost_device_draws_nothing_autosaves_and_closes() { let saved = h.state().session.autosave.last_path.clone().expect("unsaved changes auto-saved"); assert!(std::path::Path::new(&saved).is_file(), "{saved}"); assert_eq!(*told.borrow(), vec![Some(Ok(saved))], "the user is told once, with the recovery copy"); + // The close arrives: on macOS the app quits through `-[NSApplication terminate:]` instead of + // closing the window while AppKit's run loop is running (#341). + h.input_mut().viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + h.step(); + let kept = h.output().viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(&egui::ViewportCommand::CancelClose)); + assert_eq!(kept, cfg!(target_os = "macos"), "the platform quit closes the window on macOS"); + assert_eq!(told.borrow().len(), 1); let _ = std::fs::remove_dir_all(&dir); } From eff747897df39c3399ff089e63cb5d9d6ca77f16 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 24 Oct 2026 02:50:22 -0700 Subject: [PATCH 2/3] Fix #341: the close decision runs in App::logic eframe runs App::logic without App::ui while the window is minimized or hidden, and handles a close request then too. The unsaved-changes check, the device-loss close and craft_quit::on_close_request ran from ui, so such a close skipped the prompt and the macOS quit through terminate:. They now run at the end of logic, with the platform hook last, as craft-quit asks. --- crates/ui-egui/src/lib.rs | 15 ++++++++++----- crates/ui-egui/src/panels/unsaved.rs | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 90b8c7097..4e0b8b10c 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1668,7 +1668,6 @@ impl EffectcraftApp { self.run_script_tasks(&ctx); self.apply_prefs(&ctx); self.handle_events(&ctx); - panels::unsaved::close_or_quit(self, &ctx); let title = panels::unsaved::window_title(self); if title != self.window_title { ctx.send_viewport_cmd(egui::ViewportCommand::Title(title.clone())); @@ -1913,14 +1912,20 @@ impl eframe::App for EffectcraftApp { } self.issue_screenshots(ctx); self.collect_screenshots(ctx); + // eframe runs `logic` without `ui` while the window is minimized or hidden, and handles a + // close then too, so the close decision is made here, with the platform hook last. + if self.gpu_failure_applied == Some(true) { + self.close_after_device_loss(ctx); + // On macOS the app quits through `-[NSApplication terminate:]` instead of closing the + // window while AppKit's run loop is running (#341). + craft_quit::on_close_request(ctx); + } else { + panels::unsaved::close_or_quit(self, ctx); + } } fn ui(&mut self, ui: &mut egui::Ui, frame: &mut eframe::Frame) { if self.gpu_failure_applied == Some(true) { - self.close_after_device_loss(ui.ctx()); - // On macOS the app quits through `-[NSApplication terminate:]` instead of closing the - // window while AppKit's run loop is running (#341). - craft_quit::on_close_request(ui.ctx()); return; } self.init_gpu(frame); diff --git a/crates/ui-egui/src/panels/unsaved.rs b/crates/ui-egui/src/panels/unsaved.rs index e509f889e..9c8c08870 100644 --- a/crates/ui-egui/src/panels/unsaved.rs +++ b/crates/ui-egui/src/panels/unsaved.rs @@ -391,4 +391,19 @@ mod tests { let macos = cfg!(target_os = "macos"); assert_eq!(close_frame(&mut app, &ctx), (macos, macos), "Don't Save quits"); } + + /// While the window is minimized or hidden, eframe runs `App::logic` without `App::ui` and + /// still handles a close: `logic` asks about a modified project. + #[test] + fn a_close_while_only_logic_runs_asks_about_a_modified_project() { + let (mut app, ctx) = app(); + modified(&mut app); + let mut raw = egui::RawInput::default(); + raw.viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + let mut frame = eframe::Frame::_new_kittest(); + let out = ctx.run_logic(&raw, |ctx| eframe::App::logic(&mut app, ctx, &mut frame)); + let canceled = out.viewport_commands.get(&egui::ViewportId::ROOT).is_some_and(|c| c.contains(&egui::ViewportCommand::CancelClose)); + assert!(canceled, "the close is canceled"); + assert_eq!(app.dialog, Some(Dialog::UnsavedChanges)); + } } From ce0ae4e3873ffdbabb50cbb24445f189cbcabd77 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 24 Oct 2026 08:38:14 -0700 Subject: [PATCH 3/3] Fix #341: answered file dialogs run before the close decision The close decision moved to App::logic, which runs before App::ui, while the queued file-dialog answers still ran in ui. An answer that arrived in the frame that closed the window changed the project after the decision, so the window could close without asking to save. The answers now run in logic, before the decision. --- crates/ui-egui/src/lib.rs | 3 ++- crates/ui-egui/src/panels/unsaved.rs | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 4e0b8b10c..5a34927b2 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1623,7 +1623,6 @@ impl EffectcraftApp { fn frame(&mut self, ui: &mut egui::Ui) { let ctx = ui.ctx().clone(); self.apply_gpu_failure(&ctx); - self.run_dialog_answers(); self.auto.begin_frame(); self.frames.set_context(&ctx); if self.session.render_job.is_some() { @@ -1920,6 +1919,8 @@ impl eframe::App for EffectcraftApp { // window while AppKit's run loop is running (#341). craft_quit::on_close_request(ctx); } else { + // An answered file dialog can change the project, so it runs before the close decision. + self.run_dialog_answers(); panels::unsaved::close_or_quit(self, ctx); } } diff --git a/crates/ui-egui/src/panels/unsaved.rs b/crates/ui-egui/src/panels/unsaved.rs index 9c8c08870..4a94265b4 100644 --- a/crates/ui-egui/src/panels/unsaved.rs +++ b/crates/ui-egui/src/panels/unsaved.rs @@ -406,4 +406,22 @@ mod tests { assert!(canceled, "the close is canceled"); assert_eq!(app.dialog, Some(Dialog::UnsavedChanges)); } + + /// A file dialog answered in the frame that closes the window runs its command before the + /// close decision, so a change it makes is asked about. + #[test] + fn a_dialog_answer_runs_before_the_close_decision() { + let (mut app, ctx) = app(); + assert!(!app.session.is_dirty()); + let answer = crate::DialogAnswer { command: "comp.new".into(), key: "name", params: json!({"width": 64, "height": 64}), paths: vec!["Main".into()] }; + app.dialog_answers.0.send(answer).unwrap(); + let mut raw = egui::RawInput::default(); + raw.viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + let mut frame = eframe::Frame::_new_kittest(); + let out = ctx.run_logic(&raw, |ctx| eframe::App::logic(&mut app, ctx, &mut frame)); + let canceled = out.viewport_commands.get(&egui::ViewportId::ROOT).is_some_and(|c| c.contains(&egui::ViewportCommand::CancelClose)); + assert!(app.session.is_dirty(), "the answer ran"); + assert!(canceled, "the close is canceled"); + assert_eq!(app.dialog, Some(Dialog::UnsavedChanges)); + } }