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/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_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);