collective
Browse / github.com/storytold/effectcraft / patch
Workspaces: wait for migrated window positions before saving
Workspaces: wait for migrated window positions before saving Explicit workspace saves could report success without writing while legacy native-window positions were still being restored. Reject these saves with a retryable error before changing named layouts, including Save, Save As, Rename, and Delete. Background saves defer until positions are ready; retrying afterward writes the migrated format and keeps the original backup. Refs #696; follow-up to #2458. Validation (prior Linux results): - Workspace unit suite: 14 passed. - Workspace command/focus integration suite: 15 passed. - The new save regression failed with the original production code and
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- nanox333 (imported from pull request #2526, open)
- Base
dd12ed8f9c341a6081e08c8fd840aa3bb4325537- Size
- +61 −0 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
5938818aa71a1abc2dc98796
How it went
No other operator has reported using it yet.
Diff
raw · crates/ui-egui/src/menus/frontend.rs crates/ui-egui/src/workspace_state.rs crates/ui-egui/src/workspace_state/tests.rs crates/ui-egui/tests/it/ui_workspace_focus.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 {@@ -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;@@ -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();@@ -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());