Merge pull request #6539 from dotCooCoo/fix/break-link-keeps-new-art-selected

Symbols: Break Link keeps new art on a locked artboard selected

crates/engine/src/cmd/brushsym/symbols.rs

@@ -391,9 +391,10 @@ fn expanded_bound(d: &Document, inst: &Node, own: bool) -> u64 {
 }
 
 /// Replace instances `ids` with their art ([`expanded_instance`], `own` as there). A hidden or
-/// locked instance's art stays hidden or locked. Returns the art's ids. Fails before any edit when
-/// the instances together would make more than [`MAX_EXPAND_NODES`] objects.
-fn break_links(d: &mut Document, ids: &[NodeId], own: bool) -> Result<Vec<NodeId>> {
+/// locked instance's art stays hidden or locked. Returns each replaced instance's id with its art's
+/// id. Fails before any edit when the instances together would make more than
+/// [`MAX_EXPAND_NODES`] objects.
+fn break_links(d: &mut Document, ids: &[NodeId], own: bool) -> Result<Vec<(NodeId, NodeId)>> {
     let total = ids.iter().filter_map(|id| d.node(*id)).fold(0u64, |sum, n| sum.saturating_add(expanded_bound(d, n, own)));
     if total > MAX_EXPAND_NODES as u64 {
         return Err(EngineError::Other("Too many pieces to make at once".into()));
@@ -408,7 +409,7 @@ fn break_links(d: &mut Document, ids: &[NodeId], own: bool) -> Result<Vec<NodeId
         let aid = art.id;
         d.remove(*id)?;
         d.insert(par, idx, art)?;
-        out.push(aid);
+        out.push((*id, aid));
     }
     Ok(out)
 }
@@ -495,12 +496,20 @@ pub(super) fn symbol_break_link(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let iso = s.doc()?.isolation;
     let out = s.edit("Break Link to Symbol", |d, sel| {
-        let out = break_links(d, &ids, true)?;
+        let fresh = d.new_art.get();
+        let made = break_links(d, &ids, true)?;
         // Art that is hidden or locked, or in a hidden or locked group or layer, is left out of the
-        // selection.
+        // selection, as is art on a locked artboard unless the instance it replaces was new art.
         let locks = d.locked_artboards();
-        sel.set(out.iter().copied().filter(|id| crate::cmd::select::is_selectable(d, iso, &locks, *id)));
-        Ok(out)
+        sel.set(made.iter().filter_map(|(inst, art)| {
+            let keep = if fresh.contains(inst) {
+                crate::cmd::select::is_selectable_on_any_artboard(d, iso, *art)
+            } else {
+                crate::cmd::select::is_selectable(d, iso, &locks, *art)
+            };
+            keep.then_some(*art)
+        }));
+        Ok(made.into_iter().map(|(_, art)| art).collect::<Vec<_>>())
     })?;
     Ok(json!({ "ids": ids_json(&out) }))
 }

crates/engine/src/cmd/select.rs

@@ -252,6 +252,18 @@ pub(crate) fn selectable_ids(d: &Document, iso: Option<NodeId>) -> BTreeSet<Node
 /// Whether [`walk_selectable`] visits `id`, read from the objects that hold it instead of a walk of
 /// the document. `locks` is the document's [`Document::locked_artboards`].
 pub(crate) fn is_selectable(d: &Document, iso: Option<NodeId>, locks: &LockedArtboards, id: NodeId) -> bool {
+    is_selectable_within(d, iso, Some(locks), id)
+}
+
+/// [`is_selectable`] without the locked artboards, for an edit's new selection: after the edit,
+/// the session takes the art on a locked artboard out of the selection, except the new art the
+/// edit made.
+pub(crate) fn is_selectable_on_any_artboard(d: &Document, iso: Option<NodeId>, id: NodeId) -> bool {
+    is_selectable_within(d, iso, None, id)
+}
+
+/// [`is_selectable`], leaving out the locked artboards when `locks` is `None`.
+fn is_selectable_within(d: &Document, iso: Option<NodeId>, locks: Option<&LockedArtboards>, id: NodeId) -> bool {
     let Some(a) = d.ancestry(id) else { return false };
     if !a.iter().all(|i| d.node(*i).is_some_and(|n| n.visible && !n.locked && !n.is_template())) {
         return false;
@@ -262,7 +274,7 @@ pub(crate) fn is_selectable(d: &Document, iso: Option<NodeId>, locks: &LockedArt
         None => {
             // The object Select All takes: the outermost one that is not a layer.
             let top = a.iter().filter_map(|i| d.node(*i)).find(|n| !n.is_layer());
-            top.is_some_and(|t| locks.is_empty() || !locks.holds(t))
+            top.is_some_and(|t| locks.is_none_or(|l| l.is_empty() || !l.holds(t)))
         }
     }
 }

crates/engine/src/tests/brushsym.rs

@@ -1386,6 +1386,31 @@ fn break_link_leaves_art_in_hidden_or_locked_groups_unselected() {
     assert!(!a.contains(&hidden) && !a.contains(&locked), "{a:?}");
 }
 
+/// Break Link to Symbol on an instance just placed on a locked artboard selects the art that
+/// replaces it, which stays new art there. The art of an older instance there, named by id, is not
+/// selected.
+#[test]
+fn break_link_keeps_new_art_on_a_locked_artboard_selected() {
+    let mut s = session();
+    let (old, name, _) = make_symbol(&mut s);
+    s.execute("artboard.new", &json!({"x": 1000, "y": 0, "width": 400, "height": 400})).unwrap();
+    s.execute("artboard.setProps", &json!({"index": 1, "locked": true})).unwrap();
+    select(&mut s, &[old]);
+    s.execute("object.move", &json!({"dx": 1000, "dy": 0})).unwrap();
+    assert!(s.doc().unwrap().selection.objects.is_empty(), "moved onto the locked artboard");
+    let inst = NodeId(s.execute("symbol.place", &json!({"name": name, "x": 1100, "y": 100})).unwrap()["id"].as_u64().unwrap());
+    assert_eq!(s.doc().unwrap().selection.objects.to_vec(), [inst], "placed art is selected");
+    let r = s.execute("symbol.breakLink", &json!({})).unwrap();
+    let art: Vec<NodeId> = r["ids"].as_array().unwrap().iter().map(|v| NodeId(v.as_u64().unwrap())).collect();
+    assert_eq!(art.len(), 1, "{r}");
+    assert_eq!(s.doc().unwrap().selection.objects.to_vec(), art, "the art that replaces it");
+    s.execute("object.move", &json!({"dx": 10, "dy": 0})).unwrap();
+    assert_eq!(s.doc().unwrap().selection.objects.to_vec(), art, "the art stays editable");
+    let r = s.execute("symbol.breakLink", &json!({"ids": [old.0]})).unwrap();
+    assert_eq!(r["ids"].as_array().unwrap().len(), 1, "{r}");
+    assert!(s.doc().unwrap().selection.objects.is_empty(), "the older instance's art");
+}
+
 /// Delete Symbol, expanding the instances, keeps a hidden instance's art hidden and a locked
 /// instance's art locked.
 #[test]