diff --git a/apps/effectcraft/src/file_dialogs.rs b/apps/effectcraft/src/file_dialogs.rs new file mode 100644 index 000000000..a80387541 --- /dev/null +++ b/apps/effectcraft/src/file_dialogs.rs @@ -0,0 +1,360 @@ +//! Every native dialog the desktop app opens (rfd), through one path: [`guarded`]. +//! +//! [`guarded`] opens a dialog only on the main thread and catches a panic while it is made, so a +//! dialog that can't open becomes an error in the status line instead of taking the app, and +//! the open project, down with it. On macOS rfd makes the panel with `+[NSOpenPanel openPanel]`, +//! and objc2's binding panics when AppKit returns nil there (#3084). That panic used to unwind +//! out of the frame into winit's AppKit callback, which stops the app and re-raises it: the app +//! quit. The engine's last-resort guard (`effectcraft_engine::guard`) didn't catch it because it +//! wraps the commands only; a command that needs a path returns at once and its dialog opens +//! later, at the end of the frame ([`show_asked`]), outside any command. +//! +//! The commands' dialogs don't block the frame (macOS and Linux, `Hooks::file_dialog_async`). +//! A blocking `rfd::FileDialog` runs a nested modal event loop inside eframe's frame, that is +//! inside winit's event handler. On macOS a file dragged over the window while that loop ran +//! re-entered winit's handler, which panics across the Objective-C boundary and aborts the app +//! (#511). On Linux the blocking dialog is worse than slow: rfd uses the XDG desktop portal, so +//! the main thread sits waiting on the portal's answer and never pumps winit's events — the +//! window stops answering the compositor and the desktop offers to force-quit the app while the +//! Open dialog is up (#1275). `rfd::AsyncFileDialog` shows the dialog as a sheet on the window +//! (macOS) or through the portal from a worker thread (Linux) and returns at once; the answer +//! arrives when the dialog closes, outside the frame, and the app runs the command then. +//! +//! The panels' own pickers (Settings paths, Effect Controls, the shortcut editor) still use the +//! blocking hooks [`install_pickers`] sets; they go through [`guarded`] as well. + +use effectcraft_ui_egui::{DialogDone, EffectcraftApp, FileDialog}; +use rfd::{AsyncFileDialog, FileHandle}; +use std::future::Future; +use std::panic::AssertUnwindSafe; +use std::pin::Pin; +use std::sync::mpsc::{Receiver, Sender}; +use std::sync::{Arc, Mutex, PoisonError}; + +type Answer = Pin> + Send>>; + +/// A command's dialog waiting for [`show_asked`]. +pub type Asked = (FileDialog, DialogDone); + +/// Open dialog `what` (its title, for the error) with `open`: on the main thread only, and a +/// panic while it opens is an error instead of a crash (#3084). +pub fn guarded(what: &str, open: impl FnOnce() -> T) -> Result { + guarded_on(on_main_thread(), what, open) +} + +/// Whether this is the thread AppKit's panels belong to. AppKit makes panels on the main thread +/// only, and rfd would block this thread on the main one to get there; the hooks aren't `Send`, +/// so the app only ever asks from the main thread. Other platforms' dialogs open on any thread. +fn on_main_thread() -> bool { + #[cfg(target_os = "macos")] + { + objc2::MainThreadMarker::new().is_some() + } + #[cfg(not(target_os = "macos"))] + { + true + } +} + +fn guarded_on(main_thread: bool, what: &str, open: impl FnOnce() -> T) -> Result { + if !main_thread { + log::error!("{what} dialog: asked for off the main thread"); + return Err(couldnt_open(what, "it was asked for off the main thread")); + } + std::panic::catch_unwind(AssertUnwindSafe(open)).map_err(|payload| { + // The panic hook has logged the panic and where it happened. + let why = panic_message(payload.as_ref()); + log::error!("{what} dialog: {why}"); + couldnt_open(what, &why) + }) +} + +fn couldnt_open(what: &str, why: &str) -> String { + format!("The system's {what} dialog couldn't be created ({why}). Your project is still open.") +} + +fn panic_message(payload: &(dyn std::any::Any + Send)) -> String { + payload.downcast_ref::<&str>().map(|s| s.to_string()).or_else(|| payload.downcast_ref::().cloned()).unwrap_or_else(|| "internal error".into()) +} + +/// Where a blocking picker's error goes: the status line, through `Desktop`, which `ctx` +/// wakes so it shows without waiting for input. +#[derive(Clone)] +struct Errors { + tx: Sender, + ctx: egui::Context, +} + +/// Run blocking dialog `what`; one that can't open sends why to `errors` and counts as +/// cancelled (the blocking hooks can't return an error). +fn blocking(errors: &Errors, what: &str, open: impl FnOnce() -> Option) -> Option { + guarded(what, open).unwrap_or_else(|e| { + let _ = errors.tx.send(e); + errors.ctx.request_repaint(); + None + }) +} + +fn path_string(p: std::path::PathBuf) -> String { + p.to_string_lossy().to_string() +} + +/// Install the blocking pickers (`Hooks::pick_files` and friends), each through [`guarded`]. +/// Returns where the errors of those that couldn't open arrive, for the status line; `ctx` is +/// woken when one does. +pub fn install_pickers(app: &mut EffectcraftApp, ctx: &egui::Context) -> Receiver { + let (tx, rx) = std::sync::mpsc::channel::(); + let tx = Errors { tx, ctx: ctx.clone() }; + let errors = tx.clone(); + app.hooks.pick_files = Some(Box::new(move |exts: &[&str]| { + blocking(&errors, "Open", || rfd::FileDialog::new().add_filter("Media", exts).pick_files()).unwrap_or_default().into_iter().map(path_string).collect() + })); + let errors = tx.clone(); + app.hooks.pick_save = Some(Box::new(move |name: &str| { + blocking(&errors, "Save", || rfd::FileDialog::new().add_filter("EffectCraft Project", &["ecproj"]).set_file_name(name).save_file()).map(path_string) + })); + let errors = tx.clone(); + app.hooks.pick_open_project = Some(Box::new(move || { + blocking(&errors, "Open", || rfd::FileDialog::new().add_filter("EffectCraft Project", &["ecproj", "ecprojx"]).pick_file()).map(path_string) + })); + let errors = tx.clone(); + app.hooks.pick_folder = Some(Box::new(move || blocking(&errors, "Choose Folder", || rfd::FileDialog::new().pick_folder()).map(path_string))); + let errors = tx; + app.hooks.pick_save_file = Some(Box::new(move |name: &str, ext: &str| { + blocking(&errors, "Save", || { + // A default with a folder that exists opens the dialog there. + let path = std::path::Path::new(name); + // (No filter for a name without an extension, such as a folder to create.) + let mut d = if ext.is_empty() { rfd::FileDialog::new() } else { rfd::FileDialog::new().add_filter(ext, &[ext]) }; + if let Some(dir) = path.parent().filter(|d| d.is_dir()) { + d = d.set_directory(dir); + } + let file = path.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); + d.set_file_name(file).save_file() + }) + .map(path_string) + })); + rx +} + +/// An error message box, through [`guarded`] (one that can't open is logged). +pub fn message(title: &str, description: String) { + let shown = guarded("message", || rfd::MessageDialog::new().set_level(rfd::MessageLevel::Error).set_title(title).set_description(description).show()); + if let Err(e) = shown { + log::error!("{e}"); + } +} + +/// Show the dialogs the commands asked for in this frame (on the main thread: a sheet opens +/// here) and hand each one's paths to its `done` once it closes, then wake the UI. +/// +/// On macOS the sheet goes on the main window (`frame`'s). Without a parent rfd puts it on +/// AppKit's main window, which is a floating panel's OS window after a click in one; that +/// window hides while the app has no focused window, and a sheet whose window goes away ends +/// without answering, with later sheets queued behind it on the same window. +pub fn show_asked(asked: &Receiver, frame: &eframe::Frame, ctx: &egui::Context) { + while let Ok((dialog, done)) = asked.try_recv() { + show(dialog, done, ctx.clone(), |dialog| open(dialog, frame)); + } +} + +/// The dialog's title, for errors. +fn title(dialog: &FileDialog) -> &'static str { + match dialog { + FileDialog::Open { .. } | FileDialog::OpenProject => "Open", + FileDialog::Folder => "Choose Folder", + FileDialog::Save { .. } => "Save", + } +} + +/// Open `dialog` with `open` through [`guarded`] and wait for its answer on a worker thread. +/// `done` always gets an answer: the paths, or why the dialog couldn't open or answer. +fn show(dialog: FileDialog, done: DialogDone, ctx: egui::Context, open: impl FnOnce(FileDialog) -> Answer) { + let what = title(&dialog); + let answer = match guarded(what, || open(dialog)) { + Ok(answer) => answer, + Err(e) => { + done(Err(e)); + ctx.request_repaint(); + return; + } + }; + // Kept here too, so a worker that can't start still answers. + let done = Arc::new(Mutex::new(Some(done))); + let take = |done: &Mutex>| done.lock().unwrap_or_else(PoisonError::into_inner).take(); + let (worker_done, worker_ctx) = (done.clone(), ctx.clone()); + let spawned = std::thread::Builder::new().name("ec-file-dialog".into()).spawn(move || { + let paths = std::panic::catch_unwind(AssertUnwindSafe(|| pollster::block_on(answer))).map_err(|payload| { + let why = panic_message(payload.as_ref()); + log::error!("{what} dialog: {why}"); + format!("The system's {what} dialog stopped without an answer ({why}). Your project is still open.") + }); + if let Some(done) = take(&worker_done) { + done(paths); + } + worker_ctx.request_repaint(); + }); + if let Err(e) = spawned { + log::error!("file dialog: {e}"); + if let Some(done) = take(&done) { + done(Err(couldnt_open(what, &e.to_string()))); + } + ctx.request_repaint(); + } +} + +/// A dialog on the main window. Linux keeps the portal's dialog unparented: rfd exports the +/// window for the portal from the dialog's worker thread there. +fn parented(frame: &eframe::Frame) -> AsyncFileDialog { + let d = AsyncFileDialog::new(); + if cfg!(target_os = "macos") { d.set_parent(frame) } else { d } +} + +fn paths(files: Option>) -> Vec { + files.unwrap_or_default().into_iter().map(|f| f.path().to_string_lossy().to_string()).collect() +} + +fn path(file: Option) -> Vec { + paths(file.map(|f| vec![f])) +} + +/// The dialog, as the blocking hooks show it. On macOS rfd makes the panel here, before the +/// future is returned, so a panel AppKit can't make panics here, inside [`guarded`]. +/// +/// No blocking fallback when that fails: rfd's app-modal dialog makes its panel with the same +/// `+[NSOpenPanel openPanel]`, so it would fail the same way, and it would bring back the +/// nested modal loop of #511. What [`guarded`] can't reach: rfd's own panics inside AppKit's +/// completion handler (its `URL().unwrap()`, or a poisoned lock of its future's state) run +/// under an Objective-C callback and still abort; only rfd can fix those. +fn open(dialog: FileDialog, frame: &eframe::Frame) -> Answer { + match dialog { + FileDialog::Open { exts, many } => { + let d = parented(frame).add_filter("Media", &exts); + if many { + let f = d.pick_files(); + Box::pin(async move { paths(f.await) }) + } else { + let f = d.pick_file(); + Box::pin(async move { path(f.await) }) + } + } + FileDialog::OpenProject => { + let f = parented(frame).add_filter("EffectCraft Project", &["ecproj", "ecprojx"]).pick_file(); + Box::pin(async move { path(f.await) }) + } + FileDialog::Folder => { + let f = parented(frame).pick_folder(); + Box::pin(async move { path(f.await) }) + } + FileDialog::Save { default, ext, project } => { + let at = std::path::Path::new(&default); + let name = at.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); + let mut d = if project { + parented(frame).add_filter("EffectCraft Project", &["ecproj"]) + } else if ext.is_empty() { + parented(frame) + } else { + parented(frame).add_filter(&ext, &[ext.as_str()]) + }; + // A default with a folder that exists opens the dialog there. + if let Some(dir) = at.parent().filter(|d| d.is_dir()) { + d = d.set_directory(dir); + } + let f = d.set_file_name(name).save_file(); + Box::pin(async move { path(f.await) }) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::mpsc::channel; + use std::time::Duration; + + /// The panic objc2 raises when AppKit returns nil for the panel (#3084). + const NIL_PANEL: &str = "unexpected NULL returned from +[NSOpenPanel openPanel]"; + + fn answered(dialog: FileDialog, open: impl FnOnce(FileDialog) -> Answer) -> Result, String> { + let (tx, rx) = channel(); + show(dialog, Box::new(move |r| tx.send(r).unwrap()), egui::Context::default(), open); + rx.recv_timeout(Duration::from_secs(10)).expect("the dialog answers") + } + + /// #3084: a panel AppKit can't make panics inside rfd; the command's dialog answers with + /// the error instead, and the panic doesn't leave the frame. + #[test] + fn a_panel_that_cannot_be_made_answers_with_an_error() { + let r = answered(FileDialog::Open { exts: vec!["aep".into(), "aepx".into()], many: false }, |_| panic!("{NIL_PANEL}")); + let e = r.unwrap_err(); + assert!(e.starts_with("The system's Open dialog couldn't be created") && e.contains(NIL_PANEL) && e.contains("still open"), "{e}"); + let e = answered(FileDialog::Folder, |_| std::panic::panic_any(7_u8)).unwrap_err(); + assert!(e.contains("Choose Folder") && e.contains("internal error"), "{e}"); + } + + /// A dialog that opens answers with its paths from the worker thread; one whose answer + /// panics answers with an error rather than never. + #[test] + fn a_dialog_that_opens_answers_with_its_paths() { + let r = answered(FileDialog::OpenProject, |_| Box::pin(async { vec!["/a/b.ecproj".to_string()] })); + assert_eq!(r, Ok(vec!["/a/b.ecproj".to_string()])); + assert_eq!(answered(FileDialog::save("Frame.png"), |_| Box::pin(async { Vec::new() })), Ok(Vec::new()), "cancelled"); + fn lost() -> Vec { + panic!("lost the sheet") + } + let e = answered(FileDialog::OpenProject, |_| Box::pin(async { lost() })).unwrap_err(); + assert!(e.contains("stopped without an answer") && e.contains("lost the sheet"), "{e}"); + } + + /// Off the main thread a dialog isn't opened at all (AppKit makes panels on the main thread + /// only); a panic while one opens is an error; an answer passes through. + #[test] + fn guarded_opens_on_the_main_thread_and_catches_panics() { + let mut opened = false; + let e = guarded_on(false, "Open", || opened = true).unwrap_err(); + assert!(!opened && e.contains("off the main thread"), "{e}"); + assert_eq!(guarded_on(true, "Open", || 5), Ok(5)); + let e = guarded_on(true, "Save", || -> u8 { panic!("{NIL_PANEL}") }).unwrap_err(); + assert!(e.contains("Save") && e.contains(NIL_PANEL), "{e}"); + } + + /// A blocking picker that can't open counts as cancelled and reports why. + #[test] + fn a_blocking_picker_that_cannot_open_reports_why() { + let (tx, rx) = channel(); + let tx = Errors { tx, ctx: egui::Context::default() }; + assert_eq!(blocking(&tx, "Open", || -> Option { panic!("{NIL_PANEL}") }), None); + assert!(rx.try_recv().is_ok_and(|e| e.contains(NIL_PANEL))); + assert_eq!(blocking(&tx, "Open", || Some(3)), Some(3)); + assert!(rx.try_recv().is_err(), "nothing to report"); + } + + /// #3084: every rfd dialog in the desktop app goes through this module (and so through + /// [`guarded`]); no other source file opens one directly. + #[test] + fn every_dialog_opens_through_this_module() { + let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut files = vec![]; + let mut dirs = vec![src]; + while let Some(dir) = dirs.pop() { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + dirs.push(path); + } else if path.extension().is_some_and(|e| e == "rs") { + files.push(path); + } + } + } + assert!(files.len() > 3, "{files:?}"); + let exempt = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src").join("file_dialogs.rs"); + assert!(files.contains(&exempt)); + // `rfd` as a word: a path, a `use`, an alias or `extern crate`. + let word = |c: Option| c.is_some_and(|c| c.is_alphanumeric() || c == '_'); + for file in files.iter().filter(|f| **f != exempt) { + let text = std::fs::read_to_string(file).unwrap(); + let named = text.match_indices("rfd").any(|(i, _)| !word(text[..i].chars().next_back()) && !word(text[i + 3..].chars().next())); + assert!(!named, "{}: open dialogs through `file_dialogs` (#3084)", file.display()); + } + } +} diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 5350addbe..592643a3d 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -25,9 +25,10 @@ mod control_server; mod launch_guard; #[cfg(target_os = "macos")] mod native_menu; -// Built everywhere so it stays checked; the macOS and Linux apps use it. +// Every native dialog. Built everywhere; only the macOS and Linux apps show the commands' +// dialogs without blocking the frame. #[cfg_attr(not(any(target_os = "macos", target_os = "linux")), allow(dead_code))] -mod sheet_dialogs; +mod file_dialogs; mod window; use effectcraft_ui_egui::EffectcraftApp; @@ -137,12 +138,12 @@ fn control_port(name: &str, value: Option<&str>) -> Result { /// answer without pumping events, so the window stops answering the compositor and the desktop /// offers to force-quit it while the Open dialog is up (#1275). Windows' blocking dialog pumps /// its own messages, so it keeps [`effectcraft_ui_egui::Hooks::pick_files`] and friends. -/// The dialogs asked for in a frame show at its end ([`sheet_dialogs::show_asked`]), where the +/// The dialogs asked for in a frame show at its end ([`file_dialogs::show_asked`]), where the /// main window's handle is at hand. -fn install_file_dialogs(app: &mut EffectcraftApp) -> Option> { +fn install_file_dialogs(app: &mut EffectcraftApp) -> Option> { #[cfg(any(target_os = "macos", target_os = "linux"))] { - let (tx, rx) = std::sync::mpsc::channel::(); + let (tx, rx) = std::sync::mpsc::channel::(); app.hooks.file_dialog_async = Some(Box::new(move |dialog, done| { if tx.send((dialog, done)).is_err() { log::error!("file dialog: the window is gone"); @@ -334,12 +335,9 @@ fn main() -> eframe::Result { { app.ui.status = note.into(); } - app.hooks.pick_files = Some(Box::new(|exts: &[&str]| { - rfd::FileDialog::new().add_filter("Media", exts).pick_files().unwrap_or_default().into_iter().map(|p| p.to_string_lossy().to_string()).collect() - })); - app.hooks.pick_save = Some(Box::new(|name: &str| { - rfd::FileDialog::new().add_filter("EffectCraft Project", &["ecproj"]).set_file_name(name).save_file().map(|p| p.to_string_lossy().to_string()) - })); + // Every dialog opens through `file_dialogs::guarded`: one that can't open is an error + // in the status line, not a crash (#3084). + let dialog_errors = file_dialogs::install_pickers(&mut app, &cc.egui_ctx); // The window closes after a lost graphics device (#591); say why, unless an agent drives // the app through the control channel. if control_port.is_none() { @@ -349,31 +347,15 @@ fn main() -> eframe::Result { Some(Err(e)) => format!("\n\nYour unsaved changes could not be saved ({e})."), None => String::new(), }; - rfd::MessageDialog::new() - .set_level(rfd::MessageLevel::Error) - .set_title("EffectCraft: graphics device lost") - .set_description(format!("The graphics driver stopped responding, so EffectCraft has to close.{saved}")) - .show(); + file_dialogs::message( + "EffectCraft: graphics device lost", + format!("The graphics driver stopped responding, so EffectCraft has to close.{saved}"), + ); })); } - app.hooks.pick_open_project = Some(Box::new(|| { - rfd::FileDialog::new().add_filter("EffectCraft Project", &["ecproj", "ecprojx"]).pick_file().map(|p| p.to_string_lossy().to_string()) - })); app.hooks.system_theme = appearance::service(); app.hooks.audio_device = Some(Box::new(audio_out::open)); app.hooks.audio_devices = Some(Box::new(audio_out::devices)); - app.hooks.pick_folder = Some(Box::new(|| rfd::FileDialog::new().pick_folder().map(|p| p.to_string_lossy().to_string()))); - app.hooks.pick_save_file = Some(Box::new(|name: &str, ext: &str| { - // A default with a folder that exists opens the dialog there. - let path = std::path::Path::new(name); - // (No filter for a name without an extension, such as a folder to create.) - let mut d = if ext.is_empty() { rfd::FileDialog::new() } else { rfd::FileDialog::new().add_filter(ext, &[ext]) }; - if let Some(dir) = path.parent().filter(|d| d.is_dir()) { - d = d.set_directory(dir); - } - let file = path.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); - d.set_file_name(file).save_file().map(|p| p.to_string_lossy().to_string()) - })); // The commands' file dialogs don't block the frame where the platform's backend // would (#511, #1275); see [`install_file_dialogs`]. let file_dialogs = install_file_dialogs(&mut app); @@ -395,6 +377,7 @@ fn main() -> eframe::Result { apple_events: apple_events.connect(&cc.egui_ctx), app, file_dialogs, + dialog_errors, window: window::Tracker::new(opened, saved_window, control_port.is_none()), launch, drawn: false, @@ -468,19 +451,16 @@ fn startup_failed(e: &eframe::Error, gl_next: bool) { } else { eprintln!("effectcraft: {message}"); } - let _ = rfd::MessageDialog::new() - .set_level(rfd::MessageLevel::Error) - .set_title("EffectCraft") - .set_description(message) - .set_buttons(rfd::MessageButtons::Ok) - .show(); + file_dialogs::message("EffectCraft", message); } /// The desktop app: the egui app plus, on macOS, the system menu bar. struct Desktop { app: EffectcraftApp, /// The commands' file dialogs, shown at the end of the frame that asked for them. - file_dialogs: Option>, + file_dialogs: Option>, + /// Why a blocking picker couldn't open (#3084), for the status line. + dialog_errors: std::sync::mpsc::Receiver, #[cfg(target_os = "macos")] menu: native_menu::NativeBar, /// Documents opened from Finder, the Dock or `open` (macOS). @@ -501,8 +481,11 @@ impl eframe::App for Desktop { self.menu.update(&mut self.app, ctx); self.app.tick(ctx, frame); self.app.finish_ime_logic(ctx); + while let Ok(e) = self.dialog_errors.try_recv() { + self.app.ui.status = e; + } if let Some(asked) = &self.file_dialogs { - sheet_dialogs::show_asked(asked, frame, ctx); + file_dialogs::show_asked(asked, frame, ctx); } effectcraft_ui_egui::panels::unsaved::quit_on_close(ctx); } @@ -510,7 +493,7 @@ impl eframe::App for Desktop { fn ui(&mut self, ui: &mut egui::Ui, frame: &mut eframe::Frame) { self.app.ui(ui, frame); if let Some(asked) = &self.file_dialogs { - sheet_dialogs::show_asked(asked, frame, ui.ctx()); + file_dialogs::show_asked(asked, frame, ui.ctx()); } self.window.frame(ui.ctx(), self.app.session.config.as_deref()); if !self.drawn { diff --git a/apps/effectcraft/src/sheet_dialogs.rs b/apps/effectcraft/src/sheet_dialogs.rs deleted file mode 100644 index 911870190..000000000 --- a/apps/effectcraft/src/sheet_dialogs.rs +++ /dev/null @@ -1,101 +0,0 @@ -//! File dialogs that don't block the frame (macOS and Linux), for `Hooks::file_dialog_async`. -//! -//! A blocking `rfd::FileDialog` runs a nested modal event loop inside eframe's frame, that is -//! inside winit's event handler. On macOS a file dragged over the window while that loop ran -//! re-entered winit's handler, which panics across the Objective-C boundary and aborts the app -//! (#511). On Linux the blocking dialog is worse than slow: rfd uses the XDG desktop portal, so -//! the main thread sits waiting on the portal's answer and never pumps winit's events — the -//! window stops answering the compositor and the desktop offers to force-quit the app while the -//! Open dialog is up (#1275). `rfd::AsyncFileDialog` shows the dialog as a sheet on the window -//! (macOS) or through the portal from a worker thread (Linux) and returns at once; the answer -//! arrives when the dialog closes, outside the frame, and the app runs the command then. - -use effectcraft_ui_egui::{DialogDone, FileDialog}; -use rfd::{AsyncFileDialog, FileHandle}; -use std::future::Future; -use std::pin::Pin; - -type Answer = Pin> + Send>>; - -/// A command's dialog waiting for [`show_asked`]. -pub type Asked = (FileDialog, DialogDone); - -/// Show the dialogs the commands asked for in this frame (on the main thread: a sheet opens -/// here) and hand each one's paths to its `done` once it closes, then wake the UI. -/// -/// On macOS the sheet goes on the main window (`frame`'s). Without a parent rfd puts it on -/// AppKit's main window, which is a floating panel's OS window after a click in one; that -/// window hides while the app has no focused window, and a sheet whose window goes away ends -/// without answering, with later sheets queued behind it on the same window. -pub fn show_asked(asked: &std::sync::mpsc::Receiver, frame: &eframe::Frame, ctx: &egui::Context) { - while let Ok((dialog, done)) = asked.try_recv() { - show(dialog, done, frame, ctx.clone()); - } -} - -fn show(dialog: FileDialog, done: DialogDone, frame: &eframe::Frame, ctx: egui::Context) { - let answer = open(dialog, frame); - let spawned = std::thread::Builder::new().name("ec-file-dialog".into()).spawn(move || { - done(pollster::block_on(answer)); - ctx.request_repaint(); - }); - if let Err(e) = spawned { - log::error!("file dialog: {e}"); - } -} - -/// A dialog on the main window. Linux keeps the portal's dialog unparented: rfd exports the -/// window for the portal from the dialog's worker thread there. -fn parented(frame: &eframe::Frame) -> AsyncFileDialog { - let d = AsyncFileDialog::new(); - if cfg!(target_os = "macos") { d.set_parent(frame) } else { d } -} - -fn paths(files: Option>) -> Vec { - files.unwrap_or_default().into_iter().map(|f| f.path().to_string_lossy().to_string()).collect() -} - -fn path(file: Option) -> Vec { - paths(file.map(|f| vec![f])) -} - -/// The dialog, as the blocking hooks in `main.rs` show it. -fn open(dialog: FileDialog, frame: &eframe::Frame) -> Answer { - match dialog { - FileDialog::Open { exts, many } => { - let d = parented(frame).add_filter("Media", &exts); - if many { - let f = d.pick_files(); - Box::pin(async move { paths(f.await) }) - } else { - let f = d.pick_file(); - Box::pin(async move { path(f.await) }) - } - } - FileDialog::OpenProject => { - let f = parented(frame).add_filter("EffectCraft Project", &["ecproj", "ecprojx"]).pick_file(); - Box::pin(async move { path(f.await) }) - } - FileDialog::Folder => { - let f = parented(frame).pick_folder(); - Box::pin(async move { path(f.await) }) - } - FileDialog::Save { default, ext, project } => { - let at = std::path::Path::new(&default); - let name = at.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); - let mut d = if project { - parented(frame).add_filter("EffectCraft Project", &["ecproj"]) - } else if ext.is_empty() { - parented(frame) - } else { - parented(frame).add_filter(&ext, &[ext.as_str()]) - }; - // A default with a folder that exists opens the dialog there. - if let Some(dir) = at.parent().filter(|d| d.is_dir()) { - d = d.set_directory(dir); - } - let f = d.set_file_name(name).save_file(); - Box::pin(async move { path(f.await) }) - } - } -} diff --git a/crates/ui-egui/src/hooks.rs b/crates/ui-egui/src/hooks.rs index 457aeb99a..7a87eeb6c 100644 --- a/crates/ui-egui/src/hooks.rs +++ b/crates/ui-egui/src/hooks.rs @@ -26,8 +26,11 @@ impl FileDialog { } } -/// Called with the paths a [`FileDialog`] picked (empty: cancelled), from any thread. -pub type DialogDone = Box) + Send>; +/// Called with the paths a [`FileDialog`] picked (empty: cancelled), from any thread, or with +/// why the dialog couldn't open: the status line shows that and the command doesn't run. On +/// macOS AppKit can fail to make the panel at all (`+[NSOpenPanel openPanel]` returned nil, +/// #3084); the host catches that and answers with the error instead of taking the app down. +pub type DialogDone = Box, String>) + Send>; /// Host hooks provided by the native app (file pickers etc.). #[derive(Default)] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e84ef79dd..e46ae6982 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -169,7 +169,8 @@ pub(crate) struct DialogAnswer { pub command: String, pub key: &'static str, pub params: serde_json::Value, - pub paths: Vec, + /// The picked paths (empty: cancelled), or why the dialog couldn't open (#3084). + pub paths: Result, String>, } #[derive(Default)] diff --git a/crates/ui-egui/src/menus/frontend.rs b/crates/ui-egui/src/menus/frontend.rs index bd1294330..e320c540e 100644 --- a/crates/ui-egui/src/menus/frontend.rs +++ b/crates/ui-egui/src/menus/frontend.rs @@ -687,7 +687,7 @@ pub(super) fn file_dialog(app: &mut EffectcraftApp, id: &str, params: &Value) -> Ask::Save(default) => crate::FileDialog::save(&default), }; let tx = app.dialog_answers.0.clone(); - let answer = crate::DialogAnswer { command: id.to_string(), key, params: params.clone(), paths: Vec::new() }; + let answer = crate::DialogAnswer { command: id.to_string(), key, params: params.clone(), paths: Ok(Vec::new()) }; app.file_dialogs.opened(); spawn( request, @@ -737,9 +737,18 @@ fn picked_value(key: &str, paths: Vec) -> Option { } /// A file dialog shown without blocking ([`crate::Hooks::file_dialog_async`]) answered: run its -/// command with the picked path (nothing when it was cancelled). +/// command with the picked path (nothing when it was cancelled). A dialog that couldn't open +/// says why in the status line, and the command doesn't run (#3084). pub(crate) fn dialog_answered(app: &mut EffectcraftApp, answer: crate::DialogAnswer) { - let Some(v) = picked_value(answer.key, answer.paths) else { return }; + let paths = match answer.paths { + Ok(paths) => paths, + // The host logged it. + Err(e) => { + app.ui.status = e; + return; + } + }; + let Some(v) = picked_value(answer.key, paths) else { return }; match run_with_picked(app, &answer.command, answer.key, &answer.params, v) { Ok(result) => show_result_errors(app, &result), Err(e) => app.ui.status = e, diff --git a/crates/ui-egui/src/menus/tests.rs b/crates/ui-egui/src/menus/tests.rs index b12483b0a..89fc2dd8c 100644 --- a/crates/ui-egui/src/menus/tests.rs +++ b/crates/ui-egui/src/menus/tests.rs @@ -384,7 +384,7 @@ fn file_dialogs_can_answer_on_a_later_frame() { std::fs::create_dir_all(&dir).unwrap(); let path = dir.join("Answered.ecproj"); // Answered from another thread, as the host's dialog does. - std::thread::spawn(move || done(vec![path.to_string_lossy().to_string()])).join().unwrap(); + std::thread::spawn(move || done(Ok(vec![path.to_string_lossy().to_string()]))).join().unwrap(); app.run_dialog_answers(); assert_eq!(app.session.path.as_deref(), Some(dir.join("Answered.ecproj").to_string_lossy().as_ref())); assert!(dir.join("Answered.ecproj").is_file()); @@ -392,7 +392,7 @@ fn file_dialogs_can_answer_on_a_later_frame() { invoke(&mut app, &ctx, "comp.saveFrameAs", json!({})).unwrap(); let (dialog, done) = pending.lock().unwrap().pop().expect("a dialog was shown"); assert_eq!(dialog, crate::FileDialog::Save { default: "Frame.png".into(), ext: "png".into(), project: false }); - done(Vec::new()); + done(Ok(Vec::new())); app.run_dialog_answers(); assert!(!dir.join("Frame.png").exists()); let _ = std::fs::remove_dir_all(&dir); @@ -423,7 +423,7 @@ fn file_dialogs_block_floating_windows_only_while_up() { assert!(blocked(&mut app, true, 0.1), "blocked while the dialog takes the keyboard"); assert!(blocked(&mut app, false, 0.2)); let done = pending.lock().unwrap().pop().expect("a dialog was shown"); - done(Vec::new()); + done(Ok(Vec::new())); app.run_dialog_answers(); assert!(!blocked(&mut app, false, 0.3)); // Save As asks again; the sheet ends without answering. @@ -440,7 +440,7 @@ fn file_dialogs_block_floating_windows_only_while_up() { assert!(!blocked(&mut app, true, 12.5)); // Answers of dialogs taken for gone arrive late: nothing stays blocked. for done in pending.lock().unwrap().drain(..) { - done(Vec::new()); + done(Ok(Vec::new())); } app.run_dialog_answers(); assert!(!blocked(&mut app, false, 13.0)); @@ -448,6 +448,71 @@ fn file_dialogs_block_floating_windows_only_while_up() { assert!(blocked(&mut app, false, 13.1), "a later dialog blocks again"); } +/// #3084: a host whose dialog couldn't open (macOS: `+[NSOpenPanel openPanel]` returned nil) +/// answers with the error. The status line says why, the command doesn't run and the dialog +/// no longer blocks floating windows; the next dialog opens as usual. +#[test] +fn a_dialog_that_cannot_open_reports_an_error() { + use std::sync::{Arc, Mutex}; + let mut app = EffectcraftApp::new(effectcraft_engine::Session::default()); + let ctx = egui::Context::default(); + let pending: Arc>> = Arc::default(); + let queue = pending.clone(); + app.hooks.file_dialog_async = Some(Box::new(move |_dialog, done| queue.lock().unwrap().push(done))); + let blocked = |app: &mut EffectcraftApp, focused: bool, time: f64| { + let input = egui::RawInput { focused, time: Some(time), ..Default::default() }; + let mut out = false; + ctx.run_ui(input, |ui| out = app.floats_blocked(ui.ctx())).textures_delta.clear(); + out + }; + let items = app.session.project.items.len(); + assert_eq!(invoke(&mut app, &ctx, "file.importAep", json!({})).unwrap(), json!({"dialog": "file.importAep"})); + assert!(blocked(&mut app, true, 0.1)); + let done = pending.lock().unwrap().pop().expect("a dialog was asked for"); + std::thread::spawn(move || done(Err("The Open dialog couldn't open".into()))).join().unwrap(); + app.run_dialog_answers(); + assert_eq!(app.ui.status, "The Open dialog couldn't open"); + assert_eq!(app.session.project.items.len(), items, "nothing was imported"); + assert!(!blocked(&mut app, false, 0.2), "the failed dialog doesn't block floating windows"); + invoke(&mut app, &ctx, "file.importAep", json!({})).unwrap(); + assert_eq!(pending.lock().unwrap().len(), 1, "the next dialog opens"); +} + +/// Coverage for #3084 (not its regression test, which needs macOS): every enabled File ▸ Import +/// item that asks for a file asks through the host's one dialog path (`Hooks::file_dialog_async` +/// where the host has it), never through a blocking picker; After Effects Project is no +/// different from the others. +#[test] +fn every_import_item_asks_through_the_shared_dialog_path() { + use std::sync::{Arc, Mutex}; + let mut app = EffectcraftApp::new(effectcraft_engine::Session::default()); + let ctx = egui::Context::default(); + app.hooks.pick_files = Some(Box::new(|_: &[&str]| panic!("the blocking picker is not used"))); + app.hooks.pick_open_project = Some(Box::new(|| panic!("the blocking picker is not used"))); + app.hooks.pick_folder = Some(Box::new(|| panic!("the blocking picker is not used"))); + let pending: Arc>> = Arc::default(); + let queue = pending.clone(); + app.hooks.file_dialog_async = Some(Box::new(move |dialog, _done| queue.lock().unwrap().push(dialog))); + // Placeholder and Solid open a form of the app's own and ask for no file. + const IN_APP_FORM: &[&str] = &["file.importPlaceholder", "file.importSolid"]; + let import: Vec<_> = effectcraft_engine::menus::entries() + .into_iter() + .filter(|(path, _)| path.iter().map(String::as_str).eq(["File", "Import"])) + .map(|(_, e)| e) + .filter(|e| app.session.is_enabled(&e.command) && !IN_APP_FORM.contains(&e.command.as_str())) + .collect(); + assert!(import.iter().any(|e| e.command == "file.importAep"), "File ▸ Import has After Effects Project"); + for entry in import { + let r = invoke(&mut app, &ctx, &entry.command, entry.params_or_empty()); + assert_eq!(r, Ok(json!({"dialog": entry.command})), "{}", entry.command); + let dialog = pending.lock().unwrap().pop(); + assert!(dialog.is_some(), "{}: asks through the shared dialog path", entry.command); + if entry.command == "file.importAep" { + assert_eq!(dialog, Some(crate::FileDialog::Open { exts: vec!["aep".into(), "aepx".into()], many: false })); + } + } +} + #[test] fn tga_is_available_in_import_replace_and_proxy_dialogs() { use std::sync::{Arc, Mutex}; @@ -677,7 +742,10 @@ fn opening_a_project_leaves_the_home_screen_whatever_its_composition_id() { assert!(!app.ui.start_screen, "File ▸ Open leaves the Home screen"); // The path picked in a file dialog (`run_with_picked`). app.ui.start_screen = true; - super::frontend::dialog_answered(&mut app, crate::DialogAnswer { command: "file.open".into(), key: "path", params: json!({}), paths: vec![path.clone()] }); + super::frontend::dialog_answered( + &mut app, + crate::DialogAnswer { command: "file.open".into(), key: "path", params: json!({}), paths: Ok(vec![path.clone()]) }, + ); assert_eq!(app.session.active_comp_id(), comp); assert!(!app.ui.start_screen, "a picked project leaves the Home screen"); let _ = std::fs::remove_dir_all(&dir); diff --git a/crates/ui-egui/src/panels/unsaved.rs b/crates/ui-egui/src/panels/unsaved.rs index 7befef4e2..be12668da 100644 --- a/crates/ui-egui/src/panels/unsaved.rs +++ b/crates/ui-egui/src/panels/unsaved.rs @@ -440,7 +440,8 @@ mod tests { 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()] }; + let answer = + crate::DialogAnswer { command: "comp.new".into(), key: "name", params: json!({"width": 64, "height": 64}), paths: Ok(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);