From b26d5271b81cd00a0f0165c6f7613c39c434ea4d Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 17:40:20 -0400 Subject: [PATCH] Folder labels: drop stale labels on any move; clear labels of empty folders Review fixes. folder_records_follow now drops records left at the destination even when the moved folder carries none, so an unlabelled folder renamed onto an old path no longer inherits that path's label. folder.label {label: none} works once the folder's photos are gone (a label can no longer get stuck); setting a label still needs photos. Adds tests for both, undo/redo of a labelled move, and labels replayed from the journal without a snapshot. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/folders.rs | 7 ++--- crates/catalog/src/tests_folder_records.rs | 32 ++++++++++++++++++++++ crates/engine/src/cmd/library.rs | 8 ++++-- crates/engine/src/tests_folders.rs | 15 ++++++++++ 4 files changed, 56 insertions(+), 6 deletions(-) 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"); }