From d57a29b7765bf0d7499630ff1779b7178423ad56 Mon Sep 17 00:00:00 2001 From: John <110930793+mpratsaki@users.noreply.github.com> Date: Mon, 2 Nov 2026 07:38:07 +0200 Subject: [PATCH] fix(ui): Save Arrangement refuses an unknown bin (#3774 follow-up) (#3900) * fix(ui): Save Arrangement refuses an unknown bin (#3774) The engine lays out the root bin when it does not find the one asked for, so a Save Arrangement for a bin that does not exist (a scripted call, or a bin deleted while the dialog was open) could overwrite a root arrangement of the same name. The dialog now refuses a bin that does not exist when it opens and checks it again before saving. Co-Authored-By: Claude Opus 5.5 * fix(ui): Save Arrangement refuses a bin that is not a bin id (#3774) A `bin` such as -1, a string or 1.5 is not an unsigned id, so it was read as no bin and the dialog opened for the shown view. Any non-null `bin` that is not a bin id is now refused; an omitted or null one still means the shown view's. The test checks the arrangements list it meant to (`arrangements`), and covers these values. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/panels/project.rs | 23 +++++++++++++++----- crates/ui-egui/src/panels/project_dialogs.rs | 9 +++++++- crates/ui-egui/tests/it/project_panel_ui.rs | 19 ++++++++++++++++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/panels/project.rs b/crates/ui-egui/src/panels/project.rs index f1f289758..01aa8c216 100644 --- a/crates/ui-egui/src/panels/project.rs +++ b/crates/ui-egui/src/panels/project.rs @@ -958,11 +958,24 @@ pub fn route(app: &mut FilmcraftApp, ctx: &egui::Context, op: &str, params: &Val Ok(Value::Null) } "saveArrangement" => { - // the canvas the menu was opened on, which may be a floating bin window - let bin = params.get("bin").and_then(Value::as_u64).or_else(|| Some(view_of(app, inst).bin.0)); - let width = params.get("width").and_then(Value::as_f64); - app.ui.project_panel.dialog = Some(ProjectDialog::SaveArrangement { name: String::new(), bin, width }); - Ok(Value::Null) + // the canvas the menu was opened on, which may be a floating bin window; a `bin` that is + // not a bin id, or names no bin, is refused, as the engine would save to the root bin + // instead (an omitted or null `bin` is the shown view's) + let given = params.get("bin").filter(|v| !v.is_null()); + let id = given.map(|v| v.as_u64().ok_or_else(|| format!("`bin` is a bin id, not {v}"))); + if let Some(Err(e)) = id { + Err(e) + } else if let Some(Ok(b)) = id + && app.session.project.root.find_bin(BinId(b)).is_none() + { + Err(format!("no bin {b}")) + } else { + let given = id.and_then(Result::ok); + let bin = given.or_else(|| Some(view_of(app, inst).bin.0)); + let width = params.get("width").and_then(Value::as_f64); + app.ui.project_panel.dialog = Some(ProjectDialog::SaveArrangement { name: String::new(), bin, width }); + Ok(Value::Null) + } } "revealProject" => { let Some(p) = app.session.path.clone() else { return Some(Err("the project has not been saved".into())) }; diff --git a/crates/ui-egui/src/panels/project_dialogs.rs b/crates/ui-egui/src/panels/project_dialogs.rs index 99a14c5ed..9138ad6ae 100644 --- a/crates/ui-egui/src/panels/project_dialogs.rs +++ b/crates/ui-egui/src/panels/project_dialogs.rs @@ -79,7 +79,14 @@ pub fn show(app: &mut FilmcraftApp, ctx: &egui::Context) { push(app, "projectDialog.name", &r, "Name"); }); if ok_cancel(app, ui, &mut close) && !name.trim().is_empty() { - run = Some(("project.freeform.saveArrangement".into(), json!({"name": name, "bin": bin, "width": width}))); + // the bin may have been deleted while the dialog was open + match bin.filter(|b| app.session.project.root.find_bin(filmcraft_project::BinId(*b)).is_none()) { + Some(b) => { + app.ui.status = format!("no bin {b}"); + close = true; + } + None => run = Some(("project.freeform.saveArrangement".into(), json!({"name": name, "bin": bin, "width": width}))), + } } } } diff --git a/crates/ui-egui/tests/it/project_panel_ui.rs b/crates/ui-egui/tests/it/project_panel_ui.rs index 9aa774f6b..ea7b52cbe 100644 --- a/crates/ui-egui/tests/it/project_panel_ui.rs +++ b/crates/ui-egui/tests/it/project_panel_ui.rs @@ -1713,3 +1713,22 @@ fn a_card_menu_stacks_only_the_selected_cards_of_its_bin() { assert!(stack(&mut d, items[0]).is_some() && stack(&mut d, items[1]).is_some(), "this bin's cards are stacked"); assert_eq!(stack(&mut d, other), None, "the other bin's item is left alone"); } + +/// #3774 review: Save Arrangement takes the bin of the canvas it was opened on; an unknown bin +/// is refused instead of saving to the root bin. +#[test] +fn save_arrangement_refuses_an_unknown_bin() { + let mut d = project_driver(); + let (bin, _) = d.footage(); + for bad in [json!(987_654), json!(-1), json!("footage"), json!(1.5)] { + let r = d.call("ui.menu.invoke", json!({"id": "projectPanel.saveArrangement", "params": {"bin": bad, "width": 400.0}})); + assert_eq!(r["ok"], json!(false), "bin {bad}: {r}"); + assert!(d.app().ui.project_panel.dialog.is_none(), "no dialog for bin {bad}"); + } + let root = d.app().session.project.root.id.0; + let saved = d.exec("project.freeform.arrangements", json!({"bin": root})); + assert_eq!(saved["arrangements"].as_array().map(Vec::len), Some(0), "nothing saved to the root: {saved}"); + // a bin that exists opens the dialog + d.ok("ui.menu.invoke", json!({"id": "projectPanel.saveArrangement", "params": {"bin": bin, "width": 400.0}})); + assert!(d.app().ui.project_panel.dialog.is_some()); +}