From 31abbcee4f2179ad94ab3042e908b9e8384f298c Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 08:58:11 -0400 Subject: [PATCH] LRC-LIB-SMARTCOLL: rename and rules of a smart album in one step The rule editor renamed an album (album.rename) and then set its rules (album.setRules) as two commits. If the second failed (refused rules before the dialog checked them, or a save error), the album kept the new name without the rules, and undo needed two steps. album.setRules takes an optional `name` (blank keeps the album's) and commits the rename and the rules as one Op::Batch: both or neither, one undo step. The dialog edits through it. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/library.rs | 10 ++++++++-- crates/engine/src/tests_library.rs | 23 +++++++++++++++++++++++ crates/ui-egui/src/panels/dialogs.rs | 8 ++------ crates/ui-egui/src/tests_masking.rs | 14 ++++++++++++++ docs/whats-new.md | 2 ++ 5 files changed, 49 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 775106802..a07eb8b48 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -854,7 +854,7 @@ pub fn specs() -> Vec { "Edit Smart Album", [], None, - "{id, rules?: partial Filter merged onto the current rules (null clears a field), replace?: bool, fromView?: bool (use the current view)}", + "{id, rules?: partial Filter merged onto the current rules (null clears a field), replace?: bool, fromView?: bool (use the current view), name?: also rename it (blank keeps the name) — one undo step, both or neither}", always, |s, p| { let id = album_param(p, "id", "album.setRules")?; @@ -873,7 +873,13 @@ pub fn specs() -> Vec { } r }; - s.commit("Edit Smart Album", Op::SetAlbumRules { id, rules: Box::new(rules) })?; + let set = Op::SetAlbumRules { id, rules: Box::new(rules) }; + // a new name goes with the rules: one step, applied (and undone) together + let rename = str_param(p, "name").map(str::trim).filter(|n| !n.is_empty() && s.catalog.album(id).is_some_and(|a| a.name != *n)); + match rename { + Some(name) => s.commit("Edit Smart Album", Op::Batch { ops: vec![Op::RenameAlbum { id, name: name.to_string() }, set] })?, + None => s.commit("Edit Smart Album", set)?, + }; Ok(json!({"count": s.catalog.album_count(id)})) } ), diff --git a/crates/engine/src/tests_library.rs b/crates/engine/src/tests_library.rs index 9b680c0fc..45b400a5a 100644 --- a/crates/engine/src/tests_library.rs +++ b/crates/engine/src/tests_library.rs @@ -532,3 +532,26 @@ fn old_album_operator_rules_stay_editable() { assert!(saved.contains(r#""op":"isNot""#), "{saved}"); s.execute("library.filter", &serde_json::json!({"ruleSet": {"rules": [{"field": "album", "op": "lte", "value": trip}]}})).unwrap(); } + +/// Renaming and changing the rules is one step: both or neither, undone together. +#[test] +fn set_rules_with_a_name_is_one_step() { + let mut s = crate::Session::with_demo(); + let id = s.execute("album.createSmart", &serde_json::json!({"name": "Old", "rules": {"rating": 3}})).unwrap()["id"].as_u64().unwrap(); + let album = |s: &crate::Session| s.catalog.album(lightcraft_catalog::AlbumId(id)).unwrap().clone(); + let good = serde_json::json!({"ruleSet": {"rules": [{"field": "rating", "op": "gte", "value": 4}]}}); + let bad = serde_json::json!({"ruleSet": {"rules": [{"field": "captureDate", "op": "is", "value": "banana"}]}}); + // refused rules: the name stays too + assert!(s.execute("album.setRules", &serde_json::json!({"id": id, "name": "New", "replace": true, "rules": bad})).is_err()); + assert_eq!(album(&s).name, "Old"); + let undo = s.undo.len(); + s.execute("album.setRules", &serde_json::json!({"id": id, "name": "New", "replace": true, "rules": good})).unwrap(); + assert_eq!((album(&s).name.as_str(), s.undo.len()), ("New", undo + 1), "one undo step"); + assert!(album(&s).smart.unwrap().rule_set.is_some()); + s.execute("edit.undo", &serde_json::json!({})).unwrap(); + assert_eq!(album(&s).name, "Old"); + assert!(album(&s).smart.unwrap().rule_set.is_none(), "undone together"); + // a blank name keeps the album's + s.execute("album.setRules", &serde_json::json!({"id": id, "name": " ", "rules": {"rating": 5}})).unwrap(); + assert_eq!(album(&s).name, "Old"); +} diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index d29fd84fb..954cd9f59 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -1103,12 +1103,8 @@ pub fn confirm_dialog(app: &mut LightcraftApp, dlg: &Dialog) -> Result { - if app.session.catalog.album(lightcraft_catalog::AlbumId(*id)).is_some_and(|a| a.name != name) { - app.run("album.rename", json!({"id": id, "name": name}))?; - } - app.run("album.setRules", json!({"id": id, "replace": true, "rules": {"ruleSet": rules}})) - } + // the name and the rules together: one undo step, both or neither + Some(id) => app.run("album.setRules", json!({"id": id, "name": name, "replace": true, "rules": {"ruleSet": rules}})), None => created_in(app, "album.createSmart", json!({"name": name, "rules": {"ruleSet": rules}, "parent": parent})), } } diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index ca0537070..0a1d37b4d 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -446,6 +446,20 @@ fn smart_album_edit_with_bad_rules_keeps_the_name() { let _ = h.request("ui.dialog.confirm", json!({}), T); let album = h.app.session.catalog.album(lightcraft_catalog::AlbumId(id)).expect("album"); assert_eq!(album.name, "Keep", "neither OK nor confirm renamed it"); + // fixed: OK renames it and sets the rules in one undo step + let Some(crate::state::Dialog::SmartRules { rules, .. }) = &mut h.app.ui.dialog else { panic!("the editor stays open") }; + *rules = serde_json::from_value(json!({"rules": [{"field": "captureDate", "op": "is", "value": "2026"}]})).unwrap(); + h.settle(SETTLE); + let undo = h.app.session.undo.len(); + let r = h.request("ui.clickWidget", json!({"id": "button:dialogOk"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let album = h.app.session.catalog.album(lightcraft_catalog::AlbumId(id)).expect("album"); + assert_eq!((album.name.as_str(), h.app.session.undo.len()), ("Renamed", undo + 1)); + exec(&mut h, "edit.undo", json!({})); + let album = h.app.session.catalog.album(lightcraft_catalog::AlbumId(id)).expect("album"); + assert_eq!(album.name, "Keep", "one undo takes back the name with the rules"); + assert!(album.smart.as_ref().is_some_and(|f| f.rule_set.is_none())); } #[test] diff --git a/docs/whats-new.md b/docs/whats-new.md index 134de17b3..5b97d1c85 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -17,6 +17,8 @@ way round, "in the last 0 days", a colour label that doesn't exist, "contains" with nothing to look for, an album that is gone or smart, or an empty group is reported with its rule ("rule 2.1: rating is 0–5, not 9") instead of quietly matching nothing or everything. New date rules start at the current year. +- Editing a smart album's name and rules is one step: both or neither, and one undo takes both back + (`album.setRules` takes `name`). - The rule editor marks each rule that needs fixing right under it and keeps OK disabled until they are fixed. An Album rule picks its album from a list (is / isn't) instead of taking an id. - Yes/no fields (Has Edits, Cropped, Has GPS…) read Yes / No instead of true / false. A rule sent as `"false"` now