From 790a3417765183138afd898ea0f9848a08820da1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 21:15:52 +0000 Subject: [PATCH] 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);