diff --git a/crates/engine/src/cmd/brushsym/symbols.rs b/crates/engine/src/cmd/brushsym/symbols.rs index c5518c70b..556926309 100644 --- a/crates/engine/src/cmd/brushsym/symbols.rs +++ b/crates/engine/src/cmd/brushsym/symbols.rs @@ -448,6 +448,9 @@ pub(super) fn symbol_update(s: &mut Session, p: &Value) -> Result { Ok(json!({ "name": name, "id": id.0 })) } +/// How many levels of nested instances Delete Symbol expands before it gives up. +const MAX_DELETE_ROUNDS: usize = 256; + pub(super) fn symbol_delete(s: &mut Session, p: &Value) -> Result { const C: &str = "symbol.delete"; // `names` deletes several symbols in one undo step (the Symbols panel's Select All Unused, #4918). @@ -475,13 +478,36 @@ pub(super) fn symbol_delete(s: &mut Session, p: &Value) -> Result { let expand = switch_or(p, "expandInstances", true, C)?; let label = if names.len() == 1 { "Delete Symbol" } else { "Delete Symbols" }; s.edit(label, |d, _| { - for name in &names { - let inst = instances_of(d, Some(name)); + // Every instance goes before any definition, whatever the order of `names`: expanding an + // instance of one symbol can bring out instances of another being deleted (nested in its + // art), which expand in turn while their definitions are still there. Each round expands + // one level of nesting, so the rounds are capped. + let of_names = |d: &Document| { + let mut v = vec![]; + d.walk(|n| { + if let NodeKind::SymbolInstance { symbol, .. } = &n.kind + && names.contains(symbol) + { + v.push(n.id); + } + }); + v + }; + for _ in 0..=MAX_DELETE_ROUNDS { + let inst = of_names(d); + if inst.is_empty() { + break; + } if expand { break_links(d, &inst, true)?; } else { remove_each(d, &inst)?; } + } + if !of_names(d).is_empty() { + return Err(EngineError::Other("the symbols are nested too deeply to expand their instances".into())); + } + for name in &names { d.symbols.retain(|x| &x.name != name); set_natural_size(d, name, None); d.set_symbol_registration(name, SymbolRegistration::CENTER); diff --git a/crates/engine/src/tests/brushsym.rs b/crates/engine/src/tests/brushsym.rs index adfe4ef11..90da1f7f8 100644 --- a/crates/engine/src/tests/brushsym.rs +++ b/crates/engine/src/tests/brushsym.rs @@ -1483,6 +1483,18 @@ fn symbol_unused_lists_the_symbols_nothing_places_and_delete_takes_several() { assert!(unused(&mut s).is_empty()); s.execute("edit.undo", &json!({})).unwrap(); assert_eq!(names(&s), ["Placed", "Inner", "Outer", "Nested", "Lonely", "Brushed"]); + // Deleting a symbol and the one placed inside it, in either order, expands every instance + // while both definitions are still there: no instance is left without its symbol. + for order in [["Inner", "Outer"], ["Outer", "Inner"]] { + s.execute("symbol.delete", &json!({"names": order})).unwrap(); + let mut dangling = 0; + s.doc() + .unwrap() + .doc + .walk(|n| dangling += usize::from(matches!(&n.kind, NodeKind::SymbolInstance { symbol, .. } if symbol == "Inner" || symbol == "Outer"))); + assert_eq!(dangling, 0, "{order:?}"); + s.execute("edit.undo", &json!({})).unwrap(); + } // Bad `names` are refused and change nothing. for bad in [json!({"names": []}), json!({"names": "Placed"}), json!({"names": [1]}), json!({"names": ["Placed", "Nope"]})] { assert!(s.execute("symbol.delete", &bad).is_err(), "{bad}"); diff --git a/crates/ui-egui/src/dialogs/symbol_delete.rs b/crates/ui-egui/src/dialogs/symbol_delete.rs index 77b308b3c..c515f7274 100644 --- a/crates/ui-egui/src/dialogs/symbol_delete.rs +++ b/crates/ui-egui/src/dialogs/symbol_delete.rs @@ -20,7 +20,14 @@ pub const KIND: &str = "deleteSymbol"; pub const USED: &str = "deleteUsedSymbol"; pub(super) const SPEC: DialogSpec = DialogSpec { - heading: |d| if d.fields.get("names").is_some() { tl!("Delete the selected symbols?") } else { tl!("Delete the selected symbol?") }.into(), + heading: |d| { + if d.fields.get("names").and_then(Value::as_array).is_some_and(|n| n.len() > 1) { + tl!("Delete the selected symbols?") + } else { + tl!("Delete the selected symbol?") + } + .into() + }, body: |_, _, _| false, confirm: confirm_unused, ok: Some("Yes"), @@ -56,7 +63,8 @@ pub(crate) fn ask(app: &mut VectorcraftApp, name: &str, now: bool) { } /// Delete Symbol for the unused symbols `names` that Select All Unused picked (#4918): asked once -/// ("Delete the selected symbols?"), or at once with `now`. They go in one undo step. +/// ("Delete the selected symbols?", or the singular for one), or at once with `now`. Only those still +/// unused then (`symbol.unused`: brush art, patterns and nested instances count) go, in one undo step. pub(crate) fn ask_many(app: &mut VectorcraftApp, names: &[String], now: bool) { let Some(uid) = app.session.active().map(|st| st.uid) else { return }; if now { @@ -143,5 +151,7 @@ mod tests { let many = Dialog::new(KIND, json!({"names": ["A", "B"], "doc": 1})); assert_eq!((SPEC.heading)(&one), "Delete the selected symbol?"); assert_eq!((SPEC.heading)(&many), "Delete the selected symbols?"); + let picked_one = Dialog::new(KIND, json!({"names": ["A"], "doc": 1})); + assert_eq!((SPEC.heading)(&picked_one), "Delete the selected symbol?"); } } diff --git a/crates/ui-egui/src/panels/symbols.rs b/crates/ui-egui/src/panels/symbols.rs index f54d04ca6..968e7a1a3 100644 --- a/crates/ui-egui/src/panels/symbols.rs +++ b/crates/ui-egui/src/panels/symbols.rs @@ -201,7 +201,8 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) { let names: Vec = doc.symbols.iter().map(|s| s.name.clone()).collect(); let list: bool = pstate(ui.ctx(), "sym-list"); let sel = current(app); - let marked = marked(ui.ctx(), &names); + let uid = app.session.active().map(|d| d.uid); + let marked = marked(ui.ctx(), uid, &names); // The symbols drawn selected: the ones Select All Unused picked, else the current one. let on = |n: &str| if marked.is_empty() { sel.as_deref() == Some(n) } else { marked.iter().any(|m| m == n) }; let mut clicked: Option<(String, bool)> = None; @@ -262,7 +263,7 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) { } if let Some((n, place)) = clicked { // A click picks one symbol again. - set_pstate(ui.ctx(), MARKED, Vec::::new()); + set_pstate(ui.ctx(), MARKED, (uid, Vec::::new())); app.run("symbol.setCurrent", json!({"name": n})).ok(); if place { app.run("symbol.place", json!({"name": n})).ok(); @@ -300,7 +301,8 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) { pub fn menu(app: &mut VectorcraftApp, ui: &mut Ui) { let sel = current(app); let names: Vec = app.session.active().map(|d| d.doc.symbols.iter().map(|s| s.name.clone()).collect()).unwrap_or_default(); - let marked = marked(ui.ctx(), &names); + let uid = app.session.active().map(|d| d.uid); + let marked = marked(ui.ctx(), uid, &names); let has_sel = app.session.active().is_some_and(|d| !d.selection.is_empty()); let inst = has_instances_selected(app); let name = json!({"name": sel}); @@ -342,7 +344,7 @@ pub fn menu(app: &mut VectorcraftApp, ui: &mut Ui) { if unused.is_empty() { app.status(tl!("No unused symbols")); } - set_pstate(ui.ctx(), MARKED, unused); + set_pstate(ui.ctx(), MARKED, (uid, unused)); } ui.separator(); let list: bool = pstate(ui.ctx(), "sym-list"); @@ -363,9 +365,13 @@ pub fn menu(app: &mut VectorcraftApp, ui: &mut Ui) { /// The panel state key of the symbols Select All Unused picked. const MARKED: &str = "sym-unused"; -/// The symbols Select All Unused picked that the document still has (`names`). -fn marked(ctx: &egui::Context, names: &[String]) -> Vec { - let mut marked: Vec = pstate(ctx, MARKED); +/// The symbols Select All Unused picked in the active document (`uid`) that it still has (`names`); +/// none once another document is active. +fn marked(ctx: &egui::Context, uid: Option, names: &[String]) -> Vec { + let (of, mut marked): (Option, Vec) = pstate(ctx, MARKED); + if of != uid { + return vec![]; + } marked.retain(|m| names.contains(m)); marked } @@ -376,7 +382,7 @@ fn delete(app: &mut VectorcraftApp, marked: &[String], sel: Option<&str>, now: b match (marked, sel) { ([], Some(n)) => crate::dialogs::symbol_delete::ask(app, n, now), ([], None) => {} - ([n], _) => crate::dialogs::symbol_delete::ask(app, n, now), + // Picked by Select All Unused, even one: deleted only while still unused. (names, _) => crate::dialogs::symbol_delete::ask_many(app, names, now), } } @@ -660,7 +666,7 @@ mod tests { crate::theme::install_fonts(&ctx); click_menu(&ctx, &mut app, "Select All Unused"); let names = symbols(&mut app); - assert_eq!(marked(&ctx, &names), ["A", "C"]); + assert_eq!(marked(&ctx, uid(&app), &names), ["A", "C"]); let undo = app.session.active().unwrap().history.undo.len(); click_menu(&ctx, &mut app, "Delete Symbol"); assert_eq!(app.ui.dialog.as_ref().map(|d| d.kind.as_str()), Some(crate::dialogs::symbol_delete::KIND)); @@ -668,20 +674,54 @@ mod tests { crate::dialogs::confirm(&mut app).unwrap(); assert_eq!(symbols(&mut app), ["B"]); assert_eq!(app.session.active().unwrap().history.undo.len(), undo + 1, "one undo step"); - assert!(marked(&ctx, &symbols(&mut app)).is_empty(), "the deleted ones are no longer picked"); + assert!(marked(&ctx, uid(&app), &symbols(&mut app)).is_empty(), "the deleted ones are no longer picked"); // Undone, they are back; Select All Unused again, then a click on a symbol picks just it. app.run("edit.undo", json!({})).unwrap(); click_menu(&ctx, &mut app, "Select All Unused"); - assert_eq!(marked(&ctx, &symbols(&mut app)).len(), 2); + assert_eq!(marked(&ctx, uid(&app), &symbols(&mut app)).len(), 2); frame(&ctx, &mut app, vec![]); let tile = pos2(26.0, 26.0); frame(&ctx, &mut app, vec![egui::Event::PointerMoved(tile), button(tile, true)]); frame(&ctx, &mut app, vec![button(tile, false)]); - assert!(marked(&ctx, &symbols(&mut app)).is_empty()); + assert!(marked(&ctx, uid(&app), &symbols(&mut app)).is_empty()); // With nothing unused, the item reports it and picks nothing. app.run("symbol.delete", json!({"names": ["A", "C"]})).unwrap(); click_menu(&ctx, &mut app, "Select All Unused"); - assert!(marked(&ctx, &symbols(&mut app)).is_empty()); + assert!(marked(&ctx, uid(&app), &symbols(&mut app)).is_empty()); + } + + fn uid(app: &VectorcraftApp) -> Option { + app.session.active().map(|d| d.uid) + } + + /// Picks made in one document don't carry to another with symbols of the same names, and a + /// picked symbol that gained a use since (here in a brush's art) isn't deleted (#4918 review). + #[test] + fn picked_symbols_stay_with_their_document_and_are_rechecked_before_deleting() { + let mut app = VectorcraftApp::new(Session::new(), Default::default()); + let make = |app: &mut VectorcraftApp| { + app.run("file.new", json!({"width": 200, "height": 100})).unwrap(); + app.run("shape.rectangle", json!({"x": 10, "y": 10, "width": 30, "height": 20})).unwrap(); + let id = app.run("symbol.new", json!({"name": "A"})).unwrap()["id"].as_u64().unwrap(); + app.run("edit.clear", json!({"ids": [id]})).unwrap(); + }; + make(&mut app); + let ctx = egui::Context::default(); + crate::theme::install_fonts(&ctx); + click_menu(&ctx, &mut app, "Select All Unused"); + assert_eq!(marked(&ctx, uid(&app), &symbols(&mut app)), ["A"]); + make(&mut app); + assert!(marked(&ctx, uid(&app), &symbols(&mut app)).is_empty(), "not in the other document"); + // Back in the first one: A gains a use in a brush's art, then Delete (Alt: no question). + click_menu(&ctx, &mut app, "Select All Unused"); + let id = app.run("symbol.place", json!({"name": "A"})).unwrap()["id"].as_u64().unwrap(); + app.run("brush.new", json!({"type": "art", "name": "B", "ids": [id]})).unwrap(); + app.run("edit.clear", json!({"ids": [id]})).unwrap(); + let names = symbols(&mut app); + let picked = marked(&ctx, uid(&app), &names); + assert_eq!(picked, ["A"]); + delete(&mut app, &picked, None, true); + assert_eq!(symbols(&mut app), ["A"], "still used by the brush, so kept"); } #[test]