From 5c949e01e80969ce9c33e71a1afc676caaa39804 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 10:05:55 -0400 Subject: [PATCH] LRC-LIB-SMARTCOLL: rule editor offers smart albums and blocks loops; Esc closes only popups - The Album rule's list offers smart albums under a "Smart Albums" heading (translated in all seven locales). It leaves out the album being edited and any smart album that tests it, since that would loop back (album_choices, Catalog::album_reaches). - The dialog checks rules for the album being edited (RuleSet::check_for), so a loop set by other means is marked under its row, OK is disabled, and confirm refuses it. - Esc with any popup open in a dialog (a dropdown, a menu, a date picker) closed the dialog with its edits. The dialog now claims Esc for an open popup before drawing (widgets::take_escape), so only the popup closes. New headless test for a dropdown, next to the calendar's. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/locales/de.json | 3 +- crates/ui-egui/locales/es.json | 3 +- crates/ui-egui/locales/ja.json | 3 +- crates/ui-egui/locales/pt-br.json | 3 +- crates/ui-egui/locales/ru.json | 3 +- crates/ui-egui/locales/zh-hans.json | 3 +- crates/ui-egui/locales/zh-hant.json | 3 +- crates/ui-egui/src/i18n.rs | 5 ++- crates/ui-egui/src/panels/dialogs.rs | 48 ++++++++++++++--------- crates/ui-egui/src/panels/rules_editor.rs | 23 ++++++++--- crates/ui-egui/src/tests_date_picker.rs | 13 ++++++ crates/ui-egui/src/tests_masking.rs | 33 +++++++++++++++- 12 files changed, 110 insertions(+), 33 deletions(-) diff --git a/crates/ui-egui/locales/de.json b/crates/ui-egui/locales/de.json index 74a5f1ac4..fa06ae57e 100644 --- a/crates/ui-egui/locales/de.json +++ b/crates/ui-egui/locales/de.json @@ -1931,5 +1931,6 @@ "Sa": "Sa", "Su": "So", "Pick a date": "Datum wählen", - "would make this album include itself through other smart albums": "würde dieses Album über andere Smart-Alben sich selbst enthalten lassen" + "would make this album include itself through other smart albums": "würde dieses Album über andere Smart-Alben sich selbst enthalten lassen", + "Smart Albums": "Smart-Alben" } diff --git a/crates/ui-egui/locales/es.json b/crates/ui-egui/locales/es.json index 328f92b80..faf2a4e21 100644 --- a/crates/ui-egui/locales/es.json +++ b/crates/ui-egui/locales/es.json @@ -1776,5 +1776,6 @@ "Sa": "Sá", "Su": "Do", "Pick a date": "Elegir una fecha", - "would make this album include itself through other smart albums": "haría que este álbum se incluyera a sí mismo a través de otros álbumes inteligentes" + "would make this album include itself through other smart albums": "haría que este álbum se incluyera a sí mismo a través de otros álbumes inteligentes", + "Smart Albums": "Álbumes inteligentes" } diff --git a/crates/ui-egui/locales/ja.json b/crates/ui-egui/locales/ja.json index 192c4eaf1..11882d7f5 100644 --- a/crates/ui-egui/locales/ja.json +++ b/crates/ui-egui/locales/ja.json @@ -1780,5 +1780,6 @@ "Sa": "土", "Su": "日", "Pick a date": "日付を選択", - "would make this album include itself through other smart albums": "ほかのスマートアルバムを通じて、このアルバム自身を含むことになります" + "would make this album include itself through other smart albums": "ほかのスマートアルバムを通じて、このアルバム自身を含むことになります", + "Smart Albums": "スマートアルバム" } diff --git a/crates/ui-egui/locales/pt-br.json b/crates/ui-egui/locales/pt-br.json index 936fa411a..cefdaeb23 100644 --- a/crates/ui-egui/locales/pt-br.json +++ b/crates/ui-egui/locales/pt-br.json @@ -1776,5 +1776,6 @@ "Sa": "Sáb", "Su": "Dom", "Pick a date": "Escolher uma data", - "would make this album include itself through other smart albums": "faria este álbum incluir a si mesmo por meio de outros álbuns inteligentes" + "would make this album include itself through other smart albums": "faria este álbum incluir a si mesmo por meio de outros álbuns inteligentes", + "Smart Albums": "Álbuns inteligentes" } diff --git a/crates/ui-egui/locales/ru.json b/crates/ui-egui/locales/ru.json index 252141a83..40beb384a 100644 --- a/crates/ui-egui/locales/ru.json +++ b/crates/ui-egui/locales/ru.json @@ -1468,5 +1468,6 @@ "Sa": "Сб", "Su": "Вс", "Pick a date": "Выбрать дату", - "would make this album include itself through other smart albums": "альбом включал бы сам себя через другие смарт-альбомы" + "would make this album include itself through other smart albums": "альбом включал бы сам себя через другие смарт-альбомы", + "Smart Albums": "Смарт-альбомы" } diff --git a/crates/ui-egui/locales/zh-hans.json b/crates/ui-egui/locales/zh-hans.json index ad7aae9e2..c4dcefbdd 100644 --- a/crates/ui-egui/locales/zh-hans.json +++ b/crates/ui-egui/locales/zh-hans.json @@ -1776,5 +1776,6 @@ "Sa": "六", "Su": "日", "Pick a date": "选择日期", - "would make this album include itself through other smart albums": "会通过其他智能相册让此相册包含自身" + "would make this album include itself through other smart albums": "会通过其他智能相册让此相册包含自身", + "Smart Albums": "智能相册" } diff --git a/crates/ui-egui/locales/zh-hant.json b/crates/ui-egui/locales/zh-hant.json index 2574c2497..6918e393f 100644 --- a/crates/ui-egui/locales/zh-hant.json +++ b/crates/ui-egui/locales/zh-hant.json @@ -1776,5 +1776,6 @@ "Sa": "六", "Su": "日", "Pick a date": "選擇日期", - "would make this album include itself through other smart albums": "會透過其他智慧型相簿讓此相簿包含自身" + "would make this album include itself through other smart albums": "會透過其他智慧型相簿讓此相簿包含自身", + "Smart Albums": "智慧型相簿" } diff --git a/crates/ui-egui/src/i18n.rs b/crates/ui-egui/src/i18n.rs index 7ba6a47e2..efeaadb88 100644 --- a/crates/ui-egui/src/i18n.rs +++ b/crates/ui-egui/src/i18n.rs @@ -761,8 +761,11 @@ mod tests { assert_eq!(problem_text_in(Locale::Ja, &p[0]), "タイトル：検索する語句が必要です（または「が空」を使用）"); let g = problems(serde_json::json!([{"group": {"rules": []}}])); assert_eq!(problem_text_in(Locale::En, &g[0]), "This group is empty: add a rule or remove it."); - // the issues, and the operators their hints name, in every language + // the issues, the operators their hints name and the album picker's words, in every language for language in Locale::ALL.iter().filter(|l| **l != Locale::En) { + for word in ["Choose an album…", "No albums yet", "Smart Albums", "Fix the marked rules to save this album."] { + assert!(language.catalog().contains_key(word), "{} lacks {word:?}", language.code()); + } for issue in Issue::ALL { assert!(language.catalog().contains_key(issue.text()), "{} lacks {:?}", language.code(), issue.text()); } diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index 91d1af6ef..22768a03e 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -63,6 +63,11 @@ pub const WHATS_NEW: &str = include_str!("../../../../docs/whats-new.md"); pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { let Some(mut dlg) = app.ui.dialog.clone() else { return }; let at_start = dlg.clone(); + // Esc with a popup open in the dialog (a dropdown, a date picker's calendar) closes the popup + // only: claimed before the dialog is drawn, since the popup is gone once it has handled the key + if ctx.input(|i| i.key_pressed(egui::Key::Escape)) && egui::Popup::is_any_open(ctx) { + crate::widgets::take_escape(ctx); + } let t = Tokens::get(ctx); let screen = ctx.content_rect(); // The backdrop is an area below the dialog window (a bare `Middle` layer painter would be @@ -244,27 +249,15 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { 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), + problems: rules.check_for(&app.session.catalog, id.map(lightcraft_catalog::AlbumId)), today: (app.session.clock)(), - albums: { - let cat = &app.session.catalog; - let mut albums: Vec<(u64, String)> = cat - .albums() - .filter(|a| !a.is_smart() && !a.folder) - .map(|a| { - // "Folder / Album", so albums of the same name in two folders differ - let folder = a.parent.and_then(|f| cat.album(f)).map(|f| f.name.as_str()); - (a.id.0, folder.map_or_else(|| a.name.clone(), |f| format!("{f} / {}", a.name))) - }) - .collect(); - albums.sort_by_key(|a| a.1.to_lowercase()); - albums - }, + albums: album_choices(&app.session.catalog, *id, false), + smart_albums: album_choices(&app.session.catalog, *id, true), }; egui::ScrollArea::vertical().max_height(360.0).auto_shrink([false, true]).show(ui, |ui| { crate::panels::rules_editor::edit(ui, rules, "rules", 0, &[], &env); }); - let problems = rules.check(&app.session.catalog); + let problems = rules.check_for(&app.session.catalog, id.map(lightcraft_catalog::AlbumId)); // 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() }; @@ -856,7 +849,7 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { 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()); + && !matches!(&dlg, Dialog::SmartRules { id, rules, .. } if !rules.check_for(&app.session.catalog, id.map(lightcraft_catalog::AlbumId)).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); @@ -1040,6 +1033,25 @@ fn created_in(app: &mut LightcraftApp, command: &str, params: serde_json::Value) Ok(r) } +/// The albums (`smart` false) or smart albums an Album rule in smart album `editing` can test, as +/// (id, "Folder / Name"), sorted by name: no folders, and no smart album that is `editing` itself +/// or tests it (that would loop back). +fn album_choices(cat: &lightcraft_catalog::Catalog, editing: Option, smart: bool) -> Vec<(u64, String)> { + let editing = editing.map(lightcraft_catalog::AlbumId); + let mut albums: Vec<(u64, String)> = cat + .albums() + .filter(|a| !a.folder && a.is_smart() == smart) + .filter(|a| editing.is_none_or(|e| a.id != e && !cat.album_reaches(a.id, e))) + .map(|a| { + // "Folder / Album", so albums of the same name in two folders differ + let folder = a.parent.and_then(|f| cat.album(f)).map(|f| f.name.as_str()); + (a.id.0, folder.map_or_else(|| a.name.clone(), |f| format!("{f} / {}", a.name))) + }) + .collect(); + albums.sort_by_key(|a| a.1.to_lowercase()); + albums +} + pub fn confirm_dialog(app: &mut LightcraftApp, dlg: &Dialog) -> Result { match dlg { Dialog::SamModel { then, .. } => { @@ -1100,7 +1112,7 @@ 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() { + if let Some(problem) = rules.check_for(&app.session.catalog, id.map(lightcraft_catalog::AlbumId)).first() { return Err(problem.to_string()); } let name = if name.trim().is_empty() { "Smart Album".to_string() } else { name.trim().to_string() }; diff --git a/crates/ui-egui/src/panels/rules_editor.rs b/crates/ui-egui/src/panels/rules_editor.rs index 7d7b2c4b7..571be6011 100644 --- a/crates/ui-egui/src/panels/rules_editor.rs +++ b/crates/ui-egui/src/panels/rules_editor.rs @@ -113,6 +113,9 @@ pub struct Env { /// The albums an Album rule can test: (id, name shown, with its folder when it is in one), /// sorted by that name. pub albums: Vec<(u64, String)>, + /// The smart albums it can test, the same way: not the album being edited, nor one that tests + /// it (that would loop back). + pub smart_albums: Vec<(u64, String)>, } /// The album of `albums` an Album rule's value names, read as the catalog reads it (`3`, `"3"`). @@ -121,20 +124,30 @@ fn chosen_album<'a>(v: &Value, albums: &'a [(u64, String)]) -> Option<&'a (u64, albums.iter().find(|a| a.0 == id.0) } -/// An Album rule's value: one of the plain albums, picked by name. +/// An Album rule's value: an album or a smart album, picked by name. fn album_value(ui: &mut egui::Ui, v: &mut Value, salt: &str, env: &Env) { - let chosen = chosen_album(v, &env.albums); + let chosen = chosen_album(v, &env.albums).or_else(|| chosen_album(v, &env.smart_albums)); let cur = chosen.map(|a| a.0); let text = chosen.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); + let mut item = |ui: &mut egui::Ui, id: u64, name: &str| { + let r = ui.selectable_label(cur == Some(id), name); register(ui.ctx(), format!("ruleAlbumItem:{id}:{salt}"), r.rect); if r.clicked() { *v = json!(id); } + }; + for (id, name) in &env.albums { + item(ui, *id, name); } - if env.albums.is_empty() { + if !env.smart_albums.is_empty() { + ui.separator(); + ui.label(RichText::new(crate::i18n::tr("Smart Albums")).weak().small()); + for (id, name) in &env.smart_albums { + item(ui, *id, name); + } + } + if env.albums.is_empty() && env.smart_albums.is_empty() { ui.label(crate::i18n::tr("No albums yet")); } }); diff --git a/crates/ui-egui/src/tests_date_picker.rs b/crates/ui-egui/src/tests_date_picker.rs index ba5e28113..0ea04f0ce 100644 --- a/crates/ui-egui/src/tests_date_picker.rs +++ b/crates/ui-egui/src/tests_date_picker.rs @@ -101,3 +101,16 @@ fn escape_closes_only_the_calendar() { assert!(!has(&h, "datePickerCell:2026-01-20:rules-0"), "the calendar closed"); assert!(h.app.ui.dialog.is_some(), "the editor stays open"); } + +/// Esc with a dropdown open (here the field menu) closes the dropdown, not the editor. +#[test] +fn escape_closes_only_a_dropdown() { + let mut h = editor(json!([{"field": "rating", "op": "gte", "value": 3}])); + click(&mut h, "ruleField:rules-0"); + assert!(has(&h, "ruleFieldItem:rating:rules-0")); + let r = h.request("ui.key", json!({"key": "Escape"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert!(!has(&h, "ruleFieldItem:rating:rules-0"), "the menu closed"); + assert!(h.app.ui.dialog.is_some(), "the editor stays open"); +} diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index b68670177..5cf53d1c0 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -381,7 +381,7 @@ fn smart_album_editor_marks_bad_rules_and_blocks_ok() { 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. +/// An Album rule picks its album from a list of the albums and smart albums, not by typing an id. #[test] fn smart_album_editor_picks_an_album_from_a_list() { let mut h = detail("panel.edit"); @@ -397,7 +397,7 @@ fn smart_album_editor_picks_an_album_from_a_list() { 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"); + assert!(has(&h, format!("ruleAlbumItem:{smart}:rules-0")), "smart albums too"); let r = h.request("ui.clickWidget", json!({"id": format!("ruleAlbumItem:{trip}:rules-0")}), T); assert_eq!(r["ok"], true, "{r}"); h.settle(SETTLE); @@ -480,6 +480,35 @@ fn sidebar_marks_smart_albums_with_problems() { assert!(marked(&h), "its album is gone"); } +/// Editing "Excluded Photos" while "Travel" tests it: the list offers neither the album itself nor +/// Travel (testing it would loop back), and a rule set to Travel by other means is marked. +#[test] +fn smart_album_editor_keeps_albums_from_including_themselves() { + let mut h = detail("panel.edit"); + let trip = exec(&mut h, "album.create", json!({"name": "Trip", "addSelected": false}))["id"].as_u64().unwrap(); + let excluded = exec(&mut h, "album.createSmart", json!({"name": "Excluded Photos", "rules": {"rating": 1}}))["id"].as_u64().unwrap(); + let rules = json!({"ruleSet": {"rules": [{"field": "album", "op": "isNot", "value": excluded}]}}); + let travel = exec(&mut h, "album.createSmart", json!({"name": "Travel", "rules": rules}))["id"].as_u64().unwrap(); + exec(&mut h, "dialog.smartAlbum", json!({"id": excluded})); + let set = |h: &mut Headless, value: serde_json::Value| { + let Some(crate::state::Dialog::SmartRules { rules, .. }) = &mut h.app.ui.dialog else { panic!("no rule editor") }; + rules.rules = vec![serde_json::from_value(json!({"field": "album", "op": "is", "value": value})).unwrap()]; + h.settle(SETTLE); + }; + set(&mut h, json!(null)); + 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"))); + assert!(!has(&h, format!("ruleAlbumItem:{excluded}:rules-0")), "not itself"); + assert!(!has(&h, format!("ruleAlbumItem:{travel}:rules-0")), "not an album that tests it"); + let _ = h.request("ui.key", json!({"key": "Escape"}), T); + h.settle(SETTLE); + set(&mut h, json!(travel)); + assert!(has(&h, "ruleProblem:rules-0".to_string()), "a loop is marked"); +} + #[test] fn g_toggles_grids_and_shift_g_starts_guided_upright() { let mut h = detail("panel.edit");