From f0689d32710b674f01aa74b24cd5493457b478e6 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 14:41:20 -0400 Subject: [PATCH] LRC-LIB-KEYWORDS: merging overlapping keywords keeps every listing Merging `a|b` and `a|c` into `a` moved the listings one at a time: the `a|b|c` listing landed on `a|c` before that one left for `a`, merged into it, and its attributes were lost. Every moving listing now leaves first, then each lands. Each keyword also moves by the closest keyword merged (`a|b` before `a`), so the order they are given in doesn't change the result; the keyword filter follows the same way. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/keywords.rs | 44 ++++++++++++++++++++++++++----- crates/engine/src/cmd/keywords.rs | 4 +-- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs index ef3ae9fb8..1a0493b81 100644 --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -43,6 +43,11 @@ pub fn reparent(k: &str, from: &str, to: &str) -> String { std::iter::once(to).filter(|t| !t.is_empty()).chain(rest).collect::>().join("|") } +/// The keyword of `from` that `k` is under, the closest one when several are (`a|b` before `a`). +pub fn closest<'a>(k: &str, from: &'a [String]) -> Option<&'a String> { + from.iter().filter(|f| is_under(k, f)).max_by_key(|f| f.split(SEP).count()) +} + /// Keep the first of case-insensitively equal keywords. fn dedupe(v: &mut Vec) { let mut seen = std::collections::HashSet::new(); @@ -201,10 +206,12 @@ impl Catalog { /// one already there merges into it (that one's attributes stay, the synonyms gather). fn list_move_ops(&self, from: &[String], to: &str) -> Vec { self.list_ops(|list| { - let moving: Vec = list.iter().filter(|(_, l)| from.iter().any(|f| is_under(&l.path, f))).map(|(k, _)| k.clone()).collect(); - for key in moving { - let Some(l) = list.remove(&key) else { continue }; - let Some(f) = from.iter().find(|f| is_under(&l.path, f)) else { continue }; + // every moving listing leaves first, then each lands: one landing where another is + // about to leave isn't merged into it + let moving: Vec = list.iter().filter(|(_, l)| closest(&l.path, from).is_some()).map(|(k, _)| k.clone()).collect(); + let left: Vec = moving.iter().filter_map(|key| list.remove(key)).collect(); + for l in left { + let Some(f) = closest(&l.path, from) else { continue }; let path = reparent(&l.path, f, to); match list.get_mut(&path.to_lowercase()) { Some(there) => { @@ -258,9 +265,8 @@ impl Catalog { if from.iter().any(|f| is_under(&into, f)) { return Err(CatalogError::Invalid("can't merge a keyword into one below it".into())); } - let mut ops = self.keyword_ops(|kws| { - kws.iter().map(|k| from.iter().find(|f| is_under(k, f)).map(|f| reparent(k, f, &into)).unwrap_or_else(|| k.clone())).collect() - }); + let mut ops = + self.keyword_ops(|kws| kws.iter().map(|k| closest(k, &from).map(|f| reparent(k, f, &into)).unwrap_or_else(|| k.clone())).collect()); ops.extend(self.list_move_ops(&from, &into)); Ok(Op::Batch { ops }) } @@ -829,6 +835,30 @@ mod tests { assert!(listed(&c).is_empty(), "only a deleted photo has it: unused"); } + /// Merging several keywords moves each keyword under them by the closest one, whatever the + /// order they are given in, and every listing lands where its keyword does: one landing where + /// another is about to leave isn't merged into it. + #[test] + fn merging_overlapping_keywords_keeps_every_listing() { + let (mut c, ids) = lib(&[&["a|b|c"], &["a|c"]]); + let person = KeywordInfo { person: true, ..KeywordInfo::default() }; + c.apply(Op::SetKeyword { path: "a|b|c".into(), info: Some(person.clone()) }).unwrap(); + c.apply(Op::SetKeyword { path: "a|c".into(), info: Some(with_synonyms(&["sea"])) }).unwrap(); + let op = c.merge_keywords_ops(&["a|b".into(), "a|c".into()], "a").unwrap(); + c.apply(op).unwrap(); + assert_eq!((kws(&c, ids[0]), kws(&c, ids[1])), (vec!["a|c".to_string()], vec!["a".to_string()])); + assert_eq!(listed(&c), [("a".to_string(), with_synonyms(&["sea"])), ("a|c".to_string(), person)]); + // the order of the keywords merged doesn't matter + let merged = |from: [&str; 2]| { + let (mut c, ids) = lib(&[&["a|b|c", "a|d"]]); + let op = c.merge_keywords_ops(&from.map(String::from), "x").unwrap(); + c.apply(op).unwrap(); + kws(&c, ids[0]) + }; + assert_eq!(merged(["a", "a|b"]), merged(["a|b", "a"])); + assert_eq!(merged(["a", "a|b"]), ["x|c", "x|d"]); + } + #[test] fn suggestions_rank_co_occurrence_and_prefixes() { let (c, _) = lib(&[&["dog", "park"], &["dog", "park", "ball"], &["dog", "beach"], &["cat"], &["cat"], &["cat"], &["parade"], &["eagle"]]); diff --git a/crates/engine/src/cmd/keywords.rs b/crates/engine/src/cmd/keywords.rs index bf5f95fbd..460278a27 100644 --- a/crates/engine/src/cmd/keywords.rs +++ b/crates/engine/src/cmd/keywords.rs @@ -1,7 +1,7 @@ //! Library-wide keyword commands: list (tree with counts), suggestions, rename, delete, merge; //! keyword sets (nine keywords a keystroke away: ⌥1–⌥9) and Recent Keywords. -use lightcraft_catalog::keywords::{KeywordInfo, clean, is_under, reparent}; +use lightcraft_catalog::keywords::{KeywordInfo, clean, closest, is_under, reparent}; use serde_json::{Value, json}; use super::{CommandSpec, always, bad, bool_or, cmd, str_param}; @@ -390,7 +390,7 @@ pub fn specs() -> Vec { let op = s.catalog.merge_keywords_ops(&from, &into).map_err(|e| bad("keyword.merge", e.to_string()))?; let (from, into) = (from.iter().map(|f| clean(f)).collect::>(), clean(&into)); commit_keywords(s, "Merge Keywords", op, |k| { - Some(match from.iter().find(|f| is_under(k, f)) { + Some(match closest(k, &from) { Some(f) => reparent(k, f, &into), None => k.to_string(), })