Address review: undo restores visibility a recorded step changed; non-linear Delete at the oldest state

With Make Layer Visibility Changes Undoable off, stepping through history kept every layer's
current visibility, so undoing a recorded step that changed visibility (Apply Layer Comp, a
snapshot, a compound) left the new visibility in place. Each history state now remembers the
layers whose visibility its own step changed (History::current_visibility, set by the session
when it records); Undo, Redo, History rows and non-linear Delete carry only the other layers'
visibility, so only the unrecorded eye changes survive. Deleting, purging or branching keeps the
lists consistent by merging them into the following step.

history.deleteState is now enabled in non-linear history while the oldest state is current and
later states exist, so a later state can still be deleted there.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

crates/engine/src/analysis_cmds.rs

@@ -197,7 +197,8 @@ pub(crate) fn compound<R>(s: &mut Session, label: &str, f: impl FnOnce(&mut Sess
         Ok(v) => {
             // As in `Session::edit`: undo targets what was selected just before the step (#1356).
             st.history.set_current_layers(prior);
-            st.history.record(label, before, st.layer_target());
+            st.history.record(label, before.clone(), st.layer_target());
+            st.history.add_current_visibility(&crate::visibility_changes(&before, &st.doc));
             st.trim_history();
             st.coalesce = None;
             Ok(v)

crates/engine/src/history_cmds.rs

@@ -207,8 +207,13 @@ fn delete_snapshot(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({ "snapshot": snap.id, "snapshots": st.snapshots.len() }))
 }
 
+/// A state other than the oldest exists to delete: a past or the current one, and with Allow
+/// Non-Linear History a later (dimmed) one too, also while the oldest state is current.
 fn can_delete(s: &Session) -> std::result::Result<(), String> {
-    s.active().filter(|d| d.history.can_undo()).map(|_| ()).ok_or_else(|| "the oldest state can't be deleted".into())
+    s.active()
+        .filter(|d| d.history.can_undo() || (d.history.non_linear && d.history.can_redo()))
+        .map(|_| ())
+        .ok_or_else(|| "the oldest state can't be deleted".into())
 }
 
 fn has_doc(s: &Session) -> std::result::Result<(), String> {

crates/engine/src/history_options_cmds.rs

@@ -86,13 +86,15 @@ pub(crate) fn delete_state(s: &mut Session, i: usize, current: usize) -> Result<
         st.last_damage = Some(photocraft_geom::Rect::EMPTY);
         return Ok(());
     }
+    // The step that made the current state: the layers whose visibility it changed get it back.
+    let changed = st.history.current_visibility().to_vec();
     let (mut d, layers) = st.history.drop_current().ok_or_else(|| EngineError::Other("the history can't step back right now".into()))?;
     // As Undo does: Save As changes file identity outside history.
     if d.name != st.doc.name {
         std::sync::Arc::make_mut(&mut d).name.clone_from(&st.doc.name);
     }
     if keep_visibility {
-        crate::carry_visibility(&st.doc, &mut d);
+        crate::carry_visibility(&st.doc, &mut d, &changed);
     }
     let damage = crate::layer_multi_cmds::step_damage(&d, &st.doc);
     st.doc = d;
@@ -258,6 +260,55 @@ mod tests {
         assert!(red(&s, id) && !visible(&s, id), "the hide is a state of its own");
     }
 
+    /// Off, only the unrecorded visibility changes carry across history: a recorded step that
+    /// changed visibility (Apply Layer Comp) gives it back when undone and sets it again when
+    /// redone, whether by Undo / Redo, a History row or deleting the current state.
+    #[test]
+    fn stepping_over_a_recorded_visibility_change_restores_it() {
+        let (mut s, id) = doc();
+        s.execute(CMD, json!({"makeLayerVisibilityChangesUndoable": false})).unwrap();
+        // A comp with the layer hidden, then the eye shows it again (no state) and a fill.
+        s.execute("layer.setProps", json!({"layer": id, "visible": false})).unwrap();
+        s.execute("layerComp.new", json!({"name": "Hidden"})).unwrap();
+        s.execute("layer.setProps", json!({"layer": id, "visible": true})).unwrap();
+        s.execute("edit.fill", json!({"color": "#ff0000"})).unwrap();
+        s.execute("layerComp.apply", json!({"comp": "Hidden"})).unwrap();
+        assert!(red(&s, id) && !visible(&s, id), "the comp hides the layer");
+        s.execute("edit.undo", json!({})).unwrap();
+        assert!(red(&s, id) && visible(&s, id), "undoing Apply Layer Comp shows the layer again");
+        // The eye hides it (no state): undoing the fill keeps that.
+        s.execute("layer.setProps", json!({"layer": id, "visible": false})).unwrap();
+        s.execute("edit.undo", json!({})).unwrap();
+        assert!(!red(&s, id) && !visible(&s, id), "the unrecorded hide carries across the fill's undo");
+        s.execute("edit.redo", json!({})).unwrap();
+        assert!(red(&s, id) && !visible(&s, id));
+        s.execute("layer.setProps", json!({"layer": id, "visible": true})).unwrap();
+        s.execute("edit.redo", json!({})).unwrap();
+        assert!(red(&s, id) && !visible(&s, id), "redoing the comp hides the layer again");
+
+        // History rows: New, New Layer, New Layer Comp, Fill, Apply Layer Comp.
+        assert_eq!(entries(&s).len(), 5);
+        s.execute("history.selectState", json!({"state": 2})).unwrap();
+        assert!(!red(&s, id) && visible(&s, id), "jumping back over the comp shows the layer again");
+        s.execute("history.selectState", json!({"state": 4})).unwrap();
+        assert!(red(&s, id) && !visible(&s, id), "jumping forward over the comp hides it again");
+
+        // Non-linear Delete of the current state (the comp) steps back and shows the layer too.
+        s.execute(CMD, json!({"allowNonLinearHistory": true})).unwrap();
+        s.execute("history.deleteState", json!({})).unwrap();
+        assert!(red(&s, id) && visible(&s, id), "deleting the comp's state shows the layer again");
+
+        // Non-linear Delete of a past state: the step after it now starts from the state before
+        // it, so undoing that step also gives back the visibility the deleted step changed.
+        s.execute("layerComp.apply", json!({"comp": "Hidden"})).unwrap();
+        s.execute("edit.fill", json!({"color": "#0000ff"})).unwrap();
+        let comp = entries(&s).len() - 2;
+        s.execute("history.deleteState", json!({"state": comp})).unwrap();
+        assert!(!visible(&s, id));
+        s.execute("edit.undo", json!({})).unwrap();
+        assert!(red(&s, id) && visible(&s, id), "back at the red fill, where the layer was shown");
+    }
+
     #[test]
     fn non_linear_history_keeps_later_states_and_delete_removes_only_one() {
         let mut s = Session::new();
@@ -301,6 +352,31 @@ mod tests {
         assert!(s.active().unwrap().history.non_linear);
     }
 
+    /// With the oldest state current, non-linear Delete is still enabled for a later (dimmed)
+    /// state (the History panel's trash and its drop target ask for it); linear history has
+    /// nothing to delete there.
+    #[test]
+    fn non_linear_delete_is_enabled_while_the_oldest_state_is_current() {
+        let mut s = Session::new();
+        s.execute("file.new", json!({"width": 16, "height": 16})).unwrap();
+        for c in ["#ff0000", "#00ff00"] {
+            s.execute("edit.fill", json!({"color": c})).unwrap();
+        }
+        s.execute("edit.undo", json!({})).unwrap();
+        s.execute("edit.undo", json!({})).unwrap();
+        assert!(!s.is_enabled("history.deleteState"), "linear: only the oldest state is left to delete");
+        s.execute(CMD, json!({"allowNonLinearHistory": true})).unwrap();
+        assert!(s.is_enabled("history.deleteState"));
+        let doc = s.active().unwrap().doc.clone();
+        s.execute("history.deleteState", json!({"state": 1})).unwrap();
+        let st = s.active().unwrap();
+        assert!(std::sync::Arc::ptr_eq(&st.doc, &doc), "the document is unchanged");
+        assert_eq!(st.history.redo_labels().count(), 1, "only the first fill went");
+        assert!(s.execute("history.deleteState", json!({})).is_err(), "the oldest state itself stays");
+        s.execute("history.deleteState", json!({"state": 1})).unwrap();
+        assert!(!s.is_enabled("history.deleteState"), "nothing left but the oldest state");
+    }
+
     /// Non-linear Delete reaches a later (dimmed) state too, and deletes only it; linear history
     /// still refuses one past the current state.
     #[test]

crates/engine/src/history_select_cmds.rs

@@ -45,8 +45,19 @@ fn select_state(s: &mut Session, p: &Value) -> Result<Value> {
     let mut layers = None;
     let mut at = past;
     while at != target {
-        let step = if target < at { st.history.undo(doc.clone()) } else { st.history.redo(doc.clone()) };
-        let Some((d, l)) = step else { break };
+        // History Options: visibility changes that are not history states stay as they are,
+        // step by step as Undo / Redo keep them, except on the layers whose visibility the step
+        // passed over changed.
+        let undoing = target < at;
+        let mut changed = if undoing { st.history.current_visibility().to_vec() } else { Vec::new() };
+        let step = if undoing { st.history.undo(doc.clone()) } else { st.history.redo(doc.clone()) };
+        let Some((mut d, l)) = step else { break };
+        if keep_visibility {
+            if !undoing {
+                changed = st.history.current_visibility().to_vec();
+            }
+            crate::carry_visibility(&doc, &mut d, &changed);
+        }
         doc = d;
         layers = Some(l);
         at = if target < at { at - 1 } else { at + 1 };
@@ -58,10 +69,6 @@ fn select_state(s: &mut Session, p: &Value) -> Result<Value> {
     if doc.name != start.name {
         std::sync::Arc::make_mut(&mut doc).name.clone_from(&start.name);
     }
-    // Visibility changes that are not history states stay as they are (History Options).
-    if keep_visibility {
-        crate::carry_visibility(&start, &mut doc);
-    }
     let damage = crate::layer_multi_cmds::step_damage(&start, &doc);
     st.doc = doc;
     crate::restore_target(st, layers);

crates/engine/src/lib.rs

@@ -864,11 +864,13 @@ impl Session {
             st.coalesce = None;
         } else if key.is_none() || st.coalesce != key || !st.history.can_undo() {
             st.history.set_current_layers(prior);
-            st.history.record(label, before, layers);
+            st.history.record(label, before.clone(), layers);
+            st.history.add_current_visibility(&visibility_changes(&before, &st.doc));
             st.trim_history();
             st.coalesce = key;
         } else {
             st.history.set_current_layers(layers);
+            st.history.add_current_visibility(&visibility_changes(&before, &st.doc));
             st.coalesce = key;
         }
         st.revision += 1;
@@ -952,6 +954,8 @@ impl Session {
         let keep_visibility = !self.visibility_undoable();
         let Some(st) = self.active_mut() else { return false };
         st.coalesce = None;
+        // The step being undone: the layers whose visibility it changed get it back.
+        let changed = st.history.current_visibility().to_vec();
         match st.history.undo(st.doc.clone()) {
             Some((mut d, layers)) => {
                 // Save As changes file identity outside history, just like the saved path.
@@ -959,7 +963,7 @@ impl Session {
                     Arc::make_mut(&mut d).name.clone_from(&st.doc.name);
                 }
                 if keep_visibility {
-                    carry_visibility(&st.doc, &mut d);
+                    carry_visibility(&st.doc, &mut d, &changed);
                 }
                 // Pixels this step can have touched, so the canvas recomposites only that
                 // (it recomposited everything before).
@@ -987,7 +991,9 @@ impl Session {
                     Arc::make_mut(&mut d).name.clone_from(&st.doc.name);
                 }
                 if keep_visibility {
-                    carry_visibility(&st.doc, &mut d);
+                    // The step being redone sets the visibility it changed again.
+                    let changed = st.history.current_visibility().to_vec();
+                    carry_visibility(&st.doc, &mut d, &changed);
                 }
                 let damage = layer_multi_cmds::step_damage(&st.doc, &d);
                 st.doc = d;
@@ -1035,12 +1041,18 @@ impl Session {
 /// History Options › Make Layer Visibility Changes Undoable off: showing or hiding a layer is not
 /// a history state, so stepping through history must not bring the old visibility back either
 /// (history states hold whole documents). Gives every layer of `to` that `from` also has the
-/// visibility it has in `from`; layers only one of them has keep theirs. Copies `to` only when
-/// something differs.
-pub(crate) fn carry_visibility(from: &Document, to: &mut Arc<Document>) {
+/// visibility it has in `from`, except the `stepped` layers: a recorded step between the two
+/// states changed their visibility (Apply Layer Comp, a snapshot, a visibility change recorded
+/// while the option was on), so they keep the visibility `to` holds. Layers only one of them has
+/// keep theirs. Copies `to` only when something differs.
+pub(crate) fn carry_visibility(from: &Document, to: &mut Arc<Document>, stepped: &[LayerId]) {
     let visible: std::collections::HashMap<LayerId, bool> = from.walk().into_iter().map(|(_, _, l)| (l.id, l.visible)).collect();
-    let changes: Vec<(LayerId, bool)> =
-        to.walk().into_iter().filter_map(|(_, _, l)| visible.get(&l.id).filter(|v| **v != l.visible).map(|v| (l.id, *v))).collect();
+    let changes: Vec<(LayerId, bool)> = to
+        .walk()
+        .into_iter()
+        .filter(|(_, _, l)| !stepped.contains(&l.id))
+        .filter_map(|(_, _, l)| visible.get(&l.id).filter(|v| **v != l.visible).map(|v| (l.id, *v)))
+        .collect();
     if changes.is_empty() {
         return;
     }
@@ -1052,6 +1064,13 @@ pub(crate) fn carry_visibility(from: &Document, to: &mut Arc<Document>) {
     }
 }
 
+/// The layers (in both documents) whose visibility differs between `before` and `after`: what a
+/// recorded step changed, for [`photocraft_ops::History::add_current_visibility`].
+pub(crate) fn visibility_changes(before: &Document, after: &Document) -> Vec<LayerId> {
+    let visible: std::collections::HashMap<LayerId, bool> = before.walk().into_iter().map(|(_, _, l)| (l.id, l.visible)).collect();
+    after.walk().into_iter().filter(|(_, _, l)| visible.get(&l.id).is_some_and(|v| *v != l.visible)).map(|(_, _, l)| l.id).collect()
+}
+
 /// Undo / redo: target the layers the restored state targeted, as far as they still exist.
 fn restore_target(st: &mut DocState, layers: LayerTarget) {
     // Deselect Layers is a valid history target, not a missing layer to replace with the top

crates/ops/src/lib.rs

@@ -33,6 +33,9 @@ pub struct HistoryState {
     pub doc: Arc<Document>,
     /// The layers targeted when this state was created.
     pub layers: LayerTarget,
+    /// The layers whose visibility the step that created this state changed, as stepping to this
+    /// state from the one before it in the list must apply it; see [`History::current_visibility`].
+    pub visibility: Vec<LayerId>,
 }
 
 #[derive(Clone, Debug)]
@@ -50,6 +53,8 @@ pub struct History {
     current_label: String,
     /// The layers targeted when the current document was created (opened or edited).
     current_layers: LayerTarget,
+    /// [`HistoryState::visibility`] of the current state.
+    current_visibility: Vec<LayerId>,
     /// History Options › Allow Non-Linear History: a step recorded while later states exist
     /// (after stepping back) keeps them and goes to the end of the list, instead of discarding
     /// them; see [`History::record`] and [`History::remove`].
@@ -77,6 +82,7 @@ impl History {
             max_bytes: 0,
             current_label: "Open".into(),
             current_layers: LayerTarget::default(),
+            current_visibility: Vec::new(),
             non_linear: false,
             last_moved: None,
         }
@@ -98,12 +104,16 @@ impl History {
         }
         let prev_label = std::mem::replace(&mut self.current_label, label.into());
         let prev_layers = std::mem::replace(&mut self.current_layers, layers);
-        self.undo.push_back(HistoryState { label: prev_label, doc: before, layers: prev_layers });
+        let prev_visibility = std::mem::take(&mut self.current_visibility);
+        self.undo.push_back(HistoryState { label: prev_label, doc: before, layers: prev_layers, visibility: prev_visibility });
         let mut moved = 0;
         if self.non_linear {
             // `redo` holds the next state last; the list order is oldest first. Their tiles stay
             // counted: they only move from the redo side to the undo side.
             while let Some(state) = self.redo.pop() {
+                // The new step follows the last of them in the list but was made from `before`:
+                // stepping back to them walks back over their steps.
+                merge_ids(&mut self.current_visibility, &state.visibility);
                 self.undo.push_back(state);
                 moved += 1;
             }
@@ -143,6 +153,21 @@ impl History {
         self.current_layers = layers;
     }
 
+    /// The layers whose visibility the step that produced the current document changed, as the
+    /// session reported them with [`History::add_current_visibility`]. With History Options ›
+    /// Make Layer Visibility Changes Undoable off, showing or hiding a layer is not a step, so
+    /// stepping through history keeps every other layer's visibility as it is; these layers take
+    /// the visibility of the state stepped to, since a recorded step changed them.
+    pub fn current_visibility(&self) -> &[LayerId] {
+        &self.current_visibility
+    }
+
+    /// Note layers whose visibility the current step changed (after [`History::record`], and
+    /// again when a later change coalesces into the step).
+    pub fn add_current_visibility(&mut self, ids: &[LayerId]) {
+        merge_ids(&mut self.current_visibility, ids);
+    }
+
     /// Undo: `current` becomes redoable; returns the document to make current and the layers it
     /// targeted when it was created.
     pub fn undo(&mut self, current: Arc<Document>) -> Option<(Arc<Document>, LayerTarget)> {
@@ -154,7 +179,8 @@ impl History {
         }
         let label = std::mem::replace(&mut self.current_label, prev.label);
         let layers = std::mem::replace(&mut self.current_layers, prev.layers.clone());
-        self.redo.push(HistoryState { label, doc: current, layers });
+        let visibility = std::mem::replace(&mut self.current_visibility, prev.visibility);
+        self.redo.push(HistoryState { label, doc: current, layers, visibility });
         Some((prev.doc, prev.layers))
     }
 
@@ -168,7 +194,8 @@ impl History {
         }
         let label = std::mem::replace(&mut self.current_label, next.label);
         let layers = std::mem::replace(&mut self.current_layers, next.layers.clone());
-        self.undo.push_back(HistoryState { label, doc: current, layers });
+        let visibility = std::mem::replace(&mut self.current_visibility, next.visibility);
+        self.undo.push_back(HistoryState { label, doc: current, layers, visibility });
         Some((next.doc, next.layers))
     }
 
@@ -202,6 +229,8 @@ impl History {
         if let Some(tiles) = &mut self.tiles {
             tiles.remove(&state.doc);
         }
+        // The current step now starts where the purged one did.
+        merge_ids(&mut self.current_visibility, &state.visibility);
         true
     }
 
@@ -219,6 +248,11 @@ impl History {
         if let Some(tiles) = &mut self.tiles {
             tiles.remove(&state.doc);
         }
+        // The state after it now follows the one before it.
+        match self.undo.get_mut(i) {
+            Some(next) => merge_ids(&mut next.visibility, &state.visibility),
+            None => merge_ids(&mut self.current_visibility, &state.visibility),
+        }
         true
     }
 
@@ -232,6 +266,11 @@ impl History {
         if let Some(tiles) = &mut self.tiles {
             tiles.remove(&state.doc);
         }
+        // The state after it in the list (the one before it in `redo`) now follows the state
+        // before it.
+        if let Some(next) = k.checked_sub(1).and_then(|n| self.redo.get_mut(n)) {
+            merge_ids(&mut next.visibility, &state.visibility);
+        }
         true
     }
 
@@ -246,6 +285,11 @@ impl History {
         }
         self.current_label = prev.label;
         self.current_layers = prev.layers.clone();
+        let dropped = std::mem::replace(&mut self.current_visibility, prev.visibility);
+        // The next later state now follows `prev`.
+        if let Some(next) = self.redo.last_mut() {
+            merge_ids(&mut next.visibility, &dropped);
+        }
         Some((prev.doc, prev.layers))
     }
 
@@ -273,6 +317,7 @@ impl History {
         }
         self.current_label = prev.label;
         self.current_layers = prev.layers.clone();
+        self.current_visibility = prev.visibility;
         Some((prev.doc, prev.layers))
     }
 
@@ -299,6 +344,7 @@ impl History {
 
     pub fn clear(&mut self) {
         self.last_moved = None;
+        self.current_visibility.clear();
         self.undo.clear();
         self.redo.clear();
         self.tiles = None;
@@ -405,6 +451,15 @@ impl History {
     }
 }
 
+/// Add the ids of `from` that `into` lacks.
+fn merge_ids(into: &mut Vec<LayerId>, from: &[LayerId]) {
+    for id in from {
+        if !into.contains(id) {
+            into.push(*id);
+        }
+    }
+}
+
 /// Bytes of the pixel tiles of `doc` (layers, masks, fill caches, smart filter masks, alpha channels, selection) not already in `seen`.
 fn tile_bytes(doc: &Document, seen: &mut HashSet<usize>) -> usize {
     let mut n = 0usize;
@@ -617,6 +672,67 @@ mod tests {
         assert_counter(&h, &c);
     }
 
+    /// Each state keeps the layers whose visibility its own step changed; when a state goes
+    /// (Purge, a non-linear Delete) or a step is appended after the kept later states, the step
+    /// that now follows covers the changes of the steps it now spans.
+    #[test]
+    fn step_visibility_follows_steps_and_spans_removed_ones() {
+        let vis = |h: &History| {
+            let mut v: Vec<u64> = h.current_visibility().iter().map(|l| l.0).collect();
+            v.sort_unstable();
+            v
+        };
+        // Open, then steps A, B, C, D changing layers 1, 2, 3, 4.
+        let steps = |n: usize, non_linear: bool| {
+            let mut h = History { non_linear, ..History::default() };
+            let mut cur = Arc::new(base());
+            for (i, label) in ["A", "B", "C", "D"].into_iter().take(n).enumerate() {
+                edit(&mut h, &mut cur, label, |d| d.name = label.to_lowercase());
+                h.add_current_visibility(&[LayerId(i as u64 + 1)]);
+            }
+            (h, cur)
+        };
+        let (mut h, cur) = steps(3, false);
+        assert_eq!(vis(&h), [3]);
+        let (cur, _) = h.undo(cur).unwrap();
+        assert_eq!(vis(&h), [2]);
+        h.redo(cur).unwrap();
+        assert_eq!(vis(&h), [3]);
+        // Purge: C now starts from A.
+        h.purge_last();
+        assert_eq!(vis(&h), [2, 3]);
+        // Non-linear Delete of B (between A and C): C now follows A.
+        let (mut h, cur) = steps(4, true);
+        assert!(h.remove(2));
+        h.undo(cur).unwrap();
+        assert_eq!(vis(&h), [2, 3]);
+        // Of the later B (A current): C follows A.
+        let (mut h, mut cur) = steps(3, true);
+        cur = h.undo(cur).unwrap().0;
+        cur = h.undo(cur).unwrap().0;
+        assert!(h.remove_later(0));
+        h.redo(cur).unwrap();
+        assert_eq!(vis(&h), [2, 3]);
+        // Of the current B (C later): A is current, C follows it.
+        let (mut h, cur) = steps(3, true);
+        h.undo(cur).unwrap();
+        let cur = h.drop_current().unwrap().0;
+        assert_eq!(vis(&h), [1]);
+        h.redo(cur).unwrap();
+        assert_eq!(vis(&h), [2, 3]);
+        // A step after the kept later states B and C (made from A) follows C in the list.
+        let (mut h, mut cur) = steps(3, true);
+        cur = h.undo(cur).unwrap().0;
+        cur = h.undo(cur).unwrap().0;
+        edit(&mut h, &mut cur, "D", |d| d.name = "d".into());
+        h.add_current_visibility(&[LayerId(4)]);
+        assert_eq!(vis(&h), [2, 3, 4]);
+        h.rollback().unwrap();
+        assert_eq!(vis(&h), [1], "rollback restores A's");
+        h.clear();
+        assert_eq!(vis(&h), Vec::<u64>::new());
+    }
+
     #[test]
     fn rollback_takes_the_last_record_back_exactly_in_both_modes() {
         // Non-linear, with redo states: the moved states go back to the redo side and the