diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -10,7 +10,7 @@ //! photo has them, or given attributes ([`KeywordInfo`]: synonyms, export options, person). The //! tree is the keyword list together with the keywords photos carry. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap, HashSet}; use serde::{Deserialize, Serialize}; @@ -35,6 +35,18 @@ pub fn is_under(k: &str, parent: &str) -> bool { k == p || k.strip_prefix(&p).is_some_and(|rest| rest.starts_with(SEP)) } +/// The upper levels of a case folded keyword path, each as a prefix of it ending before a `|` +/// (`a|b|c` gives `a`, `a|b`), the way [`is_under`] sees them. +fn upper_levels(folded: &str) -> impl Iterator { + folded.match_indices(SEP).map(|(i, _)| &folded[..i]) +} + +/// A case folded path and its [`upper_levels`] that are as many bytes as one of `lens`: the only +/// ones that can be in a set of paths of those lengths, so only those are worth hashing. +fn named_levels<'a>(folded: &'a str, lens: &'a HashSet) -> impl Iterator { + std::iter::once(folded).chain(upper_levels(folded)).filter(|level| lens.contains(&level.len())) +} + /// Replace the `from` prefix of `k` (which [`is_under`] `from`) with `to`. By levels, not bytes: /// `from` may be written in another case, which can change a letter's length (ẞ / ß). pub fn reparent(k: &str, from: &str, to: &str) -> String { @@ -383,6 +395,63 @@ impl Catalog { }) } + /// What [`Self::list_move_ops`] of each `(from, to)` of `moves` in turn gives, one after the + /// other, without a pass over the keyword list and a copy of it per move: the moves are keywords + /// none of which is under another, so each listing leaves for at most one of them, found by + /// its levels. Each move's ops are as `list_ops` would make them: what leaves and doesn't land + /// again is taken off, then what lands (or was merged into) is set, both in key order. + fn list_moves_ops(&self, moves: &[(String, String)]) -> Vec { + let list = &self.keyword_list; + let moving: HashMap = moves.iter().enumerate().map(|(i, (f, _))| (f.to_lowercase(), i)).collect(); + let lens: HashSet = moving.keys().map(String::len).collect(); + let mut leaving: Vec> = vec![Vec::new(); moves.len()]; + for l in list.values() { + let folded = l.path.to_lowercase(); + if let Some(i) = named_levels(&folded, &lens).find_map(|level| moving.get(level)) { + leaving[*i].push(l); + } + } + let mut ops = Vec::new(); + for ((f, t), left) in moves.iter().zip(&leaving) { + let leaves: HashSet = left.iter().map(|l| l.path.to_lowercase()).collect(); + // the landings, and what they merged into, by key (what `list_move_ops` has in its copy + // of the list that isn't what the list has) + let mut after: BTreeMap = BTreeMap::new(); + for l in left { + let path = reparent(&l.path, f, t); + let key = path.to_lowercase(); + if !after.contains_key(&key) + && let Some(there) = list.get(&key).filter(|_| !leaves.contains(&key)) + { + after.insert(key.clone(), there.clone()); + } + match after.get_mut(&key) { + Some(there) => { + there.info.new_keywords_inside |= l.info.new_keywords_inside; + for s in &l.info.synonyms { + if !there.info.synonyms.iter().any(|x| same(x, s)) { + there.info.synonyms.push(s.clone()); + } + } + } + None => { + after.insert(key, ListedKeyword { path, info: l.info.clone() }); + } + } + } + ops.extend( + left.iter().filter(|l| !after.contains_key(&l.path.to_lowercase())).map(|l| Op::SetKeyword { path: l.path.clone(), info: None }), + ); + ops.extend( + after + .iter() + .filter(|(k, l)| list.get(*k) != Some(*l)) + .map(|(_, l)| Op::SetKeyword { path: l.path.clone(), info: Some(l.info.clone()) }), + ); + } + ops + } + /// Rename `from` (and the keywords below it) to `to` on every photo and in the keyword list — /// one batch. Renaming onto an existing keyword merges the two. pub fn rename_keyword_ops(&self, from: &str, to: &str) -> Result { @@ -409,6 +478,118 @@ impl Catalog { Ok(Op::Batch { ops }) } + /// [`Self::delete_keyword_ops`] for several keywords at once: one batch, so one undo step. + /// Unlike one keyword (which may be absent and then changes nothing), every keyword named must + /// exist, or the whole batch is refused. + pub fn delete_keywords_ops(&self, keywords: &[String]) -> Result { + let gone: Vec = keywords.iter().map(|k| clean(k)).collect(); + if gone.is_empty() || gone.iter().any(String::is_empty) { + return Err(CatalogError::Invalid("empty keyword".into())); + } + // Case folded once here, and each candidate once below. A candidate is under a named keyword + // when its folded path or one of its upper levels is in `named`: a lookup per level, not a + // comparison with every keyword named. Only the levels as long as some named keyword are + // hashed (see `named_levels`): at most one per named length, so a very deep keyword on a + // photo costs its length plus at most the bytes of the keywords named, not every prefix. + let mut named: HashMap = HashMap::with_capacity(gone.len()); + for (i, g) in gone.iter().enumerate() { + named.entry(g.to_lowercase()).or_insert(i); + } + let lens: HashSet = named.keys().map(String::len).collect(); + let mut found = vec![false; gone.len()]; + let listed = self.keyword_list.values().map(|l| &l.path); + for k in listed.chain(self.photos().filter(|p| p.in_library()).flat_map(|p| p.meta.keywords.iter())) { + let k = k.to_lowercase(); + for hit in named_levels(&k, &lens).filter_map(|level| named.get(level)) { + found[*hit] = true; + } + } + // a keyword named twice (or in two cases) is one entry of `named`: it is found as that one + for (i, g) in gone.iter().enumerate() { + found[i] = found[named[&g.to_lowercase()]]; + } + if let Some(i) = found.iter().position(|hit| !hit) { + return Err(CatalogError::Invalid(format!("no keyword “{}”", gone[i]))); + } + let goes = |k: &str| { + let k = k.to_lowercase(); + named_levels(&k, &lens).any(|level| named.contains_key(level)) + }; + let mut ops = self.keyword_ops(|kws| kws.iter().filter(|x| !goes(x)).cloned().collect()); + ops.extend(self.list_ops(|list| list.retain(|_, l| !goes(&l.path)))); + Ok(Op::Batch { ops }) + } + + /// Move several keywords (each with the keywords below it) inside `parent`, or to the top + /// level (`None`), as one batch. Each keeps its name. Of a keyword and one below it only the + /// upper one moves (the lower goes with it). Nothing merges here: a name that is taken where + /// they go ([`CatalogError::KeywordExists`]), or two of them with one name, refuses the whole + /// move. Returns the batch and each move as `(from, to)`. + pub fn move_keywords_ops(&self, keywords: &[String], parent: Option<&str>) -> Result<(Op, Vec<(String, String)>)> { + // Every lookup is built once for the whole call: the keywords' own paths (and the parent's) + // by `keyword_paths`, the names they move to by a second one; paths are case folded once and + // kept in sets or maps, and each photo's keywords and each listing are matched to the moves + // by level, not against every move. So the cost is the library's keywords, not their + // product with the selection. + let parent = parent.map(clean).filter(|p| !p.is_empty()); + let mut queries: Vec<&str> = keywords.iter().map(String::as_str).collect(); + queries.extend(parent.as_deref()); + let paths = self.keyword_paths(&queries); + let path_of = |k: &str| paths.get(&clean(k).to_lowercase()); + let mut from: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + for k in keywords { + let path = path_of(k).ok_or_else(|| CatalogError::Invalid(format!("no keyword “{}”", clean(k))))?; + if seen.insert(path.to_lowercase()) { + from.push(path.clone()); + } + } + if from.is_empty() { + return Err(CatalogError::Invalid("no keyword to move".into())); + } + // the upper ones only: a keyword goes when one of its upper levels is named too + from.retain(|f| !upper_levels(&f.to_lowercase()).any(|up| seen.contains(up))); + let upper: HashSet = from.iter().map(|f| f.to_lowercase()).collect(); + let parent = parent.map(|p| path_of(&p).cloned().unwrap_or(p)); + if let Some(p) = &parent { + let p = p.to_lowercase(); + if upper.contains(&p) || upper_levels(&p).any(|up| upper.contains(up)) { + return Err(CatalogError::Invalid("can't move a keyword inside itself".into())); + } + } + let mut moves: Vec<(String, String)> = Vec::new(); + let mut names: HashSet = HashSet::new(); + for f in &from { + let leaf = f.rsplit(SEP).next().unwrap_or(f); + let to = parent.as_deref().map_or_else(|| leaf.to_string(), |p| format!("{p}{SEP}{leaf}")); + if !names.insert(to.to_lowercase()) { + return Err(CatalogError::Invalid(format!("two of them are named “{leaf}”: move them one at a time"))); + } + moves.push((f.clone(), to)); + } + // one that is there already stays; the others must find their name free + moves.retain(|(f, t)| !same(f, t)); + let targets: Vec<&str> = moves.iter().map(|(_, t)| t.as_str()).collect(); + let existing = self.keyword_paths(&targets); + if let Some((_, taken)) = moves.iter().find(|(_, t)| existing.contains_key(&clean(t).to_lowercase())) { + return Err(CatalogError::KeywordExists(taken.clone())); + } + // Case folded once per photo keyword; its path and its upper levels are looked up in `moving` + // (folded from-path to its move), not compared with every move; only those as long as some + // move's path are hashed (see `named_levels`). The moves are the upper keywords only, so at + // most one level of a keyword is among them. + let moving: HashMap = moves.iter().map(|m| (m.0.to_lowercase(), m)).collect(); + let lens: HashSet = moving.keys().map(String::len).collect(); + let moved = |k: &str| { + let k_folded = k.to_lowercase(); + named_levels(&k_folded, &lens).find_map(|level| moving.get(level)).map(|(f, t)| reparent(k, f, t)) + }; + let mut ops = self.keyword_ops(|kws| kws.iter().map(|k| moved(k).unwrap_or_else(|| k.clone())).collect()); + // (the moves touch apart parts of the list: each one's listings are its own) + ops.extend(self.list_moves_ops(&moves)); + Ok((Op::Batch { ops }, moves)) + } + /// Merge several keywords (with their children) into `into`. pub fn merge_keywords_ops(&self, from: &[String], into: &str) -> Result { let into = clean(into); @@ -685,6 +866,45 @@ impl Catalog { .map(|x| x.split(SEP).map(str::trim).filter(|s| !s.is_empty()).take(levels).collect::>().join("|")) } + /// [`Self::keyword_path`] of every one of `queries` in one pass over the keyword list and the + /// photos, not one pass each: case folded cleaned query → its path. A query with no keyword (or + /// an empty one) has no entry. Only the levels as long as some query are hashed + /// (see `named_levels`). + fn keyword_paths(&self, queries: &[&str]) -> HashMap { + let mut want: HashMap> = HashMap::with_capacity(queries.len()); + let mut missing = 0usize; + for q in queries { + let k = clean(q); + if k.is_empty() { + continue; + } + let folded = k.to_lowercase(); + if want.contains_key(&folded) { + continue; + } + let listed = self.keyword_list.get(&folded).map(|l| l.path.clone()); + missing += usize::from(listed.is_none()); + want.insert(folded, listed); + } + let lens: HashSet = want.keys().map(String::len).collect(); + // the levels of a listed keyword below it, or of a photo's: the first one found names the path + let listed = self.keyword_list.values().map(|l| &l.path); + for x in listed.chain(self.photos().filter(|p| p.in_library()).flat_map(|p| p.meta.keywords.iter())) { + if missing == 0 { + break; + } + let folded = x.to_lowercase(); + for level in named_levels(&folded, &lens) { + if let Some(slot @ None) = want.get_mut(level) { + let levels = level.split(SEP).count(); + *slot = Some(x.split(SEP).map(str::trim).filter(|s| !s.is_empty()).take(levels).collect::>().join("|")); + missing -= 1; + } + } + } + want.into_iter().filter_map(|(k, v)| v.map(|v| (k, v))).collect() + } + /// Create a keyword with these attributes, and give it to `photos` — one batch. A keyword that /// exists already is refused. pub fn create_keyword_ops(&self, path: &str, info: KeywordInfo, photos: &[crate::PhotoId]) -> Result { diff --git a/crates/catalog/src/keywords/tests.rs b/crates/catalog/src/keywords/tests.rs --- a/crates/catalog/src/keywords/tests.rs +++ b/crates/catalog/src/keywords/tests.rs @@ -704,3 +704,113 @@ fn keyword_tree_caps_a_keyword_at_the_list_depth() { let deepest = tree_paths(&c.keyword_tree()).iter().map(|(p, _)| p.split(SEP).count()).max().unwrap_or(0); assert_eq!(deepest, MAX_LEVELS); } + +/// A batch delete takes each named keyword with what is below it, at any depth and in any case, +/// and leaves a keyword that only shares a name prefix ("Cats" next to "Cat"). +#[test] +fn batch_delete_takes_nested_keywords_in_any_case_and_not_a_shared_name_prefix() { + let (mut c, ids) = lib(&[&["Cat", "Cats", "Animals|CAT|Kitten|Tiny", "Animals|Dog", "Cat food"]]); + let op = c.delete_keywords_ops(&["cat".to_string(), "animals|dog".to_string()]).unwrap(); + c.apply(op).unwrap(); + assert_eq!(kws(&c, ids[0]), ["Cats", "Animals|CAT|Kitten|Tiny", "Cat food"]); + // Named twice, in two cases, and with one below the other: still one batch that works. + let op = c.delete_keywords_ops(&["Animals".to_string(), "ANIMALS".to_string(), "animals|cat".to_string()]).unwrap(); + c.apply(op).unwrap(); + assert_eq!(kws(&c, ids[0]), ["Cats", "Cat food"]); +} + +/// Every keyword named must exist: "Cat" isn't there because "Cats" is, and a keyword below a +/// named one doesn't make a second name exist. +#[test] +fn batch_delete_refuses_a_name_that_only_prefixes_another() { + let (c, _) = lib(&[&["Cats", "Animals|Dog"]]); + let err = c.delete_keywords_ops(&["Cats".to_string(), "Cat".to_string()]).unwrap_err(); + assert!(err.to_string().contains("no keyword “Cat”"), "{err}"); + let err = c.delete_keywords_ops(&["Animals".to_string(), "Animals|Do".to_string()]).unwrap_err(); + assert!(err.to_string().contains("no keyword “Animals|Do”"), "{err}"); +} + +/// A batch move reparents each moved keyword with what is below it, at any depth and in any +/// case, and leaves a keyword that only shares a name prefix with a moved one. +#[test] +fn batch_move_reparents_nested_keywords_in_any_case_and_not_a_shared_name_prefix() { + let (mut c, ids) = lib(&[&["Cat", "Cats", "Pets|DOG|Puppy|Tiny", "Pets|Doge", "Home"], &["pets|dog|puppy", "PETS|DOG"]]); + let (op, moves) = c.move_keywords_ops(&["cat".to_string(), "pets|dog".to_string()], Some("Home")).unwrap(); + c.apply(op).unwrap(); + assert_eq!(moves, [("Cat".to_string(), "Home|Cat".to_string()), ("Pets|DOG".to_string(), "Home|DOG".to_string())]); + assert_eq!(kws(&c, ids[0]), ["Home|Cat", "Cats", "Home|DOG|Puppy|Tiny", "Pets|Doge", "Home"]); + // a photo's own spelling of a moved keyword, in another case, moves too + assert_eq!(kws(&c, ids[1]), ["Home|DOG|puppy", "Home|DOG"]); +} + +fn list_it(c: &mut Catalog, path: &str, words: &str) { + let words: Vec<&str> = words.split_whitespace().collect(); + c.apply(Op::SetKeyword { path: path.into(), info: Some(with_synonyms(&words)) }).unwrap(); +} + +/// A batch move refuses, with the same message as before, a keyword that isn't there, a name that +/// is taken where it goes (by a photo's keyword, or by one only below a listed one), a move +/// inside itself and two keywords with one name. +#[test] +fn batch_move_refusals() { + let (mut c, _) = lib(&[&["Cat", "Pets|Dog", "Home|Cat", "A|Fish", "B|fish", "Top", "Pets"]]); + list_it(&mut c, "Home|Zoo|Lion", ""); + let s = |v: &[&str]| v.iter().map(|k| k.to_string()).collect::>(); + let err = c.move_keywords_ops(&s(&["cat", "Nope"]), Some("Home")).unwrap_err(); + assert!(err.to_string().contains("no keyword “Nope”"), "{err}"); + assert!(matches!(c.move_keywords_ops(&s(&["cat"]), Some("home")), Err(CatalogError::KeywordExists(t)) if t == "Home|Cat")); + assert!(c.move_keywords_ops(&s(&["Pets"]), Some("Home|Zoo")).is_ok()); + list_it(&mut c, "Home|Pets|Bird", ""); + assert!(matches!(c.move_keywords_ops(&s(&["Pets"]), Some("Home")), Err(CatalogError::KeywordExists(t)) if t == "Home|Pets")); + let err = c.move_keywords_ops(&s(&["Pets"]), Some("pets|dog")).unwrap_err(); + assert!(err.to_string().contains("can't move a keyword inside itself"), "{err}"); + let err = c.move_keywords_ops(&s(&["a|fish", "b|FISH"]), Some("Top")).unwrap_err(); + assert!(err.to_string().contains("two of them are named “fish”"), "{err}"); + let err = c.move_keywords_ops(&[], None).unwrap_err(); + assert!(err.to_string().contains("no keyword to move"), "{err}"); +} + +/// A batch move takes the listings (with their attributes) under each moved keyword along, and +/// leaves the others. +#[test] +fn batch_move_takes_listings_along() { + let (mut c, _) = lib(&[&["Home"]]); + for (p, w) in [("Pets|Dog", "hound"), ("Pets|Dog|Puppy", "pup"), ("Pets|Doge", "meme"), ("Cat", "kitty"), ("Home", "")] { + list_it(&mut c, p, w); + } + let (op, _) = c.move_keywords_ops(&["pets|dog".to_string(), "CAT".to_string()], Some("Home")).unwrap(); + c.apply(op).unwrap(); + let now: Vec<(&str, Vec<&str>)> = + c.keyword_list.values().map(|l| (l.path.as_str(), l.info.synonyms.iter().map(String::as_str).collect())).collect(); + assert_eq!( + now, + [("Home", vec![]), ("Home|Cat", vec!["kitty"]), ("Home|Dog", vec!["hound"]), ("Home|Dog|Puppy", vec!["pup"]), ("Pets|Doge", vec!["meme"])] + ); +} + +/// The one pass over the keyword list that moves every listing at once gives the ops of one +/// `list_move_ops` per move, in the same order, also where a landing merges into a listing that +/// is there already. +#[test] +fn batch_list_moves_equal_one_list_move_per_move() { + let (mut c, _) = lib(&[&[]]); + for (p, w) in [ + ("Pets|Dog", "hound"), + ("Pets|Dog|Puppy", "pup"), + ("Pets|Doge", "meme"), + ("Cat", "kitty"), + ("Home|Dog", "canine"), + ("Home|Dog|Puppy", "pup"), + ("Home", ""), + ] { + list_it(&mut c, p, w); + } + for moves in [ + vec![("Pets|Dog".to_string(), "Home|Dog".to_string()), ("Cat".to_string(), "Home|Cat".to_string())], + vec![("Cat".to_string(), "Top".to_string())], + vec![], + ] { + let one_by_one: Vec = moves.iter().flat_map(|(f, t)| c.list_move_ops(std::slice::from_ref(f), t)).collect(); + assert_eq!(c.list_moves_ops(&moves), one_by_one, "{moves:?}"); + } +} diff --git a/crates/engine/src/cmd/keywords.rs b/crates/engine/src/cmd/keywords.rs --- a/crates/engine/src/cmd/keywords.rs +++ b/crates/engine/src/cmd/keywords.rs @@ -57,6 +57,40 @@ fn info_from(p: &Value, base: KeywordInfo) -> KeywordInfo { } } +/// `keyword.move`'s `parent`: a keyword, or null for the top level. +fn move_parent(p: &Value) -> Result> { + match p.get("parent") { + Some(Value::String(k)) => Ok(Some(k.clone())), + Some(Value::Null) => Ok(None), + _ => Err(bad("keyword.move", "`parent` is a keyword, or null for the top level")), + } +} + +/// How many keywords one call may name (far more than a person chooses in a list). +const MAX_KEYWORDS: usize = 10_000; + +/// The keywords a command names in `keywords`: a list of them, each once (whatever its case), +/// none empty. +fn keyword_list(p: &Value, command: &str) -> Result> { + let list = p.get("keywords").and_then(Value::as_array).ok_or_else(|| bad(command, "`keywords` must be a list of keywords"))?; + if list.len() > MAX_KEYWORDS { + return Err(bad(command, format!("`keywords` names more than {MAX_KEYWORDS} keywords"))); + } + let mut out: Vec = Vec::new(); + let mut seen = std::collections::HashSet::new(); + for k in list { + let k = + k.as_str().map(clean).filter(|k| !k.is_empty()).ok_or_else(|| bad(command, format!("`keywords` must be a list of keywords, not {k}")))?; + if seen.insert(k.to_lowercase()) { + out.push(k); + } + } + if out.is_empty() { + return Err(bad(command, "`keywords` names no keyword")); + } + Ok(out) +} + /// The keyword a command names (`keyword`), as the library writes it. fn named_keyword(s: &Session, p: &Value, command: &str) -> Result { let k = str_param(p, "keyword").ok_or_else(|| bad(command, "missing `keyword`"))?; @@ -408,15 +442,23 @@ pub fn specs() -> Vec { "Move Keyword", [], None, - "{keyword, parent: keyword | null (the top level), merge?: bool} — with the keywords below it; onto a name that is taken only with `merge: true`", + "{keyword, parent: keyword | null (the top level), merge?: bool} — with the keywords below it; onto a name that is taken only with `merge: true`. Several at once: {keywords: [keyword], parent} as one undo step (of a keyword and one below it the upper one moves); nothing merges then: a name that is taken, or two of them with one name, refuses the whole move", always, |s, p| { + if p.get("keywords").is_some_and(|k| !k.is_null()) { + let parent = move_parent(p)?; + let keywords = keyword_list(p, "keyword.move")?; + let (op, moves) = s + .catalog + .move_keywords_ops(&keywords, parent.as_deref()) + .map_err(|e| refused("keyword.move", e, ": moving would merge the two (move that one by itself, with merge: true)"))?; + let label = if moves.len() == 1 { "Move Keyword".to_string() } else { format!("Move {} Keywords", moves.len()) }; + return commit_keywords(s, &label, op, |k| { + Some(moves.iter().find(|(f, _)| is_under(k, f)).map_or_else(|| k.to_string(), |(f, t)| reparent(k, f, t))) + }); + } let from = named_keyword(s, p, "keyword.move")?; - let parent = match p.get("parent") { - Some(Value::String(k)) => Some(k.clone()), - Some(Value::Null) => None, - _ => return Err(bad("keyword.move", "`parent` is a keyword, or null for the top level")), - }; + let parent = move_parent(p)?; // where it lands, as the catalog spells it: a parent that doesn't exist yet is made let leaf = from.rsplit('|').next().unwrap_or(&from).to_string(); let to = match parent.as_deref().map(|k| s.catalog.keyword_path(k).unwrap_or_else(|| clean(k))).filter(|k| !k.is_empty()) { @@ -532,9 +574,16 @@ pub fn specs() -> Vec { "Delete Keyword", [], None, - "{keyword} — removes it (and the keywords below it) from every photo", + "{keyword} — removes it (and the keywords below it) from every photo. Several at once: {keywords: [keyword]}, as one undo step", always, |s, p| { + if p.get("keywords").is_some_and(|k| !k.is_null()) { + let keywords = keyword_list(p, "keyword.delete")?; + let op = s.catalog.delete_keywords_ops(&keywords).map_err(|e| bad("keyword.delete", e.to_string()))?; + let gone: Vec = keywords.iter().map(|k| clean(k)).collect(); + let label = if gone.len() == 1 { "Delete Keyword".to_string() } else { format!("Delete {} Keywords", gone.len()) }; + return commit_keywords(s, &label, op, |f| (!gone.iter().any(|g| is_under(f, g))).then(|| f.to_string())); + } let k = str_param(p, "keyword").ok_or_else(|| bad("keyword.delete", "missing `keyword`"))?.to_string(); let op = s.catalog.delete_keyword_ops(&k).map_err(|e| bad("keyword.delete", e.to_string()))?; let c = clean(&k); diff --git a/crates/engine/src/cmd/library/specs.rs b/crates/engine/src/cmd/library/specs.rs --- a/crates/engine/src/cmd/library/specs.rs +++ b/crates/engine/src/cmd/library/specs.rs @@ -26,6 +26,91 @@ fn next_seed(prev: u64, now: &str) -> u64 { (lightcraft_catalog::mix64(prev.wrapping_add(1)) ^ t) & (MAX_SEED - 1) } +/// `album.move` for several albums and folders (`ids`) at once: everything is checked first, then +/// they move as one undo step. +fn move_albums(s: &mut Session, p: &Value) -> Result { + const C: &str = "album.move"; + let ids = album_ids(p.get("ids").unwrap_or(&Value::Null), C)?; + if ids.is_empty() { + return Err(bad(C, "`ids` names no album")); + } + if let Some(gone) = ids.iter().find(|a| s.catalog.album(**a).is_none()) { + return Err(bad(C, format!("no such album {}", gone.0))); + } + let parent = opt_album_param(p, "parent", C)?; + if let Some(folder) = parent + && !s.catalog.album(folder).is_some_and(|a| a.folder) + { + return Err(bad(C, format!("album {} is not a folder of albums", folder.0))); + } + // where `parent` sits: itself and the folders above it (a ring, in a damaged library, ends the walk) + let mut above: Vec = Vec::new(); + let mut cur = parent; + while let Some(c) = cur.filter(|c| !above.contains(c)) { + above.push(c); + cur = s.catalog.album(c).and_then(|a| a.parent); + } + // of a folder and what is inside it, the folder moves + let inside_another = |id: AlbumId| { + let mut seen: Vec = Vec::new(); + let mut cur = s.catalog.album(id).and_then(|a| a.parent); + while let Some(c) = cur.filter(|c| !seen.contains(c)) { + if ids.contains(&c) { + return true; + } + seen.push(c); + cur = s.catalog.album(c).and_then(|a| a.parent); + } + false + }; + let moving: Vec = ids.iter().copied().filter(|id| !inside_another(*id)).collect(); + for id in &moving { + let name = s.catalog.album(*id).map(|a| a.name.clone()).unwrap_or_default(); + if above.contains(id) { + return Err(bad(C, format!("“{name}” can't go inside itself"))); + } + } + let ops: Vec = moving.iter().map(|id| Op::MoveAlbum { id: *id, parent }).collect(); + let label = if moving.len() == 1 { "Move Album".to_string() } else { format!("Move {} Albums", moving.len()) }; + s.commit(&label, Op::Batch { ops })?; + Ok(json!({"moved": moving.len()})) +} + +/// How many albums or folders one call may name: far more than a person +/// chooses in a sidebar, and a bound on what one call can make the engine check. +pub(crate) const MAX_SOURCES: usize = 1000; + +/// A command's `ids`: album ids, each once (named twice, the last naming counts). +fn album_ids(v: &Value, c: &str) -> Result> { + let list = v.as_array().ok_or_else(|| bad(c, "`ids` must be a list of album ids"))?; + if list.len() > MAX_SOURCES { + return Err(bad(c, format!("`ids` names more than {MAX_SOURCES} albums"))); + } + let mut out: Vec = Vec::new(); + for x in list { + let id = AlbumId(x.as_u64().ok_or_else(|| bad(c, format!("`ids` must be a list of album ids, not {x}")))?); + out.retain(|o| *o != id); + out.push(id); + } + Ok(out) +} + +/// A command's `paths`: folder paths, each folder once (named twice, the last counts). +fn folder_paths(v: &Value, c: &str) -> Result> { + let list = v.as_array().ok_or_else(|| bad(c, "`paths` must be a list of folder paths"))?; + if list.len() > MAX_SOURCES { + return Err(bad(c, format!("`paths` names more than {MAX_SOURCES} folders"))); + } + let mut out: Vec = Vec::new(); + for x in list { + let path = x.as_str().ok_or_else(|| bad(c, format!("`paths` must be a list of folder paths, not {x}")))?; + let key = lightcraft_catalog::query::folder_key(path); + out.retain(|o| lightcraft_catalog::query::folder_key(o) != key); + out.push(path.to_string()); + } + Ok(out) +} + pub fn specs() -> Vec { vec![ // ---- view source / filter / sort @@ -125,35 +210,52 @@ pub fn specs() -> Vec { "Remove Folder from Library", [], None, - "{path, disk?: bool} — move the library photos imported from this folder (and the folders inside it) to Recently Deleted; one undo step, no file is touched. A whole disk or share (`/Volumes/nas`, `C:\\`, `\\\\srv\\share`) goes only with `disk: true`, the startup disk never → {removed}", + "{path, disk?: bool} — move the library photos imported from this folder (and the folders inside it) to Recently Deleted; one undo step, no file is touched. A whole disk or share (`/Volumes/nas`, `C:\\`, `\\\\srv\\share`) goes only with `disk: true`, the startup disk never → {removed}. Several at once: {paths: [path]}, still one undo step; one that can't be removed refuses the whole call", always, |s, p| { const C: &str = "library.removeFolder"; - let path = str_param(p, "path").filter(|d| !d.trim().is_empty()).ok_or_else(|| bad(C, "missing `path`"))?; - if lightcraft_catalog::query::folder_key(path).is_empty() { - return Err(bad(C, format!("{path}: not a folder"))); - } - if lightcraft_catalog::folders::is_startup_disk(path) { - return Err(bad(C, "choose a folder, not the whole startup disk")); - } - if lightcraft_catalog::folders::is_disk_root(path) && !bool_or(p, "disk", false) { - return Err(bad(C, format!("{path} is a whole disk or share: pass `disk: true` to remove everything on it"))); - } - let f = Filter { library_folder: Some(path.to_string()), ..Default::default() }; - let ids = s.catalog.query(&f, &Sort::default()); - if ids.is_empty() { - return Err(bad(C, format!("{path}: no photo in the library was imported from it"))); + let several = p.get("paths").is_some_and(|v| !v.is_null()); + let paths = match p.get("paths") { + Some(v) if several => folder_paths(v, C)?, + _ => vec![str_param(p, "path").filter(|d| !d.trim().is_empty()).ok_or_else(|| bad(C, "missing `path`"))?.to_string()], + }; + if paths.is_empty() { + return Err(bad(C, "`paths` names no folder")); } - if covers_other_disks(s, path, &ids) && !bool_or(p, "disk", false) { - return Err(bad(C, format!("{path} holds whole disks: pass `disk: true` to remove everything on them"))); + // every folder is checked before a photo leaves; a photo in two of them leaves once + let mut ids: Vec = Vec::new(); + let mut seen = std::collections::HashSet::new(); + for path in &paths { + let path = path.as_str(); + if path.trim().is_empty() || lightcraft_catalog::query::folder_key(path).is_empty() { + return Err(bad(C, format!("{path}: not a folder"))); + } + if lightcraft_catalog::folders::is_startup_disk(path) { + return Err(bad(C, "choose a folder, not the whole startup disk")); + } + if lightcraft_catalog::folders::is_disk_root(path) && !bool_or(p, "disk", false) { + return Err(bad(C, format!("{path} is a whole disk or share: pass `disk: true` to remove everything on it"))); + } + let f = Filter { library_folder: Some(path.to_string()), ..Default::default() }; + let found = s.catalog.query(&f, &Sort::default()); + if found.is_empty() { + return Err(bad(C, format!("{path}: no photo in the library was imported from it"))); + } + if covers_other_disks(s, path, &found) && !bool_or(p, "disk", false) { + return Err(bad(C, format!("{path} holds whole disks: pass `disk: true` to remove everything on them"))); + } + ids.extend(found.into_iter().filter(|id| seen.insert(*id))); } - mark_deleted(s, &ids, "Remove Folder from Library")?; - if s.filter.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) { + let label = + if paths.len() == 1 { "Remove Folder from Library".to_string() } else { format!("Remove {} Folders from Library", paths.len()) }; + mark_deleted(s, &ids, &label)?; + let within = |c: &str| paths.iter().any(|path| lightcraft_catalog::query::folder_within(c, path)); + if s.filter.library_folder.as_deref().is_some_and(within) { s.filter.library_folder = None; } // a filter put aside by Enable Filters must not bring the removed folder back if let Some(f) = s.shelved_filter.as_mut() - && f.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) + && f.library_folder.as_deref().is_some_and(within) { f.library_folder = None; } @@ -161,7 +263,7 @@ pub fn specs() -> Vec { s.shelved_filter = None; } // the folder being shown is gone: back to everything, as for a deleted album - if s.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) { + if s.library_folder.as_deref().is_some_and(within) { s.library_folder = None; if s.source == LibrarySource::LibraryFolder { s.source = LibrarySource::All; @@ -177,26 +279,46 @@ 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 (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}", + "{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}. Several at once: {paths: [path], label}, as one undo step", always, |s, p| { const C: &str = "folder.label"; - let path = str_param(p, "path").filter(|d| !d.trim().is_empty()).ok_or_else(|| bad(C, "missing `path`"))?; + let several = p.get("paths").is_some_and(|v| !v.is_null()); + let paths = match p.get("paths") { + Some(v) if several => folder_paths(v, C)?, + _ => vec![str_param(p, "path").filter(|d| !d.trim().is_empty()).ok_or_else(|| bad(C, "missing `path`"))?.to_string()], + }; + if paths.is_empty() { + return Err(bad(C, "`paths` names no folder")); + } let label = match str_param(p, "label").ok_or_else(|| bad(C, "missing `label` (red|yellow|green|blue|purple|none)"))? { "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 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"))); + let mut ops = Vec::new(); + for path in &paths { + if path.trim().is_empty() { + return Err(bad(C, "missing `path`")); + } + // taking a label off always works, also once the folder's photos are gone + let f = Filter { library_folder: Some(path.clone()), ..Default::default() }; + 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_some() || s.catalog.folder_record(path).is_some() { + ops.push(s.catalog.folder_label_op(path, label)); + } } - if label.is_none() && s.catalog.folder_record(path).is_none() { - return Ok(json!({"path": path, "label": label})); + let name = if label.is_some() { "Set Folder Color Label" } else { "Remove Folder Color Label" }; + match (ops.len(), ops.pop()) { + (0, _) | (_, None) => {} + (1, Some(op)) => s.commit(name, op)?, + (_, Some(last)) => { + ops.push(last); + s.commit(name, Op::Batch { ops })? + } } - 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})) + if several { Ok(json!({"paths": paths, "label": label})) } else { Ok(json!({"path": paths.first(), "label": label})) } } ), cmd!("library.clearFilter", "Clear Filters", ["View"], None, "{}", always, |s, _| { @@ -905,11 +1027,22 @@ pub fn specs() -> Vec { "Delete Album", [], None, - "{id} — deletes the album or folder and its descendants, keeping photos in the library; one undo step", + "{id} — deletes the album or folder and its descendants, keeping photos in the library; one undo step. Several at once: {ids: [albumId]} → {deleted}, still one undo step; one that does not exist refuses the whole call", always, |s, p| { - let id = album_param(p, "id", "album.delete")?; - let folder = s.catalog.album(id).ok_or_else(|| bad("album.delete", "no such album"))?.folder; + const C: &str = "album.delete"; + let several = p.get("ids").is_some_and(|v| !v.is_null()); + let ids = match p.get("ids") { + Some(v) if several => album_ids(v, C)?, + _ => vec![album_param(p, "id", C)?], + }; + if ids.is_empty() { + return Err(bad(C, "`ids` names no album")); + } + if let Some(gone) = ids.iter().find(|a| s.catalog.album(**a).is_none()) { + return Err(bad(C, if several { format!("no such album {}", gone.0) } else { "no such album".into() })); + } + let folder = ids.first().and_then(|id| s.catalog.album(*id)).is_some_and(|a| a.folder); let mut children = std::collections::BTreeMap::>::new(); for album in s.catalog.albums() { if let Some(parent) = album.parent { @@ -919,32 +1052,61 @@ pub fn specs() -> Vec { // Iterative traversal: even a malformed cycle is rejected before any change. // Reverse parent-first order removes children first; the batch inverse restores // parents first, including memberships, smart rules, covers and manual ordering. - let mut pending = vec![id]; let mut order = Vec::new(); let mut removed = std::collections::BTreeSet::new(); - while let Some(next) = pending.pop() { - if !removed.insert(next) { - return Err(bad("album.delete", "cycle in album folders")); + for id in &ids { + // inside one named before it: it goes with that one + if removed.contains(id) { + continue; } - order.push(next); - if let Some(nested) = children.remove(&next) { - pending.extend(nested); + let mut pending = vec![*id]; + let mut seen = std::collections::BTreeSet::new(); + while let Some(next) = pending.pop() { + if !seen.insert(next) { + return Err(bad(C, "cycle in album folders")); + } + // one named before, now found inside this one: it is listed once, here + if removed.insert(next) { + order.push(next); + } else if let Some(at) = order.iter().position(|o| *o == next) { + let moved = order.remove(at); + order.push(moved); + } + if let Some(nested) = children.get(&next) { + pending.extend(nested.iter().copied()); + } } } let ops = order.into_iter().rev().map(|id| Op::RemoveAlbum { id }).collect(); - s.commit(if folder { "Delete Folder" } else { "Delete Album" }, Op::Batch { ops })?; + let label = match (ids.len(), folder) { + (1, true) => "Delete Folder".to_string(), + (1, false) => "Delete Album".to_string(), + (n, _) => format!("Delete {n} Albums"), + }; + s.commit(&label, Op::Batch { ops })?; if matches!(s.source, LibrarySource::Album(current) if removed.contains(¤t)) { s.source = LibrarySource::All; } + if several { Ok(json!({"deleted": ids.len()})) } else { ok() } + } + ), + cmd!( + "album.move", + "Move Album", + [], + None, + "{id, parent?: folderId|null} — into a folder, or to the top level. Several at once: {ids: [albumId], parent} as one undo step (of a folder and an album inside it the folder moves); one that can't go there refuses the whole call", + always, + |s, p| { + if p.get("ids").is_some_and(|v| !v.is_null()) { + return move_albums(s, p); + } + let id = album_param(p, "id", "album.move")?; + let parent = p.get("parent").and_then(Value::as_u64).map(AlbumId); + s.commit("Move Album", Op::MoveAlbum { id, parent })?; ok() } ), - cmd!("album.move", "Move Album", [], None, "{id, parent?: folderId|null}", always, |s, p| { - let id = album_param(p, "id", "album.move")?; - let parent = p.get("parent").and_then(Value::as_u64).map(AlbumId); - s.commit("Move Album", Op::MoveAlbum { id, parent })?; - ok() - }), cmd!( "album.reorder", "Reorder Album", diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -5,6 +5,7 @@ use crate::{Selection, Session, command_specs}; mod album_order; mod album_remove; mod albums_local; +mod batch_sidebar; mod color; mod constrain_crop; mod crop_angle; diff --git a/crates/engine/src/tests/batch_sidebar.rs b/crates/engine/src/tests/batch_sidebar.rs new file mode 100644 --- /dev/null +++ b/crates/engine/src/tests/batch_sidebar.rs @@ -0,0 +1,301 @@ +//! Acting on several sidebar rows at once: the commands a context menu runs when several albums, +//! folders or keywords are chosen. Each is one step to undo, checks everything before it changes +//! anything, and a refusal names what it refused. +//! +//! Scenarios: +//! - Given three albums, when two are deleted at once (`album.delete {ids}`), then both are gone, +//! the third stays, and one undo brings both back. +//! - Given a folder of albums and an album inside it both named, then deleting them deletes the +//! folder with what is in it, once. +//! - An album that does not exist among those to delete is refused by name, and nothing is deleted. +//! - Given two albums, when both are moved into a folder (`album.move {ids, parent}`), then both +//! are inside it, and one undo puts both back; a folder is not moved into itself, and then +//! nothing moves. +//! - Given keywords on photos, when several are deleted at once (`keyword.delete {keywords}`), +//! then every photo loses them and one undo gives them back; one that is no keyword refuses the +//! whole call and nothing is deleted (one keyword that is no keyword still changes nothing). +//! - Given several keywords, when they are moved inside another (`keyword.move {keywords, +//! parent}`), then each is nested there with its photos, as one undo step; a name that is taken +//! there, or two of them with one name, is refused and nothing moves. +//! - Given two folders of the library, when both are removed at once (`library.removeFolder +//! {paths}`), then their photos go to Recently Deleted as one undo step; one that is not a +//! folder of the library refuses the whole call. +//! - Given two folders, when both get a colour label at once (`folder.label {paths}`), then both +//! show it, and one undo takes both off. + +use lightcraft_catalog::{AlbumId, Op, Photo, PhotoId, Source}; +use serde_json::json; + +use crate::Session; + +fn photos(s: &mut Session, n: u64) -> Vec { + (1..=n) + .map(|day| { + let id = s.catalog.alloc_photo_id(); + let mut p = Photo::new(id, Source::Demo { scene: 1 }, &format!("p{day}.jpg"), "JPEG", 60, 40, "2026-02-01T10:00:00"); + p.captured = Some(format!("2026-01-{day:02}T10:00:00")); + s.commit("Add Photo", Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id.0 + }) + .collect() +} + +fn album(s: &mut Session, params: serde_json::Value) -> u64 { + s.execute("album.create", ¶ms).unwrap()["id"].as_u64().unwrap() +} + +fn has_album(s: &Session, id: u64) -> bool { + s.catalog.album(AlbumId(id)).is_some() +} + +fn parent_of(s: &Session, id: u64) -> Option { + s.catalog.album(AlbumId(id)).unwrap().parent.map(|p| p.0) +} + +fn tag(s: &mut Session, id: u64, keywords: &[&str]) { + // the command wants a selection to be enabled, whatever `ids` says + s.selection.ids = vec![PhotoId(id)]; + s.execute("photo.setMeta", &json!({"ids": [id], "addKeywords": keywords})).unwrap(); + s.selection.ids.clear(); +} + +fn keywords_of(s: &Session, id: u64) -> Vec { + let mut k = s.catalog.photo(PhotoId(id)).unwrap().meta.keywords.clone(); + k.sort(); + k +} + +fn undo(s: &mut Session) { + s.execute("edit.undo", &json!({})).unwrap(); +} + +#[test] +fn several_albums_are_deleted_as_one_step() { + let mut s = Session::new(); + let (a, b, c) = (album(&mut s, json!({"name": "A"})), album(&mut s, json!({"name": "B"})), album(&mut s, json!({"name": "C"}))); + let steps = s.undo.len(); + let r = s.execute("album.delete", &json!({"ids": [a, c]})).unwrap(); + assert_eq!(r["deleted"], 2); + assert_eq!((has_album(&s, a), has_album(&s, b), has_album(&s, c)), (false, true, false)); + assert_eq!(s.undo.len(), steps + 1, "one step"); + assert_eq!(s.undo.last().unwrap().label, "Delete 2 Albums"); + undo(&mut s); + assert!(has_album(&s, a) && has_album(&s, c)); +} + +#[test] +fn a_folder_and_an_album_inside_it_are_deleted_once() { + let mut s = Session::new(); + let folder = album(&mut s, json!({"name": "Trips", "folder": true})); + let inside = album(&mut s, json!({"name": "Alps", "parent": folder})); + let other = album(&mut s, json!({"name": "Other"})); + s.execute("album.delete", &json!({"ids": [inside, folder, other]})).unwrap(); + assert!(!has_album(&s, folder) && !has_album(&s, inside) && !has_album(&s, other)); + undo(&mut s); + assert_eq!(parent_of(&s, inside), Some(folder), "back where it was"); + assert!(has_album(&s, other)); +} + +#[test] +fn deleting_several_albums_checks_them_all_first() { + let mut s = Session::new(); + let a = album(&mut s, json!({"name": "A"})); + let steps = s.undo.len(); + let e = s.execute("album.delete", &json!({"ids": [a, 4242]})).unwrap_err().to_string(); + assert!(e.contains("4242"), "{e}"); + assert!(has_album(&s, a), "nothing was deleted"); + assert_eq!(s.undo.len(), steps); + for bad in [json!([]), json!("a"), json!([a, "b"])] { + assert!(s.execute("album.delete", &json!({"ids": bad})).is_err(), "{bad}"); + } + assert!(has_album(&s, a)); + // one alone, the way it always was + s.execute("album.delete", &json!({"id": a})).unwrap(); + assert!(!has_album(&s, a)); + assert_eq!(s.undo.last().unwrap().label, "Delete Album"); +} + +#[test] +fn several_albums_move_into_a_folder_as_one_step() { + let mut s = Session::new(); + let (a, b) = (album(&mut s, json!({"name": "A"})), album(&mut s, json!({"name": "B"}))); + let folder = album(&mut s, json!({"name": "Trips", "folder": true})); + let steps = s.undo.len(); + s.execute("album.move", &json!({"ids": [a, b], "parent": folder})).unwrap(); + assert_eq!((parent_of(&s, a), parent_of(&s, b)), (Some(folder), Some(folder))); + assert_eq!(s.undo.len(), steps + 1); + assert_eq!(s.undo.last().unwrap().label, "Move 2 Albums"); + undo(&mut s); + assert_eq!((parent_of(&s, a), parent_of(&s, b)), (None, None)); + // back to the top level, both at once + s.execute("album.move", &json!({"ids": [a, b], "parent": folder})).unwrap(); + s.execute("album.move", &json!({"ids": [a, b], "parent": null})).unwrap(); + assert_eq!((parent_of(&s, a), parent_of(&s, b)), (None, None)); +} + +#[test] +fn moving_several_albums_checks_them_all_first() { + let mut s = Session::new(); + let a = album(&mut s, json!({"name": "A"})); + let folder = album(&mut s, json!({"name": "Trips", "folder": true})); + let inner = album(&mut s, json!({"name": "Inner", "folder": true, "parent": folder})); + // a folder can't go inside itself or inside a folder it holds: then nothing moves + for parent in [folder, inner] { + let e = s.execute("album.move", &json!({"ids": [a, folder], "parent": parent})).unwrap_err().to_string(); + assert!(e.contains("Trips"), "the refusal names it: {e}"); + assert_eq!(parent_of(&s, a), None, "nothing moved"); + } + // into something that is not a folder, or not there + assert!(s.execute("album.move", &json!({"ids": [folder], "parent": a})).is_err()); + assert!(s.execute("album.move", &json!({"ids": [a], "parent": 4242})).is_err()); + assert!(s.execute("album.move", &json!({"ids": [a, 4242], "parent": folder})).is_err()); + assert_eq!(parent_of(&s, a), None); + // a folder and an album already inside it: the folder moves, and the album with it + let outer = album(&mut s, json!({"name": "Outer", "folder": true})); + let within = album(&mut s, json!({"name": "Within", "parent": inner})); + s.execute("album.move", &json!({"ids": [within, inner], "parent": outer})).unwrap(); + assert_eq!((parent_of(&s, inner), parent_of(&s, within)), (Some(outer), Some(inner))); +} + +#[test] +fn several_keywords_are_deleted_as_one_step() { + let mut s = Session::new(); + let ids = photos(&mut s, 2); + tag(&mut s, ids[0], &["alps", "beach", "trips|italy"]); + tag(&mut s, ids[1], &["beach", "town"]); + let steps = s.undo.len(); + let r = s.execute("keyword.delete", &json!({"keywords": ["alps", "beach", "trips"]})).unwrap(); + assert_eq!(r["changed"], 2); + assert_eq!(keywords_of(&s, ids[0]), Vec::::new()); + assert_eq!(keywords_of(&s, ids[1]), vec!["town"]); + assert_eq!(s.undo.len(), steps + 1); + assert_eq!(s.undo.last().unwrap().label, "Delete 3 Keywords"); + undo(&mut s); + assert_eq!(keywords_of(&s, ids[0]), vec!["alps", "beach", "trips|italy"]); + assert_eq!(keywords_of(&s, ids[1]), vec!["beach", "town"]); + // nothing named: refused, as is something that is no keyword + for bad in [json!([]), json!([""]), json!(7), json!(["alps", 7])] { + assert!(s.execute("keyword.delete", &json!({"keywords": bad})).is_err(), "{bad}"); + } + assert_eq!(keywords_of(&s, ids[1]), vec!["beach", "town"]); +} + +#[test] +fn deleting_several_keywords_checks_them_all_first() { + let mut s = Session::new(); + let ids = photos(&mut s, 1); + tag(&mut s, ids[0], &["alps", "trips|italy"]); + let steps = s.undo.len(); + let e = s.execute("keyword.delete", &json!({"keywords": ["alps", "nowhere"]})).unwrap_err().to_string(); + assert!(e.contains("no keyword") && e.contains("nowhere"), "names the one that is missing: {e}"); + assert_eq!(keywords_of(&s, ids[0]), vec!["alps", "trips|italy"]); + assert_eq!(s.undo.len(), steps); + // a keyword that is only the upper level of one, in another case, is there + s.execute("keyword.delete", &json!({"keywords": ["ALPS", "Trips"]})).unwrap(); + assert_eq!(keywords_of(&s, ids[0]), Vec::::new()); + // one keyword that is not there is still a no-op, as before + let steps = s.undo.len(); + s.execute("keyword.delete", &json!({"keyword": "nowhere"})).unwrap(); + assert_eq!(s.undo.len(), steps); +} + +#[test] +fn several_keywords_move_inside_another_as_one_step() { + let mut s = Session::new(); + let ids = photos(&mut s, 2); + tag(&mut s, ids[0], &["alps", "beach"]); + tag(&mut s, ids[1], &["beach", "places"]); + let steps = s.undo.len(); + let r = s.execute("keyword.move", &json!({"keywords": ["alps", "beach"], "parent": "places"})).unwrap(); + assert_eq!(r["changed"], 2); + assert_eq!(keywords_of(&s, ids[0]), vec!["places|alps", "places|beach"]); + assert_eq!(keywords_of(&s, ids[1]), vec!["places", "places|beach"]); + assert_eq!(s.undo.len(), steps + 1); + assert_eq!(s.undo.last().unwrap().label, "Move 2 Keywords"); + undo(&mut s); + assert_eq!(keywords_of(&s, ids[0]), vec!["alps", "beach"]); + // …and back out to the top level + s.execute("keyword.move", &json!({"keywords": ["alps", "beach"], "parent": "places"})).unwrap(); + s.execute("keyword.move", &json!({"keywords": ["places|alps", "places|beach"], "parent": null})).unwrap(); + assert_eq!(keywords_of(&s, ids[0]), vec!["alps", "beach"]); +} + +#[test] +fn moving_several_keywords_checks_them_all_first() { + let mut s = Session::new(); + let ids = photos(&mut s, 2); + tag(&mut s, ids[0], &["alps", "europe|alps", "beach", "asia|beach", "places"]); + let before = keywords_of(&s, ids[0]); + let steps = s.undo.len(); + // a name that is taken where they go: moving would merge the two, which is not done in a batch + let e = s.execute("keyword.move", &json!({"keywords": ["beach", "alps"], "parent": "europe"})).unwrap_err().to_string(); + assert!(e.contains("europe|alps"), "names the one in the way: {e}"); + // two of them with one name + let e = s.execute("keyword.move", &json!({"keywords": ["beach", "asia|beach"], "parent": "places"})).unwrap_err().to_string(); + assert!(e.to_lowercase().contains("beach"), "{e}"); + // inside one of themselves; one that is no keyword + assert!(s.execute("keyword.move", &json!({"keywords": ["places", "alps"], "parent": "places"})).is_err()); + assert!(s.execute("keyword.move", &json!({"keywords": ["alps", "nope"], "parent": "places"})).is_err()); + assert!(s.execute("keyword.move", &json!({"keywords": [], "parent": "places"})).is_err()); + assert_eq!(keywords_of(&s, ids[0]), before, "nothing moved"); + assert_eq!(s.undo.len(), steps); + // a keyword and one below it: the upper one moves, and the lower with it + s.execute("keyword.move", &json!({"keywords": ["europe|alps", "europe"], "parent": "places"})).unwrap(); + assert!(keywords_of(&s, ids[0]).contains(&"places|europe|alps".to_string())); +} + +fn add_file(s: &mut Session, path: &str) -> u64 { + let id = s.catalog.alloc_photo_id(); + let p = Photo::new(id, Source::File { path: path.into() }, "x.jpg", "JPEG", 60, 40, "2026-01-01T10:00:00"); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id.0 +} + +fn deleted(s: &Session, id: u64) -> bool { + s.catalog.photo(PhotoId(id)).unwrap().deleted +} + +#[test] +fn several_folders_leave_the_library_as_one_step() { + let mut s = Session::new(); + let alps = add_file(&mut s, "/pics/2025/alps/a.jpg"); + let beach = add_file(&mut s, "/pics/2026/beach/c.jpg"); + let town = add_file(&mut s, "/pics/2026/town/d.jpg"); + let steps = s.undo.len(); + // refused as a whole when one is not a folder of the library + let e = s.execute("library.removeFolder", &json!({"paths": ["/pics/2025/alps", "/nowhere"]})).unwrap_err().to_string(); + assert!(e.contains("/nowhere"), "{e}"); + assert!(!deleted(&s, alps)); + let r = s.execute("library.removeFolder", &json!({"paths": ["/pics/2025/alps", "/pics/2026/beach"]})).unwrap(); + assert_eq!(r["removed"], 2); + assert_eq!((deleted(&s, alps), deleted(&s, beach), deleted(&s, town)), (true, true, false)); + assert_eq!(s.undo.len(), steps + 1); + assert_eq!(s.undo.last().unwrap().label, "Remove 2 Folders from Library"); + undo(&mut s); + assert!(!deleted(&s, alps) && !deleted(&s, beach)); + // a folder and one inside it: each photo once + add_file(&mut s, "/pics/2025/alps/day1/b.jpg"); + let r = s.execute("library.removeFolder", &json!({"paths": ["/pics/2025/alps/day1", "/pics/2025/alps"]})).unwrap(); + assert_eq!(r["removed"], 2); +} + +#[test] +fn several_folders_get_a_colour_label_as_one_step() { + let mut s = Session::new(); + add_file(&mut s, "/pics/2025/alps/a.jpg"); + add_file(&mut s, "/pics/2026/beach/c.jpg"); + let label = |s: &Session, path: &str| s.catalog.folder_record(path).and_then(|r| r.label); + let steps = s.undo.len(); + let e = s.execute("folder.label", &json!({"paths": ["/pics/2025/alps", "/nowhere"], "label": "red"})).unwrap_err().to_string(); + assert!(e.contains("/nowhere"), "{e}"); + assert!(label(&s, "/pics/2025/alps").is_none(), "nothing was labelled"); + s.execute("folder.label", &json!({"paths": ["/pics/2025/alps", "/pics/2026/beach"], "label": "red"})).unwrap(); + assert!(label(&s, "/pics/2025/alps").is_some() && label(&s, "/pics/2026/beach").is_some()); + assert_eq!(s.undo.len(), steps + 1); + undo(&mut s); + assert!(label(&s, "/pics/2025/alps").is_none() && label(&s, "/pics/2026/beach").is_none()); + // taking the labels off, both at once + s.execute("folder.label", &json!({"paths": ["/pics/2025/alps", "/pics/2026/beach"], "label": "blue"})).unwrap(); + s.execute("folder.label", &json!({"paths": ["/pics/2025/alps", "/pics/2026/beach"], "label": "none"})).unwrap(); + assert!(label(&s, "/pics/2025/alps").is_none() && label(&s, "/pics/2026/beach").is_none()); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Lightroom parity checklist -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-25 · **Change:** minor (LR-BEHAV-TITLEBAR: a double-click follows the system setting, and a panel over the bar keeps the window still; KEYC-METADATA, LRC-LIB-KEYWORDS: ⌥0 / ⌥⇧0 go to the next / previous keyword set; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings; KEYC-METADATA: ⌘K / Ctrl+K Add Keywords; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-25 · **Change:** minor (LR-BEHAV-MULTISELECT: several sidebar albums, keywords or folders are deleted, moved or labeled as one undo step in the engine, no sidebar caller yet; LR-BEHAV-TITLEBAR: a double-click follows the system setting, and a panel over the bar keeps the window still; KEYC-METADATA, LRC-LIB-KEYWORDS: ⌥0 / ⌥⇧0 go to the next / previous keyword set; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings; KEYC-METADATA: ⌘K / Ctrl+K Add Keywords; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good @@ -49,13 +49,13 @@ cited here still exists. Formerly docs/parity.md. | U. Map & location (MAP) | 0 | 1 | 1 | 0 | — | 0/1 (0%) | | V. Preferences (PREF) | 6 | 2 | 2 | 2 | 1/1 (100%) | 4/5 (80%) | | W. Cloud & AI infrastructure (CLOUD / AI) | 1 | 0 | 1 | 2 | — | — | -| X. Cross-cutting behaviours (BEHAV) | 23 | 6 | 1 | 1 | 8/8 (100%) | 7/10 (70%) | +| X. Cross-cutting behaviours (BEHAV) | 22 | 7 | 1 | 1 | 7/8 (88%) | 7/10 (70%) | | Y. Menus | 85 | 0 | 4 | 8 | 47/47 (100%) | 23/23 (100%) | | Z. Keyboard shortcuts (desktop) | 78 | 4 | 2 | 1 | 49/52 (94%) | 24/26 (92%) | | Lightroom Classic extras | 41 | 25 | 21 | 8 | — | 24/26 (92%) | -| **Total** | 421 | 65 | 57 | 35 | 196/203 (97%) | 146/161 (91%) | +| **Total** | 420 | 66 | 57 | 35 | 195/203 (96%) | 146/161 (91%) | -Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **83.5%** of 543 in-scope rows — P0 98.3% of 203 · P1 94.4% of 161 · P2 56.7% of 178. +Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **83.4%** of 543 in-scope rows — P0 98.0% of 203 · P1 94.4% of 161 · P2 56.7% of 178. ## Top gaps and shortcut notes @@ -457,7 +457,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). |---|---|---|---|---|---| | LR-BEHAV-AUTOSAVE | Instant autosave | P0 | ✅ | `crates/catalog/src/journal.rs`, `crates/engine/src/library.rs` | every change fsynced before the command returns; a failed write fails the command ("saved in memory but not written to disk"), keeps the change queued and retries; top-bar warning until saved; a write that fails part-way is cut back to the last whole record so the retry never reads back as damage (logs from older builds with such a fragment load in full); quitting saves queued changes in the closing snapshot even when the log can't be appended to; versioned catalog format (`journal::VERSION`): older libraries are upgraded on open, a library from a newer LightCraft is refused untouched ("written by a newer version"); a damaged or hand-edited snapshot that still reads is made consistent on open (`Catalog::repair`: records filed under another id, stacks of missing, repeated or too few photos, folders inside themselves, id counters behind the ids in use) instead of panicking or hanging later | | LR-BEHAV-UNDO | Global undo | P0 | ✅ | `cmd:edit.undo`, `crates/engine/src/tests.rs` (`rating_flag_undo_redo`), `crates/engine/src/tests/filesystem_history.rs` | covers ratings, albums, deletes, edits; partial file/XMP moves keep unfinished repairs across retries and discarded history; merged renames move directly to their final catalog path | -| LR-BEHAV-MULTISELECT | Multi-selection | P0 | ✅ | `cmd:library.select` (`replace`/`add`/`toggle`/`range`), `cmd:library.selectAll` | | +| LR-BEHAV-MULTISELECT | Multi-selection | P0 | 🟡 | `cmd:library.select` (`replace`/`add`/`toggle`/`range`), `cmd:library.selectAll`, `crates/engine/src/tests/batch_sidebar.rs` | several albums, folders of albums, keywords or library folders chosen in the sidebar are deleted, moved or labeled as one undo step: `album.delete` and `album.move` take `ids`, `keyword.delete` and `keyword.move` take `keywords`, `library.removeFolder` and `folder.label` take `paths`; each checks everything first and names what it refuses (engine only, no sidebar caller yet) | | LR-BEHAV-BATCH | Batch apply to selection | P0 | ✅ | `cmd:photo.rate`, `cmd:develop.paste`, `cmd:preset.apply`, `cmd:app.export` | | | LR-BEHAV-PREVIEW-HOVER | Hover previews | P1 | ✅ | `crates/ui-egui/src/panels/presets.rs`, `crates/ui-egui/src/panels/profiles.rs`, `crates/ui-egui/src/panels/right.rs` (`versions`), `crates/ui-egui/src/panels/detail.rs` | presets, profile menu, profile browser and versions preview in the loupe | | LR-BEHAV-PROGRESSIVE | Progressive rendering | P0 | ✅ | `crates/engine/src/media.rs`, `crates/preview/src/lib.rs` | | @@ -959,3 +959,4 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-23 | patch | LR-BEHAV-ACCESS: the name field of New Album, New Smart Album, Rename and the other one-field dialogs takes the keyboard focus once when the dialog opens instead of every frame, so Tab and Shift+Tab reach the check box, Cancel and OK; status unchanged (🟡) | | 2026-10-22 | minor | LR-CROP-AUTO: Auto straighten turns by at most 10° and only on at least 3 agreeing lines, and says when it finds nothing to level by; measured on 68 photos straightened by hand and 300 left as shot, unwanted turns of more than 1° fell from 79 to 19 of the 300. Downgraded ✅ → 🟡: it still leaves most tilted photos alone | | 2026-10-23 | patch | LRC-LIB-FILTERBAR: showing an album (`library.source`) while the filter holds an album drops the filter's album, as showing a library folder already dropped its folder filter; before, the old album stayed in the filter unseen (the filter bar has no album chip) and narrowed the next source such as All Photos; the row's note says so; status unchanged (✅) | +| 2026-10-23 | patch | LR-BEHAV-MULTISELECT: `album.delete` and `album.move` take `ids`, `keyword.delete` and `keyword.move` take `keywords`, `library.removeFolder` and `folder.label` take `paths`, each as one undo step that checks everything before it changes anything (engine only; the sidebar's Cmd/Shift-click rows are the caller to come); downgraded ✅ → 🟡 until they exist |