aleleba/vectorcraft
1
0
Fork 0
mirror of https://github.com/storytold/vectorcraft.git synced 2026-11-01 13:47:21 -06:00
Code Issues Packages Projects Releases Wiki Activity

Fix #4918: delete nested picks safely, keep picks per document

Browse Source 
symbol.delete {names} expands every instance of the named symbols,
round by round, before it removes any definition, so a symbol and the
one placed inside it delete alike in either order. The Symbols panel's
picks belong to the document they were made in, and a single picked
symbol is deleted only while symbol.unused still lists it.
This commit is contained in:
Don Omondi
2026-11-01 07:38:01 +07:00
committed by bflatastic
parent 5b639169cd
commit 961694cba6
4 changed files with 105 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/brushsym/symbols.rs
+28 -2
View File
@@ -448,6 +448,9 @@ pub(super) fn symbol_update(s: &mut Session, p: &Value) -> Result<Value> {
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<Value> {
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<Value> {
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);
crates/engine/src/tests/brushsym.rs
+12
View File
@@ -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}");
crates/ui-egui/src/dialogs/symbol_delete.rs
+12 -2
View File
@@ -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?");
}
}
crates/ui-egui/src/panels/symbols.rs
+53 -13
View File
@@ -201,7 +201,8 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) {
let names: Vec<String> = 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::<String>::new());
set_pstate(ui.ctx(), MARKED, (uid, Vec::<String>::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<String> = 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<String> {
let mut marked: Vec<String> = 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<u64>, names: &[String]) -> Vec<String> {
let (of, mut marked): (Option<u64>, Vec<String>) = 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<u64> {
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]