LRC-LIB-SMARTCOLL: rule summaries name albums, not ids

Summaries showed an Album rule's id ("album isn't 9"): the sidebar
tooltip, the "Rules:" filter chip and albums.list's rulesText for
agents.

RuleSet::describe_with(&Catalog) and Filter::describe_with name the
album ("album isn't “Excluded Photos”"). One that is gone shows as #9
(rules::album_name). The engine's rulesText and Rules chip use them, and
i18n::rules_label takes the catalog for the translated summaries.
Tests at all three levels: catalog (nested groups, a missing album),
engine (rulesText), UI (German summary).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Nathaniel Ledford
2026-10-09 10:54:57 -04:00
co-authored by Claude Opus 5.5
parent 9dcfc6faee
commit f40c7fb204
8 changed files with 72 additions and 13 deletions
+10 -1
View File
@@ -160,6 +160,15 @@ fn has_person(p: &Photo, name: &str) -> bool {
impl Filter {
/// Human-readable summary of the active rules (smart album tooltips, `album.list`).
pub fn describe(&self) -> String {
self.describe_in(None)
}
/// [`Filter::describe`] naming the albums its rules test (`RuleSet::describe_with`).
pub fn describe_with(&self, cat: &crate::Catalog) -> String {
self.describe_in(Some(cat))
}
fn describe_in(&self, cat: Option<&crate::Catalog>) -> String {
let mut v: Vec<String> = Vec::new();
if self.rating > 0 {
let op = match self.rating_op {
@@ -207,7 +216,7 @@ impl Filter {
v.push(format!("“{}”", self.text.trim()));
}
if let Some(rs) = self.rule_set.as_ref().filter(|r| !r.rules.is_empty()) {
v.push(rs.describe());
v.push(cat.map_or_else(|| rs.describe(), |c| rs.describe_with(c)));
}
if v.is_empty() { "all photos".into() } else { v.join(", ") }
}
+21 -1
View File
@@ -443,6 +443,15 @@ pub(crate) fn evaluating<T>(id: crate::AlbumId, f: impl FnOnce() -> T) -> Option
Some(f())
}
/// How a summary names an Album rule's album: “Name”, or `#9` when there is no such album.
pub fn album_name(value: &Value, cat: Option<&Catalog>) -> String {
let id = album_rule_id(value);
match id.and_then(|id| cat.and_then(|c| c.album(id))) {
Some(a) => format!("“{}”", a.name),
None => id.map_or_else(|| value.to_string(), |id| format!("#{}", id.0)),
}
}
/// What a yes/no rule's value means: `true`, `"yes"`, `1` (or `1.0`) or no value is yes; `false`,
/// `"no"`, `0` is no (strings in any case); `None` for anything else, which matches nothing.
pub fn bool_value(value: &Value) -> Option<bool> {
@@ -915,6 +924,16 @@ impl RuleSet {
/// A short readable summary ("rating is ≥ 3 and keywords contains travel").
pub fn describe(&self) -> String {
self.describe_in(None)
}
/// [`RuleSet::describe`] naming the albums Album rules test ("album isn't “Excluded
/// Photos”"); one that is gone shows as `#9`.
pub fn describe_with(&self, cat: &Catalog) -> String {
self.describe_in(Some(cat))
}
fn describe_in(&self, cat: Option<&Catalog>) -> String {
let join = match self.mode {
Match::All => " and ",
Match::Any => " or ",
@@ -924,11 +943,12 @@ impl RuleSet {
.rules
.iter()
.map(|r| match r {
Rule::Group { group } => format!("({})", group.describe()),
Rule::Group { group } => format!("({})", group.describe_in(cat)),
Rule::Field { field, op, value } => {
let label = field_label(field).unwrap_or(field).to_lowercase();
let op = field_kind(field).and_then(|k| ops_for(k).iter().find(|o| o.0 == op)).map_or(op.as_str(), |o| o.1);
let v = match value {
_ if field == "album" && cat.is_some() => album_name(value, cat),
_ if field_kind(field) == Some(Kind::Bool) => {
bool_value(value).map_or_else(|| value.to_string(), |b| bool_label(b).to_lowercase())
}
+15
View File
@@ -549,3 +549,18 @@ fn smart_album_loops_are_refused_and_survived() {
assert!(c.smart_album_problems(AlbumId(a)).iter().any(|p| p.issue == crate::rules::Issue::AlbumLoop), "album {a} is reported");
}
}
/// Summaries name the albums Album rules test ("album isn't “Excluded Photos”"), not their ids;
/// one that is gone shows as its number.
#[test]
fn summaries_name_albums() {
let mut c = Catalog::new();
c.apply(Op::AddAlbum { album: Album::new(AlbumId(4), "Excluded Photos") }).unwrap();
let rules: RuleSet = serde_json::from_value(serde_json::json!({"rules": [
{"field": "keywords", "op": "contains", "value": "travel"},
{"group": {"match": "any", "rules": [{"field": "album", "op": "isNot", "value": 4}, {"field": "album", "op": "is", "value": 9}]}}]}))
.unwrap();
assert_eq!(rules.describe_with(&c), "keywords contains travel and (album isn't “Excluded Photos” or album is #9)");
let f = Filter { rule_set: Some(rules), ..Default::default() };
assert!(f.describe_with(&c).contains("album isn't “Excluded Photos”"));
}
+1 -1
View File
@@ -43,7 +43,7 @@ fn album_json(a: &Album, all: &[Album], cat: &lightcraft_catalog::Catalog) -> Va
if let Some(rules) = &a.smart {
v["smart"] = json!(true);
v["rules"] = serde_json::to_value(rules).unwrap_or_default();
v["rulesText"] = json!(rules.describe());
v["rulesText"] = json!(rules.describe_with(cat));
// rules that no longer check (an album they test was deleted…), as `rule 1: …`
v["problems"] = json!(cat.smart_album_problems(a.id).iter().map(ToString::to_string).collect::<Vec<_>>());
}
+4
View File
@@ -616,4 +616,8 @@ fn smart_album_excluding_a_smart_album() {
&serde_json::json!({"id": excluded, "rules": {"ruleSet": {"rules": [{"field": "album", "op": "is", "value": travel}]}}, "replace": true}),
);
assert!(looped.is_err_and(|e| e.to_string().contains("would make this album include itself")), "a loop is refused");
// agents read the album by name in the summary
let list = s.execute("albums.list", &serde_json::json!({})).unwrap();
let travel_json = list.as_array().unwrap().iter().find(|a| a["id"] == travel).cloned().unwrap();
assert!(travel_json["rulesText"].as_str().unwrap_or("").contains("album isn't “Excluded Photos”"), "{travel_json}");
}
+1 -1
View File
@@ -212,7 +212,7 @@ pub fn filter_chips(f: &Filter, cat: &Catalog) -> Vec<FilterChip> {
_ => {}
}
if let Some(rs) = f.rule_set.as_ref().filter(|r| !r.rules.is_empty()) {
add(format!("Rules: {}", rs.describe()), json!({"ruleSet": Null}));
add(format!("Rules: {}", rs.describe_with(cat)), json!({"ruleSet": Null}));
}
if !f.only.is_empty() {
add(format!("Only {} photos", f.only.len()), json!({"only": []}));
+19 -8
View File
@@ -318,7 +318,7 @@ pub fn color_label(catalog: &lightcraft_catalog::Catalog, label: lightcraft_cata
/// A legacy smart-album filter summary, using the same display labels as filter chips.
pub fn filter_label(filter: &lightcraft_catalog::Filter, catalog: &lightcraft_catalog::Catalog) -> String {
if language() == Locale::En {
filter.describe()
filter.describe_with(catalog)
} else {
lightcraft_engine::filter_chips(filter, catalog)
.iter()
@@ -378,8 +378,8 @@ fn bool_text_in(language: Locale, b: bool) -> &'static str {
}
/// A rule summary for display, keeping free-text rule values verbatim.
pub fn rules_label(rules: &lightcraft_catalog::RuleSet) -> String {
fn describe(rules: &lightcraft_catalog::RuleSet, depth: usize) -> String {
pub fn rules_label(rules: &lightcraft_catalog::RuleSet, catalog: &lightcraft_catalog::Catalog) -> String {
fn describe(rules: &lightcraft_catalog::RuleSet, catalog: &lightcraft_catalog::Catalog, depth: usize) -> String {
use lightcraft_catalog::{
Match, Rule,
rules::{FIELDS, Kind, ops_for},
@@ -391,11 +391,13 @@ pub fn rules_label(rules: &lightcraft_catalog::RuleSet) -> String {
.rules
.iter()
.map(|rule| match rule {
Rule::Group { group } => format!("({})", describe(group, depth + 1)),
Rule::Group { group } => format!("({})", describe(group, catalog, depth + 1)),
Rule::Field { field, op, value } => {
let Some((_, label, kind)) = FIELDS.iter().find(|entry| entry.0 == field) else { return field.clone() };
let operator = ops_for(*kind).iter().find(|entry| entry.0 == op).map_or(op.as_str(), |entry| entry.1);
let text = match kind {
// the album's name, as people know it (a user's name is never translated)
Kind::Album => lightcraft_catalog::rules::album_name(value, Some(catalog)),
Kind::Choice(_) => value.as_str().map_or_else(|| value.to_string(), |id| choice_text(field, id).to_string()),
Kind::Bool => lightcraft_catalog::rules::bool_value(value).map_or_else(|| value.to_string(), |b| bool_text(b).to_string()),
_ if matches!(op.as_str(), "inLast" | "notInLast") => format!(
@@ -417,7 +419,7 @@ pub fn rules_label(rules: &lightcraft_catalog::RuleSet) -> String {
if rules.mode == Match::None { format!("{} ({text})", tr("none of")) } else { text }
}
// Preserve the established source-language summary.
if language() == Locale::En { rules.describe() } else { describe(rules, 0) }
if language() == Locale::En { rules.describe_with(catalog) } else { describe(rules, catalog, 0) }
}
#[cfg(test)]
@@ -865,7 +867,16 @@ mod tests {
mode: lightcraft_catalog::Match::All,
rules: vec![lightcraft_catalog::Rule::Field { field: "keywords".into(), op: "contains".into(), value: serde_json::json!("Color") }],
};
assert_eq!(rules_label(&rules), "Stichwörter enthält Color");
let mut catalog = lightcraft_catalog::Catalog::new();
catalog
.apply(lightcraft_catalog::Op::AddAlbum { album: lightcraft_catalog::Album::new(lightcraft_catalog::AlbumId(4), "Ausgeschlossen") })
.unwrap();
assert_eq!(rules_label(&rules, &catalog), "Stichwörter enthält Color");
let album = lightcraft_catalog::RuleSet {
mode: lightcraft_catalog::Match::All,
rules: vec![lightcraft_catalog::Rule::Field { field: "album".into(), op: "isNot".into(), value: serde_json::json!(4) }],
};
assert!(rules_label(&album, &catalog).ends_with(" “Ausgeschlossen”"), "{}", rules_label(&album, &catalog));
let rules = lightcraft_catalog::RuleSet {
mode: lightcraft_catalog::Match::All,
rules: vec![lightcraft_catalog::Rule::Field {
@@ -874,13 +885,13 @@ mod tests {
value: serde_json::json!("publicDomain"),
}],
};
assert!(rules_label(&rules).ends_with(" Gemeinfrei"), "{}", rules_label(&rules));
assert!(rules_label(&rules, &catalog).ends_with(" Gemeinfrei"), "{}", rules_label(&rules, &catalog));
for value in [serde_json::json!(false), serde_json::json!("false")] {
let rules = lightcraft_catalog::RuleSet {
mode: lightcraft_catalog::Match::All,
rules: vec![lightcraft_catalog::Rule::Field { field: "edited".into(), op: "is".into(), value }],
};
assert!(rules_label(&rules).ends_with(" Nein"), "{}", rules_label(&rules));
assert!(rules_label(&rules, &catalog).ends_with(" Nein"), "{}", rules_label(&rules, &catalog));
}
let filter = lightcraft_catalog::Filter {
labels: vec![lightcraft_catalog::ColorLabel::Red, lightcraft_catalog::ColorLabel::Blue],
+1 -1
View File
@@ -37,7 +37,7 @@ pub(crate) fn display_label(chip: &FilterChip, filter: &lightcraft_catalog::Filt
if chip.clear.get("ruleSet").is_some()
&& let Some(rules) = &filter.rule_set
{
return format!("{}: {}", tr("Rules"), rules_label(rules));
return format!("{}: {}", tr("Rules"), rules_label(rules, catalog));
}
if chip.clear.get("label").is_some() {
let mut labels = filter.labels.clone();