fix(actions): allow no-op Arrange steps during playback (#8457)

crates/engine/src/cmd/actions_cmds.rs

@@ -367,9 +367,18 @@ fn stop(s: &mut Session, _p: &Value) -> Result<Value> {
 }
 
 /// A step Photoshop plays as a silent no-op although its menu item is greyed: Deselect with an
-/// open document and nothing selected (actions often start with it to clear any selection, #6744).
-fn noop_step(s: &Session, id: &str) -> bool {
-    id == "select.deselect" && s.active().is_some_and(|d| d.doc.selection.is_none())
+/// open document and nothing selected (actions often start with it to clear any selection, #6744),
+/// and an Arrange that can't move the layer, such as Bring to Front on a new adjustment layer that
+/// is already on top (#8453).
+fn noop_step(s: &Session, id: &str, params: &Value) -> bool {
+    let Some(d) = s.active() else { return false };
+    let dir = match id {
+        "select.deselect" => return d.doc.selection.is_none(),
+        "layer.arrange.bringForward" | "layer.arrange.bringToFront" => 1,
+        "layer.arrange.sendBackward" | "layer.arrange.sendToBack" => -1,
+        _ => return false,
+    };
+    crate::commands::layer_param(s, params).is_ok_and(|layer| super::ops::arrange_at_end(&d.doc, layer, dir))
 }
 
 fn run_recorded(s: &mut Session, action: &Action, from: usize) -> (u64, Option<Value>) {
@@ -386,7 +395,7 @@ fn run_recorded(s: &mut Session, action: &Action, from: usize) -> (u64, Option<V
             failed = Some(json!({"step": i, "id": id, "error": e.to_string()}));
             break;
         }
-        if noop_step(s, id) {
+        if noop_step(s, id, params) {
             ran += 1;
             continue;
         }

crates/engine/src/cmd/actions_cmds/tests.rs

@@ -1040,3 +1040,48 @@ fn button_color_is_one_of_the_photoshop_swatches() {
     }
     assert!(s.actions.list[0].color.is_empty(), "rejected colours change nothing");
 }
+
+#[test]
+fn arrange_that_cannot_move_is_a_noop_in_playback() {
+    // #8453: imported actions make an adjustment layer, then Bring it to Front, where it already is.
+    let mut s = Session::new();
+    s.execute("file.new", json!({"width": 8, "height": 8})).unwrap();
+    let steps = vec![
+        ("layer.newAdjustmentLayer.curves".to_string(), json!({})),
+        ("layer.arrange.bringToFront".to_string(), json!({})),
+        ("layer.new.layer".to_string(), json!({"name": "After"})),
+    ];
+    s.actions.list.push(Action::new("Repro", steps));
+    let before = s.active().unwrap().history.past_len();
+    let played = s.execute("actions.play", json!({"action": 0})).unwrap();
+    assert_eq!(played["ran"], 3, "{played}");
+    assert!(played.get("failed").is_none(), "{played}");
+    assert_eq!(s.active().unwrap().history.past_len(), before + 2, "the no-op adds no history step");
+
+    // The predicate agrees with the command for every layer: top level, above the Background,
+    // and inside a group.
+    let a = s.execute("layer.new.layer", json!({"name": "A"})).unwrap()["layer"].clone();
+    s.execute("layer.select", json!({"layer": a})).unwrap();
+    s.execute("layer.groupLayers", json!({})).unwrap();
+    s.execute("layer.new.layer", json!({"name": "B"})).unwrap();
+    let cmds = [("layer.arrange.bringForward", 1), ("layer.arrange.bringToFront", 1), ("layer.arrange.sendBackward", -1), ("layer.arrange.sendToBack", -1)];
+    let ids: Vec<_> = s.active().unwrap().doc.walk().into_iter().map(|(_, _, l)| l.id).collect();
+    let bg = crate::background_cmds::background_id(&s.active().unwrap().doc);
+    for id in ids.into_iter().filter(|id| Some(*id) != bg) {
+        for (cmd, dir) in cmds {
+            let at_end = super::super::ops::arrange_at_end(&s.active().unwrap().doc, id, dir);
+            let ran = s.execute(cmd, json!({"layer": id.0}));
+            assert_eq!(at_end, ran.is_err(), "{cmd} on {id:?}: {ran:?}");
+            if ran.is_ok() {
+                s.execute("edit.undo", json!({})).unwrap();
+            }
+        }
+    }
+
+    // Arranging the Background still stops playback: Photoshop refuses it too.
+    let bg = bg.unwrap();
+    s.actions.list.push(Action::new("Bg", vec![("layer.arrange.bringToFront".to_string(), json!({"layer": bg.0}))]));
+    let played = s.execute("actions.play", json!({"action": 1})).unwrap();
+    assert_eq!(played["ran"], 0, "{played}");
+    assert!(played["failed"].is_object(), "{played}");
+}

crates/engine/src/cmd/ops.rs

@@ -2,6 +2,28 @@ use super::enabled::check_all_unlocked;
 use super::layer_params::{layer_param, mask_at_depth};
 use super::{Document, EngineError, Layer, LayerContent, LayerId, LayerMask, Result, Session, Value, json, pixels};
 
+/// Is layer `id` already as far as Arrange can take it in direction `dir` (up when positive)? True
+/// at the top or bottom of its stack, or just above the Background, which stays at the bottom. The
+/// Background itself is not: arranging it is refused outright.
+pub(crate) fn arrange_at_end(doc: &Document, id: LayerId, dir: i32) -> bool {
+    let Some(path) = doc.path_of(id) else { return false };
+    let Some((&idx, parent)) = path.split_last() else { return false };
+    let len = if parent.is_empty() {
+        doc.layers.len()
+    } else {
+        match doc.layer_at(parent).and_then(Layer::children) {
+            Some(children) => children.len(),
+            None => return false,
+        }
+    };
+    let bg = if parent.is_empty() { crate::background_cmds::background_id(doc) } else { None };
+    if bg == Some(id) {
+        return false;
+    }
+    let floor = usize::from(bg.is_some());
+    if dir > 0 { idx.saturating_add(1) >= len } else { idx <= floor }
+}
+
 pub(super) fn arrange(s: &mut Session, p: &Value, delta: i32) -> Result<Value> {
     let id = layer_param(s, p)?;
     s.edit("Arrange", |doc, _| {