From 286171e2e598c7a9558fcbbac9e7f756ccfbf37a Mon Sep 17 00:00:00 2001 From: tylerphotos <93777540+tylerphotos@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:41:40 -0500 Subject: [PATCH 1/2] LR-IMP-ADD-DIALOG: preserve import destination album hierarchy Reuse the shared album picker in Import Photos, retaining None/New album, editable names, album-context preselection and existing import semantics. Keep long paths within the viewport and cover hierarchy, localization, keyboard choices, real imports, undo and layout regressions. Co-Authored-By: Codex --- crates/ui-egui/src/album_picker.rs | 54 +++- crates/ui-egui/src/import.rs | 57 ++-- crates/ui-egui/src/tests_album_picker.rs | 321 ++++++++++++++++++++++- docs/control-protocol.md | 14 + docs/gaps.md | 5 +- docs/parity-checklist.md | 5 +- 6 files changed, 419 insertions(+), 37 deletions(-) diff --git a/crates/ui-egui/src/album_picker.rs b/crates/ui-egui/src/album_picker.rs index 914827e5d..05e4d6dd3 100644 --- a/crates/ui-egui/src/album_picker.rs +++ b/crates/ui-egui/src/album_picker.rs @@ -172,13 +172,14 @@ pub struct AlbumPicker<'a> { value: &'a mut Option, entries: &'a [AlbumEntry], width: f32, + empty_text: Option, } impl<'a> AlbumPicker<'a> { /// `id` must be unique among the pickers on screen (it names their widgets and keeps their /// state apart); `value` is the album picked (`None`: none yet); `entries` in tree order. pub fn new(id: impl Into, value: &'a mut Option, entries: &'a [AlbumEntry]) -> Self { - AlbumPicker { id: id.into(), value, entries, width: 180.0 } + AlbumPicker { id: id.into(), value, entries, width: 180.0, empty_text: None } } /// The button's width (180 by default). @@ -187,13 +188,27 @@ impl<'a> AlbumPicker<'a> { self } + /// Text for an empty choice (already localized by the caller); the default is "Choose an album…". + pub fn empty_text(mut self, text: impl Into) -> Self { + self.empty_text = Some(text.into()); + self + } + /// The button and, while open, the popup. The response is `changed()` when an album was picked. pub fn show(self, ui: &mut Ui) -> Response { - let AlbumPicker { id, value, entries, width } = self; + self.show_with_choices(ui, |_| false) + } + + /// Draw caller-owned choices before the search (e.g. None / New album in the import review). + /// Return true when an extra choice was picked to close the popup. The caller handles those + /// actions; `changed()` still reports only an album picked from the tree or search. + pub fn show_with_choices(self, ui: &mut Ui, choices: impl FnOnce(&mut Ui) -> bool) -> Response { + let AlbumPicker { id, value, entries, width, empty_text } = self; let chosen = value.filter(|v| entries.iter().any(|e| e.id == *v && !e.folder)); - let text = chosen.map_or_else(|| crate::i18n::tr("Choose an album…").to_string(), |c| path_label(entries, c)); + let text = chosen.map_or_else(|| empty_text.unwrap_or_else(|| crate::i18n::tr("Choose an album…").to_string()), |c| path_label(entries, c)); // the name, and a drop-down arrow painted at the right (the UI font has no arrow glyph) - let mut button = ui.add_sized(vec2(width, ui.spacing().interact_size.y), egui::Button::new(text).truncate().right_text(" ")); + let mut button = + ui.add_sized(vec2(width, ui.spacing().interact_size.y), egui::Button::new(text.as_str()).truncate().right_text(" ")).on_hover_text(text); let arrow_color = crate::theme::Tokens::get(ui.ctx()).icon; disclosure(ui.painter(), egui::pos2(button.rect.right() - 10.0, button.rect.center().y), true, arrow_color); register(ui.ctx(), format!("albumPicker:{id}"), button.rect); @@ -206,8 +221,15 @@ impl<'a> AlbumPicker<'a> { let mut state: State = ui.data(|d| d.get_temp(key)).unwrap_or_default(); let mut picked: Option = None; egui::Popup::from_toggle_button_response(&button).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| { - ui.set_min_width(width.max(260.0)); - ui.set_max_width(width.max(260.0) + 120.0); + // A wide popup remembered before a window resize must still fit the current viewport. + let margin = egui::Frame::popup(ui.style()).total_margin().sum().x; + let max_width = (ui.ctx().content_rect().width() - margin).max(1.0); + ui.set_min_width(width.max(260.0).min(max_width)); + ui.set_max_width((width.max(260.0) + 120.0).min(max_width)); + if choices(ui) { + ui.close(); + return; + } let search_box = ui.add(egui::TextEdit::singleline(&mut state.query).hint_text(crate::i18n::tr("Search albums")).desired_width(f32::INFINITY)); register(ui.ctx(), format!("albumPickerSearch:{id}"), search_box.rect); @@ -227,7 +249,9 @@ impl<'a> AlbumPicker<'a> { let (r, _) = ui.allocate_exact_size(vec2(14.0, 14.0), egui::Sense::hover()); let icon = if e.smart { Icon::SmartAlbum } else { Icon::Album }; crate::icons::paint(ui.painter(), r, icon, if e.blocked.is_some() { t.text_disabled } else { t.icon }); - let resp = ui.add_enabled(e.blocked.is_none(), egui::Button::selectable(Some(e.id) == chosen || highlighted, label)); + let resp = ui + .add_enabled(e.blocked.is_none(), egui::Button::selectable(Some(e.id) == chosen || highlighted, label).truncate()) + .on_hover_text(label); register(ui.ctx(), format!("albumPickerItem:{}:{id}", e.id), resp.rect); let resp = match &e.blocked { Some(why) => resp.on_disabled_hover_text(why.as_str()), @@ -390,6 +414,22 @@ mod tests { assert_eq!(ids(&[1, 5]), vec![1, 2, 3, 4, 5, 6]); } + #[test] + fn damaged_parent_chains_are_bounded() { + let mut entries = vec![e(1, "Loop A", Some(2), true, false), e(2, "Loop B", Some(1), true, false), e(3, "Photo", Some(1), false, false)]; + set_depths(&mut entries); + assert!(ancestors(&entries, 3).len() <= 3); + assert_eq!(search(&entries, "photo"), vec![2]); + assert_eq!(visible_tree(&entries, &[1, 2]), vec![0, 1, 2]); + let mut deep: Vec<_> = (1..1000).map(|id| e(id, "Deep", Some(id + 1), true, false)).collect(); + deep.push(e(1000, "End", None, true, false)); + set_depths(&mut deep); + assert!(deep.iter().all(|entry| entry.depth <= 65)); + assert!(ancestors(&deep, 1).len() <= 65); + assert!(path_label(&deep, 1).split(" / ").count() <= 66); + assert_eq!(path_label(&[e(1, "Orphan", Some(99), false, false)], 1), "Orphan"); + } + /// Big libraries stay quick: 20,000 albums in nested folders are laid out and searched in well /// under a second (folder paths are worked out once, not per row by scanning every album). #[test] diff --git a/crates/ui-egui/src/import.rs b/crates/ui-egui/src/import.rs index e851463d1..e7c80925a 100644 --- a/crates/ui-egui/src/import.rs +++ b/crates/ui-egui/src/import.rs @@ -1238,40 +1238,47 @@ pub fn body(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDialog) { ); }); } - let albums: Vec<(u64, String)> = { - let mut v: Vec<(u64, String)> = - app.session.catalog.albums().filter(|a| !a.folder && !a.is_smart()).map(|a| (a.id.0, a.name.clone())).collect(); - v.sort_by_key(|(_, n)| n.to_lowercase()); - v - }; + let mut albums = crate::album_picker::entries_from(&app.session.catalog, |_| None); + albums.retain(|a| a.folder || !a.smart); field(ui, "Album", |ui| { - let cur = match d.album { - Some(a) => albums.iter().find(|x| x.0 == a).map(|x| x.1.clone()).unwrap_or_default(), - None if !d.new_album.is_empty() => crate::i18n::tr("New album").into(), - None => crate::i18n::tr("None").into(), - }; - crate::widgets::combo_box("import-album").selected_text(cur).show_ui(ui, |ui| { - if ui.selectable_label(d.album.is_none() && d.new_album.is_empty(), crate::i18n::tr("None")).clicked() { - d.album = None; + ui.vertical(|ui| { + let mut chosen = d.album; + let mut extra = None; + let empty = crate::i18n::tr(if d.new_album.is_empty() { "None" } else { "New album" }); + let response = crate::album_picker::AlbumPicker::new("import-album", &mut chosen, &albums) + .width(ui.available_width()) + .empty_text(empty) + .show_with_choices(ui, |ui| { + let none = ui.selectable_label(d.album.is_none() && d.new_album.is_empty(), crate::i18n::tr("None")); + register(ui.ctx(), "importAlbum:none", none.rect); + if none.clicked() { + extra = Some(false); + } + let new = ui.selectable_label(d.album.is_none() && !d.new_album.is_empty(), crate::i18n::tr("New album…")); + register(ui.ctx(), "importAlbum:new", new.rect); + if new.clicked() { + extra = Some(true); + } + ui.separator(); + extra.is_some() + }); + if response.changed() { + d.album = chosen; d.new_album.clear(); } - if ui.selectable_label(d.album.is_none() && !d.new_album.is_empty(), crate::i18n::tr("New album…")).clicked() { + if let Some(new) = extra { d.album = None; - if d.new_album.is_empty() { + if !new { + d.new_album.clear(); + } else if d.new_album.is_empty() { d.new_album = crate::i18n::tr("Imported Photos").into(); } } - for (id, name) in &albums { - if ui.selectable_label(d.album == Some(*id), name).clicked() { - d.album = Some(*id); - d.new_album.clear(); - } + if d.album.is_none() && !d.new_album.is_empty() { + let r = ui.add(egui::TextEdit::singleline(&mut d.new_album).desired_width(f32::INFINITY)); + register(ui.ctx(), "field:importAlbumName", r.rect); } }); - if d.album.is_none() && !d.new_album.is_empty() { - let r = ui.add(egui::TextEdit::singleline(&mut d.new_album).desired_width(f32::INFINITY)); - register(ui.ctx(), "field:importAlbumName", r.rect); - } }); field(ui, "Preset", |ui| { let cur = app diff --git a/crates/ui-egui/src/tests_album_picker.rs b/crates/ui-egui/src/tests_album_picker.rs index f48e150b9..fe09afd2f 100644 --- a/crates/ui-egui/src/tests_album_picker.rs +++ b/crates/ui-egui/src/tests_album_picker.rs @@ -1,5 +1,5 @@ -//! The album picker in the smart-album rule editor: a tree like the sidebar's, a search that -//! narrows it as you type, keys to pick with, and albums that can't be picked shown greyed. +//! The shared album picker in smart rules and the import review: sidebar hierarchy, path search, +//! keyboard choices, and only valid destinations. Import also retains None and New album. use std::time::Duration; @@ -126,3 +126,320 @@ fn typing_after_a_folder_click_still_searches() { h.settle(SETTLE); assert!(has(&h, &format!("albumPickerItem:{trip}:rules-0")) && !has(&h, &format!("albumPickerItem:{excluded}:rules-0")), "searching for trip"); } + +/// The import review reuses the same tree, while accepting only regular album destinations. +mod import_review { + use super::*; + + struct Review { + h: Headless, + folder: u64, + nested: u64, + album: u64, + other: u64, + smart: u64, + } + + fn review() -> Review { + review_at([1300.0, 950.0], 1.0) + } + + fn review_at(size: [f32; 2], density: f32) -> Review { + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo().with_fs(), Services { png: None, ..Default::default() }); + let mut h = Headless::new(app, size, density); + let folder = id(exec(&mut h, "album.create", json!({"name": "Travel", "folder": true}))); + let nested = id(exec(&mut h, "album.create", json!({"name": "2026", "folder": true, "parent": folder}))); + let album = id(exec(&mut h, "album.create", json!({"name": "Favorites", "parent": nested, "addSelected": false}))); + let other = id(exec(&mut h, "album.create", json!({"name": "Favorites", "addSelected": false}))); + let smart = id(exec(&mut h, "album.createSmart", json!({"name": "Smart travel", "parent": folder, "rules": {"rating": 4}}))); + h.app.ui.dialog = Some(crate::state::Dialog::Import { opts: Box::new(crate::import::ImportDialog::new(Vec::new())) }); + h.settle(SETTLE); + Review { h, folder, nested, album, other, smart } + } + + fn destination(h: &Headless) -> &crate::import::ImportDialog { + let Some(crate::state::Dialog::Import { opts }) = &h.app.ui.dialog else { panic!("import review remains open") }; + opts + } + + /// #2362: the actual import dialog browses nested folders and remembers the chosen full path. + #[test] + fn import_album_picker_preserves_the_sidebar_hierarchy() { + let Review { mut h, folder, nested, album, other, smart } = review(); + click(&mut h, "albumPicker:import-album"); + assert!(has(&h, &format!("albumPickerFolder:{folder}:import-album"))); + assert!(has(&h, &format!("albumPickerItem:{other}:import-album"))); + assert!(!has(&h, &format!("albumPickerItem:{album}:import-album")), "nested album starts folded"); + click(&mut h, &format!("albumPickerFolder:{folder}:import-album")); + assert_eq!(destination(&h).album, None, "a folder is for browsing, not a destination"); + assert!(!has(&h, &format!("albumPickerItem:{smart}:import-album")), "smart albums cannot receive imports"); + click(&mut h, &format!("albumPickerFolder:{nested}:import-album")); + click(&mut h, &format!("albumPickerItem:{album}:import-album")); + assert_eq!(destination(&h).album, Some(album)); + assert!(destination(&h).new_album.is_empty()); + assert!(h.painted_text().iter().any(|t| t == "Travel / 2026 / Favorites"), "duplicate album names have their full path"); + click(&mut h, "albumPicker:import-album"); + assert!(has(&h, &format!("albumPickerItem:{album}:import-album")), "selected album's ancestors reopen"); + } + + fn text(h: &mut Headless, value: &str) { + let r = h.request("ui.text", json!({"text": value}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.settle(SETTLE)); + } + + fn key(h: &mut Headless, key: &str) { + let r = h.request("ui.key", json!({"key": key}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.settle(SETTLE)); + } + + fn replace_name(h: &mut Headless, name: &str) { + click(h, "field:importAlbumName"); + let r = h.request("ui.key", json!({"key": "a", "cmd": true}), T); + assert_eq!(r["ok"], true, "{r}"); + text(h, name); + } + + #[test] + fn import_album_picker_retains_none_new_and_existing_transitions() { + let Review { mut h, album, .. } = review(); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + assert_eq!(destination(&h).album, None); + assert_eq!(destination(&h).new_album, "Imported Photos"); + replace_name(&mut h, "Été 東京"); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + assert_eq!(destination(&h).new_album, "Été 東京", "choosing New again keeps the typed name"); + click(&mut h, "albumPicker:import-album"); + text(&mut h, "travel 2026 fav"); + key(&mut h, "Enter"); + assert_eq!(destination(&h).album, Some(album)); + assert!(destination(&h).new_album.is_empty(), "existing album replaces a pending new album"); + assert!(!has(&h, "field:importAlbumName")); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:none"); + assert_eq!(destination(&h).album, None); + assert!(destination(&h).new_album.is_empty()); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + replace_name(&mut h, "Leave no orphan album"); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:none"); + assert!(destination(&h).new_album.is_empty(), "None clears a pending name too"); + } + + /// The picker must leave room for a usable name field; a full-width button followed by an + /// inline name field made the resizable dialog grow on every frame, off the native window. + #[test] + fn import_album_picker_keeps_the_new_name_inside_the_dialog() { + let Review { mut h, .. } = review(); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + for _ in 0..20 { + h.step(); + } + let name = h.app.widgets.iter().find(|(id, _)| id == "field:importAlbumName").unwrap().1; + assert!(name.width() >= 160.0, "the name field is usable: {name:?}"); + assert!(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(1300.0, 950.0)).contains_rect(name), "name stays on screen: {name:?}"); + let picker = |h: &Headless| h.app.widgets.iter().find(|(id, _)| id == "albumPicker:import-album").unwrap().1; + let before = picker(&h); + for _ in 0..10 { + h.step(); + } + let after = picker(&h); + assert!( + (before.width() - after.width()).abs() < 0.5 && (before.left() - after.left()).abs() < 0.5, + "the dialog stops growing: {before:?} -> {after:?}" + ); + } + + #[test] + fn import_album_picker_search_escape_and_unicode_names() { + let Review { mut h, folder, album, .. } = review(); + let unicode = id(exec(&mut h, "album.create", json!({"name": "La fête 東京", "parent": folder, "addSelected": false}))); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + replace_name(&mut h, "Keep this name"); + click(&mut h, "albumPicker:import-album"); + text(&mut h, "no such album"); + assert!(h.painted_text().iter().any(|t| t == "No albums match")); + key(&mut h, "Enter"); + assert!(has(&h, "albumPickerSearch:import-album"), "Enter with no result keeps the picker open"); + key(&mut h, "Escape"); + assert!(!has(&h, "albumPickerSearch:import-album")); + assert_eq!(destination(&h).new_album, "Keep this name", "Escape leaves the review's edits intact"); + click(&mut h, "albumPicker:import-album"); + text(&mut h, "FÊTE"); + assert!(has(&h, &format!("albumPickerItem:{unicode}:import-album"))); + assert!(!has(&h, &format!("albumPickerItem:{album}:import-album"))); + key(&mut h, "Enter"); + assert_eq!(destination(&h).album, Some(unicode)); + assert!(h.painted_text().iter().any(|t| t == "Travel / La fête 東京"), "user names are kept verbatim"); + } + + #[test] + fn import_album_picker_follows_custom_sidebar_order() { + let Review { mut h, other, .. } = review(); + let last_by_name = id(exec(&mut h, "album.create", json!({"name": "Zebra", "addSelected": false}))); + exec(&mut h, "album.reorder", json!({"id": last_by_name, "before": other})); + click(&mut h, "albumPicker:import-album"); + let top = |id| h.app.widgets.iter().find(|(w, _)| w == &format!("albumPickerItem:{id}:import-album")).unwrap().1.top(); + assert!(top(last_by_name) < top(other), "sidebar order takes priority over alphabetical order"); + } + + fn button_text(h: &Headless, id: &str, expected: &str) -> bool { + let rect = h.app.widgets.iter().find(|(w, _)| w == id).unwrap().1; + h.view.painted_shapes().iter().any(|shape| matches!(shape, egui::Shape::Text(t) if t.galley.job.text == expected && rect.contains(t.pos))) + } + + #[test] + fn import_album_picker_fits_long_names_across_themes_sizes_and_densities() { + for (size, density) in [([960.0, 760.0], 1.0), ([1366.0, 900.0], 2.0), ([960.0, 760.0], 2.25)] { + let Review { mut h, folder, .. } = review_at(size, density); + let long = "La fête des photographies – 東京 – ".repeat(12); + let album = id(exec(&mut h, "album.create", json!({"name": long, "parent": folder, "addSelected": false}))); + let Some(crate::state::Dialog::Import { opts }) = &mut h.app.ui.dialog else { panic!("review") }; + opts.album = Some(album); + for theme in ["charcoal", "midnight", "paper", "silver"] { + exec(&mut h, "view.theme", json!({"theme": theme})); + let r = h.request("ui.set", json!({"language": "de"}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.settle(SETTLE)); + let screen = egui::Rect::from_min_size(egui::Pos2::ZERO, h.size); + let rect = |h: &Headless, name: &str| h.app.widgets.iter().find(|(id, _)| id == name).unwrap().1; + assert!(screen.contains_rect(rect(&h, "albumPicker:import-album")), "{theme} {size:?}: selected long path"); + click(&mut h, "albumPicker:import-album"); + text(&mut h, "fête"); + let row = rect(&h, &format!("albumPickerItem:{album}:import-album")); + assert!(screen.contains_rect(row), "{theme} {size:?}: search result {row:?} inside {screen:?}"); + key(&mut h, "Escape"); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:new"); + let name = rect(&h, "field:importAlbumName"); + assert!(name.width() >= 160.0 && screen.contains_rect(name), "{theme} {size:?}: name {name:?}"); + click(&mut h, "albumPicker:import-album"); + text(&mut h, "fête"); + key(&mut h, "Enter"); + assert_eq!(destination(&h).album, Some(album)); + } + } + } + + #[test] + fn import_album_picker_localizes_choices_in_every_shipped_language() { + use crate::i18n::Locale; + let Review { mut h, .. } = review(); + for locale in Locale::ALL { + for source in + ["None", "New album", "New album…", "Imported Photos", "Choose an album…", "Search albums", "No albums yet", "No albums match"] + { + assert!(!locale.tr(source).is_empty()); + if *locale != Locale::En { + assert_ne!(locale.tr(source), source, "{locale:?}: {source}"); + } + } + let r = h.request("ui.set", json!({"language": locale.code()}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.settle(SETTLE)); + assert!(button_text(&h, "albumPicker:import-album", locale.tr("None")), "{locale:?}: localized empty destination"); + click(&mut h, "albumPicker:import-album"); + assert!(button_text(&h, "importAlbum:new", locale.tr("New album…")), "{locale:?}: localized new-album option"); + click(&mut h, "importAlbum:new"); + assert_eq!(destination(&h).new_album, locale.tr("Imported Photos")); + assert!(button_text(&h, "albumPicker:import-album", locale.tr("New album"))); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:none"); + } + } + + #[test] + fn import_album_picker_handles_an_empty_catalog() { + let app = LightcraftApp::new(lightcraft_engine::Session::new(), Services { png: None, ..Default::default() }); + let mut h = Headless::new(app, [1000.0, 800.0], 1.0); + h.app.ui.dialog = Some(crate::state::Dialog::Import { opts: Box::new(crate::import::ImportDialog::new(Vec::new())) }); + assert!(h.settle(SETTLE)); + click(&mut h, "albumPicker:import-album"); + assert!(h.painted_text().iter().any(|t| t == "No albums yet")); + click(&mut h, "importAlbum:new"); + assert!(has(&h, "field:importAlbumName")); + click(&mut h, "albumPicker:import-album"); + click(&mut h, "importAlbum:none"); + assert!(destination(&h).new_album.is_empty()); + } + + struct Fixture(std::path::PathBuf); + + impl Drop for Fixture { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + fn fixture() -> Fixture { + let unique = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + let path = std::env::temp_dir().join(format!("lc-import-tree-{}-{unique}", std::process::id())); + std::fs::create_dir(&path).unwrap(); + let pixels = + lightcraft_raster::Rgba8 { width: 40, height: 30, data: (0..1200).map(|i| [(i % 40 * 6) as u8, 31, (i / 40 * 8) as u8, 255]).collect() }; + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&pixels), &lightcraft_codecs::EncodeMeta::default()).unwrap(); + std::fs::write(path.join("hierarchy.png"), png).unwrap(); + Fixture(path) + } + + /// A real file scan, the real picker, and the real import worker: the intended album receives + /// the photo, duplicate names never redirect it, and the import is still one undo step. + #[test] + fn import_album_picker_delivers_to_the_chosen_destination_and_undoes() { + use lightcraft_catalog::PhotoId; + for choice in ["existing", "new", "none"] { + let fixture = fixture(); + let Review { mut h, album, other, .. } = review(); + let before: std::collections::HashSet = h.app.session.catalog.photos().map(|p| p.id).collect(); + h.app.ui.dialog = None; + exec(&mut h, "file.addPhotos", json!({"paths": [fixture.0.to_string_lossy()], "album": album})); + assert!(h.step_until(SETTLE, |h| matches!(h.app.ui.dialog, Some(crate::state::Dialog::Import { .. })))); + assert!(h.settle(SETTLE)); + assert_eq!(destination(&h).album, Some(album), "album-context import keeps its preselection"); + click(&mut h, "albumPicker:import-album"); + assert!(has(&h, &format!("albumPickerItem:{album}:import-album")), "preselection opens the right ancestors"); + match choice { + "existing" => { + text(&mut h, "travel 2026 fav"); + key(&mut h, "Enter"); + } + "new" => { + click(&mut h, "importAlbum:new"); + replace_name(&mut h, "From card"); + } + _ => click(&mut h, "importAlbum:none"), + } + let undo = h.app.session.undo.len(); + let r = h.request("ui.dialog.confirm", json!({}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(h.step_until(SETTLE, |h| h.app.import.is_none())); + assert!(h.settle(SETTLE)); + let added: Vec = h.app.session.catalog.photos().filter(|p| !before.contains(&p.id)).map(|p| p.id).collect(); + assert_eq!(added.len(), 1, "{choice}: exactly the scanned photo is imported"); + let receives = |id| h.app.session.catalog.album(lightcraft_catalog::AlbumId(id)).unwrap().photos.contains(&added[0]); + assert!(!receives(other), "same-named album outside the chosen folder is untouched"); + match choice { + "existing" => assert!(receives(album)), + "new" => { + let created = h.app.session.catalog.albums().find(|a| a.name == "From card").unwrap(); + assert!(created.photos.contains(&added[0])); + assert!(!receives(album)); + } + _ => assert!(h.app.session.catalog.albums().all(|a| !a.photos.contains(&added[0]))), + } + assert_eq!(h.app.session.undo.len(), undo + 1, "{choice}: one undo step"); + exec(&mut h, "edit.undo", json!({})); + assert!(h.settle(SETTLE)); + assert_eq!(h.app.session.catalog.len(), before.len()); + if choice == "new" { + assert!(h.app.session.catalog.albums().all(|a| a.name != "From card"), "undo removes the new album too"); + } + } + } +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 31ecbf8c2..98ef401e7 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -281,6 +281,20 @@ one pixel. (clicks, keys, drags) has played out. Widget ids for `ui.clickWidget` come from `ui.widgets` (e.g. `button:upright-auto`). A 1600×1000 demo snapshot takes ~1–2 s (debug build). +## Import album picker + +In the Import Photos review, `albumPicker:import-album` opens the destination picker. +`albumPickerFolder::import-album` expands or folds a folder; it never chooses a destination. +`albumPickerItem::import-album` selects a regular album. The picker follows sidebar order, +opens the current choice's ancestors and shows its full path on the closed button. Smart albums +are not import destinations. Type into `albumPickerSearch:import-album` to search folder/album paths; +↑ / ↓ and Enter pick a search result, and Escape closes the picker while keeping the review open. + +`importAlbum:none` clears the destination and any pending new album name. `importAlbum:new` keeps +or initializes that name, edited through `field:importAlbumName`. Choosing an existing album clears +the pending name. `ui.dialog.confirm` starts the same import into the selected album ID; engine +`library.import` parameters `album` and `albumName` are unchanged. + ## Panel docking Library, Tools, and Presets panels can be regrouped, split, floated within the app window, closed, and reopened. Tools keeps the current Edit/Crop/Masking mode; the photo view and filmstrip stay together. The document/session view is protected. Layouts, active tabs, panel visibility, and floating rectangles are saved with the app's existing UI preferences. These operations do not edit document contents. diff --git a/docs/gaps.md b/docs/gaps.md index 1b4a385fa..2b36b012a 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -1,6 +1,6 @@ # Gaps: where LightCraft falls short of Lightroom -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-15 · **Change:** minor (gap 15: Print Page Options — page numbers, page info, crop marks) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-20 · **Change:** minor (import review album hierarchy, #2362; no ranked gap closed) Every known shortfall, one entry each, ranked by user impact (how many Lightroom users hit it, and how hard it blocks them), then by effort. **Agents pick work from the top** (the first entry nobody is @@ -60,6 +60,8 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w ## Closed recently (kept for calibration) +- 2026-10-20: the import review album picker follows the sidebar folder hierarchy and searches album paths (#2362, LR-IMP-ADD-DIALOG), retaining None/New album and existing destination IDs. This closes a usability subgap; no ranked gap or headline feature count closes. + - 2026-10-12: original-pixel detail at 100% (gap 4, LR-VIEW-ZOOM, #323): soft proofing gets zoom windows like Detail, Before/After, Compare, Reference and mask overlays; 1:1 tested equal to the full-size export. - 2026-10-11: Sony ILCA-99M2 profile. @@ -77,6 +79,7 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w | Date | Change | Summary | |---|---|---| +| 2026-10-20 | minor | #2362: import destination albums use the shared hierarchy/search picker instead of a flat name list; existing import workflows retained; ranked gaps/readiness counts unchanged | | 2026-10-18 | patch | Gap 1: six X-Trans CMOS 4 Fujifilm bodies (X-T30 …) take the X-T3's spectral matrices instead of the neutral matrix | | 2026-10-15 | minor | Gap 15 shrinks: the Page panel's Page Options (Page Numbers, Page Info, Crop Marks) print on Single Image and Contact Sheet pages; identity plate, watermark, photo info, Cut Guides, direct printing and JPEG output remain | | 2026-10-14 | minor | Gap 26: at 100k photos stack and album lookups are indexed, so an "In a Stack" or "Album" smart album counts in ~14 ms (was 0.6–2.7 s); open time unchanged | diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 4e0531793..fce186e2e 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Lightroom parity checklist -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-15 · **Change:** minor (LRC-PRINT-PAGE: Print Page Options) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-20 · **Change:** minor (LR-IMP-ADD-DIALOG: hierarchical import album destination, #2362) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good @@ -71,7 +71,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). |---|---|---|---|---|---| | 2026-10-12 | minor | LR-IMP-FORMATS: Mamiya MEF decodes from sensor data and is imported (`vendor/mef.rs`) | | 2026-10-12 | minor | LR-IMP-FORMATS: Epson ERF decodes from sensor data (`vendor/erf.rs`), no longer preview-only | -| LR-IMP-ADD-DIALOG | Add photos/folders | P0 | ✅ | `cmd:file.addPhotos`, `cmd:library.importPreview`, `cmd:library.import` (`mode`, `album`, `albumName`, `preset`, `keywords`), `crates/ui-egui/src/import.rs` | review dialog (Import Photos): the scanned source (a scanned folder is not added to Local), candidate grid with thumbnails and checkboxes (duplicates by path/content marked and unchecked; with several file types, a toggle per type checks or unchecks all of its files, e.g. the raws without the JPEGs beside them, issue #344) filling a resizable window (issue #337), transfer mode (add in place / copy / move, each explained), album (existing/new), preset, keywords; the folder scan runs on a background thread, shown in the activity stack with ✕ to cancel (a NAS folder no longer freezes the window); the import itself (probing, sidecars, copying / moving files) runs on a worker thread and its batches join the library between frames, shown in the activity stack with ✕ to cancel; one undo step. Drag-and-drop goes through the same background import. File dialogs list the importable extensions (`lightcraft_engine::import::EXTENSIONS`) in any letter case on Linux, where GTK and the desktop portal match globs case-sensitively (`apps/lightcraft/src/dialog_filter.rs`, issue #342; 0.4.0 hid `.JPG` / `.NEF`, issue #705). The folder scan, Browse Folder, drag-and-drop and the sidecar lookup match extensions in any case (`crates/engine/src/tests_import.rs`) | +| LR-IMP-ADD-DIALOG | Add photos/folders | P0 | ✅ | `cmd:file.addPhotos`, `cmd:library.importPreview`, `cmd:library.import` (`mode`, `album`, `albumName`, `preset`, `keywords`), `crates/ui-egui/src/import.rs` | review dialog (Import Photos): the scanned source (a scanned folder is not added to Local), candidate grid with thumbnails and checkboxes (duplicates by path/content marked and unchecked; with several file types, a toggle per type checks or unchecks all of its files, e.g. the raws without the JPEGs beside them, issue #344) filling a resizable window (issue #337), transfer mode (add in place / copy / move, each explained), album (existing/new): the destination picker follows the sidebar folder hierarchy and order, searches full paths, and distinguishes duplicate names by their folder path; folders navigate and smart albums are excluded, with None, New album and album-context preselection retained (#2362; `crates/ui-egui/src/album_picker.rs`, `crates/ui-egui/src/tests_album_picker.rs`); preset, keywords; the folder scan runs on a background thread, shown in the activity stack with ✕ to cancel (a NAS folder no longer freezes the window); the import itself (probing, sidecars, copying / moving files) runs on a worker thread and its batches join the library between frames, shown in the activity stack with ✕ to cancel; one undo step. Drag-and-drop goes through the same background import. File dialogs list the importable extensions (`lightcraft_engine::import::EXTENSIONS`) in any letter case on Linux, where GTK and the desktop portal match globs case-sensitively (`apps/lightcraft/src/dialog_filter.rs`, issue #342; 0.4.0 hid `.JPG` / `.NEF`, issue #705). The folder scan, Browse Folder, drag-and-drop and the sidecar lookup match extensions in any case (`crates/engine/src/tests_import.rs`) | | LR-IMP-DRAGDROP | Drop files/folders to import | P0 | ✅ | `crates/ui-egui/src/lib.rs` (dropped files → `cmd:library.import`) | dropping onto a specific album not supported | | LR-IMP-DUPES | Skip duplicates by content | P1 | ✅ | `crates/engine/src/import.rs`, `crates/engine/src/tests_import.rs` | | | LR-IMP-DEVICE | Import from camera/card | P1 | ✅ | `cmd:library.devices`, `cmd:file.addFromDevice`, `crates/engine/src/devices.rs` | mounted volumes with a DCIM folder (macOS /Volumes, Linux /media and /run/media, Windows drive letters); File → Import from Device → the import review, copying into the library by default; menus serve the last scan and rescan in the background (never blocking a frame; hot-plugs repaint); copies are verified (a new file, synced, checked against the content hash the scan computed — byte compare with the source when there is none, issue #134; a bad copy is removed and reported as failed, never counted as imported; taken names get -1, -2…, never replaced — issue #96, `crates/engine/src/import_move.rs`); no PTP/MTP (cameras that don't mount as a disk); Settings → Import → Cameras and Cards can open the import review when a camera or card is connected (off by default; a card that was already in doesn't ask, and an open dialog or running import is finished first; `crates/ui-egui/src/import.rs`) | @@ -797,6 +797,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | Date | Change | Summary | |---|---|---| +| 2026-10-20 | minor | LR-IMP-ADD-DIALOG: #2362 gives the import review the shared hierarchy/search album picker while retaining None/New album and existing destination IDs; status and feature counts unchanged | | 2026-10-14 | patch | LR-EXP-TYPE: opaque 8-bit PNG/TIFF/WebP use RGB; transparent exports retain alpha (#1670). Statuses and feature counts unchanged. | | 2026-10-15 | minor | LRC-PRINT-PAGE: Page Options (Page Numbers, Page Info, Crop Marks) on Single Image and Contact Sheet pages, in the Print dialog, `print.set` and templates; still 🟡 (no identity plate, watermark, photo info or Cut Guides) | | 2026-10-15 | minor | LR-IMP-FORMATS: Hasselblad 3FR and FFF show their uncompressed RGB preview (no JPEG in these files) in the grid and loupe until developed, and as the stand-in when a file fails to decode; status unchanged (🟡) | From 4cb90d4b7ee515917f48539f5fa10b892fcbb3b0 Mon Sep 17 00:00:00 2001 From: tylerphotos <93777540+tylerphotos@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:34:52 -0500 Subject: [PATCH 2/2] LR-IMP-ADD-DIALOG: clear deleted import destinations Keep the stored destination consistent with the picker's None fallback after catalog changes. Extend the real-file import regression to delete the chosen album before confirmation, then verify import membership and undo. Document the helper contracts and destination transitions for review coverage. Co-Authored-By: Codex --- crates/ui-egui/src/album_picker.rs | 1 + crates/ui-egui/src/import.rs | 2 ++ crates/ui-egui/src/tests_album_picker.rs | 30 +++++++++++++++++++++++- docs/control-protocol.md | 2 ++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/album_picker.rs b/crates/ui-egui/src/album_picker.rs index 05e4d6dd3..835dbcce8 100644 --- a/crates/ui-egui/src/album_picker.rs +++ b/crates/ui-egui/src/album_picker.rs @@ -414,6 +414,7 @@ mod tests { assert_eq!(ids(&[1, 5]), vec![1, 2, 3, 4, 5, 6]); } + /// Cycles, dangling parents and excessively deep chains finish with bounded paths and depths. #[test] fn damaged_parent_chains_are_bounded() { let mut entries = vec![e(1, "Loop A", Some(2), true, false), e(2, "Loop B", Some(1), true, false), e(3, "Photo", Some(1), false, false)]; diff --git a/crates/ui-egui/src/import.rs b/crates/ui-egui/src/import.rs index e7c80925a..188efca44 100644 --- a/crates/ui-egui/src/import.rs +++ b/crates/ui-egui/src/import.rs @@ -1240,6 +1240,8 @@ pub fn body(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDialog) { } let mut albums = crate::album_picker::entries_from(&app.session.catalog, |_| None); albums.retain(|a| a.folder || !a.smart); + // Catalog changes while the review is open must not leave a hidden, unselectable destination. + d.album = d.album.filter(|id| albums.iter().any(|a| a.id == *id && !a.folder)); field(ui, "Album", |ui| { ui.vertical(|ui| { let mut chosen = d.album; diff --git a/crates/ui-egui/src/tests_album_picker.rs b/crates/ui-egui/src/tests_album_picker.rs index fe09afd2f..32cc906fe 100644 --- a/crates/ui-egui/src/tests_album_picker.rs +++ b/crates/ui-egui/src/tests_album_picker.rs @@ -12,26 +12,31 @@ use crate::{LightcraftApp, Services}; const T: Duration = Duration::from_secs(20); const SETTLE: Duration = Duration::from_secs(120); +/// Dispatch a public command and return its successful engine result. fn exec(h: &mut Headless, command: &str, params: serde_json::Value) -> serde_json::Value { let r = h.request("engine.execute", json!({"command": command, "params": params}), T); assert_eq!(r["ok"], true, "{command}: {r}"); r["result"].clone() } +/// Read the album ID returned by a successful creation command. fn id(v: serde_json::Value) -> u64 { v["id"].as_u64().expect("an id") } +/// Click a registered widget through the public control channel and let frames settle. fn click(h: &mut Headless, id: &str) { let r = h.request("ui.clickWidget", json!({"id": id}), T); assert_eq!(r["ok"], true, "{id}: {r}"); h.settle(SETTLE); } +/// Check the current frame registry for an exact widget ID. fn has(h: &Headless, id: &str) -> bool { h.app.widgets.iter().any(|(w, _)| w == id) } +/// Read the Album rule value from the active smart-rule editor. fn value(h: &Headless) -> serde_json::Value { let Some(crate::state::Dialog::SmartRules { rules, .. }) = &h.app.ui.dialog else { panic!("no rule editor") }; let Some(Rule::Field { value, .. }) = rules.rules.first() else { panic!("no rule") }; @@ -140,10 +145,12 @@ mod import_review { smart: u64, } + /// Open the standard import review fixture with nested and same-named albums. fn review() -> Review { review_at([1300.0, 950.0], 1.0) } + /// Build the import review fixture at the requested logical size and display density. fn review_at(size: [f32; 2], density: f32) -> Review { let app = LightcraftApp::new(lightcraft_engine::Session::with_demo().with_fs(), Services { png: None, ..Default::default() }); let mut h = Headless::new(app, size, density); @@ -157,6 +164,7 @@ mod import_review { Review { h, folder, nested, album, other, smart } } + /// Read the active import review without accepting a silently closed dialog. fn destination(h: &Headless) -> &crate::import::ImportDialog { let Some(crate::state::Dialog::Import { opts }) = &h.app.ui.dialog else { panic!("import review remains open") }; opts @@ -182,18 +190,21 @@ mod import_review { assert!(has(&h, &format!("albumPickerItem:{album}:import-album")), "selected album's ancestors reopen"); } + /// Type through the public control channel and wait for the search or field to update. fn text(h: &mut Headless, value: &str) { let r = h.request("ui.text", json!({"text": value}), T); assert_eq!(r["ok"], true, "{r}"); assert!(h.settle(SETTLE)); } + /// Send a public keyboard event and wait for its resulting UI state. fn key(h: &mut Headless, key: &str) { let r = h.request("ui.key", json!({"key": key}), T); assert_eq!(r["ok"], true, "{r}"); assert!(h.settle(SETTLE)); } + /// Replace the pending new album name using normal field selection and typing. fn replace_name(h: &mut Headless, name: &str) { click(h, "field:importAlbumName"); let r = h.request("ui.key", json!({"key": "a", "cmd": true}), T); @@ -201,6 +212,7 @@ mod import_review { text(h, name); } + /// None, New and an existing album replace each other without retaining a stale pending name. #[test] fn import_album_picker_retains_none_new_and_existing_transitions() { let Review { mut h, album, .. } = review(); @@ -255,6 +267,7 @@ mod import_review { ); } + /// Path search is case-insensitive and Escape preserves review edits and user-provided names. #[test] fn import_album_picker_search_escape_and_unicode_names() { let Review { mut h, folder, album, .. } = review(); @@ -279,6 +292,7 @@ mod import_review { assert!(h.painted_text().iter().any(|t| t == "Travel / La fête 東京"), "user names are kept verbatim"); } + /// The catalog custom order wins over alphabetical order in the import picker. #[test] fn import_album_picker_follows_custom_sidebar_order() { let Review { mut h, other, .. } = review(); @@ -289,11 +303,13 @@ mod import_review { assert!(top(last_by_name) < top(other), "sidebar order takes priority over alphabetical order"); } + /// Match painted text inside the specified control instead of anywhere in the frame. fn button_text(h: &Headless, id: &str, expected: &str) -> bool { let rect = h.app.widgets.iter().find(|(w, _)| w == id).unwrap().1; h.view.painted_shapes().iter().any(|shape| matches!(shape, egui::Shape::Text(t) if t.galley.job.text == expected && rect.contains(t.pos))) } + /// Long paths and editable names remain usable across themes, viewport sizes and display densities. #[test] fn import_album_picker_fits_long_names_across_themes_sizes_and_densities() { for (size, density) in [([960.0, 760.0], 1.0), ([1366.0, 900.0], 2.0), ([960.0, 760.0], 2.25)] { @@ -327,6 +343,7 @@ mod import_review { } } + /// Every shipped locale paints translated destination choices and initializes a translated new name. #[test] fn import_album_picker_localizes_choices_in_every_shipped_language() { use crate::i18n::Locale; @@ -354,6 +371,7 @@ mod import_review { } } + /// An empty catalog still offers None and New without inventing an existing destination. #[test] fn import_album_picker_handles_an_empty_catalog() { let app = LightcraftApp::new(lightcraft_engine::Session::new(), Services { png: None, ..Default::default() }); @@ -372,11 +390,13 @@ mod import_review { struct Fixture(std::path::PathBuf); impl Drop for Fixture { + /// Remove only the uniquely owned fixture directory after the test session releases it. fn drop(&mut self) { let _ = std::fs::remove_dir_all(&self.0); } } + /// Write an original procedural PNG in a unique temporary directory for a real import. fn fixture() -> Fixture { let unique = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); let path = std::env::temp_dir().join(format!("lc-import-tree-{}-{unique}", std::process::id())); @@ -390,10 +410,11 @@ mod import_review { /// A real file scan, the real picker, and the real import worker: the intended album receives /// the photo, duplicate names never redirect it, and the import is still one undo step. + /// A destination deleted while the review is open falls back to no album before confirmation. #[test] fn import_album_picker_delivers_to_the_chosen_destination_and_undoes() { use lightcraft_catalog::PhotoId; - for choice in ["existing", "new", "none"] { + for choice in ["existing", "new", "none", "deleted"] { let fixture = fixture(); let Review { mut h, album, other, .. } = review(); let before: std::collections::HashSet = h.app.session.catalog.photos().map(|p| p.id).collect(); @@ -413,6 +434,13 @@ mod import_review { click(&mut h, "importAlbum:new"); replace_name(&mut h, "From card"); } + "deleted" => { + key(&mut h, "Escape"); + exec(&mut h, "album.delete", json!({"id": album})); + assert!(h.settle(SETTLE)); + assert_eq!(destination(&h).album, None, "a deleted destination matches the displayed None choice"); + assert!(button_text(&h, "albumPicker:import-album", "None")); + } _ => click(&mut h, "importAlbum:none"), } let undo = h.app.session.undo.len(); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 98ef401e7..4c4c686ac 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -289,6 +289,8 @@ In the Import Photos review, `albumPicker:import-album` opens the destination pi opens the current choice's ancestors and shows its full path on the closed button. Smart albums are not import destinations. Type into `albumPickerSearch:import-album` to search folder/album paths; ↑ / ↓ and Enter pick a search result, and Escape closes the picker while keeping the review open. +If the chosen album is deleted while the review is open, the destination returns to None before +confirmation instead of submitting the deleted ID. `importAlbum:none` clears the destination and any pending new album name. `importAlbum:new` keeps or initializes that name, edited through `field:importAlbumName`. Choosing an existing album clears