diff --git a/crates/catalog/src/folders.rs b/crates/catalog/src/folders.rs index da21328ad..aab3b867b 100644 --- a/crates/catalog/src/folders.rs +++ b/crates/catalog/src/folders.rs @@ -205,7 +205,7 @@ impl Catalog { /// (the same place below it), for a folder renamed or moved on disk: commit them with the /// photos' relinks so undo puts both back. Records already at or below `to` are left from a /// folder that is no longer there and are dropped (undo restores them). Empty when there is - /// nothing to carry. + /// nothing to carry or drop. pub fn folder_records_follow(&self, from: &str, to: &str) -> Vec { let (Some(root), Some(dest)) = (record_key(from), record_key(to)) else { return Vec::new() }; if root == dest { @@ -222,9 +222,8 @@ impl Catalog { Some((folder_key(&path), r.clone())) }) .collect(); - if moved.is_empty() { - return Vec::new(); - } + // what was known at the old place goes, and so does what is left at the new one (also + // when the moved folder carries nothing: the folder now there is not the one it was about) let stale = self.folder_records.keys().filter(|k| key_within(k, &root) || key_within(k, &dest)); let mut ops: Vec = stale.map(|k| Op::SetFolderRecord { folder: k.clone(), record: None }).collect(); ops.extend(moved.into_iter().map(|(k, r)| Op::SetFolderRecord { folder: k, record: Some(r) })); diff --git a/crates/catalog/src/tests_folder_records.rs b/crates/catalog/src/tests_folder_records.rs index 4e172f47f..3ae19c33a 100644 --- a/crates/catalog/src/tests_folder_records.rs +++ b/crates/catalog/src/tests_folder_records.rs @@ -149,6 +149,38 @@ fn a_folder_without_labels_moves_without_ops() { assert!(c.folder_records_follow("/pics/trip", "/elsewhere/trip").is_empty()); } +#[test] +fn an_unlabelled_folder_moved_onto_a_stale_label_does_not_inherit_it() { + let mut c = library(); + // a folder that was at the destination once (its photos are gone) left its label behind + label(&mut c, "/pics/holiday", Some(ColorLabel::Purple)); + let ops = c.folder_records_follow("/pics/trip", "/pics/holiday"); + let undo = c.apply(Op::Batch { ops }).unwrap(); + assert_eq!(c.folder_record("/pics/holiday"), None, "the folder now there is another one"); + c.apply(undo).unwrap(); + assert_eq!(c.folder_color_label("/pics/holiday"), Some(ColorLabel::Purple)); +} + +#[test] +fn labels_survive_the_journal_without_a_snapshot() { + let m = MemStore::new(); + let (mut j, mut c, _) = Journal::open(Box::new(m.clone())).unwrap(); + { + let p = "/pics/trip/a.jpg"; + let id = c.alloc_photo_id(); + let op = Op::AddPhoto { photo: Box::new(Photo::new(id, Source::File { path: p.into() }, "a.jpg", "JPEG", 6, 4, "2026-01-01")) }; + c.apply(op.clone()).unwrap(); + j.append(std::slice::from_ref(&op)).unwrap(); + } + let op = c.folder_label_op("/pics/trip", Some(ColorLabel::Yellow)); + c.apply(op.clone()).unwrap(); + j.append(std::slice::from_ref(&op)).unwrap(); + drop(j); + let (_, reopened, r) = Journal::open(Box::new(m)).unwrap(); + assert!(r.replayed >= 2, "{r:?}"); + assert_eq!(reopened.folder_color_label("/pics/trip"), Some(ColorLabel::Yellow)); +} + #[test] fn labels_survive_closing_and_reopening_the_library() { let mut c = library(); diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 0ae6d4ba0..569716596 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -392,7 +392,7 @@ pub fn specs() -> Vec { "Set Folder Color Label", [], None, - "{path, label: red|yellow|green|blue|purple|none} — give a folder of the library (one its photos were imported from, see library.folders) a colour label, or take it off; one undo step. The label follows the folder when it is renamed or moved with folder.rename / folder.move → {path, label}", + "{path, label: red|yellow|green|blue|purple|none} — give a folder of the library (one its photos were imported from, see library.folders) a colour label, or take it off (also once its photos are gone); one undo step. The label follows the folder when it is renamed or moved with folder.rename / folder.move → {path, label}", always, |s, p| { const C: &str = "folder.label"; @@ -401,10 +401,14 @@ pub fn specs() -> Vec { "none" => None, x => Some(ColorLabel::parse(x).ok_or_else(|| bad(C, format!("unknown label {x:?}")))?), }; + // taking a label off always works, also once the folder's photos are gone let f = Filter { library_folder: Some(path.to_string()), ..Default::default() }; - if s.catalog.query(&f, &Sort::default()).is_empty() { + if label.is_some() && s.catalog.query(&f, &Sort::default()).is_empty() { return Err(bad(C, format!("{path}: no photo in the library was imported from it"))); } + if label.is_none() && s.catalog.folder_record(path).is_none() { + return Ok(json!({"path": path, "label": label})); + } let op = s.catalog.folder_label_op(path, label); s.commit(if label.is_some() { "Set Folder Color Label" } else { "Remove Folder Color Label" }, op)?; Ok(json!({"path": path, "label": label})) diff --git a/crates/engine/src/tests_folders.rs b/crates/engine/src/tests_folders.rs index 2e60ce1ad..9f51ab408 100644 --- a/crates/engine/src/tests_folders.rs +++ b/crates/engine/src/tests_folders.rs @@ -548,4 +548,19 @@ fn labels_follow_a_moved_folder() { s.execute("folder.label", &json!({"path": trip, "label": "yellow"})).unwrap(); s.execute("folder.move", &json!({"path": trip, "into": dir.path("archive")})).unwrap(); assert_eq!(listed_label(&mut s, &dir.path("archive/trip")).as_deref(), Some("yellow")); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(listed_label(&mut s, &trip).as_deref(), Some("yellow"), "undo moves it back with the folder"); + assert_eq!(s.catalog.folder_record(&dir.path("archive/trip")), None); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(listed_label(&mut s, &dir.path("archive/trip")).as_deref(), Some("yellow")); +} + +#[test] +fn a_label_comes_off_a_folder_whose_photos_are_gone() { + let mut s = session(); + s.execute("folder.label", &json!({"path": "/pics/trip", "label": "red"})).unwrap(); + s.execute("library.removeFolder", &json!({"path": "/pics/trip"})).unwrap(); + s.execute("folder.label", &json!({"path": "/pics/trip", "label": "none"})).unwrap(); + assert_eq!(s.catalog.folder_record("/pics/trip"), None); + assert!(s.execute("folder.label", &json!({"path": "/pics/trip", "label": "red"})).is_err(), "but a new one needs photos"); }