Merge pull request #157 from grovebit/unsaved-changes

Ask before discarding unsaved changes; Save opens Save As for new sessions

apps/soundcraft/src/main.rs

@@ -20,7 +20,7 @@ mod logging;
 mod native_menu;
 
 use soundcraft_engine::Engine;
-use soundcraft_ui_egui::{Services, SoundApp, UiState};
+use soundcraft_ui_egui::{Services, SoundApp, UiState, dialogs};
 use std::sync::Arc;
 
 struct App(SoundApp, Option<&'static logging::AppLogger>, #[cfg(target_os = "macos")] Option<native_menu::NativeMenu>);
@@ -57,6 +57,8 @@ impl eframe::App for App {
         }
         if self.0.quit_requested {
             ctx.send_viewport_cmd(egui::ViewportCommand::Close);
+        } else if ctx.input(|i| i.viewport().close_requested()) && dialogs::ask_to_save(&mut self.0, "app.quit", &serde_json::json!({})) {
+            ctx.send_viewport_cmd(egui::ViewportCommand::CancelClose);
         }
         soundcraft_ui_egui::fit_window_to_screen(ctx);
         let (inner, maximized) = ctx.input(|i| (i.viewport().inner_rect, i.viewport().maximized.unwrap_or(false)));
@@ -100,7 +102,7 @@ impl eframe::App for App {
 fn open_path(app: &mut SoundApp, p: &str) {
     let lower = p.to_ascii_lowercase();
     let r = if lower.ends_with(".scraft") {
-        app.run("session.open", serde_json::json!({"path": p}))
+        dialogs::run_or_ask(app, "session.open", serde_json::json!({"path": p}))
     } else if lower.ends_with(".mid") || lower.ends_with(".midi") || lower.ends_with(".smf") {
         app.run("file.import_midi", serde_json::json!({"path": p}))
     } else {

crates/engine/src/cmd/file.rs

@@ -49,7 +49,7 @@ pub fn specs() -> Vec<CommandSpec> {
             }
             Ok(json!({"path": file}))
         }),
-        cmd!(noundo "session.revert", "Revert Session to Saved...", ["File"], None, "{}", always, |e, _| {
+        cmd!(noundo "session.revert", "Revert Session to Saved...", ["File"], None, "{}", |e| if e.path.is_some() { Ok(()) } else { Err("the session has not been saved".into()) }, |e, _| {
             let path = e.path.clone().ok_or_else(|| bad("session.revert", "the session has not been saved"))?;
             crate::io::open_session(e, &path)?;
             Ok(json!({}))
@@ -470,6 +470,17 @@ mod tests {
         assert!(res["missing"].as_array().is_some_and(|m| !m.is_empty()), "{res}");
         let _ = std::fs::remove_dir_all(&dir);
     }
+
+    #[test]
+    fn saving_a_session_without_audio_creates_its_folder() {
+        let dir = std::env::temp_dir().join(format!("soundcraft-new-folder-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        let path = dir.join("Song").join("Song.scraft");
+        Engine::default().execute("session.save_as", &json!({"path": path.to_string_lossy()})).unwrap();
+        assert!(path.exists());
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
     fn tone_engine() -> (Engine, u64) {
         let mut e = Engine::default();
         e.execute("track.new", &json!({"count": 1, "name": "Tone", "format": "mono"})).unwrap();

crates/engine/src/lib.rs

@@ -18,6 +18,7 @@ pub mod score;
 use serde_json::{Value, json};
 use soundcraft_model::{Clip, Session, TrackId, ZoomState};
 use soundcraft_time::Samples;
+use std::cell::Cell;
 use std::sync::Arc;
 
 pub use cmd::{CommandInfo, CommandSpec, command_specs, find_command};
@@ -111,6 +112,10 @@ pub struct Engine {
     /// Revision counter; bumps on every document change but zoom/scroll (UI caches key on it).
     pub revision: u64,
     dirty: bool,
+    /// The document as last opened or saved, for `is_dirty`.
+    saved: Arc<Session>,
+    /// `is_dirty` as of a revision.
+    dirty_at: Cell<(u64, bool)>,
     /// Gesture key for undo coalescing (see `execute_merged`).
     merge_key: Option<String>,
     /// Messages for the UI status line / agents.
@@ -126,8 +131,10 @@ impl Default for Engine {
 impl Engine {
     pub fn new(session: Session) -> Self {
         install_panic_hook();
+        let doc = Arc::new(session);
         Engine {
-            doc: Arc::new(session),
+            saved: Arc::clone(&doc),
+            doc,
             undo: Vec::new(),
             redo: Vec::new(),
             history_limit: 200,
@@ -138,6 +145,7 @@ impl Engine {
             path: None,
             revision: 1,
             dirty: false,
+            dirty_at: Cell::new((0, false)),
             merge_key: None,
             messages: Vec::new(),
         }
@@ -170,17 +178,30 @@ impl Engine {
         self.transport_requests.clear();
         self.transport_requests.push(TransportRequest::ResetSession);
         self.doc = Arc::new(s);
+        self.saved = Arc::clone(&self.doc);
         self.undo.clear();
         self.redo.clear();
         self.revision = self.revision.wrapping_add(1);
         self.dirty = false;
     }
 
+    /// Whether the document changed since it was opened or saved. Selecting, locating and zooming
+    /// don't count, and undoing back to the saved state makes it clean again.
     pub fn is_dirty(&self) -> bool {
-        self.dirty
+        if !self.dirty {
+            return false;
+        }
+        let (rev, dirty) = self.dirty_at.get();
+        if rev == self.revision {
+            return dirty;
+        }
+        let dirty = !same_document(&self.saved, &self.doc);
+        self.dirty_at.set((self.revision, dirty));
+        dirty
     }
     pub fn mark_clean(&mut self) {
         self.dirty = false;
+        self.saved = Arc::clone(&self.doc);
     }
 
     pub fn can_undo(&self) -> bool {
@@ -335,6 +356,19 @@ impl Engine {
     }
 }
 
+/// Equal apart from what changes without editing: the selection, the playhead and the zoom.
+fn same_document(saved: &Session, doc: &Session) -> bool {
+    let mut doc = doc.clone();
+    let (from, to) = (&saved.edit, &mut doc.edit);
+    to.selection = from.selection;
+    to.timeline_selection = from.timeline_selection;
+    to.selected_tracks.clone_from(&from.selected_tracks);
+    to.selected_clips.clone_from(&from.selected_clips);
+    to.playhead = from.playhead;
+    to.zoom = from.zoom;
+    *saved == doc
+}
+
 fn install_panic_hook() {
     static ONCE: std::sync::Once = std::sync::Once::new();
     ONCE.call_once(|| {
@@ -367,3 +401,27 @@ mod merge_tests {
         assert_eq!(e.session().track(t).unwrap().mixer.volume_db, 0.0);
     }
 }
+
+#[cfg(test)]
+mod dirty_tests {
+    use super::*;
+
+    #[test]
+    fn selecting_locating_and_zooming_leave_the_session_clean() {
+        let mut e = crate::demo::demo_engine();
+        for (id, p) in [
+            ("edit.select", json!({"tracks": ["Kick"], "start": 0, "end": 48_000})),
+            ("edit.select_none", json!({})),
+            ("transport.locate", json!({"at": 96_000})),
+            ("view.zoom_in", json!({})),
+            ("view.scroll", json!({"to": 48_000})),
+        ] {
+            e.execute(id, &p).unwrap();
+            assert!(!e.is_dirty(), "{id}");
+        }
+        e.execute("mix.mute", &json!({"track": "Kick"})).unwrap();
+        assert!(e.is_dirty());
+        e.undo();
+        assert!(!e.is_dirty());
+    }
+}

crates/ui-egui/src/dialogs.rs

@@ -24,11 +24,14 @@ pub enum Dialog {
     Group { name: String, edit: bool, mix: bool, members: Vec<(u64, String, bool)> },
     Session { frame_rate: String, bit_depth: String },
     ScoreSetup { title: String, composer: String, bars_per_system: u32, show_track_names: bool },
+    SaveChanges { then: (String, Value) },
 }
 
 #[derive(Default)]
 pub struct Dialogs {
     pub open: Option<Dialog>,
+    /// Runs once the session has been saved from `SaveChanges`.
+    after_save: Option<(String, Value)>,
 }
 
 impl Dialogs {
@@ -48,6 +51,7 @@ impl Dialogs {
             Dialog::Group { .. } => "group",
             Dialog::Session { .. } => "session",
             Dialog::ScoreSetup { .. } => "score_setup",
+            Dialog::SaveChanges { .. } => "save_changes",
         })
     }
 
@@ -68,6 +72,7 @@ impl Dialogs {
 
     /// Open the dialog for a command; false when the command has no dialog.
     pub fn open_for_command(&mut self, e: &Engine, id: &str) -> bool {
+        self.after_save = None;
         let home = default_dir();
         let name = e.session().name.clone();
         self.open = Some(match id {
@@ -85,8 +90,9 @@ impl Dialogs {
             "file.import_audio" => Dialog::PathPrompt { cmd: id.into(), title: "Import Audio".into(), path: home, key: "path".into() },
             "file.import_midi" => Dialog::PathPrompt { cmd: id.into(), title: "Import MIDI".into(), path: home, key: "path".into() },
             "session.open" => Dialog::PathPrompt { cmd: id.into(), title: "Open Session".into(), path: home, key: "path".into() },
-            "session.save_as" | "session.save_copy" => Dialog::PathPrompt {
-                cmd: id.into(),
+            "session.save" if e.path.is_some() => return false,
+            "session.save" | "session.save_as" | "session.save_copy" => Dialog::PathPrompt {
+                cmd: if id == "session.save_copy" { id } else { "session.save_as" }.into(),
                 title: "Save Session As".into(),
                 // The browser has no folders: the name becomes the bundle file's name.
                 path: if home.is_empty() {
@@ -132,6 +138,32 @@ impl Dialogs {
     }
 }
 
+/// When `id` would close a session with unsaved changes, ask first; true when the question is
+/// open instead. A take in progress is stopped first, so it counts as a change.
+pub fn ask_to_save(app: &mut SoundApp, id: &str, params: &Value) -> bool {
+    if !matches!(id, "session.new" | "session.open" | "session.close" | "session.revert" | "app.quit")
+        || soundcraft_engine::find_command(id).is_some_and(|c| (c.enabled)(&app.engine).is_err())
+    {
+        return false;
+    }
+    if app.engine.transport.recording {
+        app.stop_play();
+    }
+    if !app.engine.is_dirty() {
+        return false;
+    }
+    app.dialogs.open = Some(Dialog::SaveChanges { then: (id.into(), params.clone()) });
+    true
+}
+
+/// Runs `id`, unless it asks to save first (`ask_to_save`).
+pub fn run_or_ask(app: &mut SoundApp, id: &str, params: Value) -> Result<Value, String> {
+    if ask_to_save(app, id, &params) {
+        return Ok(Value::Null);
+    }
+    app.run(id, params)
+}
+
 /// A path prompt for any command whose parameters start with a required `path` or `dir` (plus
 /// Print Score, whose path is optional), with a default file name from the command.
 pub fn takes_path(id: &str) -> bool {
@@ -221,6 +253,8 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) {
     let Some(mut d) = app.dialogs.open.take() else { return };
     let mut keep = true;
     let mut action: Option<(String, Value)> = None;
+    let mut save_first: Option<(String, Value)> = None;
+    let confirmed = matches!(d, Dialog::SaveChanges { .. });
     let title = match &d {
         Dialog::NewTracks { .. } => "New Tracks",
         Dialog::NewSession { .. } => "New Session",
@@ -235,6 +269,8 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) {
         Dialog::Group { .. } => "Create Group",
         Dialog::Session { .. } => "Session Setup",
         Dialog::ScoreSetup { .. } => "Score Setup",
+        Dialog::SaveChanges { then } if then.0 == "session.revert" => "Revert to Saved",
+        Dialog::SaveChanges { .. } => "Unsaved Changes",
     }
     .to_string();
     let enter = ctx.input(|i| i.key_pressed(egui::Key::Enter));
@@ -484,12 +520,54 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) {
                         ));
                     }
                 }
+                Dialog::SaveChanges { then } => {
+                    let name = &app.engine.session().name;
+                    let revert = then.0 == "session.revert";
+                    if revert || cfg!(target_arch = "wasm32") {
+                        let (question, ok) = if revert {
+                            (format!("Revert “{name}” to its last saved version?"), "Revert")
+                        } else {
+                            (format!("Close “{name}” without saving?"), "Don't Save")
+                        };
+                        ui.label(format!("{question} Your changes will be lost."));
+                        if buttons(ui, ok, false) {
+                            action = Some(then.clone());
+                        }
+                    } else {
+                        ui.label(format!("Save changes to “{name}” before closing?"));
+                        ui.separator();
+                        ui.horizontal(|ui| {
+                            if ui.button("Don't Save").clicked() {
+                                action = Some(then.clone());
+                            }
+                            if buttons(ui, "Save", enter) {
+                                save_first = Some(then.clone());
+                            }
+                        });
+                    }
+                }
             }
         });
     if esc || ctx.memory(|m| m.data.get_temp::<bool>(egui::Id::new("dlg_cancel")).unwrap_or(false)) {
         keep = false;
+        app.dialogs.after_save = None;
         ctx.memory_mut(|m| m.data.remove::<bool>(egui::Id::new("dlg_cancel")));
     }
+    if let Some(then) = save_first {
+        keep = false;
+        if !app.dialogs.open_for_command(&app.engine, "session.save") {
+            action = Some(("session.save".into(), json!({})));
+        }
+        app.dialogs.after_save = Some(then);
+    }
+    if let Some((cmd, params)) = &action
+        && !confirmed
+        && ask_to_save(app, cmd, params)
+    {
+        keep = false;
+        action = None;
+    }
+    let saving = action.as_ref().is_some_and(|(cmd, _)| cmd.starts_with("session.save"));
     if let Some((cmd, params)) = action {
         keep = false;
         match app.run(&cmd, params) {
@@ -500,6 +578,13 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) {
             Err(e) => app.ui.status = e,
         }
     }
+    if saving
+        && let Some((then, params)) = app.dialogs.after_save.take()
+        && !app.engine.is_dirty()
+        && let Err(e) = app.run(&then, params)
+    {
+        app.ui.status = e;
+    }
     if keep {
         app.dialogs.open = Some(d);
     }

crates/ui-egui/src/menus.rs

@@ -152,6 +152,7 @@ const DIALOG_COMMANDS: &[&str] = &[
     "file.import_audio",
     "file.import_midi",
     "session.open",
+    "session.save",
     "session.save_as",
     "session.save_copy",
     "track.group",
@@ -223,7 +224,7 @@ pub fn menu_bar(app: &mut SoundApp, ui: &mut egui::Ui) {
                     }
                     ui.separator();
                     if ui.button("Quit").clicked() {
-                        let _ = app.run("app.quit", json!({}));
+                        let _ = crate::dialogs::run_or_ask(app, "app.quit", json!({}));
                     }
                 });
                 let extra = ui_aliases();
@@ -437,12 +438,13 @@ pub fn invoke_menu(app: &mut SoundApp, id: &str, path: &str) {
         return;
     }
     let params = params_for(path, id);
-    let _ = app.run(id, params);
+    let _ = crate::dialogs::run_or_ask(app, id, params);
 }
 
-/// Keyboard shortcut for a dialog command: open its dialog. Returns true when handled.
+/// Keyboard shortcut for a dialog command, or one that would close a modified session: open its
+/// dialog. Returns true when handled.
 pub fn invoke_shortcut_dialog(app: &mut SoundApp, id: &str) -> bool {
-    wants_dialog(id) && app.dialogs.open_for_command(&app.engine, id)
+    (wants_dialog(id) && app.dialogs.open_for_command(&app.engine, id)) || crate::dialogs::ask_to_save(app, id, &json!({}))
 }
 
 /// Commands that open a dialog first: the listed ones, Score Setup, and anything that needs a path.
@@ -656,4 +658,24 @@ mod tests {
         invoke_menu(&mut app, "ui.bounce_dialog", "File > Bounce Mix...");
         assert_eq!(app.dialogs.open_name(), Some("bounce"));
     }
+
+    #[test]
+    fn save_asks_for_a_path_until_the_session_has_one() {
+        let mut app = SoundApp::new(soundcraft_engine::Engine::default(), None, Services::default());
+        invoke_menu(&mut app, "session.save", "File > Save Session");
+        assert!(matches!(&app.dialogs.open, Some(crate::dialogs::Dialog::PathPrompt { cmd, .. }) if cmd == "session.save_as"));
+        app.dialogs.open = None;
+        assert!(invoke_shortcut_dialog(&mut app, "session.save"));
+        app.dialogs.open = None;
+
+        let dir = std::env::temp_dir().join(format!("soundcraft-save-{}", std::process::id()));
+        let path = dir.join("Song.scraft");
+        app.run("session.save_as", json!({"path": path.to_string_lossy()})).unwrap();
+        app.run("track.new", json!({})).unwrap();
+        assert!(!invoke_shortcut_dialog(&mut app, "session.save"));
+        invoke_menu(&mut app, "session.save", "File > Save Session");
+        assert!(app.dialogs.open.is_none());
+        assert!(!app.engine.is_dirty());
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }

crates/ui-egui/src/windows.rs

@@ -350,7 +350,7 @@ fn workspace(app: &mut SoundApp, ctx: &egui::Context) {
                     if is_dir {
                         app.ui.workspace_dir = path;
                     } else if name.ends_with(".scraft") {
-                        let _ = app.run("session.open", json!({"path": path}));
+                        let _ = crate::dialogs::run_or_ask(app, "session.open", json!({"path": path}));
                     } else if name.ends_with(".mid") || name.ends_with(".midi") {
                         let _ = app.run("file.import_midi", json!({"path": path}));
                     } else {

crates/ui-egui/tests/save_changes.rs

@@ -0,0 +1,97 @@
+use egui_kittest::Harness;
+use egui_kittest::kittest::Queryable;
+use serde_json::json;
+use soundcraft_ui_egui::dialogs::Dialog;
+use soundcraft_ui_egui::{Services, SoundApp, menus};
+
+fn harness() -> Harness<'static, SoundApp> {
+    let app = SoundApp::new(soundcraft_engine::demo::demo_engine(), None, Services::default());
+    Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_ui_state(
+        |ui, app: &mut SoundApp| {
+            let ctx = ui.ctx().clone();
+            app.logic(&ctx);
+            app.ui(ui);
+        },
+        app,
+    )
+}
+
+fn steps(h: &mut Harness<'_, SoundApp>) {
+    for _ in 0..4 {
+        h.step();
+    }
+}
+
+fn click(h: &mut Harness<'_, SoundApp>, label: &str) {
+    h.get_by_label(label).click();
+    steps(h);
+}
+
+fn close_session(h: &mut Harness<'_, SoundApp>) {
+    menus::invoke_menu(h.state_mut(), "session.close", "File > Close Session");
+    steps(h);
+}
+
+fn mute_kick(h: &mut Harness<'_, SoundApp>) {
+    h.state_mut().run("mix.mute", json!({"track": "Kick"})).unwrap();
+}
+
+#[test]
+fn closing_a_modified_session_asks_first() {
+    let mut h = harness();
+    let tracks = h.state().engine.session().tracks.len();
+    h.state_mut().run("edit.select", json!({"tracks": ["Kick"], "start": 0, "end": 48_000})).unwrap();
+    close_session(&mut h);
+    assert!(h.state().dialogs.open.is_none(), "a selection alone is not a change");
+    assert!(h.state().engine.session().tracks.is_empty());
+
+    let mut h = harness();
+    mute_kick(&mut h);
+    close_session(&mut h);
+    assert_eq!(h.state().dialogs.open_name(), Some("save_changes"));
+    click(&mut h, "Cancel");
+    assert!(h.state().dialogs.open.is_none());
+    assert_eq!(h.state().engine.session().tracks.len(), tracks);
+
+    close_session(&mut h);
+    click(&mut h, "Don't Save");
+    assert!(h.state().engine.session().tracks.is_empty());
+
+    let mut h = harness();
+    mute_kick(&mut h);
+    menus::invoke_menu(h.state_mut(), "app.quit", "");
+    steps(&mut h);
+    assert!(!h.state().quit_requested);
+    click(&mut h, "Don't Save");
+    assert!(h.state().quit_requested);
+}
+
+#[test]
+fn save_in_the_question_saves_before_closing() {
+    let dir = std::env::temp_dir().join(format!("soundcraft-save-changes-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    let path = dir.join("Song.scraft").to_string_lossy().into_owned();
+
+    let mut h = harness();
+    mute_kick(&mut h);
+    close_session(&mut h);
+    click(&mut h, "Save");
+    match &mut h.state_mut().dialogs.open {
+        Some(Dialog::PathPrompt { cmd, path: p, .. }) if cmd == "session.save_as" => p.clone_from(&path),
+        other => panic!("expected Save As, got {other:?}"),
+    }
+    steps(&mut h);
+    click(&mut h, "OK");
+    assert!(h.state().engine.session().tracks.is_empty());
+
+    h.state_mut().run("session.open", json!({"path": path})).unwrap();
+    assert!(h.state().engine.session().track_by_name("Kick").unwrap().mixer.mute);
+    h.state_mut().run("mix.mute", json!({"track": "Kick"})).unwrap();
+    close_session(&mut h);
+    click(&mut h, "Save");
+    assert!(h.state().dialogs.open.is_none());
+    assert!(h.state().engine.session().tracks.is_empty());
+    h.state_mut().run("session.open", json!({"path": path})).unwrap();
+    assert!(!h.state().engine.session().track_by_name("Kick").unwrap().mixer.mute);
+    let _ = std::fs::remove_dir_all(&dir);
+}