diff --git a/crates/engine/src/cmd/browse.rs b/crates/engine/src/cmd/browse.rs index 2730822c..a8a9b448 100644 --- a/crates/engine/src/cmd/browse.rs +++ b/crates/engine/src/cmd/browse.rs @@ -166,7 +166,7 @@ pub fn specs() -> Vec { "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 { "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 { ] } -/// 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 { - 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 = 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 { + 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 = 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) } diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index ac379986..ad7b0348 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/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; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 2d68329f..bb4760ad 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -71,6 +71,22 @@ pub type Result = std::result::Result; 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, +} + +/// 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 { 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 { 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 { + /// 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 { 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()) } } diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index 2e2bb3fa..e83a4d34 100644 --- a/crates/engine/src/tests_import.rs +++ b/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"), "").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] diff --git a/docs/parity.md b/docs/parity.md index 231b2400..ac62ea34 100644 --- a/docs/parity.md +++ b/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 | ⬜ | | | diff --git a/docs/whats-new.md b/docs/whats-new.md index dc7012dc..ce7cd6c1 100644 --- a/docs/whats-new.md +++ b/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.