Merge pull request #2526 from nanox333/fix/workspace-save-pending

Workspaces: wait for migrated window positions before saving

crates/ui-egui/src/menus/frontend.rs

@@ -320,6 +320,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
             Value::Null
         }
         "window.saveWorkspace" => {
+            crate::workspace_state::ensure_positions_ready(&app.ui)?;
             let name = app.ui.workspace.clone();
             app.ui.saved_workspaces.insert(name.clone(), app.ui.dock.clone());
             app.store_saved_workspaces()?;
@@ -336,6 +337,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
                 );
                 return Ok(Value::Null);
             };
+            crate::workspace_state::ensure_positions_ready(&app.ui)?;
             app.ui.saved_workspaces.insert(name.clone(), app.ui.dock.clone());
             app.ui.workspace = name.clone();
             app.store_saved_workspaces()?;
@@ -359,6 +361,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
             };
             let builtin = crate::dock::WORKSPACES.contains(&name.as_str());
             if p.get("delete").and_then(Value::as_bool) == Some(true) {
+                crate::workspace_state::ensure_positions_ready(&app.ui)?;
                 if app.ui.saved_workspaces.remove(&name).is_none() && !builtin {
                     return Err(format!("no workspace `{name}`"));
                 }
@@ -373,6 +376,7 @@ pub fn frontend(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, p: Valu
                 if builtin {
                     return Err(format!("`{name}` is a built-in workspace: save it as a new workspace instead"));
                 }
+                crate::workspace_state::ensure_positions_ready(&app.ui)?;
                 let dock = app.ui.saved_workspaces.remove(&name).ok_or_else(|| format!("no workspace `{name}`"))?;
                 app.ui.saved_workspaces.insert(new.to_string(), dock);
                 if app.ui.workspace == name {

crates/ui-egui/src/workspace_state.rs

@@ -197,9 +197,20 @@ impl WorkspaceStore {
             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"));
         }
+        ensure_positions_ready(ui)?;
         self.save(Some(config), ui).map(|_| ())
     }
 }
 
+/// Explicit saves must wait until migrated native-window positions refer to their content.
+/// Check before capturing a named layout too, so a later background save cannot persist
+/// a snapshot made with the old outer-window positions.
+pub(crate) fn ensure_positions_ready(ui: &UiState) -> Result<(), String> {
+    if !ui.native_outer.is_empty() {
+        return Err("workspace window positions are still being restored; try saving again in a moment".into());
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests;

crates/ui-egui/src/workspace_state/tests.rs

@@ -65,6 +65,29 @@ fn corrupt_future_or_unknown_workspace_keeps_defaults_and_original_file() {
 /// saved as "My Layout"; then Layer closed.
 const V1: &str = include_str!("../../tests/fixtures/workspaces-v1.json");
 
+#[test]
+fn explicit_save_reports_pending_migration_and_can_be_retried() {
+    let config = MemoryConfig::default();
+    config.write(FILE, V1).unwrap();
+    let mut ui = UiState::default();
+    let mut store = WorkspaceStore::load(Some(&config), &mut ui);
+    assert!(!ui.native_outer.is_empty());
+    assert!(!store.save(Some(&config), &ui).unwrap(), "background saves defer quietly");
+    let error = store.save_now(Some(&config), &ui).unwrap_err();
+    assert!(error.contains("positions") && error.contains("try saving again"), "{error}");
+    assert_eq!(config.read(FILE).as_deref(), Some(V1));
+    assert_eq!(config.read(BACKUP), None);
+    assert!(store.has_changes(&ui));
+
+    crate::dock_ui::native_outer_to_content(&mut ui, None, None, 3.0);
+    store.save_now(Some(&config), &ui).unwrap();
+    assert_eq!(config.read(BACKUP).as_deref(), Some(V1));
+    let mut restored = UiState::default();
+    let loaded = WorkspaceStore::load(Some(&config), &mut restored);
+    assert_eq!(loaded.error, None);
+    assert_eq!(Snapshot::capture(&restored), Snapshot::capture(&ui));
+}
+
 #[test]
 fn version_one_file_loads_with_every_panel_in_place() {
     let config = MemoryConfig::default();

crates/ui-egui/tests/it/ui_workspace_focus.rs

@@ -16,6 +16,29 @@ fn tab_id(p: PanelKind) -> Id {
     craft_ui::docking::DockArea::<PanelKind>::hit_id(effectcraft_ui_egui::dock_ui::area_id(), &craft_ui::docking::Hit::Tab(p))
 }
 
+#[test]
+fn workspace_commands_wait_for_migration_without_changing_named_layouts() {
+    let store = Arc::new(MemoryConfig::default());
+    let original = include_str!("../fixtures/workspaces-v1.json");
+    store.write("workspaces.json", original).unwrap();
+    let mut app = EffectcraftApp::new(Session { config: Some(store.clone()), ..Default::default() });
+    assert!(!app.ui.native_outer.is_empty());
+    let ctx = egui::Context::default();
+    let before = (app.ui.workspace.clone(), app.ui.saved_workspaces.clone(), app.ui.dock.clone());
+    for (id, params) in [
+        ("window.saveWorkspace", json!({})),
+        ("window.saveWorkspaceAs", json!({"name": "Too soon"})),
+        ("window.editWorkspaces", json!({"name": "My Layout", "delete": true})),
+        ("window.editWorkspaces", json!({"name": "My Layout", "rename": "Too soon"})),
+    ] {
+        let error = effectcraft_ui_egui::menus::invoke(&mut app, &ctx, id, params).unwrap_err();
+        assert!(error.contains("positions"), "{id}: {error}");
+        assert_eq!((app.ui.workspace.clone(), app.ui.saved_workspaces.clone(), app.ui.dock.clone()), before);
+        assert_eq!(store.read("workspaces.json").as_deref(), Some(original));
+        assert_eq!(store.read("workspaces.old.json"), None);
+    }
+}
+
 #[test]
 fn named_and_current_workspaces_survive_frontend_restart() {
     let store = Arc::new(MemoryConfig::default());