From 8afb30f58bca41ec765ea53e812175abdcb84351 Mon Sep 17 00:00:00 2001 From: Juan Antonio Redondo Date: Wed, 21 Oct 2026 12:51:55 +0200 Subject: [PATCH] fix(adjust): scan the LUT folder off the UI thread, skip oversized files (#6558) Review follow-up: the Color Lookup dropdown scans the LUT folder on a worker thread and shows the last finished scan meanwhile; files over 64 MB (larger than any LUT the parser takes) are not listed; the UI test uses a tempfile directory. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/engine/src/adjust_cmds.rs | 12 +++- crates/engine/src/adjust_cmds/tests.rs | 11 +++- crates/ui-egui/Cargo.toml | 2 + crates/ui-egui/src/adjust_ui.rs | 79 +++++++++++++++++++++----- 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b0ff196c55..dd59fd7d46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5075,6 +5075,7 @@ dependencies = [ "serde", "serde_json", "sys-locale", + "tempfile", "toml", "unicode-bidi", "winsafe", diff --git a/crates/engine/src/adjust_cmds.rs b/crates/engine/src/adjust_cmds.rs index accafb2bc5..f4e0694fa3 100644 --- a/crates/engine/src/adjust_cmds.rs +++ b/crates/engine/src/adjust_cmds.rs @@ -170,10 +170,14 @@ fn base_name(path: &str) -> String { /// Most LUT files [`folder_luts`] lists. const MAX_FOLDER_LUTS: usize = 500; +/// Largest LUT file [`folder_luts`] lists: a 129³ `.cube` (the parser's largest) is about 60 MB. +const MAX_FOLDER_LUT_BYTES: u64 = 64 << 20; /// The `.cube`, `.3dl` and `.look` files directly in `dir`, as (file name, path) sorted by name, /// for Color Lookup's dropdown (Preferences › File Handling › LUT Folder, #6558). An empty, -/// missing or unreadable folder lists nothing; hidden files and subfolders are skipped. +/// missing or unreadable folder lists nothing; hidden files, subfolders and files too large to be +/// a LUT are skipped. A symlink to a LUT file is listed, as a studio's shared library often is. +/// Contents are only parsed when one is picked, which reports a malformed file. #[cfg(not(target_arch = "wasm32"))] pub fn folder_luts(dir: &str) -> Vec<(String, String)> { let dir = dir.trim(); @@ -183,8 +187,12 @@ pub fn folder_luts(dir: &str) -> Vec<(String, String)> { .filter_map(|e| { let name = e.file_name().into_string().ok()?; let ext = name.rsplit_once('.')?.1.to_ascii_lowercase(); + if name.starts_with('.') || !matches!(ext.as_str(), "cube" | "3dl" | "look") { + return None; + } let path = e.path(); - (!name.starts_with('.') && matches!(ext.as_str(), "cube" | "3dl" | "look") && path.is_file()).then_some((name, path.to_str()?.to_string())) + let meta = std::fs::metadata(&path).ok()?; + (meta.is_file() && meta.len() <= MAX_FOLDER_LUT_BYTES).then_some((name, path.to_str()?.to_string())) }) .collect(); out.sort_by_key(|(name, _)| name.to_lowercase()); diff --git a/crates/engine/src/adjust_cmds/tests.rs b/crates/engine/src/adjust_cmds/tests.rs index 0d95266049..e2de68bac9 100644 --- a/crates/engine/src/adjust_cmds/tests.rs +++ b/crates/engine/src/adjust_cmds/tests.rs @@ -740,8 +740,17 @@ fn folder_luts_lists_lut_files_by_name() { std::fs::write(dir.path().join(f), "LUT_3D_SIZE 2\n").unwrap(); } std::fs::create_dir(dir.path().join("sub.cube")).unwrap(); + // Too large to be a LUT (sparse, so cheap to make). + std::fs::File::create(dir.path().join("huge.cube")).unwrap().set_len(MAX_FOLDER_LUT_BYTES + 1).unwrap(); + let mut want = vec!["A.3DL", "b.cube", "c.look"]; + // A symlink to a LUT is listed: shared LUT libraries are often linked in. + #[cfg(unix)] + { + std::os::unix::fs::symlink(dir.path().join("b.cube"), dir.path().join("linked.cube")).unwrap(); + want.push("linked.cube"); + } let names: Vec<_> = folder_luts(dir.path().to_str().unwrap()).into_iter().map(|(name, _)| name).collect(); - assert_eq!(names, ["A.3DL", "b.cube", "c.look"]); + assert_eq!(names, want); assert!(folder_luts("").is_empty()); assert!(folder_luts(dir.path().join("missing").to_str().unwrap()).is_empty()); } diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index e5bf497c81..11b7e9f779 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -62,6 +62,8 @@ egui_kittest = { workspace = true, features = ["wgpu", "snapshot", "eframe"] } photocraft-testkit = { workspace = true } # Only for examples/perf_scenarios.rs (a 500-brush .abr for the #210 import row). photocraft-psd = { workspace = true } +# Isolated, self-cleaning folders for filesystem tests (the LUT folder, #6558). +tempfile = "3" [build-dependencies] # Deflates the translation catalogs and egui's default fonts (build.rs); already linked at run time to inflate them. diff --git a/crates/ui-egui/src/adjust_ui.rs b/crates/ui-egui/src/adjust_ui.rs index 2492da68f6..927197144b 100644 --- a/crates/ui-egui/src/adjust_ui.rs +++ b/crates/ui-egui/src/adjust_ui.rs @@ -4,6 +4,8 @@ //! values). Selective Color previews live while a slider drags, via `app.live_adjust` carrying //! the full parameter set; Color Lookup commits on every change (its table isn't in the params). +use std::sync::{Arc, Mutex, PoisonError}; + use photocraft_doc::{Adjustment, LayerId}; use photocraft_engine::adjust_cmds::RANGES; use serde_json::{Value, json}; @@ -93,24 +95,63 @@ fn browse_color_lookup(app: &mut PhotocraftApp, layer: LayerId) -> Result Vec<(String, String)> { - type Cached = (String, f64, Vec<(String, String)>); +type FolderLuts = Vec<(String, String)>; + +/// The LUT folder scan kept between frames: the latest finished list of `folder`, and the scan +/// in flight, if any. +#[derive(Clone, Default)] +struct FolderScan { + folder: String, + started: Option, + luts: FolderLuts, + pending: Option>>>, +} + +/// The LUT folder's files (Preferences › File Handling › LUT Folder, #6558). The folder is +/// scanned on a worker thread at most every couple of seconds, since a network share can be +/// slow; until a scan finishes the dropdown shows the previous one. +fn folder_luts(app: &PhotocraftApp, ctx: &egui::Context) -> FolderLuts { let folder = app.session.prefs().file_handling.lut_folder.clone(); let now = ctx.input(|input| input.time); let key = egui::Id::new("color-lookup-folder"); - if let Some((cached, at, luts)) = ctx.data(|data| data.get_temp::(key)) - && cached == folder - && now - at < 2.0 - { - return luts; + let mut scan = ctx.data(|data| data.get_temp::(key)).unwrap_or_default(); + if let Some(done) = scan.pending.as_ref().and_then(|p| p.lock().unwrap_or_else(PoisonError::into_inner).take()) { + scan.luts = done; + scan.pending = None; } - let luts = photocraft_engine::adjust_cmds::folder_luts(&folder); - ctx.data_mut(|data| data.insert_temp::(key, (folder, now, luts.clone()))); + if scan.folder != folder { + // Another folder: never show the old one's files, and drop its scan's result. + scan = FolderScan { folder: folder.clone(), ..FolderScan::default() }; + } + if scan.pending.is_none() && scan.started.is_none_or(|at| now - at >= 2.0) { + scan.started = Some(now); + scan.pending = Some(start_folder_scan(folder, ctx.clone())); + } + let luts = scan.luts.clone(); + ctx.data_mut(|data| data.insert_temp(key, scan)); luts } +/// Scan `folder` on a worker thread (on the web, where there are none, right here: it lists +/// nothing there anyway). +fn start_folder_scan(folder: String, ctx: egui::Context) -> Arc>> { + let result = Arc::new(Mutex::new(None)); + let shared = Arc::clone(&result); + let work = move || { + let luts = photocraft_engine::adjust_cmds::folder_luts(&folder); + *shared.lock().unwrap_or_else(PoisonError::into_inner) = Some(luts); + ctx.request_repaint(); + }; + #[cfg(not(target_arch = "wasm32"))] + if std::thread::Builder::new().name("lut-folder-scan".into()).spawn(work).is_err() { + // No thread: list nothing this time; the next scan retries. + *result.lock().unwrap_or_else(PoisonError::into_inner) = Some(Vec::new()); + } + #[cfg(target_arch = "wasm32")] + work(); + result +} + /// The dropdown key for the current look: a built-in id, a LUT folder file, `none` or `custom`. fn current_lookup(name: &str, has_lut: bool, folder: &[(String, String)]) -> String { if !has_lut { @@ -266,13 +307,22 @@ mod lookup_picker_tests { /// #6558: a LUT folder file is offered by name, and picking it embeds it. #[test] fn lut_folder_files_are_offered_and_embedded() { - let dir = std::env::temp_dir().join(format!("pc-lut-folder-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); let path = dir.join("film.cube"); std::fs::write(&path, CUBE).unwrap(); let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); app.run("prefs.set", json!({"path": "fileHandling.lutFolder", "value": dir.to_str().unwrap()})).unwrap(); - let folder = folder_luts(&app, &egui::Context::default()); + // The scan runs on a worker thread: ask again until it has finished. + let ctx = egui::Context::default(); + let mut folder = folder_luts(&app, &ctx); + for _ in 0..500 { + if !folder.is_empty() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + folder = folder_luts(&app, &ctx); + } let key = format!("{FOLDER_LUT}{}", path.to_str().unwrap()); assert_eq!(folder, [("film.cube".to_string(), path.to_str().unwrap().to_string())]); app.run("file.new", json!({"width": 16, "height": 12})).unwrap(); @@ -289,7 +339,6 @@ mod lookup_picker_tests { assert_eq!(current_lookup(name, true, &folder), key); assert_eq!(current_lookup(name, true, &[]), "custom"); assert_eq!(lookup_params("none"), json!({"lut": "none"})); - let _ = std::fs::remove_dir_all(&dir); } #[test]