From 1028ed6b88724dcb2cc1b7ec245a629ae9203fb4 Mon Sep 17 00:00:00 2001 From: radialmonster Date: Thu, 8 Oct 2026 15:53:00 -0400 Subject: [PATCH 1/4] LRC-LIB-LIBFOLDERS: a chosen folder that follows a move keeps its names' case and the platform's separators follow_library_folder rebuilt the chosen subfolder from the lower-cased folder_key on Windows and joined it with '/', so after a move the folder shown in the sidebar came back as c:\archive\trip/day1 with its names in lower case. Add folder_rest (compares by folder_key, returns the names as spelled) and join them with the platform's separator. Tests compare folders by identity where they only mean the same folder, and a new test fails on Windows without the fix. --- crates/catalog/src/query.rs | 11 +++++++++++ crates/engine/src/cmd/browse.rs | 10 +++------- crates/engine/src/tests_folders.rs | 31 ++++++++++++++++++++++-------- 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/crates/catalog/src/query.rs b/crates/catalog/src/query.rs index ad14cd0a..47223d5d 100644 --- a/crates/catalog/src/query.rs +++ b/crates/catalog/src/query.rs @@ -525,6 +525,17 @@ pub fn folder_within(path: &str, root: &str) -> bool { key_within(&folder_key(path), &folder_key(root)) } +/// The names below folder `root` that lead to `path`, as the caller spelled them (`None` when +/// `path` is not at or inside `root`; empty when it is `root`). Compared by [`folder_key`], but +/// the names keep their case, which the key lowers on Windows. +pub fn folder_rest(path: &str, root: &str) -> Option> { + if !key_within(&folder_key(path), &folder_key(root)) { + return None; + } + let depth = split_path(root, false).parts.len(); + Some(split_path(path, false).parts.into_iter().skip(depth).collect()) +} + /// Whether a library photo (not one only browsed in Local) lies in the folder whose /// [`folder_key`] is `root`. A plain POSIX path (no `\`, `.`, `..`, repeated or trailing /// separators) is its own identity, so it is read without building a key for it: the check runs diff --git a/crates/engine/src/cmd/browse.rs b/crates/engine/src/cmd/browse.rs index 26d6130b..e5074d9e 100644 --- a/crates/engine/src/cmd/browse.rs +++ b/crates/engine/src/cmd/browse.rs @@ -268,13 +268,9 @@ pub(crate) fn follow_folder(s: &mut Session, from: &str, to: &str) { /// the folder itself. fn follow_library_folder(chosen: &mut Option, from: &str, to: &str) { let Some(c) = chosen.as_deref() else { return }; - let (ck, fk) = (lightcraft_catalog::query::folder_key(c), lightcraft_catalog::query::folder_key(from)); - if fk.is_empty() { - return; - } - if let Some(rest) = ck.strip_prefix(fk.as_str()).filter(|r| r.is_empty() || r.starts_with('/') || fk.ends_with('/')) { - let rest = rest.trim_start_matches('/'); - *chosen = Some(if rest.is_empty() { to.to_string() } else { format!("{}/{rest}", to.trim_end_matches(['/', '\\'])) }); + if let Some(rest) = lightcraft_catalog::query::folder_rest(c, from) { + // joined by the platform, so the result is spelled like `to` and like a path read from disk + *chosen = Some(rest.iter().fold(std::path::PathBuf::from(to), |p, name| p.join(name)).to_string_lossy().to_string()); } } diff --git a/crates/engine/src/tests_folders.rs b/crates/engine/src/tests_folders.rs index 6dd5fe22..a9492137 100644 --- a/crates/engine/src/tests_folders.rs +++ b/crates/engine/src/tests_folders.rs @@ -189,6 +189,12 @@ fn the_folder_filter_takes_only_a_text_path() { assert_eq!(s.visible().len(), 0, "a path that names no folder shows nothing, not everything"); } +/// Folders are compared by identity: a path read back may be spelled with the platform's separator. +fn same_folder(actual: Option<&str>, expected: &str, why: &str) { + let key = lightcraft_catalog::query::folder_key; + assert_eq!(actual.map(key), Some(key(expected)), "{why}: {actual:?} vs {expected}"); +} + #[test] fn a_renamed_folder_stays_the_chosen_one_and_undo_follows_it_back() { let dir = Scratch::new("rename"); @@ -200,10 +206,10 @@ fn a_renamed_folder_stays_the_chosen_one_and_undo_follows_it_back() { s.execute("library.filter", &json!({"libraryFolder": trip})).unwrap(); assert_eq!(s.visible().len(), 2); s.execute("folder.rename", &json!({"path": trip, "name": "holiday"})).unwrap(); - assert_eq!(s.filter.library_folder.as_deref(), Some(renamed.as_str()), "the choice follows the folder"); + same_folder(s.filter.library_folder.as_deref(), &renamed, "the choice follows the folder"); assert_eq!(s.visible().len(), 2, "and still shows its photos"); s.execute("edit.undo", &json!({})).unwrap(); - assert_eq!(s.filter.library_folder.as_deref(), Some(trip.as_str())); + same_folder(s.filter.library_folder.as_deref(), &trip, "the choice is the same folder"); assert_eq!(s.visible().len(), 2); } @@ -217,10 +223,10 @@ fn a_moved_folder_keeps_a_chosen_subfolder_chosen() { add(&mut s, &format!("{day1}/b.jpg")); s.execute("library.filter", &json!({"libraryFolder": day1})).unwrap(); s.execute("folder.move", &json!({"path": dir.path("trip"), "into": dir.path("archive")})).unwrap(); - assert_eq!(s.filter.library_folder.as_deref(), Some(moved.as_str()), "the subfolder follows its parent"); + same_folder(s.filter.library_folder.as_deref(), &moved, "the subfolder follows its parent"); assert_eq!(s.visible().len(), 1); s.execute("edit.undo", &json!({})).unwrap(); - assert_eq!(s.filter.library_folder.as_deref(), Some(day1.as_str())); + same_folder(s.filter.library_folder.as_deref(), &day1, "the choice is the same folder"); } #[test] @@ -229,12 +235,21 @@ fn following_a_folder_reads_both_spellings_the_same_way() { let mut s = Session::new(); s.filter.library_folder = Some("/a/x/../b/sub".into()); crate::cmd::browse::follow_folder(&mut s, "/a/b", "/a/c"); - assert_eq!(s.filter.library_folder.as_deref(), Some("/a/c/sub"), "a subfolder stays a subfolder, never widens to its parent"); + same_folder(s.filter.library_folder.as_deref(), "/a/c/sub", "a subfolder stays a subfolder, never widens to its parent"); s.filter.library_folder = Some("/elsewhere/b".into()); crate::cmd::browse::follow_folder(&mut s, "/a/b", "/a/c"); assert_eq!(s.filter.library_folder.as_deref(), Some("/elsewhere/b"), "other folders are left alone"); } +#[test] +fn following_a_folder_keeps_the_case_of_the_subfolder_names() { + // Windows compares folders in lower case; the name the user sees must not change with it + let mut s = Session::new(); + s.filter.library_folder = Some("/A/Trip/Day1".into()); + crate::cmd::browse::follow_folder(&mut s, "/A/Trip", "/B"); + assert_eq!(s.filter.library_folder.as_deref(), Some(std::path::Path::new("/B").join("Day1").to_str().unwrap())); +} + #[test] fn choosing_a_folder_replaces_the_source_the_way_an_album_does() { let mut s = session(); @@ -296,10 +311,10 @@ fn a_renamed_folder_stays_the_one_shown_and_undo_follows_it_back() { add(&mut s, &format!("{day1}/b.jpg")); source_folder(&mut s, &day1); s.execute("folder.rename", &json!({"path": trip, "name": "holiday"})).unwrap(); - assert_eq!(s.library_folder.as_deref(), Some(format!("{renamed}/day1").as_str()), "the subfolder follows its parent"); + same_folder(s.library_folder.as_deref(), &format!("{renamed}/day1"), "the subfolder follows its parent"); assert_eq!(s.visible().len(), 1); s.execute("edit.undo", &json!({})).unwrap(); - assert_eq!(s.library_folder.as_deref(), Some(day1.as_str())); + same_folder(s.library_folder.as_deref(), &day1, "the choice is the same folder"); assert_eq!(s.visible().len(), 1); } @@ -308,7 +323,7 @@ fn the_shown_folder_follows_reading_both_spellings_the_same_way() { let mut s = Session::new(); s.library_folder = Some("/a/x/../b/sub".into()); crate::cmd::browse::follow_folder(&mut s, "/a/b", "/a/c"); - assert_eq!(s.library_folder.as_deref(), Some("/a/c/sub")); + same_folder(s.library_folder.as_deref(), "/a/c/sub", "the shown folder reads both spellings alike"); } #[test] From 8e4d7545ffeec5d74fe4dbc57cc10f5b068a412c Mon Sep 17 00:00:00 2001 From: radialmonster Date: Thu, 8 Oct 2026 16:08:13 -0400 Subject: [PATCH 2/4] M11.4: the X-T20 corpus RAF is fitted since the away-from-edges retry; update the test and the doc that said it was rejected Found running cargo xtask ci with the corpus on Windows. The two changes landed on the same day: the Fujifilm test (#282) expected the compressed X-T20 file to keep the neutral fallback because its fit failed the gates on all pixels (held-out RMS 0.114 against 0.10). The retry away from edges (#263, issue #232) fits it (0.056), so the fit is accepted at both sizes. Expect true. --- crates/engine/src/camera_preview.rs | 8 +++++--- docs/camera-preview-colour.md | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/camera_preview.rs b/crates/engine/src/camera_preview.rs index 5909b939..858c1326 100644 --- a/crates/engine/src/camera_preview.rs +++ b/crates/engine/src/camera_preview.rs @@ -1179,15 +1179,17 @@ mod tests { assert!(fit_pairs(&sensor, &reference).is_none()); } - /// X-Trans and 16-bit Bayer RAFs use the same fixed fit at thumbnail and export sizes; - /// a poor reference keeps the fallback. Skips cleanly without these CC0 corpus files. + /// X-Trans and 16-bit Bayer RAFs use the same fixed fit at thumbnail and export sizes. + /// The X-T20 fails the gates on all pixels (held-out RMS 0.114 against 0.10) and passes away + /// from edges (0.056, issue #232), so it is fitted too. Skips cleanly without these CC0 + /// corpus files. #[test] fn corpus_raf_colour_and_white_balance() { let dir = std::env::var_os("LIGHTCRAFT_CORPUS") .map(std::path::PathBuf::from) .unwrap_or_else(|| std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus")) .join("raw"); - for (name, accepted) in [("raf-fuji-xt2-865.raf", true), ("raf-fuji-gfx100s-4503.raf", true), ("raf-fuji-xt20-compressed.raf", false)] { + for (name, accepted) in [("raf-fuji-xt2-865.raf", true), ("raf-fuji-gfx100s-4503.raf", true), ("raf-fuji-xt20-compressed.raf", true)] { let Ok(bytes) = std::fs::read(dir.join(name)) else { continue }; let header = crate::files::probe_bytes(name, &bytes).unwrap(); assert_eq!(header.as_shot_wb, Some((6500.0, 0.0)), "{name}"); diff --git a/docs/camera-preview-colour.md b/docs/camera-preview-colour.md index 876091e8..da0b5add 100644 --- a/docs/camera-preview-colour.md +++ b/docs/camera-preview-colour.md @@ -56,7 +56,7 @@ The additional supplied folder contains 748 RAFs. Profiles were fitted separatel For validation, every fifth usable photo of each model in sorted path order was withheld. Profiles trained on the remaining 160 X-H2S and 436 X-T4 photos were tested on 41 and 109 withheld photos. Mean CIE76 ΔE against the camera JPEG was 9.86 → 1.70 with per-file fitting → 1.62 with the pooled X-H2S profile, and 13.64 → 2.43 → 2.34 for X-T4. The profiles improved 108 of 150 files; the largest regression against per-file fitting was 0.60 ΔE, with no previously accepted fit rejected. These are withheld files from the same limited scene collection, not independent coverage of all subjects or illuminants. -The three original dark X-H2S wrench files have too little colour variation to learn a matrix, but now use the known profile to fit tone: ΔE 8.74 → 2.25, all accepted. The four original X-T4 files and two public X-T4 files remain accepted with the final bundled profile; they regress slightly against their own per-file fits (six-file mean 4.72 → 4.94), while remaining much closer than the neutral fallback (13.47). One public X-T20 reference still fails the gates and keeps the fallback. +The three original dark X-H2S wrench files have too little colour variation to learn a matrix, but now use the known profile to fit tone: ΔE 8.74 → 2.25, all accepted. The four original X-T4 files and two public X-T4 files remain accepted with the final bundled profile; they regress slightly against their own per-file fits (six-file mean 4.72 → 4.94), while remaining much closer than the neutral fallback (13.47). The public X-T20 reference fails the gates on all pixels (held-out RMS 0.114) and passes on the second attempt away from edges (0.056, from 0.185 uncorrected), so it is fitted too. RAF inputs also work with `lightcraft-cli calibrate`. Tests cover real X-Trans/16-bit Bayer fits, profile use on dull scenes, per-file fitting after profile rejection, monochrome rejection, resolution independence, header/decoded relative WB, old-catalog WB transitions and invalid RAF calibration inputs. A full-size 6240×4160 X-T4 JPEG export also succeeded. From 1fa3af8740ee7436a2e7d440ad42d83e8026dac4 Mon Sep 17 00:00:00 2001 From: radialmonster Date: Thu, 8 Oct 2026 16:22:59 -0400 Subject: [PATCH 3/4] LRC-LIB-LIBFOLDERS: the sidebar rename test builds its folder ids with forward slashes on Windows The folder tree writes paths with forward slashes and its widget ids carry them; the test put the temp folder in the id with backslashes. Found running cargo xtask ci on Windows. --- crates/ui-egui/src/tests_panels.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index ee4a0f8a..9a6023cb 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -377,7 +377,8 @@ fn a_renamed_folder_keeps_its_place_in_the_tree() { let _cleanup = Cleanup(base.clone()); std::fs::create_dir_all(base.join("pics/trip")).unwrap(); std::fs::create_dir_all(base.join("pics/home")).unwrap(); - let b = base.to_string_lossy().to_string(); + // the folder tree writes paths with forward slashes, and its widget ids carry them + let b = base.to_string_lossy().replace('\\', "/"); let mut h = folders_app(&[&format!("{b}/pics/trip/a.jpg"), &format!("{b}/pics/home/b.jpg")]); h.app.session.source = lightcraft_engine::LibrarySource::LibraryFolder; h.app.session.library_folder = Some(format!("{b}/pics/trip")); From 790a3417765183138afd898ea0f9848a08820da1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 21:15:52 +0000 Subject: [PATCH 4/4] Part of #374: launch imports run off the UI thread; folder walks are bounded and never take a drive root or home implicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (in 0.2.1 and still on main): files and folders named on the command line were imported from the eframe app-creator closure with `app.run("library.import", …)`, which runs the engine command synchronously: `import::expand` walked the folder recursively (one `is_dir()` open per entry, no depth or count bound, following links) and probed every file before the first frame could be drawn. An argument that resolves to a drive root (`.` or `..` with a launcher's working folder at `C:\`, `/` or `\` on Windows) or to the home folder therefore walked the whole drive with the window stuck as a black "not responding" box. The library-problem banner's pending import had the same synchronous call. Every other recursive walk (Local's Include Subfolders, Find Missing Photos, preset and LUT folders, MCP) was also unbounded, which is the "Local folders with subfolders: not responding" pattern of filmcraft#23, and a link loop recursed until the stack overflowed. - engine `walk::files_in`: one bounded, iterative walker (32 folder levels, 1,000,000 entries; `Walked::truncated` says it stopped), folder or file taken from the directory listing (no open per entry), links and junctions to a folder followed once. `import::expand` (+ `expand_within`), preset and LUT folder expansion and MCP `expand_paths` use it. - engine `import::launch_path`: an empty launch argument is ignored; a relative one is resolved against the working folder; a drive or file-system root becomes its memory card's DCIM folder or is refused, and so is the home folder (a notice says why and to use File > Import Photos). - desktop: launch arguments go through it and are imported with `import::start_paths` (the import worker), like dropped files; so does the library-problem banner's pending import. Not verified on Windows (no machine here). The report says PhotoCraft 0.5.0 and `--memory` hang the same way with no arguments; nothing in LightCraft walks the disk at startup without arguments (strace of a headless demo run with HOME=/ and cwd /: no recursive listing), so that part may be outside this code. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01912WaK6Rmew3wxZuPyX7E8 --- apps/lightcraft/src/main.rs | 54 +++++- crates/engine/src/cmd/lut_profiles.rs | 21 +-- crates/engine/src/devices.rs | 2 +- crates/engine/src/import.rs | 96 ++++++++-- crates/engine/src/lib.rs | 1 + crates/engine/src/presets.rs | 32 ++-- crates/engine/src/tests_import.rs | 55 ++++++ crates/engine/src/walk.rs | 183 +++++++++++++++++++ crates/mcp/src/headless.rs | 25 +-- crates/ui-egui/src/panels/library_problem.rs | 14 +- crates/ui-egui/src/tests_library_problem.rs | 2 + 11 files changed, 410 insertions(+), 75 deletions(-) create mode 100644 crates/engine/src/walk.rs diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 36314176..09271e6b 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -462,6 +462,31 @@ ENVIRONMENT: /logs/lightcraft.log (previous runs: lightcraft.1.log, lightcraft.2.log) "; +/// The launch arguments' files and folders to import (absolute), and a notice for each one that +/// is not imported (see [`lightcraft_engine::import::launch_path`]; issue #374). +fn launch_imports(args: &[String]) -> (Vec, Vec) { + let cwd = std::env::current_dir().ok(); + let home = std::env::var_os(if cfg!(windows) { "USERPROFILE" } else { "HOME" }).filter(|h| !h.is_empty()).map(std::path::PathBuf::from); + launch_imports_in(args, cwd.as_deref(), home.as_deref()) +} + +fn launch_imports_in(args: &[String], cwd: Option<&std::path::Path>, home: Option<&std::path::Path>) -> (Vec, Vec) { + use lightcraft_engine::import::{LaunchPath, launch_path}; + let (mut files, mut notices) = (Vec::new(), Vec::new()); + for a in args { + match launch_path(a, cwd, home, lightcraft_engine::devices::dcim_in) { + Some(LaunchPath::Import(p)) => files.push(p), + Some(LaunchPath::Refused { path, why }) => { + log::warn!("not importing {path} given at launch: {why}"); + notices + .push(format!("{path} was not imported: it is {why}. To import from it, choose a folder inside it with File ▸ Import Photos…")); + } + None => {} + } + } + (files, notices) +} + /// Where the log files live: `logs` in the settings folder, next to `ui.json` (see `logging`). /// None with `LIGHTCRAFT_NO_PREFS` (tests, scripts), so those runs don't rotate away the user's logs. fn log_dir() -> Option { @@ -534,6 +559,9 @@ fn main() -> eframe::Result { None => logger.no_file(), } } + // what the arguments name, resolved now: an empty or relative argument never becomes a drive + // root, and a whole drive or home folder is not walked (issue #374) + let (files, launch_notices) = launch_imports(&files); let (prefs, prefs_warning, keep_prefs_file) = load_prefs(in_memory); // the saved language from the start, so a failed start is reported in it too if let Some(ui) = &prefs { @@ -586,6 +614,7 @@ fn main() -> eframe::Result { lightcraft_ui_egui::i18n::set_language(app.ui.language); app.integrated_titlebar = cfg!(target_os = "macos"); app.notices.extend(prefs_warning); + app.notices.extend(launch_notices); // what's on disk now: only changes are written let writer = PrefsWriter { path: if in_memory { None } else { prefs_path() }, @@ -608,7 +637,12 @@ fn main() -> eframe::Result { p.pending_import = files; app.library_problem = Some(p); } else if !files.is_empty() { - let _ = app.run("library.import", serde_json::json!({"paths": files})); + // listed and read on the import worker once the window shows (issue #374: never + // here, before the first frame, where a large folder kept the window from + // appearing) + if let Err(e) = lightcraft_ui_egui::import::start_paths(&mut app, files) { + log::warn!("import at launch: {e}"); + } app.ui.view = lightcraft_ui_egui::state::ViewMode::PhotoGrid; } // native menu bar generated from the command registry (macOS; elsewhere the menus are @@ -658,6 +692,24 @@ mod tests { d } + /// Issue #374: launch arguments that would walk a whole drive or home folder are reported, + /// not imported; empty ones are ignored; the rest are imported as absolute paths. + #[test] + fn launch_arguments_never_walk_a_drive_or_the_home_folder() { + let home = dir("launch"); + std::fs::create_dir_all(home.join("Trip")).unwrap(); + let args: Vec = ["", " ", ".", "Trip", &home.to_string_lossy()].iter().map(|s| s.to_string()).collect(); + let (files, notices) = launch_imports_in(&args, Some(&home), Some(&home)); + assert_eq!(files, [home.join("Trip").to_string_lossy().to_string()]); + assert_eq!(notices.len(), 2, "{notices:?}"); + assert!(notices.iter().all(|n| n.contains("was not imported")), "{notices:?}"); + // the working folder is a drive root (a launcher's choice): `.` is never the whole drive + let root = home.ancestors().last().unwrap().to_path_buf(); + let (files, notices) = launch_imports_in(&[".".to_string()], Some(&root), None); + assert!(files.iter().all(|f| f.ends_with("DCIM")) && files.len() + notices.len() == 1, "{files:?} {notices:?}"); + let _ = std::fs::remove_dir_all(&home); + } + /// Issue #103: a damaged ui.json (which holds the library location) is kept aside and /// reported, not silently replaced by the defaults. #[test] diff --git a/crates/engine/src/cmd/lut_profiles.rs b/crates/engine/src/cmd/lut_profiles.rs index 5e3febd9..8eb9574e 100644 --- a/crates/engine/src/cmd/lut_profiles.rs +++ b/crates/engine/src/cmd/lut_profiles.rs @@ -41,22 +41,11 @@ fn cube_files(paths: &[String]) -> Vec<(String, String, Option)> { for p in paths { let path = Path::new(p); if path.is_dir() { - let mut stack = vec![path.to_path_buf()]; - while let Some(d) = stack.pop() { - let Ok(rd) = std::fs::read_dir(&d) else { continue }; - for e in rd.flatten() { - let ep = e.path(); - if ep.file_name().is_some_and(|n| n.to_string_lossy().starts_with('.')) { - continue; - } - if ep.is_dir() { - stack.push(ep); - } else if ep.extension().is_some_and(|x| x.eq_ignore_ascii_case("cube")) - && let Ok(t) = std::fs::read_to_string(&ep) - { - let group = ep.parent().and_then(|d| d.file_name()).map(|n| n.to_string_lossy().to_string()); - out.push((ep.file_stem().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(), t, group)); - } + let cube = |f: &Path| f.extension().is_some_and(|x| x.eq_ignore_ascii_case("cube")); + for ep in crate::walk::files_in(path, None, crate::walk::Limits::default(), cube).files { + if let Ok(t) = std::fs::read_to_string(&ep) { + let group = ep.parent().and_then(|d| d.file_name()).map(|n| n.to_string_lossy().to_string()); + out.push((ep.file_stem().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(), t, group)); } } } else if path.extension().is_some_and(|x| x.eq_ignore_ascii_case("zip")) { diff --git a/crates/engine/src/devices.rs b/crates/engine/src/devices.rs index a934ee44..399d03fd 100644 --- a/crates/engine/src/devices.rs +++ b/crates/engine/src/devices.rs @@ -191,7 +191,7 @@ pub fn devices_in(parents: &[std::path::PathBuf], extra: Vec /// The volume's `DCIM` folder: the usual spellings are checked directly, so a volume's root is /// only listed (other spellings on a case-sensitive volume) off Windows, whose drives aren't. -fn dcim_in(root: &std::path::Path) -> Option { +pub fn dcim_in(root: &std::path::Path) -> Option { if let Some(p) = ["DCIM", "dcim"].iter().map(|n| root.join(n)).find(|p| p.is_dir()) { return Some(p); } diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 5df94988..1227f3e2 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -317,32 +317,96 @@ pub fn is_supported(path: &Path) -> bool { } /// Expand files and folders (recursively) into supported files. `skip` (e.g. the library folder) -/// is never descended into. +/// is never descended into. Each folder's walk is bounded ([`crate::walk::Limits::default`]); a +/// walk that stops at a bound is logged. pub fn expand(paths: &[String], skip: Option<&Path>) -> Vec { - fn walk(p: &Path, skip: Option<&Path>, out: &mut Vec, top: bool) { - let hidden = p.file_name().is_some_and(|n| n.to_string_lossy().starts_with('.')); - if (hidden && !top) || skip.is_some_and(|s| p == s) { - return; + expand_within(paths, skip, crate::walk::Limits::default()).0 +} + +/// [`expand`] with the walk bounded by `limits` → (files, whether a walk stopped at a bound). +pub fn expand_within(paths: &[String], skip: Option<&Path>, limits: crate::walk::Limits) -> (Vec, bool) { + let mut out = Vec::new(); + let mut truncated = false; + for p in paths { + let p = Path::new(p); + if skip.is_some_and(|s| p == s) { + continue; } if p.is_dir() { - let Ok(rd) = std::fs::read_dir(p) else { return }; - let mut v: Vec = rd.flatten().map(|e| e.path()).collect(); - v.sort(); - for c in v { - walk(&c, skip, out, false); + let w = crate::walk::files_in(p, skip, limits, is_supported); + if w.truncated { + log::warn!( + "import: stopped looking in {} after {} entries (limits: {} entries, {} folder levels)", + p.display(), + w.entries, + limits.max_entries, + limits.max_depth + ); } - } else if top || is_supported(p) { + truncated |= w.truncated; + out.extend(w.files.into_iter().map(|f| f.to_string_lossy().to_string())); + } else { // explicitly named files are attempted even with an unknown extension (sniffed) out.push(p.to_string_lossy().to_string()); } } - let mut out = Vec::new(); - for p in paths { - walk(Path::new(p), skip, &mut out, true); - } let mut seen = std::collections::HashSet::new(); out.retain(|p| seen.insert(p.clone())); - out + (out, truncated) +} + +/// What a path given on the command line at launch becomes ([`launch_path`]). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum LaunchPath { + /// Import this (absolute) file or folder, in the background. + Import(String), + /// Not imported: `path` (absolute) is a whole drive or the whole home folder, which is never + /// walked without being chosen in the app (issue #374). + Refused { path: String, why: String }, +} + +/// Resolve a launch argument for importing (issue #374). An empty argument is ignored (`None`); +/// a relative one (`.`, `..`, `photos`; `/` or `\` on Windows, the current drive's root) is +/// resolved against `cwd` — the working folder a launcher picked, often a drive root or +/// `C:\Windows\System32` — and refused when that leaves no folder (`cwd` unknown). A drive +/// or file-system root is replaced by its camera folder (`DCIM`, from `dcim`: a memory card) or +/// refused, and so is the home folder `home`: either would walk most of the disk. +pub fn launch_path(arg: &str, cwd: Option<&Path>, home: Option<&Path>, dcim: impl Fn(&Path) -> Option) -> Option { + if arg.trim().is_empty() { + return None; + } + let given = Path::new(arg); + let joined = if given.is_absolute() { + given.to_path_buf() + } else { + match cwd { + Some(c) => c.join(given), + None => return Some(LaunchPath::Refused { path: arg.to_string(), why: "a relative path, and the working folder is unknown".into() }), + } + }; + // `.` and `..` resolved by name (no file-system access) + let mut abs = PathBuf::new(); + for c in joined.components() { + match c { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + abs.pop(); + } + other => abs.push(other.as_os_str()), + } + } + let shown = abs.to_string_lossy().to_string(); + if abs.parent().is_none() { + return Some(match dcim(&abs) { + Some(d) => LaunchPath::Import(d.to_string_lossy().to_string()), + None => LaunchPath::Refused { path: shown, why: "a whole drive".into() }, + }); + } + let key = lightcraft_catalog::query::folder_key; + if home.is_some_and(|h| key(&h.to_string_lossy()) == key(&shown)) { + return Some(LaunchPath::Refused { path: shown, why: "the whole home folder".into() }); + } + Some(LaunchPath::Import(shown)) } /// Probe files (headers + content hash) as an import would. diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index fab17896..53a525c2 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -38,6 +38,7 @@ pub mod segment; pub mod sidecar; pub mod smart; mod view; +pub mod walk; use std::sync::Arc; diff --git a/crates/engine/src/presets.rs b/crates/engine/src/presets.rs index ec63cc23..8c36aa74 100644 --- a/crates/engine/src/presets.rs +++ b/crates/engine/src/presets.rs @@ -83,28 +83,22 @@ pub fn parse_preset_file(name: &str, bytes: &[u8]) -> Result, String Ok(out) } -/// Expand files/folders (recursively) into preset files (`.lcpreset`, `.xmp`). +/// Expand files/folders (recursively, bounded: see [`crate::walk`]) into preset files +/// (`.lcpreset`, `.xmp`). pub fn expand_preset_paths(paths: &[String]) -> Vec { - fn walk(p: &Path, out: &mut Vec, top: bool) { - if p.is_dir() { - let Ok(rd) = std::fs::read_dir(p) else { return }; - let mut v: Vec<_> = rd.flatten().map(|e| e.path()).collect(); - v.sort(); - for c in v { - if !c.file_name().is_some_and(|n| n.to_string_lossy().starts_with('.')) { - walk(&c, out, false); - } - } - } else { - let ext = p.extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default(); - if top || ext == LCPRESET_EXT || ["xmp", "lrtemplate", "zip", "lmp", "mplumpack"].contains(&ext.as_str()) { - out.push(p.to_string_lossy().to_string()); - } - } - } + let preset_file = |p: &Path| { + let ext = p.extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default(); + ext == LCPRESET_EXT || ["xmp", "lrtemplate", "zip", "lmp", "mplumpack"].contains(&ext.as_str()) + }; let mut out = Vec::new(); for p in paths { - walk(Path::new(p), &mut out, true); + let path = Path::new(p); + if path.is_dir() { + let w = crate::walk::files_in(path, None, crate::walk::Limits::default(), preset_file); + out.extend(w.files.into_iter().map(|f| f.to_string_lossy().to_string())); + } else { + out.push(p.clone()); + } } out } diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index ce36c447..69d75ec2 100644 --- a/crates/engine/src/tests_import.rs +++ b/crates/engine/src/tests_import.rs @@ -1007,3 +1007,58 @@ fn trashed_photo_ids(tag: &str) -> (Session, std::path::PathBuf, [u64; 1], u64) let (s, src, id) = trashed_photo(tag); (s, src, [id], 0) } + +/// Issue #374: launch arguments never turn into a walk of a whole drive or home folder. An empty +/// argument is ignored; `.`, `..` and (on Windows) `/` are resolved against the working folder a +/// launcher picked, which is often a drive root; a root is replaced by its memory card's DCIM +/// folder or refused, and so is the home folder. +#[test] +fn launch_paths_never_become_a_drive_root_or_the_home_folder() { + use crate::import::{LaunchPath, launch_path}; + let root = if cfg!(windows) { Path::new("C:\\") } else { Path::new("/") }; + let home = if cfg!(windows) { Path::new("C:\\Users\\me") } else { Path::new("/home/me") }; + let none = |_: &Path| None; + let refused = |r: Option| matches!(r, Some(LaunchPath::Refused { .. })); + for empty in ["", " "] { + assert_eq!(launch_path(empty, Some(root), Some(home), none), None, "{empty:?}"); + } + // the working folder is the drive root: `.` (and `..`) name the whole drive + for arg in [".", "..", "./", ".\\..", "/"] { + let arg = if cfg!(windows) { arg.to_string() } else { arg.replace('\\', "/") }; + assert!(refused(launch_path(&arg, Some(root), Some(home), none)), "{arg:?}"); + } + assert!(refused(launch_path("..", Some(&home.join("Pictures")), Some(home), none)), "up to the home folder"); + assert!(refused(launch_path(&home.to_string_lossy(), None, Some(home), none))); + assert!(refused(launch_path(".", None, Some(home), none)), "relative, no working folder"); + // a memory card's root: its camera folder + let dcim = |r: &Path| Some(r.join("DCIM")); + assert_eq!(launch_path(".", Some(root), Some(home), dcim), Some(LaunchPath::Import(root.join("DCIM").to_string_lossy().to_string()))); + // ordinary folders and files resolve to absolute paths + let pics = home.join("Pictures"); + assert_eq!(launch_path("Pictures", Some(home), Some(home), none), Some(LaunchPath::Import(pics.to_string_lossy().to_string()))); + assert_eq!( + launch_path(&pics.join("a.jpg").to_string_lossy(), None, Some(home), none), + Some(LaunchPath::Import(pics.join("a.jpg").to_string_lossy().to_string())) + ); +} + +/// Issue #374: a folder import is bounded — a tree deeper or larger than the limits is not walked +/// to the end (it stops and says so), and what it found is still returned. +#[test] +fn expand_stops_at_the_walk_limits() { + let d = temp_dir("bounded"); + let mut deep = d.clone(); + for i in 0..8 { + deep = deep.join(format!("d{i}")); + std::fs::create_dir_all(&deep).unwrap(); + std::fs::write(deep.join(format!("p{i}.jpg")), b"x").unwrap(); + } + let top = [d.to_string_lossy().to_string()]; + let (all, cut) = crate::import::expand_within(&top, None, crate::walk::Limits::default()); + assert_eq!((all.len(), cut), (8, false)); + let (some, cut) = crate::import::expand_within(&top, None, crate::walk::Limits { max_depth: 3, max_entries: 1000 }); + assert_eq!((some.len(), cut), (2, true), "{some:?}"); + let (few, cut) = crate::import::expand_within(&top, None, crate::walk::Limits { max_depth: 32, max_entries: 5 }); + assert!(cut && few.len() < 8, "{few:?}"); + let _ = std::fs::remove_dir_all(&d); +} diff --git a/crates/engine/src/walk.rs b/crates/engine/src/walk.rs new file mode 100644 index 00000000..479c877c --- /dev/null +++ b/crates/engine/src/walk.rs @@ -0,0 +1,183 @@ +//! Bounded folder walks (issue #374): every recursive listing of the disk — import, Local's +//! Include Subfolders, Find Missing Photos, preset and LUT folders, MCP — goes through +//! [`files_in`], which stops at a depth and a number of directory entries instead of walking a +//! whole drive, follows a symbolic link or junction to a folder at most once (no loops), and +//! decides what is a folder from the directory listing itself (no extra open per entry, which +//! is what makes a large walk slow on Windows). +//! +//! It never chooses where to walk: callers pass a folder the user named. Nothing in the app walks +//! at startup or on the UI thread. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +/// How far a walk may go. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Limits { + /// Folder levels below the starting folder that are listed (its own entries are level 1). + pub max_depth: usize, + /// Directory entries looked at in all (files and folders, hidden ones included). + pub max_entries: usize, +} + +impl Default for Limits { + fn default() -> Self { + Limits { max_depth: MAX_DEPTH, max_entries: MAX_ENTRIES } + } +} + +/// Folder levels a walk descends at most; photo trees are a handful of levels deep. +pub const MAX_DEPTH: usize = 32; +/// Directory entries a walk looks at at most: far more than any photo folder holds, far fewer +/// than a whole system drive. +pub const MAX_ENTRIES: usize = 1_000_000; + +/// What a walk found. +#[derive(Debug, Default, PartialEq)] +pub struct Walked { + /// The files `keep` accepted, in path order, folder by folder (depth first). + pub files: Vec, + /// The walk stopped at a [`Limits`] bound: there may be more files. + pub truncated: bool, + /// Directory entries looked at. + pub entries: usize, +} + +fn hidden(p: &Path) -> bool { + p.file_name().is_some_and(|n| n.to_string_lossy().starts_with('.')) +} + +/// The files below folder `root` that `keep` accepts. Hidden entries (names starting with `.`) +/// are left out, the folder `skip` (e.g. the library's own) is never entered, and the walk stops +/// at `limits` (see [`Walked::truncated`]). Unreadable folders are skipped. +pub fn files_in(root: &Path, skip: Option<&Path>, limits: Limits, mut keep: impl FnMut(&Path) -> bool) -> Walked { + let mut out = Walked::default(); + // canonical folders reached through a link, so a link loop is walked at most once + let mut linked: HashSet = HashSet::new(); + if let Ok(c) = std::fs::canonicalize(root) { + linked.insert(c); + } + // (path, depth of the folder it is in, is a folder); popped in path order + let mut stack: Vec<(PathBuf, usize, bool)> = vec![(root.to_path_buf(), 0, true)]; + while let Some((path, depth, is_dir)) = stack.pop() { + if !is_dir { + if keep(&path) { + out.files.push(path); + } + continue; + } + if skip.is_some_and(|s| path == s) { + continue; + } + if depth >= limits.max_depth { + out.truncated = true; + continue; + } + let Ok(rd) = std::fs::read_dir(&path) else { continue }; + let mut children: Vec<(PathBuf, bool)> = Vec::new(); + for e in rd.flatten() { + if out.entries >= limits.max_entries { + out.truncated = true; + break; + } + out.entries = out.entries.saturating_add(1); + let p = e.path(); + if hidden(&p) { + continue; + } + let Ok(ft) = e.file_type() else { continue }; + let dir = if ft.is_symlink() { + // a link (or a Windows junction): a folder is entered once, a file is a file + match std::fs::metadata(&p) { + Ok(m) if m.is_dir() => { + let Ok(c) = std::fs::canonicalize(&p) else { continue }; + if !linked.insert(c) { + continue; + } + true + } + _ => false, + } + } else { + ft.is_dir() + }; + children.push((p, dir)); + } + children.sort(); + let below = depth.saturating_add(1); + stack.extend(children.into_iter().rev().map(|(p, d)| (p, below, d))); + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + fn tmp(tag: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("lc-walk-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d + } + + #[test] + fn files_come_in_path_order_without_hidden_ones_or_the_skipped_folder() { + let d = tmp("order"); + for f in ["b/2.jpg", "a/1.jpg", "a/x/0.jpg", "c.jpg", ".hidden/h.jpg", "a/.h.jpg", "lib/l.jpg", "a/n.txt"] { + let p = d.join(f); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, b"x").unwrap(); + } + let lib = d.join("lib"); + let w = files_in(&d, Some(&lib), Limits::default(), |p| p.extension().is_some_and(|e| e == "jpg")); + let rel: Vec = w.files.iter().map(|p| p.strip_prefix(&d).unwrap().to_string_lossy().replace('\\', "/")).collect(); + assert_eq!(rel, ["a/1.jpg", "a/x/0.jpg", "b/2.jpg", "c.jpg"]); + assert!(!w.truncated); + let _ = std::fs::remove_dir_all(&d); + } + + /// Issue #374: a walk never runs on without end — a tree deeper than the limit, or with more + /// entries than the limit, stops and says so. + #[test] + fn deep_and_wide_trees_stop_at_the_limits() { + let d = tmp("bounds"); + let mut deep = d.join("deep"); + for i in 0..12 { + deep = deep.join(format!("l{i}")); + } + std::fs::create_dir_all(&deep).unwrap(); + std::fs::write(deep.join("bottom.jpg"), b"x").unwrap(); + let wide = d.join("wide"); + std::fs::create_dir_all(&wide).unwrap(); + for i in 0..50 { + std::fs::write(wide.join(format!("{i:02}.jpg")), b"x").unwrap(); + } + let all = |_: &Path| true; + let w = files_in(&d.join("deep"), None, Limits { max_depth: 5, max_entries: 1000 }, all); + assert!(w.files.is_empty() && w.truncated, "{w:?}"); + let w = files_in(&d.join("deep"), None, Limits { max_depth: 13, max_entries: 1000 }, all); + assert_eq!((w.files.len(), w.truncated), (1, false), "{w:?}"); + let w = files_in(&wide, None, Limits { max_depth: 5, max_entries: 20 }, all); + assert_eq!((w.files.len(), w.entries, w.truncated), (20, 20, true)); + let w = files_in(&wide, None, Limits::default(), all); + assert_eq!((w.files.len(), w.truncated), (50, false)); + let _ = std::fs::remove_dir_all(&d); + } + + /// A link back up the tree (a loop) is followed at most once: the walk ends. + #[cfg(unix)] + #[test] + fn link_loops_end() { + let d = tmp("loop"); + std::fs::create_dir_all(d.join("a/b")).unwrap(); + std::fs::write(d.join("a/b/p.jpg"), b"x").unwrap(); + std::os::unix::fs::symlink(&d, d.join("a/b/up")).unwrap(); + std::os::unix::fs::symlink(d.join("a"), d.join("alias")).unwrap(); + let w = files_in(&d, None, Limits::default(), |_| true); + assert!(!w.truncated, "{w:?}"); + assert!(w.files.len() <= 2, "{w:?}"); + assert!(w.files.iter().any(|p| p.ends_with("a/b/p.jpg")), "{w:?}"); + let _ = std::fs::remove_dir_all(&d); + } +} diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index bfc5ea1e..e423e3f6 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -139,31 +139,20 @@ impl Backend for Headless { } } -/// Expand folders (recursively, sorted) into photo files and make paths absolute. +/// Expand folders (recursively, sorted; bounded, see `lightcraft_engine::walk`) into photo files +/// and make paths absolute. pub fn expand_paths(paths: &[String]) -> Vec { - fn walk(p: &Path, out: &mut Vec) { - if p.is_dir() { - if let Ok(rd) = std::fs::read_dir(p) { - let mut v: Vec<_> = rd.flatten().map(|e| e.path()).collect(); - v.sort(); - for c in v { - if c.file_name().is_some_and(|n| !n.to_string_lossy().starts_with('.')) { - walk(&c, out); - } - } - } - } else if p.extension().is_some_and(|e| PHOTO_EXTENSIONS.contains(&e.to_string_lossy().to_lowercase().as_str())) { - out.push(std::path::absolute(p).unwrap_or_else(|_| p.to_path_buf()).to_string_lossy().to_string()); - } - } + let absolute = |p: &Path| std::path::absolute(p).unwrap_or_else(|_| p.to_path_buf()).to_string_lossy().to_string(); + let photo = |p: &Path| p.extension().is_some_and(|e| PHOTO_EXTENSIONS.contains(&e.to_string_lossy().to_lowercase().as_str())); let mut out = Vec::new(); for p in paths { let path = Path::new(p); if path.is_dir() { - walk(path, &mut out); + let w = lightcraft_engine::walk::files_in(path, None, lightcraft_engine::walk::Limits::default(), photo); + out.extend(w.files.iter().map(|f| absolute(f))); } else { // Explicit files are kept even with unknown extensions (the probe decides). - out.push(std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()).to_string_lossy().to_string()); + out.push(absolute(path)); } } out diff --git a/crates/ui-egui/src/panels/library_problem.rs b/crates/ui-egui/src/panels/library_problem.rs index 3cdcc605..212e57d9 100644 --- a/crates/ui-egui/src/panels/library_problem.rs +++ b/crates/ui-egui/src/panels/library_problem.rs @@ -56,8 +56,11 @@ fn open(app: &mut LightcraftApp, path: Option) { Ok(serde_json::Value::Null) => {} // folder dialog cancelled Ok(_) => { let files = app.library_problem.take().map(|p| p.pending_import).unwrap_or_default(); - if !files.is_empty() { - let _ = app.run("library.import", json!({"paths": files})); + // in the background, like files dropped on the window (issue #374) + if !files.is_empty() + && let Err(e) = crate::import::start_paths(app, files) + { + log::warn!("import: {e}"); } } Err(e) => { @@ -144,8 +147,11 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { p.dismissed = true; } let files = app.library_problem.as_mut().map(|p| std::mem::take(&mut p.pending_import)).unwrap_or_default(); - if !files.is_empty() { - let _ = app.run("library.import", json!({"paths": files})); + // in the background, like files dropped on the window (issue #374) + if !files.is_empty() + && let Err(e) = crate::import::start_paths(app, files) + { + log::warn!("import: {e}"); } } Some(Choice::Quit) => app.ui.quit = true, diff --git a/crates/ui-egui/src/tests_library_problem.rs b/crates/ui-egui/src/tests_library_problem.rs index b6b79604..10783ff1 100644 --- a/crates/ui-egui/src/tests_library_problem.rs +++ b/crates/ui-egui/src/tests_library_problem.rs @@ -75,6 +75,8 @@ fn unopenable_library_asks_instead_of_running_a_demo() { assert!(!has(&h, "button:libraryRetry")); assert!(has(&h, "indicator:temporarySession")); assert!(h.app.session.library.is_none()); + // imported on the import worker (issue #374: never synchronously on the UI thread) + h.step_until(T, |h| h.app.import.is_none() && h.app.session.catalog.len() == 1); assert_eq!(h.app.session.catalog.len(), 1, "the command-line photo is imported into the temporary session"); if let Some(p) = std::env::var_os("LIGHTCRAFT_TEST_SHOTS") { let r = h.request("ui.screenshot", json!({"path": std::path::Path::new(&p).join("banner.png").to_string_lossy(), "headless": true}), T);