mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-26 02:59:17 -06:00
LRC-LIB-SMARTCOLL: rule editor never rewrites a rule by drawing it
From review: the number spinner was fixed, but the "in the last" spinner
still clamped a stored N on first draw ("0 days" became 1, hiding the
problem and saving a different rule; 20,000 became 10,000). Values
that weren't an {n, unit} object or a two-item "between" were replaced
by the defaults as soon as they were drawn.
- In the last: shown as stored ({n, unit}, any case, or a plain number
of days) and written only when the N or the unit is changed, with
clamp_existing_to_range(false).
- Between: anything but a pair is shown as the default pair but kept
until one bound is edited, so the check marks it. A slice pattern
replaces the [0] / [1] indexing.
- Album picker: reads the id as the catalog does; names albums with
their folder ("Trips / Rome") and sorts them by name.
- Opening an album's rules applies RuleSet::upgrade.
- New tests: drawing leaves five unreadable values as stored and marks
the three that can't match, and an edit with bad rules doesn't rename
the album (neither OK nor confirm). The parity note no longer
overclaims.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Nathaniel Ledford
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
850311a884
commit
920658c76d
@@ -873,7 +873,16 @@ pub fn run_ui_command(app: &mut LightcraftApp, id: &str, p: &Value) -> Option<Re
|
||||
Some(a) => Dialog::SmartRules {
|
||||
id: Some(a.id.0),
|
||||
name: a.name.clone(),
|
||||
rules: a.smart.as_ref().and_then(|f| f.rule_set.clone()).unwrap_or_default(),
|
||||
// older rules read as they always matched (RuleSet::upgrade)
|
||||
rules: a
|
||||
.smart
|
||||
.as_ref()
|
||||
.and_then(|f| f.rule_set.clone())
|
||||
.map(|mut r| {
|
||||
r.upgrade();
|
||||
r
|
||||
})
|
||||
.unwrap_or_default(),
|
||||
parent: None,
|
||||
},
|
||||
None => Dialog::SmartRules {
|
||||
|
||||
@@ -245,7 +245,20 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) {
|
||||
// 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(),
|
||||
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
|
||||
},
|
||||
};
|
||||
egui::ScrollArea::vertical().max_height(360.0).auto_shrink([false, true]).show(ui, |ui| {
|
||||
crate::panels::rules_editor::edit(ui, rules, "rules", 0, &[], &env);
|
||||
|
||||
@@ -94,14 +94,22 @@ fn number_value(ui: &mut egui::Ui, v: &mut Value, field: &str) {
|
||||
#[derive(Default)]
|
||||
pub struct Env {
|
||||
pub problems: Vec<Problem>,
|
||||
/// The plain albums: (id, name), in the sidebar's order.
|
||||
/// 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 album of `albums` an Album rule's value names, read as the catalog reads it (`3`, `"3"`).
|
||||
fn chosen_album<'a>(v: &Value, albums: &'a [(u64, String)]) -> Option<&'a (u64, String)> {
|
||||
let id = lightcraft_catalog::rules::album_rule_id(v)?;
|
||||
albums.iter().find(|a| a.0 == id.0)
|
||||
}
|
||||
|
||||
/// 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 chosen = chosen_album(v, &env.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);
|
||||
@@ -123,39 +131,50 @@ fn value_editor(ui: &mut egui::Ui, field: &str, op: &str, v: &mut Value, salt: &
|
||||
(_, "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);
|
||||
}
|
||||
let mut n = v["n"].as_f64().unwrap_or(30.0);
|
||||
if ui.add(egui::DragValue::new(&mut n).range(1.0..=10_000.0).speed(0.2).fixed_decimals(0)).changed() {
|
||||
v["n"] = json!(n.round());
|
||||
}
|
||||
let unit = v["unit"].as_str().unwrap_or("days").to_string();
|
||||
// shown as stored ({n, unit}, or a plain number of days) and only written when changed:
|
||||
// drawing never turns "0 days" into 1 or 20,000 into the spinner's 10,000
|
||||
let (stored_n, stored_unit) = match &*v {
|
||||
Value::Object(o) => (o.get("n").and_then(Value::as_f64), o.get("unit").and_then(Value::as_str).unwrap_or("days").to_string()),
|
||||
Value::Number(n) => (n.as_f64(), "days".to_string()),
|
||||
_ => (None, "days".to_string()),
|
||||
};
|
||||
let mut n = stored_n.unwrap_or(30.0);
|
||||
let mut unit = stored_unit.to_lowercase();
|
||||
let mut changed =
|
||||
ui.add(egui::DragValue::new(&mut n).range(1.0..=10_000.0).clamp_existing_to_range(false).speed(0.2).fixed_decimals(0)).changed();
|
||||
egui::ComboBox::from_id_salt(format!("{salt}-unit")).width(70.0).selected_text(crate::i18n::tr(&unit)).show_ui(ui, |ui| {
|
||||
for u in ["hours", "days", "weeks", "months", "years"] {
|
||||
if ui.selectable_label(unit == u, crate::i18n::tr(u)).clicked() {
|
||||
v["unit"] = json!(u);
|
||||
if ui.selectable_label(unit.trim_end_matches('s') == u.trim_end_matches('s'), crate::i18n::tr(u)).clicked() {
|
||||
unit = u.to_string();
|
||||
changed = true;
|
||||
}
|
||||
}
|
||||
});
|
||||
if changed {
|
||||
*v = json!({"n": n.round(), "unit": unit});
|
||||
}
|
||||
}
|
||||
(Some(k), "between") => {
|
||||
if v.as_array().is_none_or(|a| a.len() != 2) {
|
||||
*v = default_value(field, op);
|
||||
}
|
||||
let Value::Array(a) = v else { return };
|
||||
// a value that isn't a pair is shown as the default pair but kept until one is edited,
|
||||
// so the check can mark it
|
||||
let mut pair = v.as_array().filter(|a| a.len() == 2).cloned().map_or_else(|| default_value(field, op), Value::Array);
|
||||
let before = pair.clone();
|
||||
let Some([lo, hi]) = pair.as_array_mut().map(Vec::as_mut_slice) else { return };
|
||||
if field == "shutterSpeed" {
|
||||
text_value(ui, &mut a[0], 60.0, "1/1000", &format!("{salt}-a"));
|
||||
text_value(ui, lo, 60.0, "1/1000", &format!("{salt}-a"));
|
||||
ui.label(crate::i18n::tr("and"));
|
||||
text_value(ui, &mut a[1], 60.0, "1/125", &format!("{salt}-b"));
|
||||
text_value(ui, hi, 60.0, "1/125", &format!("{salt}-b"));
|
||||
} else if k == Kind::Number {
|
||||
number_value(ui, &mut a[0], field);
|
||||
number_value(ui, lo, field);
|
||||
ui.label(crate::i18n::tr("and"));
|
||||
number_value(ui, &mut a[1], field);
|
||||
number_value(ui, hi, field);
|
||||
} else {
|
||||
text_value(ui, &mut a[0], 86.0, "2026-01-01", &format!("{salt}-a"));
|
||||
text_value(ui, lo, 86.0, "2026-01-01", &format!("{salt}-a"));
|
||||
ui.label(crate::i18n::tr("and"));
|
||||
text_value(ui, &mut a[1], 86.0, "2026-12", &format!("{salt}-b"));
|
||||
text_value(ui, hi, 86.0, "2026-12", &format!("{salt}-b"));
|
||||
}
|
||||
if pair != before {
|
||||
*v = pair;
|
||||
}
|
||||
}
|
||||
// camera notation: 1/250, 0.5, 2"
|
||||
@@ -383,6 +402,11 @@ mod tests {
|
||||
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);
|
||||
// the picker shows the album a rule names, however the id is written
|
||||
let albums = vec![(3, "Trip".to_string())];
|
||||
assert_eq!(chosen_album(&json!(3), &albums).map(|a| a.1.as_str()), Some("Trip"));
|
||||
assert_eq!(chosen_album(&json!("3"), &albums).map(|a| a.1.as_str()), Some("Trip"));
|
||||
assert_eq!(chosen_album(&json!(4), &albums), None);
|
||||
assert_eq!(default_value("shutterSpeed", "gte"), json!("1/250"));
|
||||
assert_eq!(default_value("shutterSpeed", "between"), json!(["1/1000", "1/125"]));
|
||||
}
|
||||
|
||||
@@ -407,6 +407,47 @@ fn smart_album_editor_picks_an_album_from_a_list() {
|
||||
assert!(!h.app.widgets.iter().any(|(w, _)| w == "ruleProblem:rules-0"));
|
||||
}
|
||||
|
||||
/// Drawing the editor never changes a rule: values it can't show as they are (in the last 0 days,
|
||||
/// 20,000 days, a plain number of days, a "between" that isn't a pair) stay as stored, and the
|
||||
/// ones the check refuses are marked.
|
||||
#[test]
|
||||
fn smart_album_editor_never_rewrites_what_it_draws() {
|
||||
let mut h = detail("panel.edit");
|
||||
exec(&mut h, "dialog.smartAlbum", json!({"name": "Odd"}));
|
||||
let rules = json!({"rules": [
|
||||
{"field": "captureDate", "op": "inLast", "value": {"n": 0, "unit": "days"}},
|
||||
{"field": "captureDate", "op": "inLast", "value": {"n": 20000, "unit": "Days"}},
|
||||
{"field": "captureDate", "op": "notInLast", "value": 7},
|
||||
{"field": "captureDate", "op": "between", "value": "2026"},
|
||||
{"field": "iso", "op": "between", "value": [100]}
|
||||
]});
|
||||
let Some(crate::state::Dialog::SmartRules { rules: r, .. }) = &mut h.app.ui.dialog else { panic!("no rule editor") };
|
||||
*r = serde_json::from_value(rules.clone()).unwrap();
|
||||
h.settle(SETTLE);
|
||||
let Some(crate::state::Dialog::SmartRules { rules: r, .. }) = &h.app.ui.dialog else { panic!("no rule editor") };
|
||||
assert_eq!(serde_json::to_value(r).unwrap()["rules"], rules["rules"], "unchanged by drawing");
|
||||
let marked: Vec<&str> = h.app.widgets.iter().filter_map(|(w, _)| w.strip_prefix("ruleProblem:")).collect();
|
||||
assert_eq!(marked, ["rules-0", "rules-3", "rules-4"], "0 days and the half-made betweens");
|
||||
}
|
||||
|
||||
/// Editing an album with rules that need fixing changes nothing, not even its name.
|
||||
#[test]
|
||||
fn smart_album_edit_with_bad_rules_keeps_the_name() {
|
||||
let mut h = detail("panel.edit");
|
||||
let id = exec(&mut h, "album.createSmart", json!({"name": "Keep", "rules": {"rating": 3}}))["id"].as_u64().unwrap();
|
||||
exec(&mut h, "dialog.smartAlbum", json!({"id": id}));
|
||||
let Some(crate::state::Dialog::SmartRules { name, rules, .. }) = &mut h.app.ui.dialog else { panic!("no rule editor") };
|
||||
*name = "Renamed".into();
|
||||
*rules = serde_json::from_value(json!({"rules": [{"field": "captureDate", "op": "is", "value": "banana"}]})).unwrap();
|
||||
h.settle(SETTLE);
|
||||
let r = h.request("ui.clickWidget", json!({"id": "button:dialogOk"}), T);
|
||||
assert_eq!(r["ok"], true, "{r}");
|
||||
h.settle(SETTLE);
|
||||
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");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn g_toggles_grids_and_shift_g_starts_guided_upright() {
|
||||
let mut h = detail("panel.edit");
|
||||
|
||||
+1
-1
@@ -744,7 +744,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06-
|
||||
| LRC-LIB-FOLDERS | Disk folder tree | P1 | ✅ | `cmd:library.browse`, `cmd:folder.rename`, `cmd:folder.move`, `crates/ui-egui/src/panels/left.rs` (`local_section`, `folder_tree`) | standard places + the browsed folder + Browse Folder…, each an expandable tree of subfolders, breadcrumb navigation; context menu: Rename Folder…, Move Folder To…, Show in Finder (on disk, photos relinked; one undo step each: undo / redo move the folder back and forth, refused when the destination is taken) |
|
||||
| LRC-LIB-LIBFOLDERS | Library folders | P1 | ✅ | `cmd:library.folders`, `cmd:library.source` (`libraryFolder`), `cmd:library.filter` (`libraryFolder`), `cmd:library.removeFolder`, `cmd:folder.rename`, `cmd:folder.move`, `crates/catalog/src/folders.rs`, `crates/ui-egui/src/panels/left.rs` (`folders_section`, `folder_menu_for_library`, `row_menu`), `crates/ui-egui/src/tests_panels.rs` | sidebar Folders: the folders the library's photos were imported from, with photo counts (subfolders included), as another way to explore the library next to By Date and Keywords; Local stays the way to browse any folder on disk. Each disk is a row of its own and starts open: a drive letter, a Windows share (`\\server\share`, `\\?\UNC\…`, `\\wsl$\…`), a macOS share or disk (`/Volumes/tokyo`), a Linux mount (`/media`, `/run/media`, `/mnt`, gvfs `smb-share:…`, `/net/<host>`), NFS (`/Network/Servers/<host>`), else “This Computer” — told apart from the stored paths alone, so an offline share lists like a mounted one and the sidebar never waits on a network. Only folders that hold imported photos are listed (browsed, deleted and demo photos don't count); every folder is a row of its own (`Users` → `me` → `Pictures`), and a folder that holds nothing itself and leads to one folder starts open; every level indents one step more than its parent however deep it goes: the whole sidebar (Albums, Local, By Date, Folders, Keywords) scrolls sideways when its widest row is wider than the panel (rows are measured one frame behind; Create Album and the counts stay at the visible edge); a name that does not fit ends in an ellipsis at the count (the full name on hover; a row asks for room for at most 140 pt of its name, so depth, not one long name, makes the sidebar scroll), as does the grid title; selection and hover bars end at the panel's edge; click makes the folder the source, like an album or a Local folder (its photos and those of the folders inside it fill the grid, titled “photos/travel”; the filter bar narrows it further; `library.source` `libraryFolder` + `path`; kept with the library's view), and its row is always opened into view; the shown folder follows a rename or move and their undo (engine-tested; the sidebar test covers rename), is dropped back to All Photos when it is removed or no longer holds a photo, and a smart album made from the view keeps it, also when its rules are edited (the editor has no folder field). Only a folder that shows exactly its own photos can be shown (not the startup disk, not `/Volumes`-like folders above disks, not an empty one). A row that would cover other disks' photos too (the startup disk, `/Volumes`, `/mnt`…) only opens. Right-click (row or triangle): Rename Folder…, Move Folder To…, Show in Finder (on disk, photos relinked, as in Local) and Remove from Library… (asks; moves the folder's photos to Recently Deleted, one undo step, no file touched); a disk row offers Remove Disk from Library… (the command needs `disk: true` for a whole disk or a folder spanning several). Limits: names are compared as written (macOS `/Volumes/tokyo` vs `/Volumes/TOKYO` are two disks; `/Volumes/Macintosh HD/…` and `/…` two rows); no disk capacity / online status; no “include subfolders” switch (a folder always includes the ones inside it); creating a folder and moving photos between folders are not here |
|
||||
| LRC-LIB-COLLECTIONS | Collections & sets | P1 | ✅ | `cmd:album.create`, `cmd:album.createSmart`, `cmd:album.toggleTarget`, `cmd:album.setTarget`, `cmd:album.clearQuick` | albums + folders (sets), smart albums (rule editor; rules incl. File Path — the whole string, either separator, case-insensitive), Quick Collection and target album (B in the grids adds / removes the selection; album menu ▸ Set as Target Album; the target is marked +) |
|
||||
| LRC-LIB-SMARTCOLL | Smart-collection rules | P1 | ✅ | `cmd:album.createSmart`, `cmd:album.setRules`, `cmd:album.ruleFields`, `cmd:dialog.smartAlbum`, `crates/catalog/src/rules.rs`, `crates/ui-egui/src/panels/rules_editor.rs` | rule editor: match all / any / none, nested groups (⌥+ or + Group), 47 fields. The field menu shows Rating, Pick Flag, Color Label and Any Searchable Text, then one submenu per group (`FIELD_GROUPS`; `album.ruleFields` reports each field's `group`): Source (album, virtual copy, copy name, in a stack), File (filename, extension, file path, type, format, video duration), Date (capture / import / edit), Keywords & People (keywords, keyword count, people on named faces, people count), Description (title, caption, alt text, creator, copyright, copyright status), Camera Info (camera, lens, focal length, aperture, shutter speed as 1/250, ISO), Location (any, city, state / province, country, GPS), Size (long / short edge, aspect ratio and megapixels, all of the photo as shown: oriented and cropped), Develop (has edits, cropped, treatment), Assisted Culling (focus, best of similar shots); text / number / date / in-the-last operators; live match count; also usable as a library filter (`ruleSet`). Missing vs Classic: develop preset, flash, exposure bias, camera serial, colour profile / bit depth, IPTC Job, smart-preview status, membership of smart collections. Choice values read as words in every language (Public Domain, Black & White; `album.ruleFields` gives `choiceLabels`), while rules store the ids; yes/no fields read Yes / No and take `true`/`false`, `"yes"`/`"no"`, `"true"`/`"false"` or 1/0. Every value is checked against its field (`RuleSet::check`): numbers (rating 0–5), shutter speeds, dates (`2026`, `2026-08`, `2026-08-14`, a full time; no 30 February), between = two bounds in order, in the last = more than 0 of a known unit, choices (ids, the flag's picked / rejected, a colour label's custom name), text with something to look for, an existing plain album, no empty groups; each problem names its rule (`rule 2.1: rating is 0–5, not 9`), and album.createSmart / setRules / library.filter refuse them. The editor marks each bad rule under its row (inside groups too), keeps OK disabled until they are fixed (and its confirm refuses them before renaming anything), picks an Album rule's album from a list of the plain albums (is / isn't), and never rewrites a stored value it can't show (a rating of 9 isn't clamped to 5). Not yet: problem messages are English and name fields by id |
|
||||
| LRC-LIB-SMARTCOLL | Smart-collection rules | P1 | ✅ | `cmd:album.createSmart`, `cmd:album.setRules`, `cmd:album.ruleFields`, `cmd:dialog.smartAlbum`, `crates/catalog/src/rules.rs`, `crates/ui-egui/src/panels/rules_editor.rs` | rule editor: match all / any / none, nested groups (⌥+ or + Group), 47 fields. The field menu shows Rating, Pick Flag, Color Label and Any Searchable Text, then one submenu per group (`FIELD_GROUPS`; `album.ruleFields` reports each field's `group`): Source (album, virtual copy, copy name, in a stack), File (filename, extension, file path, type, format, video duration), Date (capture / import / edit), Keywords & People (keywords, keyword count, people on named faces, people count), Description (title, caption, alt text, creator, copyright, copyright status), Camera Info (camera, lens, focal length, aperture, shutter speed as 1/250, ISO), Location (any, city, state / province, country, GPS), Size (long / short edge, aspect ratio and megapixels, all of the photo as shown: oriented and cropped), Develop (has edits, cropped, treatment), Assisted Culling (focus, best of similar shots); text / number / date / in-the-last operators; live match count; also usable as a library filter (`ruleSet`). Missing vs Classic: develop preset, flash, exposure bias, camera serial, colour profile / bit depth, IPTC Job, smart-preview status, membership of smart collections. Choice values read as words in every language (Public Domain, Black & White; `album.ruleFields` gives `choiceLabels`), while rules store the ids; yes/no fields read Yes / No and take `true`/`false`, `"yes"`/`"no"`, `"true"`/`"false"` or 1/0. Every value is checked against its field (`RuleSet::check`): numbers (rating 0–5), shutter speeds, dates (`2026`, `2026-08`, `2026-08-14`, a full time; no 30 February), between = two bounds in order, in the last = more than 0 of a known unit, choices (ids, the flag's picked / rejected, a colour label's custom name), text with something to look for, an existing plain album, no empty groups; each problem names its rule (`rule 2.1: rating is 0–5, not 9`), and album.createSmart / setRules / library.filter refuse them. The editor marks each bad rule under its row (inside groups too), keeps OK disabled until they are fixed (and its confirm refuses them before renaming anything), picks an Album rule's album from a list of the plain albums, named with their folder and sorted by name (is / isn't; a folder can't be tested), and drawing it never changes a rule: a rating of 9, "in the last 0 days" or 20,000 days, a "between" that isn't a pair all stay as stored (and are marked where they can't match) until edited. Album rules saved with an old operator (≥, between) are read as the "isn't" they always meant (`RuleSet::upgrade`). Not yet: problem messages are English and name fields by id |
|
||||
| LRC-LIB-PUBLISH | Publish services | P2 | ⬜ | | |
|
||||
| LRC-LIB-FILTERBAR | Library filter bar | P1 | ✅ | `cmd:view.filterBar`, `cmd:filter.applyPreset`, `cmd:library.filter` (`labels`, `ruleSet`), `crates/ui-egui/src/panels/filterbar.rs` | rating/flag/label (several labels at once = any of them)/kind/edited/camera/lens/keyword, clear, save as smart album, filter presets; the filter stays as you change albums (always locked, as in the desktop app); arbitrary conditions via the smart-album rule editor |
|
||||
| LRC-LIB-STACKS | Stacks (full) | P1 | ✅ | `crates/catalog/src/stacks.rs`, `cmd:stack.group`, `cmd:stack.split`, `cmd:stack.moveUp`, `cmd:stack.moveDown` | group / ungroup / toggle / set top / remove / auto by time / split / move up and down |
|
||||
|
||||
Reference in New Issue
Block a user