From 3a9b6ccee4c2a103aea001a2b548c19fcafa965c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 18:43:18 +0000 Subject: [PATCH] Fix #293: undo and redo show the photo they change After an undo or redo that changed one photo (its edit, rating, flag, label, metadata or versions), that photo becomes the active one, as in Lightroom Classic, so the change is on screen instead of happening to a photo out of sight. It joins the selection if it isn't in it; steps that touch several photos, albums or files, and photos outside the current view, leave the selection alone. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01912WaK6Rmew3wxZuPyX7E8 --- crates/engine/src/lib.rs | 41 ++++++++++++++++++++++++++++++++++++++ crates/engine/src/tests.rs | 26 ++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ec39222a..41f1299c 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -82,6 +82,27 @@ pub struct UndoEntry { pub folder: Option, } +/// The one photo an undo step changes, when it changes exactly one through its edit, rating, flag, +/// label, metadata or versions (`depth` bounds nested batches). +fn single_photo(op: &Op, depth: usize) -> Option { + match op { + Op::SetDevelop { id, .. } + | Op::SetRating { id, .. } + | Op::SetFlag { id, .. } + | Op::SetLabel { id, .. } + | Op::SetMeta { id, .. } + | Op::SetVersions { id, .. } + | Op::SetHistory { id, .. } + | Op::PushHistory { id, .. } => Some(*id), + Op::Batch { ops } if depth < 8 => { + let mut ids = ops.iter().map(|o| single_photo(o, depth + 1)); + let first = ids.next()??; + ids.all(|id| id == Some(first)).then_some(first) + } + _ => None, + } +} + /// A folder renamed or moved on disk as part of an undo step. #[derive(Clone, Debug, PartialEq, Eq)] pub struct FolderMove { @@ -423,6 +444,7 @@ impl Session { return Err(err); } }; + self.show_undone(&e.op); self.pending_log.push(e.op); self.redo.push(UndoEntry { label: e.label.clone(), op: redo, folder: e.folder.as_ref().map(FolderMove::reversed) }); Ok(e.label) @@ -437,11 +459,30 @@ impl Session { return Err(err); } }; + self.show_undone(&e.op); self.pending_log.push(e.op); self.undo.push(UndoEntry { label: e.label.clone(), op: undo, folder: e.folder.as_ref().map(FolderMove::reversed) }); Ok(e.label) } + /// After an undo or redo that changed one photo (its edit, rating, flag, label, metadata or + /// versions), that photo becomes the active one, as in Lightroom Classic, so the change is on + /// screen (issue #293). It joins the selection if it isn't in it; nothing changes for steps + /// that touch several photos, albums or files, or for a photo not in the current view. + fn show_undone(&mut self, op: &Op) { + let Some(id) = single_photo(op, 0) else { return }; + if self.selection.active == Some(id) || !self.visible().contains(&id) { + return; + } + if !self.selection.contains(id) { + self.selection = Selection { ids: vec![id], active: Some(id) }; + } else { + self.selection.active = Some(id); + } + self.active_mask = None; + self.active_spot = None; + } + /// Apply an undo/redo op, first renaming the step's folder and moving the files its renames /// imply (all or nothing). Files that can't be moved back after a failure are reported and the /// library follows them (a logged change outside the undo history), so none goes missing; the diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index d005d508..83dad8db 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -817,3 +817,29 @@ fn face_job_follows_rotate_right() { let (pw, ph) = ((r.x1 - r.x0) * h, (r.y1 - r.y0) * w); assert!((pw - ph).abs() < 1e-6 * w.max(h), "square in rotated pixels: {pw} × {ph}"); } + +/// Undo and redo bring the photo they change on screen (issue #293): after editing one photo and +/// moving on, Undo makes it the active photo again; a step that changes several photos leaves the +/// selection alone. +#[test] +fn undo_and_redo_show_the_photo_they_change() { + let mut s = demo(); + let vis = s.visible_cloned(); + let (a, b) = (vis[0], vis[1]); + s.execute("library.select", &json!({"ids": [a.0]})).unwrap(); + s.execute("develop.set", &json!({"control": "light.exposure", "value": 1.0})).unwrap(); + s.execute("library.select", &json!({"ids": [b.0]})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.active(), Some(a), "undo shows the photo it changed"); + assert_eq!(s.selection.ids, vec![a]); + s.execute("library.select", &json!({"ids": [b.0]})).unwrap(); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(s.active(), Some(a), "so does redo"); + // rating both selected photos is one step over two photos: undoing it leaves the selection + s.execute("library.select", &json!({"ids": [a.0, b.0], "active": b.0})).unwrap(); + s.execute("photo.rate", &json!({"rating": 3})).unwrap(); + s.execute("library.select", &json!({"ids": [a.0, b.0], "active": a.0})).unwrap(); + let before = s.selection.clone(); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.selection, before, "a step over several photos keeps the selection"); +}