diff --git a/crates/engine/src/analysis_cmds.rs b/crates/engine/src/analysis_cmds.rs index 92b630aa44..4c1b85e393 100644 --- a/crates/engine/src/analysis_cmds.rs +++ b/crates/engine/src/analysis_cmds.rs @@ -197,7 +197,8 @@ pub(crate) fn compound(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) diff --git a/crates/engine/src/history_cmds.rs b/crates/engine/src/history_cmds.rs index 9e9362a626..d09cd42900 100644 --- a/crates/engine/src/history_cmds.rs +++ b/crates/engine/src/history_cmds.rs @@ -207,8 +207,13 @@ fn delete_snapshot(s: &mut Session, p: &Value) -> Result { 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> { diff --git a/crates/engine/src/history_options_cmds.rs b/crates/engine/src/history_options_cmds.rs index bdd6e75eac..db415d7316 100644 --- a/crates/engine/src/history_options_cmds.rs +++ b/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] diff --git a/crates/engine/src/history_select_cmds.rs b/crates/engine/src/history_select_cmds.rs index 2b4adc21d7..5fddd9c5ca 100644 --- a/crates/engine/src/history_select_cmds.rs +++ b/crates/engine/src/history_select_cmds.rs @@ -45,8 +45,19 @@ fn select_state(s: &mut Session, p: &Value) -> Result { 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 { 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); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index d4f986844a..c1f900ff04 100644 --- a/crates/engine/src/lib.rs +++ b/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) { +/// 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, stepped: &[LayerId]) { let visible: std::collections::HashMap = 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) { } } +/// 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 { + let visible: std::collections::HashMap = 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 diff --git a/crates/ops/src/lib.rs b/crates/ops/src/lib.rs index c4da888e5a..0b3e7f9151 100644 --- a/crates/ops/src/lib.rs +++ b/crates/ops/src/lib.rs @@ -33,6 +33,9 @@ pub struct HistoryState { pub doc: Arc, /// 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, } #[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, /// 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) -> Option<(Arc, 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, 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 { 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 = 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::::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