diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index 9035a67304..6701b85801 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -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, + 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 = 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 { + 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 { + 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 { + 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 { + 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 { @@ -541,23 +665,32 @@ impl RuleSet { }) } - /// Unknown fields or operators (for command validation), as readable messages. - pub fn problems(&self) -> Vec { + /// 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 { 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, out: &mut Vec) { + 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 = 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 = 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!["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] diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 0a7a320864..7d6c30ae32 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -331,8 +331,8 @@ pub fn specs() -> Vec { 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::>().join("; "))); } s.filter = f; Ok(json!({"count": s.visible().len()})) @@ -813,7 +813,7 @@ pub fn specs() -> Vec { 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 { 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 { } /// `base` with a partial Filter (JSON) merged on top. -fn merge_rules(base: &lightcraft_catalog::Filter, patch: &Value, c: &str) -> Result { +fn merge_rules(base: &lightcraft_catalog::Filter, patch: &Value, c: &str, cat: &lightcraft_catalog::Catalog) -> Result { 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::>().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); diff --git a/crates/engine/src/tests_library.rs b/crates/engine/src/tests_library.rs index bc98b803f7..8d5d559872 100644 --- a/crates/engine/src/tests_library.rs +++ b/crates/engine/src/tests_library.rs @@ -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}"); +} diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index f2bdd668ce..e6e8ce6264 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -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 = 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() }; diff --git a/crates/ui-egui/src/panels/rules_editor.rs b/crates/ui-egui/src/panels/rules_editor.rs index b85d7b3be6..8c802c4801 100644 --- a/crates/ui-egui/src/panels/rules_editor.rs +++ b/crates/ui-egui/src/panels/rules_editor.rs @@ -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"])); }