From 7b7807b499a55c2cb7cc89ffbca4d6a9e3d362e8 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 08:43:31 -0400 Subject: [PATCH] LRC-LIB-SMARTCOLL: rule editor marks bad rules, blocks OK and picks albums from a list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dialog showed only the first problem, in place of the match count, without saying which rule it was about. OK stayed clickable, and an edit renamed the album before its rules were refused. - Each rule with a problem gets the message right under its row, in the caution colour; inside groups too, and on an empty group's frame. The rules_editor::Env carries the problems from RuleSet::check, and each row finds its own by path. Markers register `ruleProblem:`. - The summary reads "Fix the marked rules to save this album." and OK is disabled while any problem remains. - confirm_dialog refuses problems before anything changes (not even the name), and the control channel's ui.dialog.confirm keeps the editor open on that error, as the button does. - Album is its own field kind: is / isn't only (it was a number field that offered ≥ and between). It picks its album from a list of the plain albums (`ruleAlbum:` / `ruleAlbumItem::`) and starts with none chosen ("choose an album"). album.ruleFields reports kind "album". - Found while testing: the number editor's spinner clamped a stored value outside its range as soon as it was drawn, so "Rating is 9" quietly became "Rating is 5". It now uses clamp_existing_to_range(false): the range limits dragging, and a stored value stays as it is, marked by the check. New labels (Choose an album…, No albums yet, Fix the marked rules…) are translated in all seven locales. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/rules.rs | 47 ++++++++++------ crates/engine/src/cmd/library.rs | 3 +- crates/engine/src/tests_library.rs | 2 + crates/ui-egui/locales/de.json | 5 +- crates/ui-egui/locales/es.json | 5 +- crates/ui-egui/locales/ja.json | 5 +- crates/ui-egui/locales/pt-br.json | 5 +- crates/ui-egui/locales/ru.json | 5 +- crates/ui-egui/locales/zh-hans.json | 5 +- crates/ui-egui/locales/zh-hant.json | 5 +- crates/ui-egui/src/control.rs | 8 ++- crates/ui-egui/src/panels/dialogs.rs | 33 +++++++---- crates/ui-egui/src/panels/rules_editor.rs | 64 +++++++++++++++++++--- crates/ui-egui/src/tests_masking.rs | 67 +++++++++++++++++++++++ 14 files changed, 215 insertions(+), 44 deletions(-) diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index 702b2191c..d8d75f118 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -48,6 +48,8 @@ pub enum Kind { Keywords, Number, Date, + /// A plain album, by id: in it or not. + Album, /// One of a fixed set: (id, label). Rules store the id; people see the label. Choice(&'static [(&'static str, &'static str)]), Bool, @@ -65,7 +67,7 @@ pub const FIELDS: &[(&str, &str, Kind)] = &[ ), ("text", "Any Searchable Text", Kind::Text), // Source - ("album", "Album", Kind::Number), + ("album", "Album", Kind::Album), ("virtualCopy", "Virtual Copy", Kind::Bool), ("copyName", "Copy Name", Kind::Text), ("stacked", "In a Stack", Kind::Bool), @@ -173,11 +175,12 @@ fn field_problem(field: &str, op: &str, value: &Value, cat: &Catalog) -> Option< } match kind { Kind::Bool => bool_value(value).is_none().then(|| format!("`{field}` is yes or no, not {value}")), + Kind::Album => album_problem(value, cat), Kind::Number => { let each = match value { - Value::Array(a) if op == "between" && a.len() == 2 => a.iter().find_map(|v| number_problem(field, v, cat)), + Value::Array(a) if op == "between" && a.len() == 2 => a.iter().find_map(|v| number_problem(field, v)), _ if op == "between" => Some(format!("`{field}` between needs two values, not {value}")), - v => number_problem(field, v, cat), + v => number_problem(field, v), }; // a rating rule no rating could meet ("is 9", "≥ 7"); "< 6" is fine, if broad each.or_else(|| { @@ -199,21 +202,23 @@ fn field_problem(field: &str, op: &str, value: &Value, cat: &Catalog) -> Option< } } -fn number_problem(field: &str, v: &Value, cat: &Catalog) -> Option { +fn number_problem(field: &str, v: &Value) -> 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 { - "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, + number(v).filter(|n| n.is_finite()).is_none().then(|| format!("`{field}` needs a number, not {v}")) +} + +/// An Album rule's value: an album that exists and holds photos itself (not a smart album). +fn album_problem(v: &Value, cat: &Catalog) -> Option { + if v.is_null() { + return Some("choose an album".into()); + } + let id = number(v).filter(|n| n.is_finite() && *n >= 0.0 && n.fract() == 0.0); + match id.and_then(|n| cat.album(crate::AlbumId(n as u64))) { + 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, } } @@ -342,7 +347,7 @@ pub fn ops_for(kind: Kind) -> &'static [(&'static str, &'static str)] { ("notInLast", "isn't in the last"), ("isEmpty", "is unknown"), ], - Kind::Choice(_) => &[("is", "is"), ("isNot", "isn't")], + Kind::Choice(_) | Kind::Album => &[("is", "is"), ("isNot", "isn't")], Kind::Bool => &[("is", "is")], } } @@ -1235,6 +1240,16 @@ mod tests { set_now(None); } + /// An album is in it or not: Album offers "is" and "isn't", never "≥" or "between". + #[test] + fn album_rules_are_is_or_isnt() { + assert_eq!(field_kind("album"), Some(Kind::Album)); + let ops: Vec<&str> = ops_for(Kind::Album).iter().map(|o| o.0).collect(); + assert_eq!(ops, ["is", "isNot"]); + let r = rs(json!({"rules": [{"field": "album", "op": "gte", "value": 1}]})); + assert_eq!(r.check(&Catalog::new()).first().map(|p| p.message.as_str()), Some("`album` has no operator `gte`")); + } + /// 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 820f511e5..eef94db63 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -823,7 +823,7 @@ pub fn specs() -> Vec { Ok(json!({"id": id.0, "count": s.catalog.album_count(id)})) } ), - cmd!(query "album.ruleFields", "Smart Album Rule Fields", [], None, "{} → [{field, label, kind, group (field-menu submenu, null at the top level), ops: [{op, label}], choices? (ids), choiceLabels? ({id: label})}] for ruleSet rules, in menu order", always, |_, _| { + cmd!(query "album.ruleFields", "Smart Album Rule Fields", [], None, "{} → [{field, label, kind (text|keywords|number|date|choice|bool|album: an album id), group (field-menu submenu, null at the top level), ops: [{op, label}], choices? (ids), choiceLabels? ({id: label})}] for ruleSet rules, in menu order", always, |_, _| { use lightcraft_catalog::rules::{FIELDS, Kind, field_group, ops_for}; Ok(json!(FIELDS .iter() @@ -832,6 +832,7 @@ pub fn specs() -> Vec { Kind::Text => "text", Kind::Keywords => "keywords", Kind::Number => "number", + Kind::Album => "album", Kind::Date => "date", Kind::Choice(_) => "choice", Kind::Bool => "bool", diff --git a/crates/engine/src/tests_library.rs b/crates/engine/src/tests_library.rs index fce8ef843..b937d68dc 100644 --- a/crates/engine/src/tests_library.rs +++ b/crates/engine/src/tests_library.rs @@ -260,6 +260,8 @@ fn smart_album_rule_sets() { let flag = fields.as_array().unwrap().iter().find(|f| f["field"] == "copyrightStatus").unwrap(); assert_eq!(flag["choices"], serde_json::json!(["copyrighted", "publicDomain", "unknown"])); assert_eq!(flag["choiceLabels"]["publicDomain"], "Public Domain"); + let album = fields.as_array().unwrap().iter().find(|f| f["field"] == "album").unwrap(); + assert_eq!((album["kind"].as_str(), album["ops"].as_array().map(Vec::len)), (Some("album"), Some(2)), "an album id, is / isn't"); let bad = s.execute( "album.createSmart", &serde_json::json!({"name": "Bad", "rules": {"ruleSet": {"rules": [{"field": "rating", "op": "contains", "value": 1}]}}}), diff --git a/crates/ui-egui/locales/de.json b/crates/ui-egui/locales/de.json index 936a11ec0..2ae0af114 100644 --- a/crates/ui-egui/locales/de.json +++ b/crates/ui-egui/locales/de.json @@ -1896,5 +1896,8 @@ "Portrait (tall)": "Hochformat", "Square": "Quadratisch", "In Color": "Farbig", - "No Label": "Ohne Markierung" + "No Label": "Ohne Markierung", + "Choose an album…": "Album wählen…", + "No albums yet": "Noch keine Alben", + "Fix the marked rules to save this album.": "Korrigiere die markierten Regeln, um dieses Album zu speichern." } diff --git a/crates/ui-egui/locales/es.json b/crates/ui-egui/locales/es.json index 475744d20..812841ab9 100644 --- a/crates/ui-egui/locales/es.json +++ b/crates/ui-egui/locales/es.json @@ -1741,5 +1741,8 @@ "Portrait (tall)": "Vertical (retrato)", "Square": "Cuadrada", "In Color": "En color", - "No Label": "Sin etiqueta" + "No Label": "Sin etiqueta", + "Choose an album…": "Elige un álbum…", + "No albums yet": "Aún no hay álbumes", + "Fix the marked rules to save this album.": "Corrige las reglas marcadas para guardar este álbum." } diff --git a/crates/ui-egui/locales/ja.json b/crates/ui-egui/locales/ja.json index de1a87de2..84dce170d 100644 --- a/crates/ui-egui/locales/ja.json +++ b/crates/ui-egui/locales/ja.json @@ -1745,5 +1745,8 @@ "smart album value|Yes": "あり", "smart album value|No": "なし", "In a Stack": "スタック", - "Cropped": "切り抜き" + "Cropped": "切り抜き", + "Choose an album…": "アルバムを選択…", + "No albums yet": "アルバムはまだありません", + "Fix the marked rules to save this album.": "このアルバムを保存するには、印の付いた条件を修正してください。" } diff --git a/crates/ui-egui/locales/pt-br.json b/crates/ui-egui/locales/pt-br.json index 2bac81d40..230ca8af0 100644 --- a/crates/ui-egui/locales/pt-br.json +++ b/crates/ui-egui/locales/pt-br.json @@ -1741,5 +1741,8 @@ "Portrait (tall)": "Retrato (vertical)", "Square": "Quadrada", "In Color": "Colorida", - "No Label": "Sem rótulo" + "No Label": "Sem rótulo", + "Choose an album…": "Escolha um álbum…", + "No albums yet": "Ainda não há álbuns", + "Fix the marked rules to save this album.": "Corrija as regras marcadas para salvar este álbum." } diff --git a/crates/ui-egui/locales/ru.json b/crates/ui-egui/locales/ru.json index cd36a04ec..4a8da93bd 100644 --- a/crates/ui-egui/locales/ru.json +++ b/crates/ui-egui/locales/ru.json @@ -1413,5 +1413,8 @@ "Portrait (tall)": "Книжная", "Square": "Квадратная", "In Color": "Цветное", - "No Label": "Без метки" + "No Label": "Без метки", + "Choose an album…": "Выберите альбом…", + "No albums yet": "Альбомов пока нет", + "Fix the marked rules to save this album.": "Исправьте отмеченные условия, чтобы сохранить альбом." } diff --git a/crates/ui-egui/locales/zh-hans.json b/crates/ui-egui/locales/zh-hans.json index e1d579ac1..f9c093fda 100644 --- a/crates/ui-egui/locales/zh-hans.json +++ b/crates/ui-egui/locales/zh-hans.json @@ -1741,5 +1741,8 @@ "Portrait (tall)": "纵向", "Square": "正方形", "In Color": "彩色", - "No Label": "无标签" + "No Label": "无标签", + "Choose an album…": "选择相册…", + "No albums yet": "还没有相册", + "Fix the marked rules to save this album.": "请修正标记的规则以保存此相册。" } diff --git a/crates/ui-egui/locales/zh-hant.json b/crates/ui-egui/locales/zh-hant.json index dc80a7b30..1c13a96e1 100644 --- a/crates/ui-egui/locales/zh-hant.json +++ b/crates/ui-egui/locales/zh-hant.json @@ -1741,5 +1741,8 @@ "Portrait (tall)": "直向", "Square": "正方形", "In Color": "彩色", - "No Label": "無標籤" + "No Label": "無標籤", + "Choose an album…": "選擇相簿…", + "No albums yet": "還沒有相簿", + "Fix the marked rules to save this album.": "請修正標記的規則以儲存此相簿。" } diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index dcd3d1f8b..3512d8f79 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -310,8 +310,12 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest "ui.dialog.confirm" => match app.ui.dialog.take() { Some(d) => { let r = crate::panels::dialogs::confirm_dialog(app, &d); - // the import review stays open on an error, as with its button - if (r.is_err() && matches!(d, crate::state::Dialog::Import { .. } | crate::state::Dialog::SamModel { .. })) + // the import review and the rule editor stay open on an error, as with their buttons + if (r.is_err() + && matches!( + d, + crate::state::Dialog::Import { .. } | crate::state::Dialog::SamModel { .. } | crate::state::Dialog::SmartRules { .. } + )) || (r.is_ok() && crate::panels::dialogs::keeps_open(app, &d)) { app.ui.dialog = Some(d); diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index e6e8ce626..3d429444a 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -242,22 +242,28 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { let r = ui.add(egui::TextEdit::singleline(name).hint_text(crate::i18n::tr("Name")).desired_width(f32::INFINITY)); crate::widgets::register(ui.ctx(), "field:smartName", r.rect); ui.add_space(6.0); + // checked before drawing, so each row marks its own problem + let env = crate::panels::rules_editor::Env { + problems: rules.check(&app.session.catalog), + albums: app.session.catalog.albums().filter(|a| !a.is_smart() && !a.folder).map(|a| (a.id.0, a.name.clone())).collect(), + }; egui::ScrollArea::vertical().max_height(360.0).auto_shrink([false, true]).show(ui, |ui| { - crate::panels::rules_editor::edit(ui, rules, "rules", 0); + crate::panels::rules_editor::edit(ui, rules, "rules", 0, &[], &env); }); - let problems: Vec = rules.check(&app.session.catalog).iter().map(ToString::to_string).collect(); + let problems = rules.check(&app.session.catalog); // 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() }; let n = if problems.is_empty() { app.session.catalog.query(&f, &Default::default()).len() } else { 0 }; ui.add_space(4.0); - ui.label( - egui::RichText::new(match problems.first() { - Some(p) => p.clone(), - None => crate::i18n::tr_format!("{n} photo{} match · updates automatically as photos change", if n == 1 { "" } else { "s" }, n = n), - }) - .color(t.text_dim), - ); + if problems.is_empty() { + ui.label( + egui::RichText::new(crate::i18n::tr_format!("{n} photo{} match · updates automatically as photos change", if n == 1 { "" } else { "s" }, n = n)) + .color(t.text_dim), + ); + } else { + ui.label(egui::RichText::new(crate::i18n::tr("Fix the marked rules to save this album.")).color(t.caution)); + } } Dialog::NewSmartAlbum { name, .. } => { let r = ui.add(egui::TextEdit::singleline(name).hint_text(crate::i18n::tr("Name")).desired_width(f32::INFINITY)); @@ -833,7 +839,10 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { _ => "OK", }; // installing waits for the licence to be accepted - let can_confirm = informational || !matches!(&dlg, Dialog::DenoiseModel { accepted: false, .. } | Dialog::FaceModel { accepted: false, .. }); + let can_confirm = informational + || !matches!(&dlg, Dialog::DenoiseModel { accepted: false, .. } | Dialog::FaceModel { accepted: false, .. }) + // smart-album rules that can't mean anything wait to be fixed + && !matches!(&dlg, Dialog::SmartRules { rules, .. } if !rules.check(&app.session.catalog).is_empty()); let r = (!ok.is_empty()).then(|| ui.add_enabled(can_confirm, egui::Button::new(crate::i18n::tr(ok)))); if let Some(r) = &r { crate::widgets::register(ui.ctx(), "button:dialogOk", r.rect); @@ -1075,6 +1084,10 @@ pub fn confirm_dialog(app: &mut LightcraftApp, dlg: &Dialog) -> Result { + // refused before anything changes (not even the name) + if let Some(problem) = rules.check(&app.session.catalog).first() { + return Err(problem.to_string()); + } let name = if name.trim().is_empty() { "Smart Album".to_string() } else { name.trim().to_string() }; match id { Some(id) => { diff --git a/crates/ui-egui/src/panels/rules_editor.rs b/crates/ui-egui/src/panels/rules_editor.rs index 8c802c480..2cf09c297 100644 --- a/crates/ui-egui/src/panels/rules_editor.rs +++ b/crates/ui-egui/src/panels/rules_editor.rs @@ -2,7 +2,7 @@ //! value), with nested groups. Field and operator lists come from `lightcraft_catalog::rules`. use egui::RichText; -use lightcraft_catalog::rules::{FIELD_GROUPS, Kind, TOP_LEVEL_FIELDS, bool_value, field_kind, field_label, ops_for}; +use lightcraft_catalog::rules::{FIELD_GROUPS, Kind, Problem, TOP_LEVEL_FIELDS, bool_value, field_kind, field_label, ops_for}; use lightcraft_catalog::{Match, Rule, RuleSet}; use serde_json::{Value, json}; @@ -27,6 +27,8 @@ pub fn default_value(field: &str, op: &str) -> Value { (Some(Kind::Number), _) => json!(if field == "rating" { 3 } else { 0 }), (Some(Kind::Choice(c)), _) => json!(c.first().map_or("", |c| c.0)), (Some(Kind::Bool), _) => json!(true), + // picked from the list + (Some(Kind::Album), _) => Value::Null, _ => json!(""), } } @@ -77,18 +79,49 @@ fn number_value(ui: &mut egui::Ui, v: &mut Value, field: &str) { "longEdge" | "shortEdge" => (0.0, 100_000.0, 10.0), _ => (0.0, 100_000.0, 0.5), }; - let whole = matches!(field, "rating" | "iso" | "album" | "keywordCount" | "personCount" | "longEdge" | "shortEdge"); - let dv = egui::DragValue::new(&mut n).range(lo..=hi).speed(speed); + let whole = matches!(field, "rating" | "iso" | "keywordCount" | "personCount" | "longEdge" | "shortEdge"); + // the range limits dragging; a stored value outside it (a rating of 9 from an agent) is shown + // as it is, marked by the check, not quietly clamped to something that matches + let dv = egui::DragValue::new(&mut n).range(lo..=hi).clamp_existing_to_range(false).speed(speed); let dv = if whole { dv.fixed_decimals(0) } else { dv.max_decimals(2) }; if ui.add(dv).changed() { *v = if whole { json!(n.round() as i64) } else { json!(n) }; } } +/// What the editor shows besides the rules: the problems `RuleSet::check` found in them (each +/// row marks its own) and the albums an Album rule can test. +#[derive(Default)] +pub struct Env { + pub problems: Vec, + /// The plain albums: (id, name), in the sidebar's order. + pub albums: Vec<(u64, String)>, +} + +/// An Album rule's value: one of the plain albums, picked by name. +fn album_value(ui: &mut egui::Ui, v: &mut Value, salt: &str, env: &Env) { + let cur = v.as_u64(); + let text = env.albums.iter().find(|a| Some(a.0) == cur).map_or_else(|| crate::i18n::tr("Choose an album…").to_string(), |a| a.1.clone()); + let r = egui::ComboBox::from_id_salt(format!("{salt}-album")).width(160.0).selected_text(text).show_ui(ui, |ui| { + for (id, name) in &env.albums { + let r = ui.selectable_label(cur == Some(*id), name); + register(ui.ctx(), format!("ruleAlbumItem:{id}:{salt}"), r.rect); + if r.clicked() { + *v = json!(id); + } + } + if env.albums.is_empty() { + ui.label(crate::i18n::tr("No albums yet")); + } + }); + register(ui.ctx(), format!("ruleAlbum:{salt}"), r.response.rect); +} + /// The value editor for one rule. -fn value_editor(ui: &mut egui::Ui, field: &str, op: &str, v: &mut Value, salt: &str) { +fn value_editor(ui: &mut egui::Ui, field: &str, op: &str, v: &mut Value, salt: &str, env: &Env) { match (field_kind(field), op) { (_, "isEmpty" | "isNotEmpty") => {} + (Some(Kind::Album), _) => album_value(ui, v, salt, env), (Some(Kind::Date), "inLast" | "notInLast") => { if !v.is_object() { *v = default_value(field, op); @@ -209,8 +242,18 @@ fn field_menu(ui: &mut egui::Ui, salt: &str, field: &mut String, op: &mut String *value = default_value(field, op); } -/// Edit `rs` in place; `salt` keeps widget ids apart between groups. -pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize) { +/// A rule's problem, shown under it in the caution colour (`ruleProblem:` for tests and +/// agents). +fn problem_note(ui: &mut egui::Ui, problem: Option<&Problem>, salt: &str) { + let Some(p) = problem else { return }; + let t = Tokens::get(ui.ctx()); + let r = ui.label(RichText::new(format!("⚠ {}", p.message)).color(t.caution).small()); + register(ui.ctx(), format!("ruleProblem:{salt}"), r.rect); +} + +/// Edit `rs` in place; `salt` keeps widget ids apart between groups, `path` is where `rs` sits +/// (empty at the top) so each row finds its own problem in `env`. +pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize, path: &[usize], env: &Env) { let t = Tokens::get(ui.ctx()); ui.horizontal(|ui| { ui.label(RichText::new(crate::i18n::tr("Match")).color(t.text_label)); @@ -221,6 +264,8 @@ pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize) { let mut insert: Option<(usize, Rule)> = None; for (i, rule) in rs.rules.iter_mut().enumerate() { let rsalt = format!("{salt}-{i}"); + let here: Vec = path.iter().copied().chain([i]).collect(); + let problem = env.problems.iter().find(|p| p.path == here); match rule { Rule::Group { group } => { egui::Frame::group(ui.style()).show(ui, |ui| { @@ -228,8 +273,9 @@ pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize) { if ui.small_button("−").on_hover_text(crate::i18n::tr("Remove this group")).clicked() { remove = Some(i); } - ui.vertical(|ui| edit(ui, group, &rsalt, depth + 1)); + ui.vertical(|ui| edit(ui, group, &rsalt, depth + 1, &here, env)); }); + problem_note(ui, problem, &rsalt); }); } Rule::Field { field, op, value } => { @@ -248,7 +294,7 @@ pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize) { } } }); - value_editor(ui, field, op, value, &rsalt); + value_editor(ui, field, op, value, &rsalt, env); ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { let add = ui.small_button("+").on_hover_text(crate::i18n::tr("Add a rule (⌥-click: a group)")); register(ui.ctx(), format!("button:ruleAdd-{rsalt}"), add.rect); @@ -266,6 +312,7 @@ pub fn edit(ui: &mut egui::Ui, rs: &mut RuleSet, salt: &str, depth: usize) { } }); }); + problem_note(ui, problem, &rsalt); } } } @@ -320,6 +367,7 @@ mod tests { Kind::Number if *field == "shutterSpeed" => v.as_str().and_then(lightcraft_catalog::parse_shutter_seconds).is_some(), Kind::Number => v.is_number(), Kind::Bool => v.is_boolean(), + Kind::Album => v.is_null(), Kind::Text | Kind::Keywords | Kind::Date => v.is_string(), }; match *op { diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index fd64b0a94..3fba77f3c 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -340,6 +340,73 @@ fn smart_album_editor_keeps_an_unreadable_yes_no_value() { assert!(h.app.session.catalog.albums().all(|a| a.name != "Odd"), "OK refuses it: {r}"); } +/// The rule editor marks each rule that can't mean anything, inside groups too, and keeps OK +/// disabled until they are fixed; the control channel's confirm is refused the same way. +#[test] +fn smart_album_editor_marks_bad_rules_and_blocks_ok() { + let mut h = detail("panel.edit"); + exec(&mut h, "dialog.smartAlbum", json!({"name": "Checked"})); + let set = |h: &mut Headless, rules: serde_json::Value| { + let Some(crate::state::Dialog::SmartRules { rules: r, .. }) = &mut h.app.ui.dialog else { panic!("no rule editor") }; + *r = serde_json::from_value(json!({"rules": rules})).unwrap(); + h.settle(SETTLE); + }; + let has = |h: &Headless, id: &str| h.app.widgets.iter().any(|(w, _)| w == id); + set( + &mut h, + json!([ + {"field": "rating", "op": "gte", "value": 3}, + {"field": "captureDate", "op": "is", "value": "banana"}, + {"group": {"match": "any", "rules": [{"field": "iso", "op": "is", "value": 100}, {"field": "rating", "op": "is", "value": 9}]}} + ]), + ); + assert!(!has(&h, "ruleProblem:rules-0"), "a good rule isn't marked"); + assert!(has(&h, "ruleProblem:rules-1"), "the bad date is marked"); + assert!(has(&h, "ruleProblem:rules-2-1") && !has(&h, "ruleProblem:rules-2-0"), "inside a group, the bad rule itself"); + let Some(crate::state::Dialog::SmartRules { rules, .. }) = &h.app.ui.dialog else { panic!("no rule editor") }; + assert!(serde_json::to_string(rules).unwrap().contains(r#""value":9"#), "drawing doesn't clamp the rating to 5"); + let r = h.request("ui.clickWidget", json!({"id": "button:dialogOk"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert!(h.app.session.catalog.albums().all(|a| a.name != "Checked"), "OK is disabled"); + let r = h.request("ui.dialog.confirm", json!({}), T); + assert!(h.app.session.catalog.albums().all(|a| a.name != "Checked"), "confirm refuses too: {r}"); + assert!(h.app.ui.dialog.is_some(), "and the dialog stays open with the rules"); + // fixed, the marks go and OK saves + set(&mut h, json!([{"field": "rating", "op": "gte", "value": 3}, {"field": "captureDate", "op": "is", "value": "2026"}])); + assert!(!h.app.widgets.iter().any(|(w, _)| w.starts_with("ruleProblem:"))); + let r = h.request("ui.clickWidget", json!({"id": "button:dialogOk"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert!(h.app.session.catalog.albums().any(|a| a.name == "Checked")); +} + +/// An Album rule picks its album from a list of the plain albums, not by typing an id. +#[test] +fn smart_album_editor_picks_an_album_from_a_list() { + let mut h = detail("panel.edit"); + let trip = exec(&mut h, "album.create", json!({"name": "Trip", "addSelected": false}))["id"].as_u64().unwrap(); + let smart = exec(&mut h, "album.createSmart", json!({"name": "Fives", "rules": {"rating": 5}}))["id"].as_u64().unwrap(); + exec(&mut h, "dialog.smartAlbum", json!({"name": "In Trip"})); + let Some(crate::state::Dialog::SmartRules { rules, .. }) = &mut h.app.ui.dialog else { panic!("no rule editor") }; + rules.rules[0] = serde_json::from_value(json!({"field": "album", "op": "is", "value": 0})).unwrap(); + h.settle(SETTLE); + assert!(h.app.widgets.iter().any(|(w, _)| w == "ruleProblem:rules-0"), "no album chosen yet"); + let r = h.request("ui.clickWidget", json!({"id": "ruleAlbum:rules-0"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let has = |h: &Headless, id: String| h.app.widgets.iter().any(|(w, _)| *w == id); + assert!(has(&h, format!("ruleAlbumItem:{trip}:rules-0")), "plain albums are offered"); + assert!(!has(&h, format!("ruleAlbumItem:{smart}:rules-0")), "smart albums can't be tested"); + let r = h.request("ui.clickWidget", json!({"id": format!("ruleAlbumItem:{trip}:rules-0")}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let Some(crate::state::Dialog::SmartRules { rules, .. }) = &h.app.ui.dialog else { panic!("no rule editor") }; + let Rule::Field { value, .. } = &rules.rules[0] else { panic!("not a field rule") }; + assert_eq!(value, &json!(trip)); + assert!(!h.app.widgets.iter().any(|(w, _)| w == "ruleProblem:rules-0")); +} + #[test] fn g_toggles_grids_and_shift_g_starts_guided_upright() { let mut h = detail("panel.edit");