diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 3ad15bb5..149c98ab 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -469,6 +469,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 { @@ -541,6 +566,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 { @@ -593,6 +621,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() }, @@ -615,7 +644,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 @@ -665,6 +699,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/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/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/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/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 94d644dc..33d48b1e 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 0db9ea62..f26b4ca7 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -42,6 +42,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_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] 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); diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index cb2d8e30..aad86fd5 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -397,7 +397,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")); 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.