Merge branch 'fix/issue-97-folder-undo' into integrate/safety

# Conflicts:
#	crates/engine/src/lib.rs
#	docs/parity.md
#	docs/whats-new.md

crates/engine/src/cmd/browse.rs

@@ -166,7 +166,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Rename Folder",
             [],
             None,
-            "{path, name} — rename a folder on disk (its files and sidecars go along) and relink the photos in it; not an undo step → {path, relinked}",
+            "{path, name} — rename a folder on disk (its files and sidecars go along) and relink the photos in it; one undo step (undo renames it back, refused if the old name is taken) → {path, relinked}",
             always,
             |s, p| {
                 let from = str_param(p, "path").ok_or_else(|| bad("folder.rename", "missing path"))?.trim_end_matches(['/', '\\']).to_string();
@@ -184,7 +184,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Move Folder",
             [],
             None,
-            "{path, into: destination folder} — move a folder on disk into another and relink the photos in it; not an undo step → {path, relinked}",
+            "{path, into: destination folder} — move a folder on disk into another and relink the photos in it; one undo step (undo moves it back, refused if the old place is taken) → {path, relinked}",
             always,
             |s, p| {
                 let from = str_param(p, "path").ok_or_else(|| bad("folder.move", "missing path"))?.trim_end_matches(['/', '\\']).to_string();
@@ -217,58 +217,85 @@ pub fn specs() -> Vec<CommandSpec> {
     ]
 }
 
-/// Rename or move a folder on disk (everything in it goes along, sidecars included) and relink
-/// the photos inside. Like a rename in the file manager it is not an undo step: undoing would
-/// point the photos back at a folder that no longer exists.
-pub(crate) fn move_folder(s: &mut crate::Session, from: &str, to: &str) -> std::result::Result<usize, String> {
-    use std::path::Path;
+/// Rename `from` to `to` on disk (a folder; everything in it goes along). Never overwrites:
+/// refused when `to` exists (a case-only rename on a case-insensitive file system goes through
+/// a temporary name). Missing parents of `to` are created.
+pub(crate) fn rename_folder_on_disk(from: &str, to: &str) -> std::result::Result<(), String> {
     let (src, dst) = (Path::new(from), Path::new(to));
     if !src.is_dir() {
-        return Err(format!("{from} is not a folder"));
+        return Err(format!("{from} is not a folder (moved or deleted?)"));
     }
-    if dst.exists() {
+    let case_only = from != to && from.to_lowercase() == to.to_lowercase();
+    if dst.exists() && !case_only {
         return Err(format!("{to} already exists"));
     }
     if dst.starts_with(src) {
         return Err("a folder can't move into itself".into());
     }
-    if s.library.as_ref().is_some_and(|l| l.dir.starts_with(src) || src.starts_with(&l.dir)) {
-        return Err("the library's own folders can't be moved here".into());
-    }
     if let Some(parent) = dst.parent() {
         std::fs::create_dir_all(parent).map_err(|e| format!("{}: {e}", parent.display()))?;
     }
-    std::fs::rename(src, dst).map_err(|e| format!("{from} → {to}: {e}"))?;
-    let mut n = 0;
-    let ops: Vec<lightcraft_catalog::Op> = s
+    if case_only {
+        let tmp = src.with_file_name(format!(".lc-rename-{}", std::process::id()));
+        if tmp.exists() {
+            return Err(format!("{} already exists", tmp.display()));
+        }
+        std::fs::rename(src, &tmp).map_err(|e| format!("{from} → {to}: {e}"))?;
+        return std::fs::rename(&tmp, dst).map_err(|e| {
+            let _ = std::fs::rename(&tmp, src);
+            format!("{from} → {to}: {e}")
+        });
+    }
+    std::fs::rename(src, dst).map_err(|e| format!("{from} → {to}: {e}"))
+}
+
+/// After a folder moved from `from` to `to`: a browsed folder at or below it follows.
+pub(crate) fn follow_folder(s: &mut Session, from: &str, to: &str) {
+    if let Some(b) = &mut s.browse
+        && let Ok(rel) = Path::new(&b.path).strip_prefix(from)
+    {
+        b.path = Path::new(to).join(rel).to_string_lossy().to_string();
+        let f = b.path.clone();
+        s.filter.folder = Some(f);
+    }
+}
+
+/// Rename or move a folder on disk (everything in it goes along, sidecars included) and relink
+/// the photos inside, as one undo step: undo renames the folder back and restores the photos'
+/// paths, redo repeats the move — each refused (and reported) when the destination is taken.
+pub(crate) fn move_folder(s: &mut Session, from: &str, to: &str) -> std::result::Result<usize, String> {
+    let (src, dst) = (Path::new(from), Path::new(to));
+    if !src.is_dir() {
+        return Err(format!("{from} is not a folder"));
+    }
+    if s.library.as_ref().is_some_and(|l| l.dir.starts_with(src) || src.starts_with(&l.dir)) {
+        return Err("the library's own folders can't be moved here".into());
+    }
+    rename_folder_on_disk(from, to)?;
+    let ops: Vec<Op> = s
         .catalog
         .photos()
         .filter_map(|p| match &p.source {
             lightcraft_catalog::Source::File { path } => Path::new(path).strip_prefix(src).ok().map(|rel| (p.id, p.file_name.clone(), dst.join(rel))),
             _ => None,
         })
-        .map(|(id, file_name, np)| {
-            n += 1;
-            lightcraft_catalog::Op::Relink {
-                id,
-                file_name,
-                source: lightcraft_catalog::Source::File { path: np.to_string_lossy().to_string() },
-                format: None,
-            }
+        .map(|(id, file_name, np)| Op::Relink {
+            id,
+            file_name,
+            source: lightcraft_catalog::Source::File { path: np.to_string_lossy().to_string() },
+            format: None,
         })
         .collect();
-    for op in ops {
-        if s.catalog.apply(op.clone()).is_ok() {
-            s.pending_log.push(op);
-        }
-    }
-    // a browsed folder at or below it follows
-    if let Some(b) = &mut s.browse
-        && let Ok(rel) = Path::new(&b.path).strip_prefix(src)
-    {
-        b.path = dst.join(rel).to_string_lossy().to_string();
-        let f = b.path.clone();
-        s.filter.folder = Some(f);
+    let n = ops.len();
+    let name = Path::new(to).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default();
+    let renamed = src.parent() == dst.parent();
+    let label = if renamed { format!("Rename Folder to “{name}”") } else { format!("Move Folder “{name}”") };
+    let folder = crate::FolderMove { from: to.to_string(), to: from.to_string() };
+    if let Err(e) = s.commit_with_folder(&label, Op::Batch { ops }, folder) {
+        // the catalog didn't take it: put the folder back where the photos still point
+        let back = rename_folder_on_disk(to, from).err().map(|b| format!("; moving it back failed too: {b}")).unwrap_or_default();
+        return Err(format!("{e}{back}"));
     }
+    follow_folder(s, from, to);
     Ok(n)
 }

crates/engine/src/cmd/mod.rs

@@ -5,7 +5,7 @@
 //! `edit.*` (undo/redo), and queries (`catalog.query`, `photo.inspect`, `develop.get`…).
 
 mod before;
-mod browse;
+pub(crate) mod browse;
 mod color;
 pub(crate) mod convert;
 mod cull;

crates/engine/src/lib.rs

@@ -71,6 +71,22 @@ pub type Result<T> = std::result::Result<T, EngineError>;
 pub struct UndoEntry {
     pub label: String,
     pub op: Op,
+    /// A folder to rename on disk (`from` → `to`) before `op` is applied: Rename / Move Folder
+    /// (whose `op` relinks the photos inside). Never overwrites; refused when `to` exists.
+    pub folder: Option<FolderMove>,
+}
+
+/// A folder renamed or moved on disk as part of an undo step.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct FolderMove {
+    pub from: String,
+    pub to: String,
+}
+
+impl FolderMove {
+    fn reversed(&self) -> Self {
+        Self { from: self.to.clone(), to: self.from.clone() }
+    }
 }
 
 /// An in-progress slider drag / brush stroke: one undo step when it ends.
@@ -305,7 +321,7 @@ impl Session {
         let fwd = op.clone();
         let inv = self.catalog.apply(op)?;
         self.pending_log.push(fwd);
-        self.undo.push(UndoEntry { label: label.to_string(), op: inv });
+        self.undo.push(UndoEntry { label: label.to_string(), op: inv, folder: None });
         if self.undo.len() > 1000 {
             self.undo.remove(0);
         }
@@ -313,6 +329,16 @@ impl Session {
         Ok(())
     }
 
+    /// [`Session::commit`] for an op that goes with a folder already renamed on disk; the undo
+    /// step renames it back (`folder` is the undo direction: current place → old place).
+    pub(crate) fn commit_with_folder(&mut self, label: &str, op: Op, folder: FolderMove) -> Result<()> {
+        self.commit(label, op)?;
+        if let Some(e) = self.undo.last_mut() {
+            e.folder = Some(folder);
+        }
+        Ok(())
+    }
+
     /// Leaving photo `id` after editing it: keep its settings as an automatic version (when they
     /// differ from its latest version; at most [`AUTO_VERSIONS`] auto versions, oldest dropped).
     /// Saved with the library but not an undo step.
@@ -343,9 +369,13 @@ impl Session {
         if n < 2 || n > self.undo.len() {
             return;
         }
+        // a step that moves a folder on disk stays on its own
+        if self.undo[self.undo.len() - n..].iter().any(|e| e.folder.is_some()) {
+            return;
+        }
         let tail = self.undo.split_off(self.undo.len() - n);
         let ops = tail.into_iter().rev().map(|e| e.op).collect();
-        self.undo.push(UndoEntry { label: label.to_string(), op: Op::Batch { ops } });
+        self.undo.push(UndoEntry { label: label.to_string(), op: Op::Batch { ops }, folder: None });
     }
 
     /// Apply without recording undo (interactive previews).
@@ -356,7 +386,7 @@ impl Session {
 
     pub fn undo_step(&mut self) -> Result<String> {
         let e = self.undo.pop().ok_or_else(|| EngineError::Other("nothing to undo".into()))?;
-        let redo = match self.apply_with_files(&e.op) {
+        let redo = match self.apply_with_files(&e.op, e.folder.as_ref()) {
             Ok(r) => r,
             Err(err) => {
                 self.undo.push(e);
@@ -364,13 +394,13 @@ impl Session {
             }
         };
         self.pending_log.push(e.op);
-        self.redo.push(UndoEntry { label: e.label.clone(), op: redo });
+        self.redo.push(UndoEntry { label: e.label.clone(), op: redo, folder: e.folder.as_ref().map(FolderMove::reversed) });
         Ok(e.label)
     }
 
     pub fn redo_step(&mut self) -> Result<String> {
         let e = self.redo.pop().ok_or_else(|| EngineError::Other("nothing to redo".into()))?;
-        let undo = match self.apply_with_files(&e.op) {
+        let undo = match self.apply_with_files(&e.op, e.folder.as_ref()) {
             Ok(r) => r,
             Err(err) => {
                 self.redo.push(e);
@@ -378,26 +408,44 @@ impl Session {
             }
         };
         self.pending_log.push(e.op);
-        self.undo.push(UndoEntry { label: e.label.clone(), op: undo });
+        self.undo.push(UndoEntry { label: e.label.clone(), op: undo, folder: e.folder.as_ref().map(FolderMove::reversed) });
         Ok(e.label)
     }
 
-    /// Apply an undo/redo op, first 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.
-    fn apply_with_files(&mut self, op: &Op) -> Result<Op> {
+    /// 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
+    /// folder is only moved back when no file was left behind at its new path.
+    fn apply_with_files(&mut self, op: &Op, folder: Option<&FolderMove>) -> Result<Op> {
         let fs = rename::RealFs;
+        if let Some(f) = folder {
+            cmd::browse::rename_folder_on_disk(&f.from, &f.to).map_err(|e| EngineError::Other(format!("can't move the folder back: {e}")))?;
+        }
+        let undo_folder = || {
+            if let Some(f) = folder {
+                let _ = cmd::browse::rename_folder_on_disk(&f.to, &f.from);
+            }
+        };
         let moves = self.file_moves(op);
         if let Err(e) = rename::move_all(&fs, &moves) {
+            if e.stuck.is_empty() {
+                undo_folder();
+            }
             return Err(EngineError::Other(format!("can't move the files back: {}", self.follow_stuck(op, e, false))));
         }
         match self.catalog.apply(op.clone()) {
-            Ok(inv) => Ok(inv),
+            Ok(inv) => {
+                if let Some(f) = folder {
+                    cmd::browse::follow_folder(self, &f.from, &f.to);
+                }
+                Ok(inv)
+            }
             Err(e) => {
                 let back: Vec<(String, String)> = moves.iter().rev().map(|(a, b)| (b.clone(), a.clone())).collect();
                 if let Err(be) = rename::move_all(&fs, &back) {
                     return Err(EngineError::Other(format!("{e}; the files could not all be moved back: {}", be.message)));
                 }
+                undo_folder();
                 Err(e.into())
             }
         }

crates/engine/src/tests_import.rs

@@ -518,6 +518,70 @@ fn folders_rename_and_move_with_their_photos() {
     let _ = std::fs::remove_dir_all(&root);
 }
 
+/// Rename / Move Folder are undo steps (issue #97): undo renames the folder back on disk (files and
+/// sidecars along) and restores the photos' paths, redo repeats it; a destination taken in the
+/// meantime is never overwritten — the step is refused, reported and stays on the stack.
+#[test]
+fn folder_rename_and_move_undo_and_redo_on_disk() {
+    let root = temp_dir("folders-undo");
+    let a = root.join("Trip");
+    write_png(&a.join("one.png"), 3);
+    std::fs::write(a.join("one.xmp"), "<x:xmpmeta xmlns:x=\"adobe:ns:meta/\"/>").unwrap();
+    let mut s = Session::new().with_fs();
+    s.execute("library.import", &json!({"paths": [a.to_string_lossy()]})).unwrap();
+    let id = s.active().unwrap();
+    let path = |s: &Session| match &s.catalog.photo(id).unwrap().source {
+        lightcraft_catalog::Source::File { path } => path.clone(),
+        _ => panic!(),
+    };
+    let undo0 = s.undo.len();
+    let b = root.join("Italy");
+    s.execute("folder.rename", &json!({"path": a.to_string_lossy(), "name": "Italy"})).unwrap();
+    assert_eq!(s.undo.len(), undo0 + 1, "one undo step");
+    assert!(s.undo.last().unwrap().label.contains("Rename Folder"));
+    assert!(b.join("one.png").exists() && !a.exists());
+
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert!(a.join("one.png").exists() && a.join("one.xmp").exists() && !b.exists(), "renamed back on disk");
+    assert_eq!(path(&s), a.join("one.png").to_string_lossy());
+    s.media.forget(id);
+    assert!(s.render_now(id, 16, 16).is_ok());
+
+    s.execute("edit.redo", &json!({})).unwrap();
+    assert!(b.join("one.png").exists() && b.join("one.xmp").exists() && !a.exists());
+    assert_eq!(path(&s), b.join("one.png").to_string_lossy());
+
+    // the old name is taken now: undo is refused, nothing is overwritten, the step stays
+    std::fs::create_dir_all(&a).unwrap();
+    std::fs::write(a.join("keep.txt"), "mine").unwrap();
+    let undo_n = s.undo.len();
+    let e = s.execute("edit.undo", &json!({})).unwrap_err().to_string();
+    assert!(e.contains("already exists"), "{e}");
+    assert_eq!(s.undo.len(), undo_n, "the step stays on the stack");
+    assert_eq!(std::fs::read_to_string(a.join("keep.txt")).unwrap(), "mine");
+    assert!(b.join("one.png").exists());
+    assert_eq!(path(&s), b.join("one.png").to_string_lossy());
+    std::fs::remove_dir_all(&a).unwrap();
+
+    // move into another folder, undo, redo
+    let into = root.join("Archive");
+    s.execute("folder.move", &json!({"path": b.to_string_lossy(), "into": into.to_string_lossy()})).unwrap();
+    let moved = into.join("Italy");
+    assert_eq!(path(&s), moved.join("one.png").to_string_lossy());
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert!(b.join("one.png").exists() && !moved.exists());
+    assert_eq!(path(&s), b.join("one.png").to_string_lossy());
+    // the redo target is taken (even by an empty folder): refused, nothing replaced
+    std::fs::create_dir_all(&moved).unwrap();
+    assert!(s.execute("edit.redo", &json!({})).is_err());
+    assert!(b.join("one.png").exists());
+    std::fs::remove_dir(&moved).unwrap();
+    s.execute("edit.redo", &json!({})).unwrap();
+    assert!(moved.join("one.png").exists() && moved.join("one.xmp").exists() && !b.exists());
+    assert_eq!(path(&s), moved.join("one.png").to_string_lossy());
+    let _ = std::fs::remove_dir_all(&root);
+}
+
 /// Assisted culling: a burst (same scene, seconds apart) of a sharp and two soft shots, plus an
 /// unrelated photo: the burst is grouped, the sharp one is its best, blurry shots can be rejected.
 #[test]

docs/parity.md

@@ -714,7 +714,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06-
 | LRC-LIB-REFVIEW | Reference view | P2 | ✅ | `cmd:view.reference`, `cmd:photo.setReference`, `crates/ui-egui/src/panels/compare.rs` (`show_reference`) | View ▸ Reference View (⇧R): the reference photo beside the active one (edited with the Edit panel); photo menu ▸ Set as Reference Photo |
 | LRC-LIB-CATALOG-PANEL | Catalog sets | P1 | ✅ | `cmd:library.source` (`missing`) | all, recently added, picks, recently deleted, missing photos (when any) |
 | LRC-LIB-CATALOG-LOCK | Catalog open in one program at a time | P1 | ✅ | `cmd:app.openLibrary`, `crates/catalog/src/lock.rs`, `crates/engine/src/library.rs` (`open_library`) | exclusive OS lock on `catalog.lock` for the session (app and `lightcraft-cli --library`); a second opener is refused with who has it (`catalog.lock.owner`: program, process, computer); released by the OS on a crash, so never stale; file systems without locks open unprotected with a warning; no read-only mode; the browser build (OPFS) has no guard across tabs yet. A library that can't be opened at launch (locked, unreadable, newer format, drive missing) shows a window — Try Again, Choose Another Library…, Continue Without Saving (then a banner; nothing written), Quit — instead of a silent demo session (`crates/ui-egui/src/panels/library_problem.rs`) |
-| LRC-LIB-FOLDERS | Disk folder tree | P1 | ✅ | `cmd:library.browse`, `cmd:folder.rename`, `cmd:folder.move`, `crates/ui-egui/src/panels/left.rs` (`local_section`, `folder_tree`) | standard places + the browsed folder + Browse Folder…, each an expandable tree of subfolders, breadcrumb navigation; context menu: Rename Folder…, Move Folder To…, Show in Finder (on disk, photos relinked; like the file manager, not an undo step) |
+| LRC-LIB-FOLDERS | Disk folder tree | P1 | ✅ | `cmd:library.browse`, `cmd:folder.rename`, `cmd:folder.move`, `crates/ui-egui/src/panels/left.rs` (`local_section`, `folder_tree`) | standard places + the browsed folder + Browse Folder…, each an expandable tree of subfolders, breadcrumb navigation; context menu: Rename Folder…, Move Folder To…, Show in Finder (on disk, photos relinked; one undo step each: undo / redo move the folder back and forth, refused when the destination is taken) |
 | LRC-LIB-COLLECTIONS | Collections & sets | P1 | ✅ | `cmd:album.create`, `cmd:album.createSmart`, `cmd:album.toggleTarget`, `cmd:album.setTarget`, `cmd:album.clearQuick` | albums + folders (sets), smart albums (rule editor), Quick Collection and target album (B in the grids adds / removes the selection; album menu ▸ Set as Target Album; the target is marked +) |
 | LRC-LIB-SMARTCOLL | Smart-collection rules | P1 | ✅ | `cmd:album.createSmart`, `cmd:album.setRules`, `cmd:album.ruleFields`, `cmd:dialog.smartAlbum`, `crates/catalog/src/rules.rs`, `crates/ui-egui/src/panels/rules_editor.rs` | rule editor: match all / any / none, nested groups (⌥+ or + Group), 26 fields (rating, flag, label, type, edits, keywords, any text, filename, format, title, caption, camera, lens, location, creator, copyright, copyright status, capture / import / edit date, ISO, aperture, focal length, megapixels, GPS, virtual copy, album) with text / number / date / in-the-last operators; live match count; also usable as a library filter (`ruleSet`) |
 | LRC-LIB-PUBLISH | Publish services | P2 | ⬜ | | |

docs/whats-new.md

@@ -68,6 +68,9 @@
   so none shows as missing. Renaming one of a raw + JPEG pair copies their shared `IMG_0001.xmp` instead of taking
   it away from the other (issue #92). Find Missing Photos also finds renamed files by their content, prefers a content match
   over a same-name same-size look-alike, and skips (and reports) photos it can't tell apart instead of guessing.
+- Rename Folder… and Move Folder To… (Local) can be undone and redone (issue #97): the folder moves back on disk with
+  its photos and XMP sidecars, and the photos point at it again. If something now occupies the old place, the undo
+  is refused and nothing is overwritten.
 - Smart albums with a rule editor: match all / any / none, nested groups, 26 fields.
 - Quick Collection and target album (B in the grid), keyword sets (⌥1–⌥9), colour-label sets.
 - Colour-label filter with several labels at once; expandable folder tree in Local.