Merge pull request #1176 from tylerjenningsw/fix/report-workspace-save-errors

crates/ui-egui/src/lib.rs

@@ -430,9 +430,17 @@ impl EffectcraftApp {
     }
 
     /// Keep the saved workspaces for the next session (see [`EffectcraftApp::new`]): After
-    /// Effects' saved workspaces last until they are deleted (#320).
-    pub(crate) fn store_saved_workspaces(&mut self) {
-        self.save_workspace_state();
+    /// Effects' saved workspaces last until they are deleted (#320). `Err` when they weren't
+    /// written (#696): the change applies for this session and a later save retries it, but until
+    /// then a restart brings back what is on disk.
+    pub(crate) fn store_saved_workspaces(&mut self) -> Result<(), String> {
+        let Err(error) = self.workspace_store.save_now(self.session.config.as_deref(), &self.ui) else { return Ok(()) };
+        let message = format!("Workspace changes couldn't be saved and may be lost after a restart: {error}");
+        log::warn!("{message}");
+        self.ui.status = message.clone();
+        // The raw error, so the background retry doesn't report the same failure again.
+        self.workspace_store.error = Some(error);
+        Err(message)
     }
 
     /// Offer the frontend's commands (tools, timeline navigation…) for keyboard shortcuts.

crates/ui-egui/src/menus.rs

@@ -839,18 +839,26 @@ fn clipboard_note(s: &effectcraft_engine::Session) -> String {
 }
 
 pub(crate) fn run_engine(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: Value) -> Result<Value, String> {
-    let r = app.session.execute(id, params).map_err(|e| e.to_string());
+    // Events already queued belong to earlier invocations (`engine.execute` defers them).
+    let queued = app.session.events.len();
+    let mut r = app.session.execute(id, params).map_err(|e| e.to_string());
     if let Err(e) = &r {
         app.ui.status = e.clone();
     } else if COPY_COMMANDS.contains(&id) && app.session.state.text_edit.is_none() {
         ctx.copy_text(clipboard_note(&app.session));
     }
     let events = app.session.drain_events();
-    for ev in events {
+    for (index, ev) in events.into_iter().enumerate() {
         match ev {
             effectcraft_engine::Event::Frontend { command, params } => {
                 if let Err(e) = frontend(app, ctx, &command, params) {
-                    app.ui.status = e;
+                    app.ui.status = e.clone();
+                    // A frontend-only command's own failure is the command's result (#696: a
+                    // workspace save that didn't reach disk reported success). An earlier
+                    // invocation's failure only sets the status.
+                    if index >= queued && command == id && r.is_ok() {
+                        r = Err(e);
+                    }
                 }
             }
             other => app.session.events.push(other),
@@ -1099,7 +1107,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
             let name = app.ui.workspace.clone();
             app.ui.saved_workspaces.insert(name.clone(), app.ui.dock.clone());
             app.ui.saved_floating.insert(name.clone(), app.ui.floating.clone());
-            app.store_saved_workspaces();
+            app.store_saved_workspaces()?;
             json!({"workspace": name})
         }
         "window.saveWorkspaceAs" => {
@@ -1116,7 +1124,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
             app.ui.saved_workspaces.insert(name.clone(), app.ui.dock.clone());
             app.ui.saved_floating.insert(name.clone(), app.ui.floating.clone());
             app.ui.workspace = name.clone();
-            app.store_saved_workspaces();
+            app.store_saved_workspaces()?;
             json!({"workspace": name})
         }
         "window.editWorkspaces" => {
@@ -1141,10 +1149,11 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
                 if app.ui.saved_workspaces.remove(&name).is_none() && !builtin {
                     return Err(format!("no workspace `{name}`"));
                 }
-                app.store_saved_workspaces();
+                // Leave the deleted workspace first: a current workspace needs a definition.
                 if app.ui.workspace == name {
                     app.set_workspace("Default");
                 }
+                app.store_saved_workspaces()?;
                 return Ok(json!({"deleted": name}));
             }
             if let Some(new) = p.get("rename").and_then(Value::as_str).map(str::trim).filter(|n| !n.is_empty() && *n != name) {
@@ -1159,7 +1168,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
                 if app.ui.workspace == name {
                     app.ui.workspace = new.to_string();
                 }
-                app.store_saved_workspaces();
+                app.store_saved_workspaces()?;
                 return Ok(json!({"renamed": new}));
             }
             json!({"workspaces": app.workspace_names()})

crates/ui-egui/src/native_menu.rs

@@ -429,6 +429,7 @@ mod tests {
     #[test]
     fn saved_workspaces_are_in_the_native_workspace_menu() {
         let mut app = app();
+        app.session.config = Some(std::sync::Arc::new(effectcraft_engine::config::MemoryConfig::default()));
         let ctx = egui::Context::default();
         crate::menus::invoke(&mut app, &ctx, "window.saveWorkspaceAs", json!({"name": "My Layout"})).unwrap();
         let entry = |app: &EffectcraftApp| build(app).items().into_iter().find(|i| i.label == "My Layout").cloned();

crates/ui-egui/src/workspace_state.rs

@@ -263,6 +263,19 @@ impl WorkspaceStore {
         self.error = None;
         Ok(true)
     }
+
+    /// An explicit save (Save Workspace, Save as New Workspace, Edit Workspaces): unlike
+    /// [`Self::save`], no settings folder or a file blocked at load is an error, not "nothing to do".
+    pub(crate) fn save_now(&mut self, config: Option<&dyn ConfigStore>, ui: &UiState) -> Result<(), String> {
+        let Some(config) = config else {
+            return Err("there is no settings folder to save them in".into());
+        };
+        if !self.writable {
+            let path = config.dir().map_or_else(|| FILE.to_string(), |dir| dir.join(FILE).display().to_string());
+            return Err(format!("{path} couldn't be loaded at startup, so it is left untouched; fix or move it and restart"));
+        }
+        self.save(Some(config), ui).map(|_| ())
+    }
 }
 
 #[cfg(test)]
@@ -283,10 +296,31 @@ mod tests {
             ui.focused = PanelKind::Project;
             ui.dock = crate::dock::workspace("Minimal");
             assert!(!store.save(Some(&config), &ui).unwrap());
+            assert!(store.save_now(Some(&config), &ui).unwrap_err().contains("couldn't be loaded"));
             assert_eq!(config.read(FILE).as_deref(), Some(text.as_str()));
         }
     }
 
+    #[test]
+    fn explicit_save_without_a_settings_folder_is_an_error() {
+        let mut ui = UiState::default();
+        let mut store = WorkspaceStore::load(None, &mut ui);
+        ui.focused = PanelKind::Project;
+        assert!(!store.save(None, &ui).unwrap());
+        assert!(store.save_now(None, &ui).is_err());
+    }
+
+    #[test]
+    fn explicit_save_with_nothing_new_succeeds() {
+        let config = MemoryConfig::default();
+        let mut ui = UiState::default();
+        let mut store = WorkspaceStore::load(Some(&config), &mut ui);
+        ui.focused = PanelKind::Project;
+        store.save_now(Some(&config), &ui).unwrap();
+        store.save_now(Some(&config), &ui).unwrap();
+        assert!(config.read(FILE).is_some());
+    }
+
     #[test]
     fn invalid_tabs_split_and_deep_tree_are_rejected() {
         let mut snapshot = Snapshot::capture(&UiState::default());

crates/ui-egui/tests/ui_panels.rs

@@ -172,6 +172,8 @@ fn saved_workspaces_persist_across_sessions() {
 #[test]
 fn saved_workspace_is_listed_in_the_workspace_menu() {
     let mut h = harness();
+    // Saving needs a settings folder.
+    h.state_mut().session.config = Some(Arc::new(MemoryConfig::default()));
     let ctx = h.ctx.clone();
     effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "window.workspace", json!({"name": "Minimal"})).unwrap();
     effectcraft_ui_egui::menus::invoke(h.state_mut(), &ctx, "window.saveWorkspaceAs", json!({"name": "My Layout"})).unwrap();

crates/ui-egui/tests/ui_workspace_focus.rs

@@ -85,7 +85,11 @@ fn unreadable_native_workspace_is_preserved_after_later_layout_changes() {
         assert!(effectcraft_ui_egui::control::inspect(&app, &ctx)["workspaceStorageError"].is_string());
         app.set_workspace("Minimal");
         app.ui.focused = PanelKind::Project;
-        effectcraft_ui_egui::menus::invoke(&mut app, &ctx, "window.saveWorkspaceAs", json!({"name":"Must not overwrite"})).unwrap();
+        let error = effectcraft_ui_egui::menus::invoke(&mut app, &ctx, "window.saveWorkspaceAs", json!({"name":"Must not overwrite"})).unwrap_err();
+        assert!(error.contains("may be lost after a restart") && error.contains("workspaces.json"), "{error}");
+        assert_eq!(app.ui.status, error, "the failure is in the status bar");
+        assert_eq!(app.ui.workspace, "Must not overwrite", "the layout stays usable for this session");
+        assert!(app.ui.saved_workspaces.contains_key("Must not overwrite"));
         eframe::App::on_exit(&mut app);
         assert_eq!(std::fs::read(&path).unwrap(), bytes);
         assert!(effectcraft_ui_egui::control::inspect(&app, &ctx)["workspaceStorageError"].is_string());
@@ -330,3 +334,160 @@ fn unowned_tab_keeps_the_timeline_mini_flowchart_shortcut() {
     output.textures_delta.clear();
     assert!(app.ui.mini_flowchart.is_some(), "an unowned Tab still invokes the application binding");
 }
+
+/// Deleting the current workspace writes the file straight away (it used to be rejected for
+/// having no definition until the next frame retried).
+#[test]
+fn deleting_the_current_workspace_saves_immediately() {
+    let store = Arc::new(MemoryConfig::default());
+    let mut app = EffectcraftApp::new(Session { config: Some(store.clone()), ..Default::default() });
+    let ctx = egui::Context::default();
+    effectcraft_ui_egui::menus::invoke(&mut app, &ctx, "window.saveWorkspaceAs", json!({"name": "Gone"})).unwrap();
+    effectcraft_ui_egui::menus::invoke(&mut app, &ctx, "window.editWorkspaces", json!({"name": "Gone", "delete": true})).unwrap();
+    assert_eq!(app.ui.workspace, "Default");
+    assert!(effectcraft_ui_egui::control::inspect(&app, &ctx)["workspaceStorageError"].is_null());
+    let text = store.read("workspaces.json").unwrap();
+    assert!(!text.contains("Gone"), "the deleted workspace must be gone from the file: {text}");
+}
+
+/// A settings store whose writes fail while `full` is set (a full or read-only disk).
+#[derive(Default)]
+struct FullDisk {
+    inner: MemoryConfig,
+    full: std::sync::atomic::AtomicBool,
+}
+
+impl ConfigStore for FullDisk {
+    fn read(&self, name: &str) -> Option<String> {
+        self.inner.read(name)
+    }
+    fn write(&self, name: &str, data: &str) -> std::io::Result<()> {
+        if self.full.load(std::sync::atomic::Ordering::Relaxed) {
+            return Err(std::io::Error::other("synthetic full disk"));
+        }
+        self.inner.write(name, data)
+    }
+    fn remove(&self, name: &str) -> std::io::Result<()> {
+        self.inner.remove(name)
+    }
+}
+
+/// Issue #696: Save as New Workspace said nothing when the file couldn't be written, so the
+/// workspace was gone after a restart. The command fails, the layout stays usable, and saving
+/// again once the disk is writable keeps it.
+#[test]
+fn failed_workspace_save_is_reported_and_retryable() {
+    let store = Arc::new(FullDisk::default());
+    store.full.store(true, std::sync::atomic::Ordering::Relaxed);
+    let mut app = EffectcraftApp::new(Session { config: Some(store.clone()), ..Default::default() });
+    let ctx = egui::Context::default();
+    let invoke = |app: &mut EffectcraftApp, id: &str, params| effectcraft_ui_egui::menus::invoke(app, &ctx, id, params);
+    let error = invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "caption craft"})).unwrap_err();
+    assert!(error.contains("synthetic full disk") && error.contains("may be lost after a restart"), "{error}");
+    assert_eq!(app.ui.status, error);
+    let storage_error = effectcraft_ui_egui::control::inspect(&app, &ctx)["workspaceStorageError"].clone();
+    assert!(storage_error.as_str().is_some_and(|e| e.contains("synthetic full disk")), "{storage_error}");
+    assert_eq!(app.ui.workspace, "caption craft");
+    assert_eq!(app.saved_workspace_names(), ["caption craft"]);
+    assert!(store.read("workspaces.json").is_none());
+    // Rename and Delete report the same way.
+    assert!(invoke(&mut app, "window.editWorkspaces", json!({"name": "caption craft", "rename": "Captions"})).is_err());
+    assert_eq!(app.saved_workspace_names(), ["Captions"]);
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "Scratch"})).unwrap_err();
+    assert!(invoke(&mut app, "window.editWorkspaces", json!({"name": "Scratch", "delete": true})).is_err());
+    assert_eq!(app.saved_workspace_names(), ["Captions"]);
+    // Writable again: an explicit Save keeps it.
+    store.full.store(false, std::sync::atomic::Ordering::Relaxed);
+    invoke(&mut app, "window.workspace", json!({"name": "Captions"})).unwrap();
+    invoke(&mut app, "window.saveWorkspace", json!({})).unwrap();
+    assert!(effectcraft_ui_egui::control::inspect(&app, &ctx)["workspaceStorageError"].is_null());
+    let restarted = EffectcraftApp::new(Session { config: Some(store), ..Default::default() });
+    assert_eq!(restarted.saved_workspace_names(), ["Captions"]);
+}
+
+/// Without a settings folder there is nowhere to keep a workspace: saving says so.
+#[test]
+fn workspace_save_without_a_settings_folder_is_reported() {
+    let mut app = EffectcraftApp::new(Session::default());
+    let ctx = egui::Context::default();
+    let error = effectcraft_ui_egui::menus::invoke(&mut app, &ctx, "window.saveWorkspaceAs", json!({"name": "Mine"})).unwrap_err();
+    assert!(error.contains("no settings folder"), "{error}");
+    assert_eq!(app.saved_workspace_names(), ["Mine"]);
+}
+
+/// Save as New Workspace, Rename and Delete reach the real settings folder straight away (no
+/// exit needed) and come back from it.
+#[test]
+fn saved_workspaces_reach_the_settings_folder_and_reload() {
+    let dir = scratch_dir("disk");
+    let launch = || EffectcraftApp::new(Session { config: Some(Arc::new(DirConfig::new(&dir))), ..Default::default() });
+    let mut app = launch();
+    let ctx = egui::Context::default();
+    let invoke = |app: &mut EffectcraftApp, id: &str, params| effectcraft_ui_egui::menus::invoke(app, &ctx, id, params).unwrap();
+    invoke(&mut app, "window.workspace", json!({"name": "Minimal"}));
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "caption craft"}));
+    let saved = app.ui.dock.clone();
+    assert!(std::fs::read_to_string(dir.join("workspaces.json")).unwrap().contains("caption craft"));
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "Scratch"}));
+    invoke(&mut app, "window.editWorkspaces", json!({"name": "caption craft", "rename": "Captions"}));
+    invoke(&mut app, "window.editWorkspaces", json!({"name": "Scratch", "delete": true}));
+    drop(app);
+    let mut app = launch();
+    assert_eq!(app.saved_workspace_names(), ["Captions"]);
+    invoke(&mut app, "window.workspace", json!({"name": "Captions"}));
+    assert_eq!(app.ui.dock, saved);
+    std::fs::remove_dir_all(dir).unwrap();
+}
+
+/// A frontend event left queued by an earlier invocation (`engine.execute` defers them) only
+/// sets the status: its failure is not the result of a later command with the same id.
+#[test]
+fn an_earlier_queued_frontend_failure_is_not_a_later_commands_result() {
+    let store = Arc::new(MemoryConfig::default());
+    let mut app = EffectcraftApp::new(Session { config: Some(store.clone()), ..Default::default() });
+    let ctx = egui::Context::default();
+    let invoke = |app: &mut EffectcraftApp, id: &str, params| effectcraft_ui_egui::menus::invoke(app, &ctx, id, params);
+    // An earlier failure, then a success.
+    app.session.execute("window.workspace", json!({"name": "missing-old"})).unwrap();
+    invoke(&mut app, "window.workspace", json!({"name": "Minimal"})).unwrap();
+    assert_eq!(app.ui.workspace, "Minimal");
+    // An earlier failing delete, then a rename that reaches the store.
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "Original"})).unwrap();
+    app.session.execute("window.editWorkspaces", json!({"name": "missing-old", "delete": true})).unwrap();
+    invoke(&mut app, "window.editWorkspaces", json!({"name": "Original", "rename": "Renamed"})).unwrap();
+    assert!(store.read("workspaces.json").unwrap().contains("Renamed"));
+    // Both fail: the result is the current invocation's own failure.
+    app.session.execute("window.workspace", json!({"name": "missing-old"})).unwrap();
+    let error = invoke(&mut app, "window.workspace", json!({"name": "missing-new"})).unwrap_err();
+    assert!(error.contains("missing-new") && !error.contains("missing-old"), "{error}");
+    // An earlier failure of another command.
+    app.session.execute("window.workspace", json!({"name": "missing-old"})).unwrap();
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "Fine"})).unwrap();
+    // Its own failure with nothing queued is still the result.
+    assert!(invoke(&mut app, "window.workspace", json!({"name": "missing"})).is_err());
+}
+
+/// A failed Save Workspace or Delete of a workspace already on disk leaves the file as it was:
+/// after a restart the earlier layout, and the workspace that wasn't deleted, come back.
+#[test]
+fn failed_save_or_delete_of_a_saved_workspace_keeps_the_file() {
+    let store = Arc::new(FullDisk::default());
+    let mut app = EffectcraftApp::new(Session { config: Some(store.clone()), ..Default::default() });
+    let ctx = egui::Context::default();
+    let invoke = |app: &mut EffectcraftApp, id: &str, params| effectcraft_ui_egui::menus::invoke(app, &ctx, id, params);
+    invoke(&mut app, "window.workspace", json!({"name": "Minimal"})).unwrap();
+    invoke(&mut app, "window.saveWorkspaceAs", json!({"name": "Kept"})).unwrap();
+    let saved = app.ui.dock.clone();
+    let before = store.read("workspaces.json").unwrap();
+    store.full.store(true, std::sync::atomic::Ordering::Relaxed);
+    app.ui.dock.close(PanelKind::Timeline);
+    let error = invoke(&mut app, "window.saveWorkspace", json!({})).unwrap_err();
+    assert!(error.contains("synthetic full disk") && error.contains("may be lost after a restart"), "{error}");
+    assert!(invoke(&mut app, "window.editWorkspaces", json!({"name": "Kept", "delete": true})).is_err());
+    assert!(app.saved_workspace_names().is_empty(), "the delete applies for this session");
+    assert_eq!(store.read("workspaces.json").unwrap(), before);
+    let mut restarted = EffectcraftApp::new(Session { config: Some(store), ..Default::default() });
+    assert_eq!(restarted.saved_workspace_names(), ["Kept"]);
+    restarted.set_workspace("Kept");
+    assert_eq!(restarted.ui.dock, saved);
+}

docs/control-protocol.md

@@ -84,6 +84,12 @@ errors. Unreadable, invalid or newer files leave the default workspace available
 preserved for the entire session, including after later layout changes. Load and save errors
 appear in the status message and the nullable
 `workspaceStorageError` field; a failed write remains eligible for retry.
+`window.saveWorkspace`, `window.saveWorkspaceAs` and `window.editWorkspaces` (rename, delete)
+write the file immediately. Through `ui.menu.invoke` they return an error when it isn't written
+(a failed write, a file preserved after a failed load, or no configuration store);
+`engine.execute` hands them to the UI and returns `{frontend}` first, so check
+`workspaceStorageError` there. The changed workspaces still apply for the session, and a later
+save retries them.
 
 ## Input