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 <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 4e3b15ff6f
commit 5c949e01e8
12 changed files with 110 additions and 33 deletions
+2 -1
View File
@@ -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"
}
+2 -1
View File
@@ -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"
}
+2 -1
View File
@@ -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": "スマートアルバム"
}
+2 -1
View File
@@ -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"
}
+2 -1
View File
@@ -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": "Смарт-альбомы"
}
+2 -1
View File
@@ -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": "智能相册"
}
+2 -1
View File
@@ -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": "智慧型相簿"
}
+4 -1
View File
@@ -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());
}
+30 -18
View File
@@ -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<u64>, 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<serde_json::Value, String> {
match dlg {
Dialog::SamModel { then, .. } => {
@@ -1100,7 +1112,7 @@ pub fn confirm_dialog(app: &mut LightcraftApp, dlg: &Dialog) -> Result<serde_jso
}
Dialog::SmartRules { id, name, rules, parent } => {
// 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() };
+18 -5
View File
@@ -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"));
}
});
+13
View File
@@ -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");
}
+31 -2
View File
@@ -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");