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 <noreply@anthropic.com>
This commit is contained in:
Nathaniel Ledford
2026-10-09 17:40:20 -04:00
co-authored by Claude Opus 5.5
parent c40a5675bb
commit b26d5271b8
4 changed files with 56 additions and 6 deletions
+3 -4
View File
@@ -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<Op> {
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<Op> = 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) }));
@@ -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();
+6 -2
View File
@@ -392,7 +392,7 @@ pub fn specs() -> Vec<CommandSpec> {
"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<CommandSpec> {
"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}))
+15
View File
@@ -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");
}