mirror of
https://github.com/storytold/photocraft.git
synced 2026-10-24 09:57:08 -06:00
fix(actions): a recorded Layers-panel drag keeps its drop target by name (#8781)
* fix(actions): a recorded Layers-panel drag keeps its drop target by name Dragging a layer while recording stored layer.moveTo's target as a layer id, which means nothing in the document the action later plays in, so playback stopped with "no such layer". The target is now recorded by name (with targetOccurrence among same-named layers), as layer.select records its layer, and layer.moveTo accepts a target by name. Fixes #7845 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(actions): count a drag's same-named targets without the moved layer Steps are recorded after they run, so a target's occurrence among same-named layers could shift when the moved layer shares its name. It is now counted among the other layers only, at recording and at replay, which the move doesn't reorder. Only a drag of the selected layer is recorded by name; drags of several layers, or of an unselected one, keep their ids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(actions): a moved group's contents don't count as drop targets Same-named layers inside a moved group also changed order with the move, so they are left out of the occurrence count with the group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(actions): leave every moved layer out of a named drop target A move of several layers given a target by name no longer counts the moved layers (or their contents) as candidates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(actions): a recorded ⌥-drag copy beside itself replays by name A copy leaves its source in place, so the source stays a candidate for the named target, at recording and at replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(actions): a recorded copy's own contents don't count as drop targets The step is recorded after the copy exists; the copy (and, for a group, its same-named contents) is left out of the occurrence count, as at replay it doesn't exist yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
FinnNk
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
10a20743ea
commit
0986582a61
@@ -1167,6 +1167,173 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// #7845: dragging a layer in the Layers panel while recording keeps the drop target by name,
|
||||
/// so the action plays on another document (whose layer ids differ) instead of failing with
|
||||
/// "no such layer". The reporter's steps, replayed on a new document.
|
||||
#[test]
|
||||
fn recorded_layer_drag_replays_on_another_document() {
|
||||
for depth in [8, 16, 32] {
|
||||
let mut s = Session::new();
|
||||
s.execute("file.new", json!({"width": 16, "height": 16, "depth": depth})).unwrap();
|
||||
s.execute("actions.record", json!({"name": "Retouch"})).unwrap();
|
||||
s.execute("layer.stampVisible", json!({})).unwrap();
|
||||
let layer1 = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.new.layerViaCopy", json!({})).unwrap();
|
||||
let layer2 = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.select", json!({"layer": layer1.0, "mode": "replace"})).unwrap();
|
||||
s.execute("filter.noise.median", json!({"radius": 6})).unwrap();
|
||||
s.execute("layer.new.layerViaCopy", json!({})).unwrap();
|
||||
let copy = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.moveTo", json!({"layer": copy.0, "target": layer2.0, "position": "above"})).unwrap();
|
||||
s.execute("actions.stop", json!({})).unwrap();
|
||||
let (id, step) = s.actions.list[0].steps.last().unwrap();
|
||||
assert_eq!((id.as_str(), step), ("layer.moveTo", &json!({"target": "Layer 1 copy", "position": "above"})));
|
||||
let names = |s: &Session| s.active().unwrap().doc.layers.iter().rev().map(|l| l.name.clone()).collect::<Vec<_>>();
|
||||
let recorded = names(&s);
|
||||
let saved = serde_json::to_string(&s.actions.list).unwrap();
|
||||
let mut replay = Session::new();
|
||||
replay.actions.list = serde_json::from_str(&saved).unwrap();
|
||||
replay.execute("file.new", json!({"width": 16, "height": 16, "depth": depth})).unwrap();
|
||||
let result = replay.execute("actions.play", json!({"action": 0})).unwrap();
|
||||
assert!(result.get("failed").is_none(), "{depth}: {result}");
|
||||
assert_eq!(names(&replay), recorded, "{depth}: the same stack");
|
||||
}
|
||||
// Several layers of the drop target's name: the recorded occurrence picks the right one.
|
||||
let mut s = Session::new();
|
||||
s.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
let upper = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Mover"})).unwrap();
|
||||
let mover = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("actions.record", json!({"name": "Drop"})).unwrap();
|
||||
s.execute("layer.moveTo", json!({"layer": mover.0, "target": upper.0, "position": "below"})).unwrap();
|
||||
s.execute("actions.stop", json!({})).unwrap();
|
||||
assert_eq!(s.actions.list[0].steps[0].1, json!({"target": "Twin", "targetOccurrence": 1, "position": "below"}));
|
||||
// The moved layer shares the target's name: the occurrence counts the other "Twin"s only,
|
||||
// so it means the same layer before the move (on replay) as after it (when recorded).
|
||||
let build = |s: &mut Session| {
|
||||
s.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
for _ in 0..3 {
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
}
|
||||
s.active().unwrap().doc.layers.iter().map(|l| l.id).collect::<Vec<_>>()
|
||||
};
|
||||
let mut s = Session::new();
|
||||
let ids = build(&mut s);
|
||||
let (lowest, top) = (ids[1], ids[3]);
|
||||
s.execute("actions.record", json!({"name": "Twins"})).unwrap();
|
||||
s.execute("layer.moveTo", json!({"layer": top.0, "target": lowest.0, "position": "below"})).unwrap();
|
||||
s.execute("actions.stop", json!({})).unwrap();
|
||||
assert_eq!(s.actions.list[0].steps[0].1, json!({"target": "Twin", "targetOccurrence": 0, "position": "below"}));
|
||||
let order = |s: &Session| s.active().unwrap().doc.layers.iter().map(|l| l.id).collect::<Vec<_>>();
|
||||
let want: Vec<_> = order(&s).iter().map(|id| ids.iter().position(|x| x == id).unwrap()).collect();
|
||||
let saved = serde_json::to_string(&s.actions.list).unwrap();
|
||||
let mut replay = Session::new();
|
||||
replay.actions.list = serde_json::from_str(&saved).unwrap();
|
||||
let ids2 = build(&mut replay);
|
||||
let result = replay.execute("actions.play", json!({"action": 0})).unwrap();
|
||||
assert!(result.get("failed").is_none(), "{result}");
|
||||
let got: Vec<_> = order(&replay).iter().map(|id| ids2.iter().position(|x| x == id).unwrap()).collect();
|
||||
assert_eq!(got, want, "the same layer became the lowest");
|
||||
// A moved group holding a same-named layer: its contents don't count either, or moving the
|
||||
// group past another "Twin" would shift the occurrence between recording and replay.
|
||||
let build_group = |s: &mut Session| {
|
||||
s.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
s.execute("layer.groupLayers", json!({})).unwrap();
|
||||
let group = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
let top = s.active().unwrap().active_layer.unwrap();
|
||||
s.execute("layer.select", json!({"layer": group.0, "mode": "replace"})).unwrap();
|
||||
(group, top)
|
||||
};
|
||||
let mut g = Session::new();
|
||||
let (group, top) = build_group(&mut g);
|
||||
g.execute("actions.record", json!({"name": "Group"})).unwrap();
|
||||
g.execute("layer.moveTo", json!({"layer": group.0, "target": top.0, "position": "above"})).unwrap();
|
||||
g.execute("actions.stop", json!({})).unwrap();
|
||||
let recorded = g.actions.list[0].steps[0].1.clone();
|
||||
let saved = serde_json::to_string(&g.actions.list).unwrap();
|
||||
let mut replay = Session::new();
|
||||
replay.actions.list = serde_json::from_str(&saved).unwrap();
|
||||
let (group2, _) = build_group(&mut replay);
|
||||
let result = replay.execute("actions.play", json!({"action": 0})).unwrap();
|
||||
assert!(result.get("failed").is_none(), "{recorded}: {result}");
|
||||
let top_layer = |s: &Session| s.active().unwrap().doc.layers.last().map(|l| l.id);
|
||||
assert_eq!(top_layer(&replay), Some(group2), "{recorded}: the group went to the top");
|
||||
// Several moved layers named like the target are left out too, so the name is unambiguous.
|
||||
let mut m = Session::new();
|
||||
m.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
let a = photocraft_doc::LayerId(m.execute("layer.new.layer", json!({"name": "Twin"})).unwrap()["layer"].as_u64().unwrap());
|
||||
let b = photocraft_doc::LayerId(m.execute("layer.new.layer", json!({"name": "Twin"})).unwrap()["layer"].as_u64().unwrap());
|
||||
let target = photocraft_doc::LayerId(m.execute("layer.new.layer", json!({"name": "Twin"})).unwrap()["layer"].as_u64().unwrap());
|
||||
m.execute("layer.moveTo", json!({"layers": [a.0, b.0], "target": "Twin", "position": "above"})).unwrap();
|
||||
assert_eq!(m.active().unwrap().doc.layers.last().map(|l| l.id), Some(b), "both moved above the third Twin");
|
||||
assert_eq!(m.active().unwrap().doc.layers.iter().rev().nth(2).map(|l| l.id), Some(target));
|
||||
// An ⌥-drag copy of the selected layer beside itself replays by name too.
|
||||
let mut c = Session::new();
|
||||
c.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
let src = photocraft_doc::LayerId(c.execute("layer.new.layer", json!({"name": "Solo"})).unwrap()["layer"].as_u64().unwrap());
|
||||
c.execute("actions.record", json!({"name": "Copy"})).unwrap();
|
||||
c.execute("layer.moveTo", json!({"layer": src.0, "target": src.0, "position": "above", "copy": true})).unwrap();
|
||||
c.execute("actions.stop", json!({})).unwrap();
|
||||
assert_eq!(c.actions.list[0].steps[0].1, json!({"target": "Solo", "position": "above", "copy": true}));
|
||||
let saved = serde_json::to_string(&c.actions.list).unwrap();
|
||||
let mut replay = Session::new();
|
||||
replay.actions.list = serde_json::from_str(&saved).unwrap();
|
||||
replay.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
replay.execute("layer.new.layer", json!({"name": "Solo"})).unwrap();
|
||||
let before = replay.active().unwrap().doc.layers.len();
|
||||
let result = replay.execute("actions.play", json!({"action": 0})).unwrap();
|
||||
assert!(result.get("failed").is_none(), "{result}");
|
||||
assert_eq!(replay.active().unwrap().doc.layers.len(), before + 1, "one copy");
|
||||
// Copying a group holding a layer named like the target: the new copy's contents (made
|
||||
// before the step is recorded) don't count, so the occurrence matches the replay.
|
||||
let build_copy = |s: &mut Session| {
|
||||
s.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
|
||||
s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap();
|
||||
s.execute("layer.groupLayers", json!({})).unwrap();
|
||||
let group = s.active().unwrap().active_layer.unwrap();
|
||||
let a = photocraft_doc::LayerId(s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap()["layer"].as_u64().unwrap());
|
||||
let b = photocraft_doc::LayerId(s.execute("layer.new.layer", json!({"name": "Twin"})).unwrap()["layer"].as_u64().unwrap());
|
||||
s.execute("layer.select", json!({"layer": group.0, "mode": "replace"})).unwrap();
|
||||
(group, a, b)
|
||||
};
|
||||
let mut gc = Session::new();
|
||||
let (group, a, _) = build_copy(&mut gc);
|
||||
gc.execute("actions.record", json!({"name": "Copy group"})).unwrap();
|
||||
// Below `a`: the copy's "Twin" then comes before `a` in document order.
|
||||
gc.execute("layer.moveTo", json!({"layer": group.0, "target": a.0, "position": "below", "copy": true})).unwrap();
|
||||
gc.execute("actions.stop", json!({})).unwrap();
|
||||
let saved = serde_json::to_string(&gc.actions.list).unwrap();
|
||||
let mut replay = Session::new();
|
||||
replay.actions.list = serde_json::from_str(&saved).unwrap();
|
||||
let (_, a2, _) = build_copy(&mut replay);
|
||||
let result = replay.execute("actions.play", json!({"action": 0})).unwrap();
|
||||
assert!(result.get("failed").is_none(), "{result}");
|
||||
let layers = &replay.active().unwrap().doc.layers;
|
||||
let at = layers.iter().position(|l| l.id == a2).unwrap();
|
||||
let below = at.checked_sub(1).and_then(|i| layers.get(i)).map(|l| l.name.as_str());
|
||||
assert_eq!(below, Some("Group 1 copy"), "the copy landed below the same Twin");
|
||||
// A drag of several layers keeps its ids (not covered by name).
|
||||
s.execute("actions.record", json!({"name": "Several"})).unwrap();
|
||||
s.execute("layer.moveTo", json!({"layers": [ids[2].0, ids[3].0], "target": ids[1].0, "position": "above"})).unwrap();
|
||||
s.execute("actions.stop", json!({})).unwrap();
|
||||
assert_eq!(s.actions.list[1].steps[0].1["target"], json!(ids[1].0));
|
||||
// Bad targets are errors, not panics.
|
||||
for bad in [
|
||||
json!({"target": "Nobody"}),
|
||||
json!({"target": "Twin"}),
|
||||
json!({"target": "Twin", "targetOccurrence": 9}),
|
||||
json!({"target": "Twin", "targetOccurrence": -1}),
|
||||
json!({"target": true}),
|
||||
] {
|
||||
assert!(s.execute("layer.moveTo", bad.clone()).is_err(), "{bad}");
|
||||
}
|
||||
}
|
||||
|
||||
/// An Alt-drag copy records `inPlace` (#7200), which only matters for an artboard: a plain
|
||||
/// layer's recorded copy must still export to .atn, while an artboard's must be refused rather
|
||||
/// than exported as a copy beside the original.
|
||||
|
||||
@@ -1135,7 +1135,7 @@ fn build() -> Vec<CommandSpec> {
|
||||
"Reorder Layer",
|
||||
[],
|
||||
None,
|
||||
r##"{"layer":id?| "layers":[id,…]?, "target":id,"position":"above|below|into"="above","copy":bool=false} (layers: one undoable move, document order preserved; copy: place duplicates there and leave the layers, as ⌥-dragging a Layers panel row)"##,
|
||||
r##"{"layer":id?| "layers":[id,…]?, "target":id|name,"targetOccurrence":n? (which of several layers named `target`, in document order),"position":"above|below|into"="above","copy":bool=false} (layers: one undoable move, document order preserved; copy: place duplicates there and leave the layers, as ⌥-dragging a Layers panel row; a recorded action keeps the target by name)"##,
|
||||
has_layer,
|
||||
crate::layer_multi_cmds::move_to
|
||||
),
|
||||
|
||||
@@ -118,6 +118,86 @@ pub(crate) fn recorded_selection(s: &Session, mut p: Value) -> Value {
|
||||
p
|
||||
}
|
||||
|
||||
/// The layers named `name`, in document order, other than the ones being moved and (groups) their
|
||||
/// contents. Moving layers never changes the others' order, so an occurrence counted among these
|
||||
/// means the same layer before the move (when an action replays it) and after it (when the step
|
||||
/// is recorded).
|
||||
fn same_named_targets(doc: &photocraft_doc::Document, name: &str, moved: &[LayerId]) -> Vec<LayerId> {
|
||||
fn subtree(l: &Layer, out: &mut Vec<LayerId>, depth: usize) {
|
||||
out.push(l.id);
|
||||
if depth < 256
|
||||
&& let Some(children) = l.children()
|
||||
{
|
||||
children.iter().for_each(|c| subtree(c, out, depth + 1));
|
||||
}
|
||||
}
|
||||
let mut skip = Vec::new();
|
||||
for l in moved.iter().filter_map(|id| doc.layer(*id)) {
|
||||
subtree(l, &mut skip, 0);
|
||||
}
|
||||
doc.walk().into_iter().filter(|(_, _, l)| l.name == name && !skip.contains(&l.id)).map(|(_, _, l)| l.id).collect()
|
||||
}
|
||||
|
||||
/// A Layers-panel drag of the selected layer records its drop target by name (and which of
|
||||
/// several same-named layers it is), as [`recorded_selection`] does: the id means nothing in the
|
||||
/// document the action plays in later, so playback failed with "no such layer" (#7845). Drags of
|
||||
/// several layers, or of a layer that isn't the selected one, keep their ids.
|
||||
fn recorded_move(s: &Session, mut p: Value, pre_selected: Option<&[LayerId]>) -> Value {
|
||||
if p.get("layers").is_some() {
|
||||
return p;
|
||||
}
|
||||
let Some(st) = s.active() else { return p };
|
||||
let moved = match p.get("layer") {
|
||||
None => st.active_layer,
|
||||
Some(v) => match v.as_u64().map(LayerId) {
|
||||
Some(id) if pre_selected.map_or_else(|| st.selected_layers().as_slice() == [id], |sel| sel == [id]) => Some(id),
|
||||
_ => return p,
|
||||
},
|
||||
};
|
||||
if let Some(fields) = p.as_object_mut() {
|
||||
fields.remove("layer");
|
||||
}
|
||||
let Some(id) = p.get("target").and_then(Value::as_u64).map(LayerId) else { return p };
|
||||
let Some(layer) = st.doc.layer(id) else { return p };
|
||||
// A copy leaves its source where it is (it may even land beside it), so the source stays a
|
||||
// candidate; the new copy (the active layer once the step has run, which is when it is
|
||||
// recorded) is left out instead, as it doesn't exist yet when the step replays.
|
||||
let copy = p.get("copy").and_then(Value::as_bool).unwrap_or(false);
|
||||
let skip = if copy { st.active_layer.filter(|a| Some(*a) != moved) } else { moved };
|
||||
let matches = same_named_targets(&st.doc, &layer.name, skip.as_slice());
|
||||
let Some(occurrence) = matches.iter().position(|candidate| *candidate == id) else { return p };
|
||||
if let Some(fields) = p.as_object_mut() {
|
||||
fields.insert("target".into(), json!(layer.name));
|
||||
if matches.len() > 1 {
|
||||
fields.insert("targetOccurrence".into(), json!(occurrence));
|
||||
}
|
||||
}
|
||||
p
|
||||
}
|
||||
|
||||
/// `layer.moveTo`'s drop target: an id, or a name with `targetOccurrence` choosing among layers
|
||||
/// that share it (in document order, the `moved` layers aside; required when several do).
|
||||
fn move_target(s: &Session, p: &Value, moved: &[LayerId]) -> Result<LayerId> {
|
||||
const CMD: &str = "layer.moveTo";
|
||||
match p.get("target") {
|
||||
Some(Value::String(name)) => {
|
||||
let occurrence = p
|
||||
.get("targetOccurrence")
|
||||
.map(|v| v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or_else(|| bad(CMD, "`targetOccurrence` must be a non-negative integer")))
|
||||
.transpose()?;
|
||||
let st = s.active().ok_or(EngineError::NoDocument)?;
|
||||
let matches = same_named_targets(&st.doc, name, moved);
|
||||
if matches.len() > 1 && occurrence.is_none() {
|
||||
return Err(bad(CMD, format!("multiple layers named `{name}`; specify `targetOccurrence`")));
|
||||
}
|
||||
let index = occurrence.unwrap_or(0);
|
||||
matches.get(index).copied().ok_or_else(|| bad(CMD, format!("no layer named `{name}` at occurrence {index}")))
|
||||
}
|
||||
Some(v) => v.as_u64().map(LayerId).ok_or_else(|| bad(CMD, "`target` must be a layer id or name")),
|
||||
None => Err(bad(CMD, "missing `target`")),
|
||||
}
|
||||
}
|
||||
|
||||
/// Normalise parameters when recording an action step.
|
||||
///
|
||||
/// For `layer.select`, captures the layer by name. For other commands, if a numeric `layer`
|
||||
@@ -128,6 +208,9 @@ pub(crate) fn recorded_step(s: &Session, id: &str, p: Value, pre_selected: Optio
|
||||
if id == "layer.select" {
|
||||
return recorded_selection(s, p);
|
||||
}
|
||||
if id == "layer.moveTo" {
|
||||
return recorded_move(s, p, pre_selected);
|
||||
}
|
||||
let Some(layer_id) = p.get("layer").and_then(Value::as_u64).map(LayerId) else { return p };
|
||||
let is_sole_selected = match pre_selected {
|
||||
Some(sel) => sel == [layer_id],
|
||||
@@ -1267,7 +1350,6 @@ fn apply_move(doc: &mut Document, moved_ids: &[LayerId], target: LayerId, pos: &
|
||||
/// selected nodes are detached, and every reference is validated before editing.
|
||||
pub fn move_to(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
const CMD: &str = "layer.moveTo";
|
||||
let target = LayerId(p.get("target").and_then(Value::as_u64).ok_or_else(|| bad(CMD, "missing `target`"))?);
|
||||
let pos = p.get("position").and_then(Value::as_str).unwrap_or("above");
|
||||
if !matches!(pos, "above" | "below" | "into") {
|
||||
return Err(bad(CMD, "position must be above, below, or into"));
|
||||
@@ -1293,6 +1375,8 @@ pub fn move_to(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
} else {
|
||||
vec![crate::commands::layer_param(s, p)?]
|
||||
};
|
||||
// A copy moves nothing, so its source stays a candidate (⌥-dragging a layer beside itself).
|
||||
let target = move_target(s, p, if copy { &[] } else { &requested })?;
|
||||
let st = s.active().ok_or(EngineError::NoDocument)?;
|
||||
let doc = &st.doc;
|
||||
let target_path = doc.path_of(target).ok_or(EngineError::NoLayer(target))?;
|
||||
|
||||
Reference in New Issue
Block a user