Merge pull request #2006 from ssandweiss/fix/1943-macos-cmd-q-save-prompt

fix(macos): ask to save on Cmd-Q instead of quitting outright (#1943)

apps/filmcraft/src/native_menu.rs

@@ -19,6 +19,15 @@ fn accel(s: &str) -> Option<Accelerator> {
     mapped.parse().ok()
 }
 
+/// The app menu's Quit item: the `app.quit` command (which asks to save unsaved changes) on its
+/// shortcut, Cmd+Q unless remapped in the Keyboard Shortcuts editor. The key equivalent is read
+/// once, when the menu bar is built: `app.quit` has no menu path, so it is not in the menu tree the
+/// updater receives, and a remap shows on the Quit item after the next launch.
+fn quit_item(app: &FilmcraftApp) -> (&'static str, Option<Accelerator>) {
+    let keys = app.session.shortcuts.primary("app.quit").unwrap_or_else(|| "Cmd+Q".into());
+    ("app.quit", accel(&keys))
+}
+
 /// Updates native item labels, key equivalents and enablement when the menu tree changes.
 pub type MenuUpdater = Box<dyn FnMut(&[Item])>;
 
@@ -53,9 +62,12 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Men
     let predefined = [
         (hide_label, PredefinedMenuItem::hide(Some(app.ui.language.tr(hide_label)))),
         (hide_others_label, PredefinedMenuItem::hide_others(Some(app.ui.language.tr(hide_others_label)))),
-        (quit_label, PredefinedMenuItem::quit(Some(app.ui.language.tr(quit_label)))),
     ];
-    let [(_, hide), (_, hide_others), (_, quit)] = &predefined;
+    let [(_, hide), (_, hide_others)] = &predefined;
+    // Quit is ours, not the predefined item: that one calls `terminate:` and skips the unsaved
+    // changes prompt (#1943); the `app.quit` command closes the window through it
+    let (quit_id, quit_accel) = quit_item(app);
+    let quit = MenuItem::with_id(quit_id, app.ui.language.tr(quit_label), true, quit_accel);
     let _ = app_menu.append_items(&[
         &about,
         &discord,
@@ -65,7 +77,7 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Men
         hide,
         hide_others,
         &PredefinedMenuItem::separator(),
-        quit,
+        &quit,
     ]);
     let _ = bar.append(&app_menu);
     for top in MENUS {
@@ -147,6 +159,7 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Men
         for (label, item) in &predefined {
             item.set_text(language.tr(label));
         }
+        quit.set_text(language.tr(quit_label));
         for (shown, mi) in &mut native {
             let Some(it) = items.iter().find(|i| i.id == shown.id) else { continue };
             if it.label != shown.label {
@@ -164,3 +177,20 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Men
     });
     (rx, update)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use filmcraft_engine::Session;
+
+    /// #1943: Cmd+Q must reach the `app.quit` command, which closes the window through the unsaved
+    /// changes prompt, instead of the predefined Quit item's `terminate:`.
+    #[test]
+    fn quit_item_runs_the_app_quit_command_on_cmd_q() {
+        let app = FilmcraftApp::new(Session::default());
+        let (id, accelerator) = quit_item(&app);
+        assert_eq!(id, "app.quit");
+        assert_eq!(accelerator, accel("Cmd+Q"));
+        assert!(accelerator.is_some(), "Cmd+Q is a native key equivalent");
+    }
+}

crates/ui-egui/src/lib.rs

@@ -1821,7 +1821,8 @@ impl FilmcraftApp {
         self.handle_drops(&ctx);
         if let Some(rx) = self.command_inbox.take() {
             while let Ok(id) = rx.try_recv() {
-                if self.control_input_only {
+                // control-only mode ignores native menu commands, but Quit stays available (#1943)
+                if self.control_input_only && id != "app.quit" {
                     continue;
                 }
                 if let Err(e) = menus::invoke(self, &ctx, &id, json!({})) {

crates/ui-egui/tests/clip_dialogs_ui.rs

@@ -347,6 +347,26 @@ fn quitting_with_unsaved_changes_asks_to_save() {
     assert!(!d.sent(&egui::ViewportCommand::CancelClose), "the confirmed quit goes through");
 }
 
+/// #1943: the macOS menu bar's Quit (Cmd+Q) sends `app.quit` to the command inbox; with unsaved
+/// changes that closes the window through the prompt instead of quitting outright.
+#[test]
+fn native_menu_quit_with_unsaved_changes_asks_to_save() {
+    let mut d = Driver::new();
+    d.edit();
+    let (tx, rx) = channel();
+    d.app().command_inbox = Some(rx);
+    tx.send("app.quit".to_string()).unwrap();
+    d.frames(1);
+    assert!(d.sent(&egui::ViewportCommand::Close), "Quit asks the window to close");
+    assert!(!d.app().quit_confirmed, "nothing is confirmed yet");
+    // the window's close request is cancelled and the prompt asks instead
+    d.request_close();
+    assert!(d.sent(&egui::ViewportCommand::CancelClose), "the window must stay open");
+    d.frames(2);
+    assert!(d.has("quit.save") && d.has("quit.dontSave") && d.has("quit.cancel"));
+    assert!(d.app().session.is_dirty());
+}
+
 #[test]
 fn quit_save_on_a_new_project_uses_the_save_dialog_and_scripted_quit_does_not_ask() {
     let path = std::env::temp_dir().join(format!("fc-quit-save-{}.fcproj", std::process::id()));

crates/ui-egui/tests/ui_control_input.rs

@@ -275,3 +275,22 @@ fn modified_control_drag_keeps_shift_until_release_and_clears_it_afterward() {
     d.frames(3, false);
     assert_eq!(d.h.ctx.input(|input| input.modifiers), Modifiers::NONE);
 }
+
+/// CodeRabbit on #2006: in control-only mode the native menu's commands are ignored, but its Quit
+/// item (now `app.quit` through the command inbox, #1943) must still close the window, as the
+/// predefined Quit item did.
+#[test]
+fn native_quit_still_quits_in_control_only_mode() {
+    let mut d = Driver::new(true);
+    let (tx, rx) = channel();
+    d.h.state_mut().command_inbox = Some(rx);
+    // another menu command is ignored
+    let project = d.h.state().session.project.clone();
+    tx.send("file.newBin".to_string()).unwrap();
+    d.frames(2, true);
+    assert!(Arc::ptr_eq(&project, &d.h.state().session.project), "menu commands do not run in control-only mode");
+    tx.send("app.quit".to_string()).unwrap();
+    d.frames(1, true);
+    let closing = d.h.output().viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(&egui::ViewportCommand::Close));
+    assert!(closing, "Quit asks the window to close");
+}