From 1580ebb520c9e22d0f94047db0656d2a84d07fca Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sat, 10 Oct 2026 21:33:45 +0200 Subject: [PATCH] #266: quitting with unsaved changes asks to save first Problem: Quit, Cmd+Q or the window's close button exited at once with unsaved changes, leaving them to the recovery journal, so every launch after an unsaved session offered a recovery (docs/project-files.md: no "Save changes?" prompt on quit yet). Fix: a close request with unsaved changes is cancelled and the existing Save Project prompt asks Cancel / Don't Save / Save (automation ids quit.*), as in Premiere. Don't Save discards the changes and their recovery snapshot; Save on a never-saved project (here and in Close Project) opens the Save dialog and returns to the prompt when cancelled. The control channel's app.quit still quits without asking. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/control.rs | 2 + crates/ui-egui/src/lib.rs | 8 +++ crates/ui-egui/src/panels/clip_dialogs.rs | 60 +++++++++++++++---- crates/ui-egui/tests/clip_dialogs_ui.rs | 72 +++++++++++++++++++++++ docs/control-protocol.md | 2 +- docs/project-files.md | 8 ++- 6 files changed, 135 insertions(+), 17 deletions(-) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 3cd86c35d..cd4084815 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -429,6 +429,8 @@ pub fn handle(app: &mut FilmcraftApp, ctx: &egui::Context, req: &ControlRequest) ok(Value::Null) } "app.quit" => { + // scripted quits don't wait on the save prompt; unsaved changes stay in the recovery journal + app.quit_confirmed = true; ctx.send_viewport_cmd(egui::ViewportCommand::Close); ok(Value::Null) } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 0faa82e1b..4c5f4ab03 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -291,6 +291,9 @@ pub struct FilmcraftApp { /// Fault injection for robustness tests: the next UI pass panics. #[doc(hidden)] pub panic_next_frame: bool, + /// The next close request quits without asking to save (Save / Don't Save was chosen in the + /// quit prompt, or the control channel's `app.quit`). + pub quit_confirmed: bool, fonts_ready: bool, pub integrated_titlebar: bool, pub last_timeline_width: f32, @@ -511,6 +514,7 @@ impl FilmcraftApp { styled: false, ui_error: None, panic_next_frame: false, + quit_confirmed: false, fonts_ready: false, integrated_titlebar: false, last_timeline_width: 1000.0, @@ -1882,6 +1886,10 @@ impl eframe::App for FilmcraftApp { self.playback.hidden = true; } self.timeline_still = if self.ui.timeline.animating() { 0 } else { self.timeline_still.saturating_add(1) }; + // Quitting with unsaved changes asks to save them first. + if ctx.input(|i| i.viewport().close_requested()) && panels::clip_dialogs::intercept_quit(self) { + ctx.send_viewport_cmd(egui::ViewportCommand::CancelClose); + } let had_synthetic = !self.synthetic.is_empty(); self.drain_control(ctx); if !self.synthetic.is_empty() && !had_synthetic { diff --git a/crates/ui-egui/src/panels/clip_dialogs.rs b/crates/ui-egui/src/panels/clip_dialogs.rs index 104961e35..2d5b8b536 100644 --- a/crates/ui-egui/src/panels/clip_dialogs.rs +++ b/crates/ui-egui/src/panels/clip_dialogs.rs @@ -1,7 +1,7 @@ //! Edit / Clip / File menu dialogs (M3.10): Paste Attributes, Remove Attributes, Offline File, //! Make Subclip, Edit Subclip, Modify ▸ Audio Channels, Modify ▸ Timecode, Frame Hold Options, -//! Field Options, Clip Speed / Duration, Nest… (Nested Sequence Name), and the Close Project -//! "save changes?" prompt. +//! Field Options, Clip Speed / Duration, Nest… (Nested Sequence Name), and the Close Project and +//! Quit "save changes?" prompts. //! //! The open dialog lives in `UiState::clip_dialog` (serde): the engine command it runs and the //! parameters being edited — the same JSON the command takes — so agents can open a dialog from @@ -26,7 +26,9 @@ //! - Field Options `fieldOptions.*`: `reverseFieldDominance`, `processing.`; //! - Clip Speed / Duration `speedDuration.*`: `speed`, `reverse`, `ripple`, //! `interpolation.`; -//! - Close Project `closeProject.save`, `closeProject.dontSave`, `closeProject.cancel`. +//! - Close Project `closeProject.save`, `closeProject.dontSave`, `closeProject.cancel`; +//! - Quit (asked when the window is closed with unsaved changes, see [`intercept_quit`]) +//! `quit.save`, `quit.dontSave`, `quit.cancel`. use egui::{Align2, RichText}; use filmcraft_project::{AudioChannelMap, AudioChannels, ItemKind, TrackKind}; @@ -56,6 +58,7 @@ fn meta(command: &str) -> Option<(&'static str, &'static str)> { "clip.fieldOptions" => (tl!("Field Options"), "fieldOptions"), "clip.speedDuration" => (tl!("Clip Speed / Duration"), "speedDuration"), "file.closeProject" => (tl!("Save Project"), "closeProject"), + "app.quit" => (tl!("Save Project"), "quit"), _ => return None, }) } @@ -89,7 +92,7 @@ pub fn route(app: &mut FilmcraftApp, id: &str, params: &Value) -> Option Option bool { + if app.quit_confirmed || !app.session.is_dirty() { + return false; + } + if app.ui.clip_dialog.as_ref().is_none_or(|d| d.command != "app.quit") { + app.ui.clip_dialog = Some(ClipDialogDraft { command: "app.quit".into(), params: json!({}), info: Value::Null, error: String::new() }); + } + true +} + /// Initial parameters (and display info) of a command's dialog. fn defaults(app: &FilmcraftApp, id: &str) -> (Value, Value) { let s = &app.session; @@ -290,7 +306,8 @@ pub fn show(app: &mut FilmcraftApp, ctx: &egui::Context) { let mut elems: Elems = Vec::new(); let mut action: Option<&'static str> = None; let project_name = app.session.project.name.clone(); - let can_save = app.session.path.is_some(); + // a never-saved project is saved through the Save dialog + let can_save = app.session.path.is_some() || app.hooks.pick_save.is_some(); // the title is translated; the window keeps one id whatever the interface language let id = egui::Id::new(("clip-dialog", pre)); egui::Window::new(title).id(id).collapsible(false).resizable(false).default_width(360.0).anchor(Align2::CENTER_CENTER, [0.0, 0.0]).show(ctx, |ui| { @@ -520,7 +537,7 @@ pub fn show(app: &mut FilmcraftApp, ctx: &egui::Context) { } } } - "file.closeProject" => { + "file.closeProject" | "app.quit" => { ui.label(tlf!("Save changes to “{project_name}” before closing?", project_name)); ui.add_space(8.0); ui.horizontal(|ui| { @@ -586,16 +603,33 @@ pub fn show(app: &mut FilmcraftApp, ctx: &egui::Context) { } } Some("dontSave") | Some("save") => { - if action == Some("save") - && let Err(e) = app.session.execute("file.save", json!({})) - { - app.ui.status = e.to_string(); - app.ui.clip_dialog = None; - return; + if action == Some("save") { + let saved = if app.session.path.is_some() { + app.session.execute("file.save", json!({})).map(|_| ()).map_err(|e| e.to_string()) + } else { + app.file_dialog("file.save", &json!({})).map(|_| ()) + }; + if let Err(e) = saved { + app.ui.status = e; + app.ui.clip_dialog = None; + return; + } + // the Save dialog was cancelled: back to the prompt + if app.session.is_dirty() { + app.ui.clip_dialog = Some(d); + return; + } } - if let Err(e) = app.session.execute("file.closeProject", json!({"force": true})) { + // Quit ▸ Don't Save discards the changes: closing the project also clears their recovery + // snapshot, so the next launch doesn't offer them back + let close = d.command != "app.quit" || app.session.is_dirty(); + if close && let Err(e) = app.session.execute("file.closeProject", json!({"force": true})) { app.ui.status = e.to_string(); } + if d.command == "app.quit" { + app.quit_confirmed = true; + ctx.send_viewport_cmd(egui::ViewportCommand::Close); + } app.ui.clip_dialog = None; return; } diff --git a/crates/ui-egui/tests/clip_dialogs_ui.rs b/crates/ui-egui/tests/clip_dialogs_ui.rs index d02c16667..b60493718 100644 --- a/crates/ui-egui/tests/clip_dialogs_ui.rs +++ b/crates/ui-egui/tests/clip_dialogs_ui.rs @@ -88,6 +88,25 @@ impl Driver { fn item_named(&mut self, name: &str) -> u64 { self.app().session.project.items.values().find(|i| i.name == name).unwrap().id.0 } + + /// The window is asked to close (Quit, Cmd+Q, its close button) for one frame. + fn request_close(&mut self) { + self.harness.input_mut().viewports.entry(egui::ViewportId::ROOT).or_default().events.push(egui::ViewportEvent::Close); + self.frames(1); + } + + /// The app sent `cmd` to the window in the last frame. + fn sent(&self, cmd: &egui::ViewportCommand) -> bool { + self.harness.output().viewport_output.get(&egui::ViewportId::ROOT).is_some_and(|v| v.commands.contains(cmd)) + } + + /// Make an unsaved change. + fn edit(&mut self) { + let c = self.v1(0); + self.exec("timeline.select", json!({"clips": [c.id.0]})); + self.menu("edit.label.yellow"); + assert!(self.app().session.is_dirty()); + } } #[test] @@ -269,3 +288,56 @@ fn speed_duration_from_the_clip_menu_opens_the_dialog() { assert_eq!(got.speed, 2.0); assert_eq!(got.time_interpolation, filmcraft_project::TimeInterpolation::FrameBlending); } + +#[test] +fn quitting_with_unsaved_changes_asks_to_save() { + let mut d = Driver::new(); + d.edit(); + // the close 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")); + // Cancel keeps the app and the changes + d.click("quit.cancel"); + assert!(d.app().ui.clip_dialog.is_none()); + assert!(d.app().session.is_dirty() && !d.app().quit_confirmed); + d.request_close(); + assert!(d.sent(&egui::ViewportCommand::CancelClose), "asks again"); + d.frames(2); + // Don't Save discards the changes (and their recovery snapshot) and closes the window + d.click("quit.dontSave"); + assert!(d.app().quit_confirmed && !d.app().session.is_dirty()); + assert!(d.app().ui.clip_dialog.is_none()); + d.request_close(); + assert!(!d.sent(&egui::ViewportCommand::CancelClose), "the confirmed quit goes through"); +} + +#[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())); + let _ = std::fs::remove_file(&path); + let mut d = Driver::new(); + d.app().session.path = None; + d.edit(); + let picked = path.to_string_lossy().into_owned(); + let mut answers = vec![None, Some(picked)]; + d.app().hooks.pick_save = Some(Box::new(move |_| if answers.is_empty() { None } else { answers.remove(0) })); + d.request_close(); + d.frames(2); + // the Save dialog is cancelled: back to the prompt, nothing quits + d.click("quit.save"); + assert!(d.app().ui.clip_dialog.is_some() && !d.app().quit_confirmed); + // saved: the project is written and the window closes + d.click("quit.save"); + assert!(d.app().quit_confirmed && !d.app().session.is_dirty()); + assert!(path.exists(), "saved to the chosen file"); + let _ = std::fs::remove_file(&path); + d.request_close(); + assert!(!d.sent(&egui::ViewportCommand::CancelClose)); + // the control channel's app.quit quits without the prompt + let mut d = Driver::new(); + d.edit(); + d.ok("app.quit", json!({})); + assert!(d.app().quit_confirmed); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 28b86c30a..2a287a88a 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -38,7 +38,7 @@ Methods (handlers in `crates/ui-egui/src/control.rs`): | `ui.timeline.hit` / `ui.timeline.locate` | `{x,y,modifiers?}` / `{clip, edge?}` | timeline hit-testing. `hit` reports a clip's `edge` (`In`/`Out`) within 7 px of it (a third of a narrow clip); just left of a cut that is the left clip's Out, from the cut rightwards the right clip's In. `kind` is the trim a press of the current tool with `modifiers` starts there (`trim`, `ripple`, `roll`, `rateStretch`, `remix`), `volume` on an audio clip's Volume line (Selection and Pen tools), or null | | `ui.playback` | `{action: play|stop|toggle, speed?}` | | | `ui.screenshot` | `{path?, panel?}` | PNG of the window or one panel; fails after 10 s when no frame is presented (window hidden, display asleep) | -| `ui.resize`, `ui.focus`, `app.quit` | | `ui.focus` is the only request that activates the app and takes keyboard focus | +| `ui.resize`, `ui.focus`, `app.quit` | | `ui.focus` is the only request that activates the app and takes keyboard focus. `app.quit` quits without the save prompt (unsaved changes stay in the recovery journal); the command `app.quit` (Quit, Cmd+Q) asks first | | `perf.stats` | – | performance counters (also the command `perf.stats`, so `engine.execute` and MCP `command_run` reach it; headless sessions return the engine part). `decode`: GOP-cache requests / hits / `cacheHitRate`, decoder seeks, samples decoded and skipped while catching up, `draftFrames` (decoded in draft mode), `h264Threads` (frame-threading workers per H.264 decoder), `liveDecoders` (sources holding a decoder now), `cacheMB` / `cacheBudgetMB` (decoded frames all sources hold, and the shared budget), `planePoolMB` / `planesReused` (idle buffers of the frame pool, the planes of evicted frames and the compositor's float images, which a standalone export frees when it ends; and buffers taken from it instead of allocated), decoder ms (total and per sample), `framesDecoded`, `hardware` (Settings ▸ Playback ▸ Hardware decoding: `enabled`, pictures from hardware decoders `frames` vs `softwareFrames`, `sessions`, `declined` streams, mid-stream `fallbacks`); `playback`: playing, shown / dropped frames and `dropRate` of the current or last play, resolution, `draftDecode` (Settings ▸ Playback ▸ Draft decoding), `audio` (desktop: sound is mixed 200 ms ahead of the device; device `callbacks`, `underruns` and `missingFrames` played as silence because the mixer fell behind, `minLeadMs` the least sound left buffered); `frames`: frame-worker jobs, cancelled, `requestHitRate`, `decodeMs` / `renderMs` (mean, p50, p95 of the last 256 jobs), queue, render-cost estimate, cache use; `monitor`: the `frame` and `revision` of the picture the Program monitor shows against `playheadFrame` and `projectRevision` (how far a drag or a scrub is ahead of the picture); `ui`: fps and frame ms; `process`: CPU seconds; `media`, `jobs`. Counters are cumulative: diff two readings to measure an interval ([performance.md](performance.md)) | **Timeline thumbnails.** `ui.set {timeline: {thumbnails}}` sets the Timeline wrench menu's Show Video diff --git a/docs/project-files.md b/docs/project-files.md index 5c1b08fdb..9f5be7285 100644 --- a/docs/project-files.md +++ b/docs/project-files.md @@ -152,9 +152,11 @@ Premiere's auto-save can lose up to one interval of work. FilmCraft also keeps a over the original file. The recovered state goes into the new session's own journal before the old journal is deleted. - A normal quit with nothing unsaved removes the session directory. Quitting with unsaved changes - keeps the snapshot, marked `cleanExit`, and FilmCraft offers it on the next launch ("FilmCraft was - closed while … had unsaved changes"). There is no "Save changes?" prompt on quit yet, so this - keeps the work. + (Quit, Cmd+Q or the window's close button) first asks *"Save changes to 'X' before closing?"* + with **Cancel**, **Don't Save** and **Save** (Save on a never-saved project opens the Save dialog); + Don't Save discards the changes and their snapshot. A quit that does not ask (the control + channel's `app.quit`, the OS ending the session) keeps the snapshot, marked `cleanExit`, and FilmCraft offers it on the next launch ("FilmCraft was + closed while … had unsaved changes"). Desktop flags: `--recover` recovers the newest candidate without asking. `--no-recover` starts without asking; the changes stay available through File ▸ Recover Unsaved Changes…. `--data-dir