LRC-LIB-SMARTCOLL: check every smart-album rule value against its field

RuleSet::problems only checked that a field and operator exist (and,
lately, yes/no values). Any other value was accepted and silently
matched nothing or everything: "Rating ≥ abc", a date of "banana" or
"2026-13", "Color Label isn't pink" (everything), "Title contains ''"
(everything), "in the last 0 days", an album that is gone.

RuleSet::check(&Catalog) -> Vec<Problem> replaces it. Each Problem has
the rule's position (path [1, 0]; shown as "rule 2.1: …"), so the
editor can point at the row. It checks:
- numbers (rating 0–5), shutter speeds (1/250, 2)
- dates: 2026, 2026-08, 2026-08-14 or a full time; real dates only
- between: two bounds; dates in order (2026-08-14 … 2026-08 is fine)
- in the last: N > 0 and a known unit
- choices: the ids, the flag's picked / rejected, a colour label's
  custom name (from the catalog), copyright-status spellings
- text and keyword / people rules: something to look for, or "is empty"
- album: exists and is a plain album
- no empty groups (an empty rule list at the top stays "all photos")

album.createSmart, album.setRules and library.filter report every
problem. The editor's date defaults move from "" (which, as "between",
matched every dated photo) to the current year, and its test now
requires every default to be valid, except text and album rules, which
wait for input.

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 7ba1470940
commit b5fe62e426
5 changed files with 276 additions and 26 deletions
+222 -15
View File
@@ -146,6 +146,130 @@ pub fn field_group(field: &str) -> Option<&'static str> {
FIELD_GROUPS.iter().find(|g| g.1.contains(&field)).map(|g| g.0)
}
/// A rule that can't mean anything ([`RuleSet::check`]): where it is and what is wrong.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Problem {
/// Positions from the top rule list down: `[1, 0]` is the first rule of the second rule's group.
pub path: Vec<usize>,
pub message: String,
}
impl std::fmt::Display for Problem {
/// `rule 2.1: rating is 0–5, not 9` (counting from 1, as people do).
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let at: Vec<String> = self.path.iter().map(|i| (i.saturating_add(1)).to_string()).collect();
write!(f, "rule {}: {}", at.join("."), self.message)
}
}
/// What is wrong with one rule, if anything ([`RuleSet::check`]).
fn field_problem(field: &str, op: &str, value: &Value, cat: &Catalog) -> Option<String> {
let Some(kind) = field_kind(field) else { return Some(format!("unknown field `{field}`")) };
if !ops_for(kind).iter().any(|o| o.0 == op) {
return Some(format!("`{field}` has no operator `{op}`"));
}
if matches!(op, "isEmpty" | "isNotEmpty") {
return None;
}
match kind {
Kind::Bool => bool_value(value).is_none().then(|| format!("`{field}` is yes or no, not {value}")),
Kind::Number if op == "between" => match value {
Value::Array(a) if a.len() == 2 => a.iter().find_map(|v| number_problem(field, v, cat)),
_ => Some(format!("`{field}` between needs two values, not {value}")),
},
Kind::Number => number_problem(field, value, cat),
Kind::Date => date_problem(field, op, value),
Kind::Choice(choices) => choice_problem(field, value, choices, cat),
Kind::Text | Kind::Keywords => match value {
Value::String(s) if s.trim().is_empty() => Some(format!("`{field}` needs something to look for (or use “is empty”)")),
Value::Null => Some(format!("`{field}` needs something to look for (or use “is empty”)")),
Value::String(_) | Value::Number(_) => None,
other => Some(format!("`{field}` needs text, not {other}")),
},
}
}
fn number_problem(field: &str, v: &Value, cat: &Catalog) -> Option<String> {
if field == "shutterSpeed" {
return shutter_value(v).is_null().then(|| format!("`shutterSpeed` needs a time like 1/250 or 2, not {v}"));
}
let Some(n) = number(v).filter(|n| n.is_finite()) else { return Some(format!("`{field}` needs a number, not {v}")) };
match field {
"rating" if !(0.0..=5.0).contains(&n) => Some(format!("rating is 0–5, not {v}")),
"album" => {
let album = (n >= 0.0 && n.fract() == 0.0).then(|| cat.album(crate::AlbumId(n as u64))).flatten();
match album {
None => Some(format!("no album {v}")),
Some(a) if a.is_smart() => Some(format!("album {v} is a smart album; only plain albums can be tested")),
Some(_) => None,
}
}
_ => None,
}
}
/// A date as rules take it: `2026`, `2026-08`, `2026-08-14` or a full ISO time, and a real one
/// (no month 13, no 30 February).
fn is_rule_date(s: &str) -> bool {
let s = s.trim();
let full = match s.len() {
4 => format!("{s}-01-01"),
7 => format!("{s}-01"),
_ => s.to_string(),
};
s.len() >= 4 && crate::dates::normalize_iso(&full).is_some()
}
fn date_problem(field: &str, op: &str, value: &Value) -> Option<String> {
let date = |v: &Value| match v.as_str() {
Some(s) if is_rule_date(s) => None,
_ => Some(format!("`{field}` needs a date like 2026, 2026-08 or 2026-08-14, not {v}")),
};
match op {
"inLast" | "notInLast" => {
let (n, unit) = match value {
Value::Object(o) => (o.get("n").and_then(number), o.get("unit").cloned().unwrap_or(json_str("days"))),
v => (number(v), json_str("days")),
};
if !n.is_some_and(|n| n.is_finite() && n > 0.0) {
return Some(format!("`{field}` needs a number of hours, days, weeks, months or years more than 0"));
}
match unit.as_str() {
Some("hours" | "days" | "weeks" | "months" | "years") => None,
_ => Some(format!("`{field}`: unknown unit {unit} (hours, days, weeks, months or years)")),
}
}
"between" => match value {
Value::Array(a) if a.len() == 2 => a.iter().find_map(date).or_else(|| {
let bound = |i: usize| a.get(i).and_then(Value::as_str).unwrap_or("").trim();
let (from, to) = (bound(0), bound(1));
(from > to && !from.starts_with(to)).then(|| format!("`{field}`: the first date is after the second"))
}),
_ => Some(format!("`{field}` between needs two dates, not {value}")),
},
_ => date(value),
}
}
fn json_str(s: &str) -> Value {
Value::String(s.to_string())
}
fn choice_problem(field: &str, value: &Value, choices: &[(&str, &str)], cat: &Catalog) -> Option<String> {
let known = value.as_str().is_some_and(|s| {
choices.iter().any(|c| c.0.eq_ignore_ascii_case(s.trim()))
|| match field {
"flag" => matches!(s.trim().to_lowercase().as_str(), "picked" | "rejected"),
// a colour's name or the custom name given to it
"label" => cat.label_from_name(s).is_some(),
"copyrightStatus" => crate::CopyrightStatus::parse(s).is_some(),
_ => false,
}
});
let ids: Vec<&str> = choices.iter().map(|c| c.0).collect();
(!known).then(|| format!("`{field}` is one of {}, not {value}", ids.join(", ")))
}
/// 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> {
@@ -541,23 +665,32 @@ impl RuleSet {
})
}
/// Unknown fields or operators (for command validation), as readable messages.
pub fn problems(&self) -> Vec<String> {
/// The rules that can't mean anything, in order: an unknown field, an operator the field
/// doesn't have, a value that isn't one of the field's (a number, a date, a choice, yes or
/// no, a plain album…), text with nothing to look for, an empty group. Commands refuse a rule
/// set with problems; the editor shows them. `cat` knows the albums and colour-label names.
pub fn check(&self, cat: &Catalog) -> Vec<Problem> {
let mut out = Vec::new();
for r in &self.rules {
match r {
Rule::Group { group } => out.extend(group.problems()),
Rule::Field { field, op, value } => match field_kind(field) {
None => out.push(format!("unknown field `{field}`")),
Some(k) if !ops_for(k).iter().any(|o| o.0 == op) => out.push(format!("`{field}` has no operator `{op}`")),
Some(Kind::Bool) if bool_value(value).is_none() => out.push(format!("`{field}` is yes or no, not {value}")),
_ => {}
},
}
}
self.check_into(cat, &mut Vec::new(), &mut out);
out
}
fn check_into(&self, cat: &Catalog, path: &mut Vec<usize>, out: &mut Vec<Problem>) {
for (i, rule) in self.rules.iter().enumerate() {
path.push(i);
match rule {
Rule::Group { group } if group.rules.is_empty() => out.push(Problem { path: path.clone(), message: "an empty group".into() }),
Rule::Group { group } => group.check_into(cat, path, out),
Rule::Field { field, op, value } => {
if let Some(message) = field_problem(field, op, value, cat) {
out.push(Problem { path: path.clone(), message });
}
}
}
path.pop();
}
}
/// A short readable summary ("rating is ≥ 3 and keywords contains travel").
pub fn describe(&self) -> String {
let join = match self.mode {
@@ -679,7 +812,8 @@ mod tests {
#[test]
fn problems_and_description() {
let r = rs(json!({"rules": [{"field": "rating", "op": "contains", "value": 1}, {"group": {"rules": [{"field": "nope", "op": "is"}]}}]}));
assert_eq!(r.problems(), vec!["`rating` has no operator `contains`".to_string(), "unknown field `nope`".to_string()]);
let messages: Vec<String> = r.check(&Catalog::new()).into_iter().map(|p| p.message).collect();
assert_eq!(messages, vec!["`rating` has no operator `contains`".to_string(), "unknown field `nope`".to_string()]);
let r = rs(
json!({"match": "any", "rules": [{"field": "rating", "op": "gte", "value": 3}, {"field": "captureDate", "op": "inLast", "value": {"n": 2, "unit": "weeks"}}]}),
);
@@ -945,7 +1079,8 @@ mod tests {
let r = rs(json!({"rules": [{"field": "edited", "op": "is", "value": true}, {"field": "cropped", "op": "is", "value": "false"}]}));
assert_eq!(r.describe(), "has edits is yes and cropped is no");
let bad = rs(json!({"rules": [{"field": "edited", "op": "is", "value": "maybe"}]}));
assert_eq!(bad.problems(), vec!["`edited` is yes or no, not \"maybe\"".to_string()]);
let messages: Vec<String> = bad.check(&cat).into_iter().map(|p| p.message).collect();
assert_eq!(messages, vec!["`edited` is yes or no, not \"maybe\"".to_string()]);
}
/// "In the last N" never overflows, whatever N an agent or a saved file holds: a zero,
@@ -972,6 +1107,78 @@ mod tests {
set_now(None);
}
/// Every rule's value is checked against its field, so a rule that can't mean anything is
/// reported instead of quietly matching nothing (or everything). Each problem says which rule
/// it is about: "rule 2", or "rule 2.1" inside a group.
#[test]
fn values_are_checked_against_their_field() {
use crate::{Album, AlbumId, Op};
let mut cat = Catalog::new();
cat.apply(Op::AddAlbum { album: Album::new(AlbumId(1), "Trip") }).unwrap();
let smart = Album { smart: Some(Box::default()), ..Album::new(AlbumId(2), "Best") };
cat.apply(Op::AddAlbum { album: smart }).unwrap();
cat.apply(Op::SetLabelName { label: crate::ColorLabel::Red, name: Some("Client".into()) }).unwrap();
let check = |rule: serde_json::Value| rs(json!({"rules": [rule]})).check(&cat);
let ok = |rule: serde_json::Value| assert!(check(rule.clone()).is_empty(), "{rule}: {:?}", check(rule.clone()));
let bad = |rule: serde_json::Value, want: &str| {
let p = check(rule.clone());
assert!(p.iter().any(|p| p.message.contains(want)), "{rule}: wanted {want:?}, got {p:?}");
};
let r = |field: &str, op: &str, value: serde_json::Value| json!({"field": field, "op": op, "value": value});
// numbers
ok(r("rating", "gte", json!(3)));
ok(r("rating", "gte", json!("4")));
ok(r("rating", "between", json!([1, 5])));
ok(r("aperture", "lte", json!("f/4")));
ok(r("focalLength", "is", json!("50mm")));
bad(r("rating", "gte", json!("abc")), "needs a number");
bad(r("rating", "gte", json!(7)), "0–5");
bad(r("iso", "between", json!([100])), "two");
bad(r("iso", "between", json!("100")), "two");
ok(r("shutterSpeed", "lte", json!("1/60")));
bad(r("shutterSpeed", "lte", json!("fast")), "1/250");
// dates
for d in ["2026", "2026-08", "2026-08-14", "2026-08-14T10:00:00"] {
ok(r("captureDate", "is", json!(d)));
}
ok(r("captureDate", "between", json!(["2026-01", "2026-12"])));
for d in [json!("banana"), json!("2026-13"), json!("2026-02-30"), json!(""), json!(2026)] {
bad(r("captureDate", "after", d), "needs a date");
}
bad(r("captureDate", "between", json!(["2026-12", "2026-01"])), "after the second");
bad(r("captureDate", "between", json!(["", "2026"])), "needs a date");
ok(r("captureDate", "inLast", json!({"n": 2, "unit": "weeks"})));
ok(r("captureDate", "inLast", json!(7)));
bad(r("captureDate", "inLast", json!({"n": 0, "unit": "days"})), "more than 0");
bad(r("captureDate", "notInLast", json!({"n": 3, "unit": "fortnights"})), "fortnights");
ok(r("captureDate", "isEmpty", json!(null)));
// choices: the ids, the flag's aliases and a colour label's custom name
ok(r("kind", "is", json!("raw")));
ok(r("flag", "is", json!("picked")));
ok(r("label", "is", json!("client")));
bad(r("label", "is", json!("pink")), "pink");
bad(r("kind", "isNot", json!("photo")), "photo");
// text needs something to look for
ok(r("title", "contains", json!("kite")));
ok(r("title", "isEmpty", json!(null)));
bad(r("title", "contains", json!(" ")), "something to look for");
bad(r("keywords", "startsWith", json!("")), "something to look for");
// albums: a plain album that exists
ok(r("album", "is", json!(1)));
bad(r("album", "is", json!(999)), "no album 999");
bad(r("album", "is", json!(2)), "smart album");
// groups: an empty one is a mistake; problems inside one say where they are
let p = rs(json!({"rules": [r("rating", "gte", json!(3)), {"group": {"match": "any", "rules": []}}]})).check(&cat);
assert_eq!(p.iter().map(ToString::to_string).collect::<Vec<_>>(), vec!["rule 2: an empty group".to_string()]);
let p = rs(json!({"rules": [r("rating", "gte", json!(3)), {"group": {"rules": [r("iso", "is", json!(100)), r("rating", "is", json!(9))]}}]}))
.check(&cat);
assert_eq!(p.len(), 1);
assert_eq!(p[0].path, vec![1, 1]);
assert_eq!(p[0].to_string(), "rule 2.2: rating is 0–5, not 9");
// an empty rule list at the top is "all photos" (or none, for any), not a mistake
assert!(rs(json!({"rules": []})).check(&cat).is_empty());
}
/// The field menu shows every rule field once: at the top level or in exactly one group,
/// and [`FIELDS`] lists them in menu order.
#[test]
+9 -8
View File
@@ -331,8 +331,8 @@ pub fn specs() -> Vec<CommandSpec> {
return Err(bad("library.filter", "a folder is already shown (library.source): show another source first"));
}
// checked like a smart album's rules: an unknown field or a value that can't match is an error, not an empty grid
if let Some(problems) = f.rule_set.as_ref().map(|rs| rs.problems()).filter(|p| !p.is_empty()) {
return Err(bad("library.filter", problems.join("; ")));
if let Some(problems) = f.rule_set.as_ref().map(|rs| rs.check(&s.catalog)).filter(|p| !p.is_empty()) {
return Err(bad("library.filter", problems.iter().map(ToString::to_string).collect::<Vec<_>>().join("; ")));
}
s.filter = f;
Ok(json!({"count": s.visible().len()}))
@@ -813,7 +813,7 @@ pub fn specs() -> Vec<CommandSpec> {
return Err(bad("album.createSmart", "empty name"));
}
let rules = match p.get("rules") {
Some(r) => merge_rules(&lightcraft_catalog::Filter::default(), r, "album.createSmart")?,
Some(r) => merge_rules(&lightcraft_catalog::Filter::default(), r, "album.createSmart", &s.catalog)?,
None => view_rules(s),
};
let parent = p.get("parent").and_then(Value::as_u64).map(AlbumId);
@@ -861,7 +861,7 @@ pub fn specs() -> Vec<CommandSpec> {
let replace = bool_or(p, "replace", false);
let folder = cur.library_folder.clone();
let base = if replace { Default::default() } else { cur };
let mut r = merge_rules(&base, p.get("rules").unwrap_or(&Value::Null), "album.setRules")?;
let mut r = merge_rules(&base, p.get("rules").unwrap_or(&Value::Null), "album.setRules", &s.catalog)?;
// the rules dialog has no folder field: replacing its rules keeps the folder
// unless the call says (`libraryFolder: null`) to drop it
if replace && p.get("rules").and_then(|r| r.get("libraryFolder")).is_none() {
@@ -1278,12 +1278,13 @@ pub fn specs() -> Vec<CommandSpec> {
}
/// `base` with a partial Filter (JSON) merged on top.
fn merge_rules(base: &lightcraft_catalog::Filter, patch: &Value, c: &str) -> Result<lightcraft_catalog::Filter> {
fn merge_rules(base: &lightcraft_catalog::Filter, patch: &Value, c: &str, cat: &lightcraft_catalog::Catalog) -> Result<lightcraft_catalog::Filter> {
let mut v = serde_json::to_value(base).unwrap_or_default();
lightcraft_develop::presets::deep_merge(&mut v, patch);
let f: lightcraft_catalog::Filter = serde_json::from_value(v).map_err(|e| bad(c, e.to_string()))?;
if let Some(problem) = f.rule_set.as_ref().and_then(|r| r.problems().into_iter().next()) {
return Err(bad(c, problem));
let problems = f.rule_set.as_ref().map(|r| r.check(cat)).unwrap_or_default();
if !problems.is_empty() {
return Err(bad(c, problems.iter().map(ToString::to_string).collect::<Vec<_>>().join("; ")));
}
Ok(f)
}
@@ -1319,7 +1320,7 @@ fn view_rules(s: &Session) -> lightcraft_catalog::Filter {
}
}
}
merge_rules(&base, &Value::Object(patch), "").unwrap_or(base)
merge_rules(&base, &Value::Object(patch), "", &s.catalog).unwrap_or(base)
}
None => {
let mut f = s.source.to_filter(&s.filter, &s.catalog);
+22
View File
@@ -491,3 +491,25 @@ fn capture_time_shift_is_bounded() {
s.execute("photo.setCaptureTime", &serde_json::json!({"ids": [id.0], "hours": 1})).unwrap();
assert_ne!(s.catalog.photo(id).unwrap().captured, before);
}
/// Values are checked too: an agent's rule set with a date that isn't one, or a rating of 9, is
/// refused with the rule's position, by album.createSmart and library.filter alike.
#[test]
fn smart_rule_values_are_checked_by_commands() {
let mut s = crate::Session::with_demo();
let r = s.execute(
"album.createSmart",
&serde_json::json!({"name": "Odd", "rules": {"ruleSet": {"rules": [
{"field": "rating", "op": "gte", "value": 3},
{"field": "captureDate", "op": "is", "value": "banana"}
]}}}),
);
let e = r.expect_err("a date that isn't one").to_string();
assert!(e.contains("rule 2:") && e.contains("needs a date"), "{e}");
assert!(s.catalog.albums().all(|a| a.name != "Odd"));
let e = s
.execute("library.filter", &serde_json::json!({"ruleSet": {"rules": [{"field": "rating", "op": "gte", "value": 9}]}}))
.expect_err("rating 9")
.to_string();
assert!(e.contains("rule 1: rating is 0–5, not 9"), "{e}");
}
+1 -1
View File
@@ -245,7 +245,7 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) {
egui::ScrollArea::vertical().max_height(360.0).auto_shrink([false, true]).show(ui, |ui| {
crate::panels::rules_editor::edit(ui, rules, "rules", 0);
});
let problems = rules.problems();
let problems: Vec<String> = rules.check(&app.session.catalog).iter().map(ToString::to_string).collect();
// the folder an album made from a folder view carries is not in the editor, but it counts
let folder = id.and_then(|id| app.session.catalog.album(lightcraft_catalog::AlbumId(id))).and_then(|a| a.smart.as_deref().and_then(|f| f.library_folder.clone()));
let f = lightcraft_catalog::Filter { rule_set: Some(rules.clone()), library_folder: folder, ..Default::default() };
+22 -2
View File
@@ -14,7 +14,13 @@ pub fn default_value(field: &str, op: &str) -> Value {
match (field_kind(field), op) {
(_, "isEmpty" | "isNotEmpty") => Value::Null,
(Some(Kind::Date), "inLast" | "notInLast") => json!({"n": 30, "unit": "days"}),
(Some(Kind::Date), "between") => json!(["", ""]),
// this year (the catalog's clock): a date that means something, not an empty one that
// matches nothing ("is") or every date ("between")
(Some(Kind::Date), "between") => {
let year = this_year();
json!([format!("{year}-01"), format!("{year}-12")])
}
(Some(Kind::Date), _) => json!(this_year()),
(Some(Kind::Number), "between") if field == "shutterSpeed" => json!(["1/1000", "1/125"]),
(Some(Kind::Number), _) if field == "shutterSpeed" => json!("1/250"),
(Some(Kind::Number), "between") => json!([0, 0]),
@@ -25,6 +31,11 @@ pub fn default_value(field: &str, op: &str) -> Value {
}
}
/// The current year, `2026`.
fn this_year() -> String {
lightcraft_catalog::rules::now().get(..4).unwrap_or("2026").to_string()
}
/// A new rule (Rating ≥ 3).
pub fn new_rule() -> Rule {
Rule::Field { field: "rating".into(), op: "gte".into(), value: json!(3) }
@@ -297,7 +308,11 @@ mod tests {
mode: Match::All,
rules: vec![Rule::Field { field: field.to_string(), op: op.to_string(), value: default_value(field, op) }],
};
assert!(rs.problems().is_empty(), "{field} {op}: {:?}", rs.problems());
// only a text rule (nothing to look for yet) and Album (no album picked yet) wait for
// input; every other default is a rule that means something
let problems = rs.check(&cat);
let waits = matches!(kind, Kind::Text | Kind::Keywords) || *field == "album";
assert!(problems.is_empty() || waits && !matches!(*op, "isEmpty" | "isNotEmpty"), "{field} {op}: {problems:?}");
let _ = rs.matches(&p, &cat);
let v = default_value(field, op);
let one = |v: &Value| match kind {
@@ -315,6 +330,11 @@ mod tests {
}
}
}
// a new date rule starts at this year, not at "" (and a between doesn't match every date)
lightcraft_catalog::rules::set_now(Some("2026-10-09T12:00:00".into()));
assert_eq!(default_value("captureDate", "is"), json!("2026"));
assert_eq!(default_value("captureDate", "between"), json!(["2026-01", "2026-12"]));
lightcraft_catalog::rules::set_now(None);
assert_eq!(default_value("shutterSpeed", "gte"), json!("1/250"));
assert_eq!(default_value("shutterSpeed", "between"), json!(["1/1000", "1/125"]));
}