From b9a9a0a4148f16add9783e6d536e6b301591486b Mon Sep 17 00:00:00 2001 From: tylerjenningsw Date: Mon, 12 Oct 2026 21:32:18 -0400 Subject: [PATCH] Workspaces: Save, Save As, Rename and Delete fail when the file isn't written (#696) --- crates/ui-egui/src/lib.rs | 13 +++++++-- crates/ui-egui/src/menus.rs | 8 ++--- crates/ui-egui/src/native_menu.rs | 1 + crates/ui-egui/src/workspace_state.rs | 34 ++++++++++++++++++++++ crates/ui-egui/tests/ui_panels.rs | 2 ++ crates/ui-egui/tests/ui_workspace_focus.rs | 6 +++- 6 files changed, 56 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 746d917bc..df886759b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -428,9 +428,16 @@ 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 layouts stay usable and a later save retries, but a restart loses them. + 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!("Workspaces are kept for this session only and won't be there 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. diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index ae0834ca2..4b8d14a42 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1017,7 +1017,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" => { @@ -1034,7 +1034,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" => { @@ -1063,7 +1063,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu if app.ui.workspace == name { app.set_workspace("Default"); } - app.store_saved_workspaces(); + 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) { @@ -1078,7 +1078,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()}) diff --git a/crates/ui-egui/src/native_menu.rs b/crates/ui-egui/src/native_menu.rs index 5d0a3c531..c3266acf5 100644 --- a/crates/ui-egui/src/native_menu.rs +++ b/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(); diff --git a/crates/ui-egui/src/workspace_state.rs b/crates/ui-egui/src/workspace_state.rs index b7233a806..c2ac9e373 100644 --- a/crates/ui-egui/src/workspace_state.rs +++ b/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()); diff --git a/crates/ui-egui/tests/ui_panels.rs b/crates/ui-egui/tests/ui_panels.rs index e3fee868c..38df1ecbc 100644 --- a/crates/ui-egui/tests/ui_panels.rs +++ b/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(); diff --git a/crates/ui-egui/tests/ui_workspace_focus.rs b/crates/ui-egui/tests/ui_workspace_focus.rs index fe1ab2d72..ecd3a16ad 100644 --- a/crates/ui-egui/tests/ui_workspace_focus.rs +++ b/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("won't be there 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());